diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 9103390..d848d3b 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -156,3 +156,32 @@ jobs: ${{ github.ref == 'refs/heads/main' && 'git.farh.net/groombook/reset:latest' || '' }} cache-from: type=registry,ref=git.farh.net/groombook/cache:reset cache-to: type=registry,ref=git.farh.net/groombook/cache:reset,mode=max + + - name: Smoke test seed image (blackhole npmjs.org) + run: | + set -euo pipefail + IMAGE="git.farh.net/groombook/seed:${{ steps.version.outputs.tag }}" + docker pull "$IMAGE" + # GRO-1985: pnpm must be a real binary, not a Corepack shim, and must + # not try to reach registry.npmjs.org on invocation. + docker run --rm \ + --add-host registry.npmjs.org:127.0.0.1 \ + --entrypoint="" \ + "$IMAGE" \ + sh -c 'set -e; test "$(which pnpm)" = "/usr/local/bin/pnpm"; pnpm --version' + echo "seed image: pnpm resolves to /usr/local/bin/pnpm and runs offline ✓" + + - name: Smoke test reset image (blackhole npmjs.org) + run: | + set -euo pipefail + IMAGE="git.farh.net/groombook/reset:${{ steps.version.outputs.tag }}" + docker pull "$IMAGE" + # GRO-1985: pnpm must be a real binary, not a Corepack shim, and must + # not try to reach registry.npmjs.org on invocation. Validates the + # hard requirement from the issue: reset runs offline. + docker run --rm \ + --add-host registry.npmjs.org:127.0.0.1 \ + --entrypoint="" \ + "$IMAGE" \ + sh -c 'set -e; test "$(which pnpm)" = "/usr/local/bin/pnpm"; echo "HOME=$HOME"; pnpm --version' + echo "reset image: pnpm resolves to /usr/local/bin/pnpm, HOME=/tmp, runs offline ✓" diff --git a/Dockerfile b/Dockerfile index 5fea669..a77a7df 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,8 +3,12 @@ FROM node:22-alpine AS base # invocations of `pnpm` work without DNS access to registry.npmjs.org. # The corepack shim delegates to corepack, which re-validates against # npmjs.org on first use — that fails in air-gapped UAT seed/migrate/reset -# Jobs. GRO-1983 / GRO-1889 / GRO-1909. +# Jobs. GRO-1983 / GRO-1889 / GRO-1909 / GRO-1981 / GRO-1985. RUN npm install -g pnpm@9.15.4 +# Belt-and-braces: disable Corepack's download fallback so that even if a +# Corepack shim is somehow invoked at runtime, it will not try to fetch +# pnpm from registry.npmjs.org. Belt for the real-binary trousers. GRO-1985. +ENV COREPACK_ENABLE_DOWNLOAD_FALLBACK=0 WORKDIR /app # Install deps @@ -26,6 +30,8 @@ RUN pnpm --filter @groombook/types build && \ # Runtime FROM node:22-alpine AS runner RUN npm install -g pnpm@9.15.4 +# Same defence-in-depth as base: no Corepack fallback. GRO-1985. +ENV COREPACK_ENABLE_DOWNLOAD_FALLBACK=0 WORKDIR /app ENV NODE_ENV=production @@ -46,12 +52,18 @@ CMD ["node", "dist/index.js"] # Migrate stage — runs drizzle-kit migrate against the database FROM builder AS migrate +# pnpm needs a writable HOME for any config/state it writes. With +# readOnlyRootFilesystem: true and runAsUser: 1000, /home/node is read-only. +# The job pods mount a writable emptyDir at /tmp; point HOME there. GRO-1985. +ENV HOME=/tmp CMD ["pnpm", "--filter", "@groombook/db", "migrate"] # Seed stage — populates the database with test data FROM builder AS seed +ENV HOME=/tmp CMD ["pnpm", "--filter", "@groombook/db", "seed"] # Reset stage — drops all tables, re-runs migrations, and re-seeds FROM builder AS reset +ENV HOME=/tmp CMD ["pnpm", "--filter", "@groombook/db", "reset"] diff --git a/UAT_PLAYBOOK.md b/UAT_PLAYBOOK.md index 4bb82ed..cf80f3f 100644 --- a/UAT_PLAYBOOK.md +++ b/UAT_PLAYBOOK.md @@ -128,6 +128,9 @@ CUSTOMER=$(kubectl get secret seed-uat-passwords -n groombook-uat \ | TC-API-3.19a | Get pet profile summary — customer owner-bypass (GRO-2013) | Sign in as `uat-customer@groombook.dev`; `POST /api/portal/session-from-auth`; then `GET /api/pets/{ownPetId}/profile-summary` with header `X-Impersonation-Session-Id: {sessionId}` for either of the customer's seeded pets (`c0000001-0000-0000-0000-000000000002` UAT Pup Alpha, `c0000001-0000-0000-0000-000000000003` UAT Pup Beta) | 200 OK, aggregated profile returned (owner-bypass: customer with valid portal session for pet's clientId is allowed even though rbac.ts auto-provisions them as a `groomer` staff row with no appointment linkage) | | TC-API-3.19b | Get pet profile summary — customer cross-tenant blocked (GRO-2013) | Sign in as `uat-customer@groombook.dev`; reuse the customer's sessionId from TC-API-3.19a; `GET /api/pets/{otherClientPetId}/profile-summary` for a pet owned by a different client (`c0000002-...` or any non-customer pet) | 403 Forbidden (owner-bypass requires session.clientId === pet.clientId) | | TC-API-3.19c | Get pet profile summary — customer without portal session header | Same as TC-API-3.19a but omit the `X-Impersonation-Session-Id` header | 403 Forbidden (no owner-bypass without valid portal session) | +| TC-API-3.29 | Get pet profile summary — unknown UUID returns 404 (GRO-2014) | GET /api/pets/00000000-0000-0000-0000-000000000001/profile-summary while authenticated (any role) | 404 Not Found with body `{"error":"Not found"}` (was empty-body 500 in GRO-2014) | +| TC-API-3.30 | Get pet profile summary — malformed UUID returns 404 (GRO-2014) | GET /api/pets/not-a-uuid/profile-summary while authenticated | 404 Not Found with body `{"error":"Not found"}` (was empty-body 500 in GRO-2014 — Postgres uuid cast failure) | +| TC-API-3.31 | Get pet profile summary — never empty-body 500 (GRO-2014) | GET /api/pets/{anyId}/profile-summary across the test sweep | No response has status 500 with an empty body. Any 500 must include a JSON body `{"error":"Internal Server Error"}` | #### Seed Data Verification (GRO-1898) diff --git a/src/__tests__/petProfileSummary.test.ts b/src/__tests__/petProfileSummary.test.ts index d3d529f..069d1cb 100644 --- a/src/__tests__/petProfileSummary.test.ts +++ b/src/__tests__/petProfileSummary.test.ts @@ -1,23 +1,34 @@ /** * Pet Profile Summary Tests * - * Covers GET /api/pets/:id/profile-summary in the deployed tree - * (root src/). The headline cases validate the GRO-2013 owner-bypass: - * a customer who is auto-provisioned as a `groomer` staff row by rbac.ts - * (with no appointment linkage) may still read their own pet's summary - * when they supply a valid X-Impersonation-Session-Id whose clientId - * matches the pet's clientId. + * Covers GET /api/pets/:id/profile-summary in the deployed tree (root src/). * - * Deployed tree: src/routes/pets.ts. This test mirrors the live handler - * (which queries the `appointments` table for visit history, not - * `groomingVisitLogs`). + * Two suites share one mock harness: + * + * 1. GRO-2013 owner-bypass (the deployed-tree port of #135): + * A customer who is auto-provisioned as a `groomer` staff row by rbac.ts + * (with no appointment linkage) may still read their own pet's summary + * when they supply a valid X-Impersonation-Session-Id whose clientId + * matches the pet's clientId. + * + * 2. GRO-2014 error handling (deployed tree): + * - Empty-body 500 must never escape the route — the onError handler + * converts unhandled errors into a structured JSON 500. + * - Malformed UUIDs must return 404 (not 500 via a Postgres uuid cast). + * - Missing staff context must return 401 (not TypeError on staffRow.id). + * - Pet not found must return 404. + * - Groomer with no appointment linkage must return 403. + * - Manager and groomer with linkage must receive the summary body. + * + * Deployed tree handler: src/routes/pets.ts. The mock queries the + * `appointments` table (the live schema) for visit history, not + * `groomingVisitLogs`. */ - import { describe, it, expect, vi, beforeEach } from "vitest"; import { Hono } from "hono"; import type { AppEnv, StaffRow } from "../middleware/rbac.js"; -// ─── Mock staff fixtures ────────────────────────────────────────────────────── +// ─── Staff fixtures ────────────────────────────────────────────────────────── const MANAGER: StaffRow = { id: "staff-manager-id", @@ -38,6 +49,7 @@ const GROOMER: StaffRow = { id: "staff-groomer-id", oidcSub: "oidc-groomer-sub", role: "groomer", + isSuperUser: false, name: "Groomer Gary", email: "groomer@example.com", }; @@ -57,11 +69,12 @@ const CUSTOMER_STAFF: StaffRow = { email: "uat-customer@groombook.dev", }; -// ─── Mutable mock state ─────────────────────────────────────────────────────── +// ─── Mutable mock state ───────────────────────────────────────────────────── const CLIENT_ID = "c0000001-0000-0000-0000-000000000001"; const PET_ID = "c0000001-0000-0000-0000-000000000002"; const OTHER_CLIENT_PET_ID = "c0000002-0000-0000-0000-000000000099"; +const UNKNOWN_PET_UUID = "00000000-0000-0000-0000-000000000001"; const futureDate = () => new Date(Date.now() + 30 * 60_000); const pastDate = () => new Date(Date.now() - 5 * 60_000); @@ -147,7 +160,7 @@ function makeSession(overrides: Record = {}) { }; } -// ─── DB mock state ──────────────────────────────────────────────────────────── +// ─── DB mock state ────────────────────────────────────────────────────────── let petsTable: Record[]; let appointmentsTable: Record[]; @@ -157,12 +170,26 @@ let sessionsTable: Record[]; // selectQueue: queries resolve in FIFO order. Each .from(table) result // returns a chain that resolves to the next queued row set on a terminal // call (.where()/.orderBy()/.limit()). -let selectQueue: Array<{ table: string; rows: unknown[] }> = []; +// +// A queued entry of `{ table: "pets", rows: null, throw: "..." }` tells the +// mock to throw instead of returning rows — used by the GRO-2014 "JSON +// envelope on downstream error" test. Any other queued entry with `rows` +// resolves to those rows. An entry with `rows: []` returns an empty array +// (no rows, no throw). +let selectQueue: Array<{ + table: string; + rows: unknown[] | null; + throw?: string; +}> = []; function enqueue(table: string, rows: unknown[] = []) { selectQueue.push({ table, rows }); } +function enqueueThrow(table: string, message: string) { + selectQueue.push({ table, rows: null, throw: message }); +} + function resetMock() { petsTable = [makePet()]; appointmentsTable = [makeAppointment()]; @@ -171,7 +198,7 @@ function resetMock() { selectQueue = []; } -// ─── Module mocks ───────────────────────────────────────────────────────────── +// ─── Module mocks ─────────────────────────────────────────────────────────── vi.mock("@groombook/db", () => { function makeTable(name: string) { @@ -201,7 +228,12 @@ vi.mock("@groombook/db", () => { function takeQueuedRows(tableName: string): unknown[] { const next = selectQueue.shift(); - if (next && next.table === tableName) return next.rows; + if (next && next.table === tableName) { + if (next.throw) { + throw new Error(next.throw); + } + return next.rows ?? []; + } return []; } @@ -261,35 +293,113 @@ vi.mock("../lib/s3.js", () => ({ deleteObject: vi.fn(), })); -// ─── Import after mocks are set up ──────────────────────────────────────────── +// ─── Import after mocks are set up ────────────────────────────────────────── const { petsRouter } = await import("../routes/pets.js"); -// ─── App builder ────────────────────────────────────────────────────────────── +// ─── App builder ──────────────────────────────────────────────────────────── -function buildApp(staffRow: StaffRow) { +function buildApp(staffRow: StaffRow | null) { const app = new Hono(); app.use("*", async (c, next) => { - c.set("jwtPayload", { sub: staffRow.oidcSub ?? staffRow.userId ?? "" }); - c.set("staff", staffRow); + if (staffRow) { + c.set("jwtPayload", { sub: staffRow.oidcSub ?? staffRow.userId ?? "" }); + c.set("staff", staffRow); + } await next(); }); app.route("/pets", petsRouter); return app; } -// ─── Reset before each test ─────────────────────────────────────────────────── +// ─── Reset before each test ───────────────────────────────────────────────── beforeEach(() => { resetMock(); vi.clearAllMocks(); }); -// ─── Tests ──────────────────────────────────────────────────────────────────── +// ─── GRO-2014 error-handling suite ────────────────────────────────────────── -describe("GET /:id/profile-summary — basic access", () => { +describe("GET /:id/profile-summary — GRO-2014 error handling", () => { + it("returns 404 (not 500) for a malformed UUID path param", async () => { + const app = buildApp(MANAGER); + const res = await app.request("/pets/not-a-uuid/profile-summary"); + expect(res.status).toBe(404); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe("Not found"); + }); + + it("returns 401 when staff context is missing (defense in depth)", async () => { + const app = buildApp(null); + const res = await app.request(`/pets/${UNKNOWN_PET_UUID}/profile-summary`); + expect(res.status).toBe(401); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe("Unauthorized"); + }); + + it("returns 404 when authenticated and pet does not exist", async () => { + enqueue("pets", []); + const app = buildApp(MANAGER); + const res = await app.request(`/pets/${UNKNOWN_PET_UUID}/profile-summary`); + expect(res.status).toBe(404); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe("Not found"); + }); + + it("returns 403 when groomer has no appointment linkage to the pet's client", async () => { + enqueue("pets", petsTable); + enqueue("appointments", []); // linkage check returns empty → 403 + const app = buildApp(GROOMER); + const res = await app.request(`/pets/${PET_ID}/profile-summary`); + expect(res.status).toBe(403); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe("Forbidden"); + }); + + it("returns 200 with summary for a manager (no groomer linkage check)", async () => { + enqueue("pets", petsTable); + enqueue("appointments", appointmentsTable); // history + enqueue("appointments", [{ count: 1 }]); // visit count + enqueue("appointments", []); // upcoming (none) + const app = buildApp(MANAGER); + const res = await app.request(`/pets/${PET_ID}/profile-summary`); + expect(res.status).toBe(200); + const body = (await res.json()) as Record; + expect(body.id).toBe(PET_ID); + expect(body.name).toBe("Biscuit"); + expect(body.visitCount).toBe(1); + expect(body.upcomingAppointment).toBeNull(); + expect(body.recentGroomingHistory).toBeInstanceOf(Array); + }); + + it("returns 200 with summary for a groomer with appointment linkage", async () => { + enqueue("pets", petsTable); + enqueue("appointments", [{ id: "appt-1" }]); // linkage found + enqueue("appointments", appointmentsTable); // history + enqueue("appointments", [{ count: 1 }]); // visit count + enqueue("appointments", []); // upcoming + const app = buildApp(GROOMER); + const res = await app.request(`/pets/${PET_ID}/profile-summary`); + expect(res.status).toBe(200); + const body = (await res.json()) as Record; + expect(body.id).toBe(PET_ID); + }); + + it("returns a JSON envelope (not empty body) when a downstream query throws", async () => { + enqueueThrow("pets", "simulated postgres uuid cast failure"); + const app = buildApp(MANAGER); + const res = await app.request(`/pets/${PET_ID}/profile-summary`); + expect(res.status).toBe(500); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe("Internal Server Error"); + }); +}); + +// ─── GRO-2013 owner-bypass suite ──────────────────────────────────────────── + +describe("GET /:id/profile-summary — owner-bypass (GRO-2013)", () => { it("returns 404 when the pet does not exist", async () => { - petsTable = []; enqueue("pets", []); const app = buildApp(MANAGER); const res = await app.request(`/pets/${PET_ID}/profile-summary`); @@ -297,7 +407,6 @@ describe("GET /:id/profile-summary — basic access", () => { }); it("returns 200 with aggregated profile for a manager", async () => { - // Query order: pets, recent history, visit count, upcoming enqueue("pets", petsTable); enqueue("appointments", appointmentsTable); enqueue("appointments", [{ count: 1 }]); @@ -315,10 +424,9 @@ describe("GET /:id/profile-summary — basic access", () => { }); it("returns 200 for a groomer with appointment linkage to the pet's client", async () => { - // Query order: pets, linkage check, recent history, visit count, upcoming enqueue("pets", petsTable); enqueue("appointments", [{ id: "appt-1" }]); // linkage found - enqueue("appointments", appointmentsTable); // history + enqueue("appointments", appointmentsTable); enqueue("appointments", [{ count: 1 }]); enqueue("appointments", []); @@ -328,7 +436,6 @@ describe("GET /:id/profile-summary — basic access", () => { }); it("returns 403 for a groomer with no appointment linkage and no bypass header", async () => { - // Query order: pets, linkage check (returns empty → 403) enqueue("pets", petsTable); enqueue("appointments", []); // no linkage @@ -336,14 +443,8 @@ describe("GET /:id/profile-summary — basic access", () => { const res = await app.request(`/pets/${PET_ID}/profile-summary`); expect(res.status).toBe(403); }); -}); -// ─── GRO-2013 owner-bypass ──────────────────────────────────────────────────── - -describe("GET /:id/profile-summary — owner-bypass (GRO-2013)", () => { it("customer-as-groomer with valid active session for pet's client returns 200", async () => { - // Query order: pets, session lookup (found, active, future), recent history, - // visit count, upcoming enqueue("pets", petsTable); enqueue("impersonationSessions", sessionsTable); // active session found enqueue("appointments", appointmentsTable); @@ -360,7 +461,6 @@ describe("GET /:id/profile-summary — owner-bypass (GRO-2013)", () => { }); it("customer-as-groomer with no header still gets 403 (no bypass)", async () => { - // Query order: pets, session lookup (header missing → returns [], 403) enqueue("pets", petsTable); const app = buildApp(CUSTOMER_STAFF); diff --git a/src/routes/pets.ts b/src/routes/pets.ts index a16582c..45dcde2 100644 --- a/src/routes/pets.ts +++ b/src/routes/pets.ts @@ -24,6 +24,23 @@ import { export const petsRouter = new Hono(); +// Convert Zod validation errors from 422 to 400 and ensure any thrown error +// returns a structured JSON body rather than Hono's default empty-body 500. +// GRO-2014: profile-summary previously bubbled unhandled errors and produced +// an empty-body 500. Mirror the onError pattern already used in invoices.ts +// and reports.ts so every error has a JSON envelope. +petsRouter.onError((err, c) => { + if (err instanceof z.ZodError) { + return c.json({ error: "Validation failed", issues: err.issues }, 400); + } + console.error("[pets] unhandled error", err); + return c.json({ error: "Internal Server Error" }, 500); +}); + +// UUID format used by all pet routes — guards path params against malformed +// values before they hit Drizzle / Postgres uuid columns (which would throw). +const uuidSchema = z.string().uuid(); + const createPetSchema = z.object({ clientId: z.string().uuid(), name: z.string().min(1).max(200), @@ -142,8 +159,24 @@ async function resolveImpersonationClientId( petsRouter.get("/:id/profile-summary", async (c) => { const db = getDb(); const petId = c.req.param("id"); + + // GRO-2014: validate UUID format before hitting Postgres. Passing a non-UUID + // string to a uuid column makes the driver throw, which previously surfaced + // as an empty-body 500 to clients. + const parsedId = uuidSchema.safeParse(petId); + if (!parsedId.success) { + return c.json({ error: "Not found" }, 404); + } + + // Defense in depth: resolveStaffMiddleware should always populate `staff` + // for protected routes (or short-circuit with 401/403 of its own). Guard + // anyway so a misconfigured route mount can't trigger a TypeError on + // staffRow.id when the linkage check runs. const staffRow = c.get("staff"); - const isGroomer = staffRow?.role === "groomer"; + if (!staffRow) { + return c.json({ error: "Unauthorized" }, 401); + } + const isGroomer = staffRow.role === "groomer"; // Fetch the pet const [pet] = await db.select().from(pets).where(eq(pets.id, petId));