groombook/api
13
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(portal): drop writable photoKey from PATCH /portal/pets — S3 key-hijack (GRO-2187/GRO-2198) #172

Merged
Flea Flicker merged 1 commits from fix/gro-2187-portal-photokey-hijack into dev 2026-06-08 12:39:03 +00:00
Conversation 0 Commits 1 Files Changed 2
+64 -7
Flea Flicker commented 2026-06-08 12:37:17 +00:00
Member
Copy Link
Copy Source

Security fix — drop writable photoKey from portal pet PATCH (S3 key-hijack)

Closes the HIGH Security finding on the GRO-2187 portal pet PATCH (GRO-2198), ratified by the CTO.

The defect

PATCH /api/portal/pets/:petId mapped body.photoUrl → updateData.photoKey with no validation. photoKey is a trusted S3 object key consumed server-side by getPresignedGetUrl(pet.photoKey) (read) and deleteObject(pet.photoKey) (destructive). The upload path (src/routes/pets.ts:444) already guards keys with a pets/{petId}/ prefix "to prevent key hijacking" — the portal PATCH bypassed that guard, so an authenticated customer could point their pet's photoKey at any object key → cross-tenant disclosure/destruction on a later staff action.

The fix (per CTO directive — smallest safe surface)

  • Remove photoUrl from portalPetUpdateSchema and delete the updateData.photoKey = body.photoUrl line. 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 existing free-text caps.

Tests (src/__tests__/portalPets.test.ts)

  • Regression (gate evidence): foreign photoUrl on portal PATCH → 200 but photoKey unchanged.
  • Over-long medicalAlert.description → 400 (zValidator).
  • Updated the existing "persists mapped columns" test: photoKey is no longer written by the portal PATCH.

Verification

  • npm run typecheck ✅ · npm run lint ✅ (0 errors; only pre-existing warnings) · npm test ✅ 612 passed.

Sequencing

dev → promote dev→uat → bump UAT GitOps overlay tag → redeploy. Then Security re-runs the gate and Shedward re-runs §4.8/§5.23. GRO-2198 stays blocked until the fixed image is live on uat; GRO-1480 stays open.

Refs GRO-2187, GRO-2198. 🤖 Generated with Claude Code

## Security fix — drop writable `photoKey` from portal pet PATCH (S3 key-hijack) Closes the **HIGH** Security finding on the GRO-2187 portal pet PATCH ([GRO-2198](/GRO/issues/GRO-2198)), ratified by the CTO. ### The defect `PATCH /api/portal/pets/:petId` mapped `body.photoUrl → updateData.photoKey` with no validation. `photoKey` is a trusted **S3 object key** consumed server-side by `getPresignedGetUrl(pet.photoKey)` (read) and `deleteObject(pet.photoKey)` (destructive). The upload path (`src/routes/pets.ts:444`) already guards keys with a `pets/{petId}/` prefix *"to prevent key hijacking"* — the portal PATCH bypassed that guard, so an authenticated customer could point their pet's `photoKey` at any object key → cross-tenant disclosure/destruction on a later staff action. ### The fix (per CTO directive — smallest safe surface) - Remove `photoUrl` from `portalPetUpdateSchema` and delete the `updateData.photoKey = body.photoUrl` line. 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 existing free-text caps. ### Tests (`src/__tests__/portalPets.test.ts`) - **Regression (gate evidence):** foreign `photoUrl` on portal PATCH → `200` but `photoKey` unchanged. - Over-long `medicalAlert.description` → `400` (zValidator). - Updated the existing "persists mapped columns" test: `photoKey` is no longer written by the portal PATCH. ### Verification - `npm run typecheck` ✅ · `npm run lint` ✅ (0 errors; only pre-existing warnings) · `npm test` ✅ **612 passed**. ### Sequencing dev → promote `dev→uat` → bump UAT GitOps overlay tag → redeploy. Then Security re-runs the gate and Shedward re-runs §4.8/§5.23. [GRO-2198](/GRO/issues/GRO-2198) stays blocked until the fixed image is live on `uat`; [GRO-1480](/GRO/issues/GRO-1480) stays open. Refs GRO-2187, GRO-2198. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Flea Flicker added 1 commit 2026-06-08 12:37:17 +00:00
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
Details
CI / Lint & Typecheck (pull_request) Successful in 26s
Details
CI / Build & Push Docker Images (pull_request) Successful in 58s
Details
21c678f72c 
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>
Flea Flicker merged commit 14d7889ec0 into dev 2026-06-08 12:39:03 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
Something isn't working
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
ai-review (AI Review) gb_barkley (Barkley Trimsworth) cpfarhood (Chris Farhood) ci (Continuous Integration [bot]) gb_flea (Flea Flicker) flux (Flux CD) admin (Gitea Admin) gb_lint (Lint Roller) renovate (Mend Renovate) gb_pawla (Pawla Abdul) gb_scrubs (Scrubs McBarkley) gb_shedward (Shedward Scissorhands) gb_dogfather (The Dogfather)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: groombook/api#172
Delete Branch "fix/gro-2187-portal-photokey-hijack"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?