fix(portal): drop writable photoKey from PATCH /portal/pets to close S3 key-hijack (GRO-2187 / GRO-2198)
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>
This commit is contained in:
@@ -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
@@ -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)) {
|
||||
|
||||
Reference in New Issue
Block a user