Compare commits

...

1 Commits

Author SHA1 Message Date
Savannah Savings 21c678f72c fix(portal): drop writable photoKey from PATCH /portal/pets to close S3 key-hijack (GRO-2187 / GRO-2198)
CI / Test (pull_request) Successful in 25s
CI / Lint & Typecheck (pull_request) Successful in 26s
CI / Build & Push Docker Images (pull_request) Successful in 58s
Security gate FAIL (Barkley, CTO-ratified): the portal pet PATCH mapped
`body.photoUrl -> updateData.photoKey` with no validation. photoKey is a
trusted S3 object key consumed server-side by getPresignedGetUrl (read) and
deleteObject (destructive); the upload path (pets.ts) already guards keys with
a pets/{petId}/ prefix to prevent hijacking. The portal PATCH bypassed that
guard, letting an authenticated customer point their pet's photoKey at any
object key (cross-tenant disclosure/destruction on a later staff action).

Fix (per CTO directive — smallest safe surface):
- Remove `photoUrl` from portalPetUpdateSchema and delete the
  `updateData.photoKey = body.photoUrl` mapping. Photo changes already have a
  dedicated, key-validated flow (upload + /photo/confirm). The web form's
  round-trip of the GET-shaped photoUrl is a no-op (Zod strips the unknown key).
- LOW hardening: cap preferredCuts (string max 2000, array max 50),
  medicalAlerts (array max 50) and medicalAlert type/description (max 2000),
  mirroring the existing free-text max(2000) caps.

Tests:
- New regression: foreign photoUrl on portal PATCH returns 200 but leaves
  photoKey unchanged (gate evidence).
- New: over-long medicalAlert description rejected (400, zValidator).
- Updated the existing "persists mapped columns" test: photoKey is no longer
  written by the portal PATCH.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
2026-06-08 12:36:56 +00:00
2 changed files with 64 additions and 7 deletions
+53 -1
View File
@@ -177,7 +177,10 @@ describe("PATCH /portal/pets/:petId", () => {
expect(persisted.weightKg).toBe("18.25");
expect(persisted.groomingNotes).toBe("old grooming notes");
expect(persisted.healthAlerts).toBe("Allergic to oatmeal shampoo");
expect(persisted.photoKey).toBe("pets/rex.jpg");
// photoKey is NOT writable via portal PATCH (GRO-2187 S3 key-hijack fix):
// the web form round-trips the GET-shaped photoUrl, but the server must not
// persist it. Photo changes go through the key-validated upload flow.
expect(persisted.photoKey).toBeUndefined();
expect(persisted.coatType).toBe("double");
expect(persisted.petSizeCategory).toBe("extra_large");
expect(persisted.preferredCuts).toEqual(["teddy bear", "puppy cut"]);
@@ -187,6 +190,55 @@ describe("PATCH /portal/pets/:petId", () => {
expect(persisted.updatedAt).toBeInstanceOf(Date);
});
// GRO-2187 security regression: a portal customer must not be able to set the
// S3 object key. photoKey is consumed server-side by getPresignedGetUrl /
// deleteObject; the upload path guards keys with a pets/{petId}/ prefix, and the
// portal PATCH must not offer a bypass. A foreign/arbitrary photoUrl is accepted
// (Zod strips the unknown key) but must leave photoKey untouched.
it("does not mutate photoKey when a foreign photoUrl is supplied (200)", async () => {
selectSessionRow = ACTIVE_SESSION;
const ownKey = `pets/${PET_ID}/original.jpg`;
selectPetRow = { ...PET, photoKey: ownKey };
const res = await jsonPatch(
`/portal/pets/${PET_ID}`,
{
name: "Rex",
// attacker-chosen key pointing at another tenant's object
photoUrl: "pets/00000000-0000-0000-0000-0000000000ff/victim-secret.jpg",
},
{ "X-Impersonation-Session-Id": SESSION_ID }
);
expect(res.status).toBe(200);
const persisted = updatedValues[0]!;
// The attacker-supplied key never reaches the update payload.
expect(persisted.photoKey).toBeUndefined();
// And the stored key is unchanged from the pet's own value.
const body = await res.json();
expect(body.photoUrl).toBe(ownKey);
});
// The length/array caps live in the Zod schema, so violations are rejected by
// zValidator with 400 (in-handler enum checks are what return 422).
it("returns 400 when a medicalAlert description exceeds the length cap", async () => {
selectSessionRow = ACTIVE_SESSION;
selectPetRow = PET;
const res = await jsonPatch(
`/portal/pets/${PET_ID}`,
{
medicalAlerts: [
{ type: "allergy", description: "x".repeat(2001), severity: "low" },
],
},
{ "X-Impersonation-Session-Id": SESSION_ID }
);
expect(res.status).toBe(400);
expect(updatedValues).toHaveLength(0);
});
it("falls back to the weight key when weightKg is absent", async () => {
selectSessionRow = ACTIVE_SESSION;
selectPetRow = PET;
+11 -6
View File
@@ -261,8 +261,8 @@ const PORTAL_PET_SIZE_ALIASES: Record<string, string> = { xlarge: "extra_large"
const portalMedicalAlertSchema = z.object({
id: z.string().optional(),
type: z.string(),
description: z.string(),
type: z.string().max(2000),
description: z.string().max(2000),
severity: z.enum(["low", "medium", "high"]),
});
@@ -275,12 +275,16 @@ const portalPetUpdateSchema = z.object({
birthDate: z.string().nullable().optional(),
notes: z.string().max(2000).nullable().optional(),
healthAlerts: z.string().max(2000).nullable().optional(),
photoUrl: z.string().nullable().optional(),
// photoUrl/photoKey are intentionally NOT writable here: photoKey is a trusted
// S3 object key consumed server-side (getPresignedGetUrl / deleteObject), and the
// upload path (pets.ts) already enforces a pets/{petId}/ prefix guard against key
// hijacking. Photo changes go through the dedicated upload + /photo/confirm flow.
// The web form round-trips the GET-shaped photoUrl; Zod strips it as an unknown key.
// coatType / petSizeCategory validated in-handler so bad values return 422.
coatType: z.string().nullable().optional(),
petSizeCategory: z.string().nullable().optional(),
preferredCuts: z.array(z.string()).nullable().optional(),
medicalAlerts: z.array(portalMedicalAlertSchema).nullable().optional(),
preferredCuts: z.array(z.string().max(2000)).max(50).nullable().optional(),
medicalAlerts: z.array(portalMedicalAlertSchema).max(50).nullable().optional(),
});
portalRouter.patch(
@@ -322,7 +326,8 @@ portalRouter.patch(
if (body.notes !== undefined) updateData.groomingNotes = body.notes;
if (body.healthAlerts !== undefined) updateData.healthAlerts = body.healthAlerts;
if (body.photoUrl !== undefined) updateData.photoKey = body.photoUrl;
// photoKey is intentionally not writable here — see portalPetUpdateSchema note.
// Photo changes go through the key-validated upload + /photo/confirm flow.
if (body.coatType !== undefined) {
if (body.coatType !== null && !PORTAL_COAT_TYPES.includes(body.coatType)) {