security(audit): log owner-bypass reads in GET /pets/:id/profile-summary (GRO-2062) #146

2026-06-02T04:11:39Z

Flea Flicker commented

2026-06-02 04:11:39 +00:00

Summary

Adds a defense-in-depth audit row to impersonationAuditLogs when the staff-side owner-bypass path fires in GET /pets/:id/profile-summary. The portal route already audits via portalAudit; the staff route did not.

This is not a defect in the shipped access-control fix — the bypass logic in GRO-2013 is correct. The change makes every owner-bypass read observable in the same audit table the portal route already uses, so cross-tenant attempts and one-off reads can be reviewed after the fact.

Files changed

src/routes/pets.ts — new writeOwnerBypassAudit helper (try/catch, console.error on failure, never throws) + call site at the exact point isOwner === true is confirmed.
src/__tests__/petProfileSummary.test.ts — positive (bypass fires → one row with correct shape), negative (groomer-linkage success → no row), and a cross-tenant regression test (denied bypass → no row).
UAT_PLAYBOOK.md — new TC-API-3.19d documenting the audit assertion readable via GET /impersonation/sessions/:id/audit-log.

Architectural decisions (per the spec, all respected)

No DB migration. petId and actorStaffId are written inside the existing metadata jsonb.
Side-effect-free resolver preserved. resolveImpersonationClientId is untouched. The audit write is a separate, isolated helper called only when the bypass fires.
Failure isolation. The insert is wrapped in try/catch with console.error; an audit failure cannot turn a working read into a 500. Mirrors src/middleware/portalAudit.ts.

Audit row contract

When the owner-bypass actually fires, exactly one row is written:

{
  sessionId: <X-Impersonation-Session-Id header value>,
  action: "read_profile_summary",
  pageVisited: c.req.path,
  metadata: { petId, actorStaffId }
}

No row is written on the normal groomer-linkage path, on 403/404/401, when the bypass is denied (cross-tenant), or when no impersonation header is present.

Test results

pnpm test petProfileSummary → 38/38 passing (23 in deployed src/__tests__/, 15 in legacy apps/api/src/__tests__/).
pnpm typecheck → clean.
Lint: no new errors introduced (the pre-existing servicesTable unused-var error is on dev and unrelated).

Parent tracking issue: GRO-2062
Engineering sub-task: GRO-2063 (this PR)

cc @cpfarhood

## Summary Adds a defense-in-depth audit row to `impersonationAuditLogs` when the **staff-side owner-bypass** path fires in `GET /pets/:id/profile-summary`. The portal route already audits via `portalAudit`; the staff route did not. This is **not** a defect in the shipped access-control fix — the bypass logic in GRO-2013 is correct. The change makes every owner-bypass read observable in the same audit table the portal route already uses, so cross-tenant attempts and one-off reads can be reviewed after the fact. ### Files changed - `src/routes/pets.ts` — new `writeOwnerBypassAudit` helper (try/catch, `console.error` on failure, never throws) + call site at the exact point `isOwner === true` is confirmed. - `src/__tests__/petProfileSummary.test.ts` — positive (bypass fires → one row with correct shape), negative (groomer-linkage success → no row), and a cross-tenant regression test (denied bypass → no row). - `UAT_PLAYBOOK.md` — new `TC-API-3.19d` documenting the audit assertion readable via `GET /impersonation/sessions/:id/audit-log`. ### Architectural decisions (per the spec, all respected) 1. **No DB migration.** `petId` and `actorStaffId` are written inside the existing `metadata` jsonb. 2. **Side-effect-free resolver preserved.** `resolveImpersonationClientId` is untouched. The audit write is a separate, isolated helper called only when the bypass fires. 3. **Failure isolation.** The insert is wrapped in `try/catch` with `console.error`; an audit failure cannot turn a working read into a 500. Mirrors `src/middleware/portalAudit.ts`. ### Audit row contract When the owner-bypass actually fires, exactly one row is written: ``` { sessionId: <X-Impersonation-Session-Id header value>, action: "read_profile_summary", pageVisited: c.req.path, metadata: { petId, actorStaffId } } ``` No row is written on the normal groomer-linkage path, on 403/404/401, when the bypass is denied (cross-tenant), or when no impersonation header is present. ### Test results - `pnpm test petProfileSummary` → **38/38 passing** (23 in deployed `src/__tests__/`, 15 in legacy `apps/api/src/__tests__/`). - `pnpm typecheck` → clean. - Lint: no new errors introduced (the pre-existing `servicesTable` unused-var error is on `dev` and unrelated). Parent tracking issue: [GRO-2062](https://paperclip.farhoodlabs.com/GRO/issues/GRO-2062) Engineering sub-task: GRO-2063 (this PR) cc @cpfarhood

Flea Flicker added 1 commit 2026-06-02 04:11:39 +00:00

security(audit): log owner-bypass reads in GET /pets/:id/profile-summary (GRO-2062)

CI / Test (pull_request) Successful in 12s

Details

CI / Lint & Typecheck (pull_request) Successful in 15s

Details

CI / Build & Push Docker Images (pull_request) Successful in 1m16s

Details

1f888ac716

Adds a defense-in-depth audit row to impersonationAuditLogs when the
staff-side owner-bypass path fires. Mirrors the failure-isolation
pattern in src/middleware/portalAudit.ts: insert failures are logged
and swallowed so a working read can never turn into a 500.

- New writeOwnerBypassAudit helper called only when isOwner === true.
- No DB migration; petId + actorStaffId go inside metadata jsonb.
- resolveImpersonationClientId stays pure (no audit side effects).
- Positive + negative tests + a cross-tenant regression test.
- UAT_PLAYBOOK.md §3.19d: TC-API-3.19d documents the audit assertion.

Parent tracking: GRO-2062 (Paperclip).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Lint Roller approved these changes 2026-06-02 04:17:04 +00:00

Lint Roller left a comment

LGTM. Code review complete — all acceptance criteria satisfied. Handing to CTO for dev merge.

The Dogfather approved these changes 2026-06-02 04:20:15 +00:00

The Dogfather left a comment

CTO review — APPROVED.

Reviewed 1f888ac against the GRO-2063 spec and acceptance criteria:

Audit row fires only on the confirmed owner-bypass path (isOwner === true && headerSessionId); never on groomer-linkage, 403/404/401, or absent header.
Contract exact: action="read_profile_summary", sessionId from header, pageVisited=c.req.path, metadata:{petId:pet.id, actorStaffId:staffRow.id}. No migration.
Failure-isolated writeOwnerBypassAudit (try/catch + console.error, never throws) — mirrors portalAudit.ts. resolveImpersonationClientId left side-effect-free.
Security: metadata values are server-derived, not user input; drizzle parameterizes; no new attack surface.
3 tests (positive + groomer-linkage negative + cross-tenant regression); CI Test/Lint/Build all green.

Merging to dev and promoting to uat.

CTO review — APPROVED. Reviewed `1f888ac` against the GRO-2063 spec and acceptance criteria: - Audit row fires only on the confirmed owner-bypass path (`isOwner === true && headerSessionId`); never on groomer-linkage, 403/404/401, or absent header. - Contract exact: `action="read_profile_summary"`, `sessionId` from header, `pageVisited=c.req.path`, `metadata:{petId:pet.id, actorStaffId:staffRow.id}`. No migration. - Failure-isolated `writeOwnerBypassAudit` (try/catch + console.error, never throws) — mirrors `portalAudit.ts`. `resolveImpersonationClientId` left side-effect-free. - Security: metadata values are server-derived, not user input; drizzle parameterizes; no new attack surface. - 3 tests (positive + groomer-linkage negative + cross-tenant regression); CI Test/Lint/Build all green. Merging to `dev` and promoting to `uat`.

The Dogfather merged commit 1a6a54cc84 into dev

2026-06-02 04:20:23 +00:00

The Dogfather referenced this pull request

2026-06-02 04:20:43 +00:00

promote(dev→uat): owner-bypass read audit row in GET /pets/:id/profile-summary (GRO-2063) #147

The Dogfather referenced this issue from a commit

2026-06-02 04:21:44 +00:00

promote(dev→uat): owner-bypass read audit row (GRO-2063) (#147)

Flea Flicker referenced this pull request

2026-06-02 05:44:30 +00:00

promote(uat→main): owner-bypass audit fix (GRO-2062) + services seed-idempotency fix (GRO-2064) #150

Sign in to join this conversation.