diff --git a/src/__tests__/portalPets.test.ts b/src/__tests__/portalPets.test.ts index a95cb23..c23bca0 100644 --- a/src/__tests__/portalPets.test.ts +++ b/src/__tests__/portalPets.test.ts @@ -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; diff --git a/src/routes/portal.ts b/src/routes/portal.ts index 3f15745..0b106fb 100644 --- a/src/routes/portal.ts +++ b/src/routes/portal.ts @@ -261,8 +261,8 @@ const PORTAL_PET_SIZE_ALIASES: Record = { 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)) {