From 745593ee3514870c433226134e6a2cd7d8add183 Mon Sep 17 00:00:00 2001 From: Dave Date: Fri, 6 Nov 2026 15:43:12 +0200 Subject: [PATCH] fix(repairs): send the session on the detail photo upload, and cover every handler --- AGENTS.md | 18 +- .../plans/2026-11-06-repairs-app-phase-2.md | 12 +- src/app/(app)/repairs/[id]/page.tsx | 33 +++- .../[recordId]/[filename]/route.ts | 5 +- src/app/api/v1/repairs/repair-routes.test.ts | 174 +++++++++++++++++- 5 files changed, 217 insertions(+), 25 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9dbba63..7c41c1b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -376,12 +376,18 @@ planned phone app lives at `/repairs-app`, not built yet. stored, and the agent is told the upload worked. Add the fields FIRST, then verify by round-tripping a save — do not wait for an error, there will not be one. -- **The repairs REST family has no session check** (pre-existing; found - 2026-11-06). All six routes under `/api/v1/repairs/` read and write - through the PocketBase superuser client with no credential check. The - `(app)` layout gates the PAGES client-side; the API is not gated. The - `repairs.*` MCP tools ARE gated by `assertAgentAuth`. Operator - decision pending on adding a session check to all six routes together. +- **The repairs REST family requires a browser session** (closed + 2026-11-06). Every handler under `/api/v1/repairs/` checks + `assertBrowserSession` first and answers **401** without one — they + read and write through the PocketBase superuser client, so the check is + the only thing standing between an anonymous caller and a customer's + address, serial number and fault description. The `repairs` slice of + the `/api/v1/files/` proxy is gated the same way (photos, the Repair + Request PDF, the supplier quote PDF **with the pricing**); every other + collection's files are unaffected. That slice also accepts bearer + `AGENT_API_KEY`, so the MCP `files.fetch` tool keeps working. The + `repairs.*` MCP tools are gated by `assertAgentAuth` and call the + service layer directly, so they are unaffected. - Phase 1 of the repairs work landed on `main` 2026-11-06 (`9358b37`): the shared vocabulary, the service layer (`supplier_scope`, `signed_request`, `getRepairStatusCounts`, `linkRepairToOrder`), diff --git a/docs/superpowers/plans/2026-11-06-repairs-app-phase-2.md b/docs/superpowers/plans/2026-11-06-repairs-app-phase-2.md index 695855c..357e8c1 100644 --- a/docs/superpowers/plans/2026-11-06-repairs-app-phase-2.md +++ b/docs/superpowers/plans/2026-11-06-repairs-app-phase-2.md @@ -91,7 +91,7 @@ Do not try to base64 the photos into the create call. - Test: `src/app/api/v1/repairs/repair-routes.test.ts` (extend) **Interfaces:** -- Consumes: `assertBrowserSession(req)` from `@/lib/server/browser-session`; `useAuth()` from `@/components/auth-provider`. +- Consumes: `assertBrowserSession(req)` from `@/lib/server/browser-session`; the browser token from `getPocketBase().authStore.token` (see "How the browser token is obtained" above — `useAuth()` does NOT expose a token). - Produces: every repairs route returns **401 without a browser session**, and the office screens keep working. **Why this task exists.** The reviews proved the repairs API is anonymously readable on production: repair records (customer addresses, serial numbers, fault descriptions) and files (photos, the repair request PDF, the supplier quote PDF *with pricing*) all return HTTP 200 to a caller with no credentials. The MCP tools are correctly gated by `assertAgentAuth` — this is a REST-only hole, and the phone app would sit right on top of it. @@ -192,7 +192,7 @@ Use the real repairs collection id from the constant block at the top of that fi In `src/app/(app)/repairs/page.tsx` and `src/app/(app)/repairs/[id]/page.tsx`, every fetch to `/api/v1/repairs*` must carry the browser token. -Read the token from `useAuth()` — the same hook `MobileAuthGate` uses. Add a small helper in each page, matching the repo's existing shape: +Read the token from the PocketBase auth store — `useAuth()` does NOT expose a `token` (see "How the browser token is obtained" above). Add a small helper in each page, matching the repo's existing shape: ```ts const token = getPocketBase().authStore.token; @@ -229,7 +229,7 @@ Expected: PASS. The three original route tests plus the new 401 cases. Run: `npx tsc --noEmit` -Expected: 0 errors. If `useAuth()` does not expose a `token`, read `src/components/auth-provider.tsx` and use the field it does expose. +Expected: 0 errors. The token comes from `getPocketBase().authStore.token` — `useAuth()` exposes no `token` field, so never write `useAuth().token` (it is `undefined` and the route answers 401). - [ ] **Step 8: Commit** @@ -528,7 +528,7 @@ git commit -m "feat(repairs-app): the phone app shell, manifest and service work - Test: `src/app/(repairs-app)/repairs-app/repair-board.test.tsx` **Interfaces:** -- Consumes: `REPAIR_STATUS`, `REPAIR_STATUS_LABEL`, `REPAIR_STATUS_VARIANT` from `@/lib/repair-status`; `useAuth()` for the session header; `GET /api/v1/repairs` and `GET /api/v1/repairs/status-counts`. +- Consumes: `REPAIR_STATUS`, `REPAIR_STATUS_LABEL`, `REPAIR_STATUS_VARIANT` from `@/lib/repair-status`; the session header built from `getPocketBase().authStore.token` (NOT `useAuth()` — it exposes no token); `GET /api/v1/repairs` and `GET /api/v1/repairs/status-counts`. - Produces: the board screen at `/repairs-app`, and a `` client component. - [ ] **Step 1: Write the failing test** @@ -898,7 +898,7 @@ git commit -m "feat(repairs-app): the status board with reconciling counts" - Test: `src/app/(repairs-app)/repairs-app/book-in-form.test.tsx` **Interfaces:** -- Consumes: `CustomerSearch`, `SupplierSearch`, `useAuth()`, `POST /api/v1/repairs`, `POST /api/v1/repairs/[id]/files/photos`. +- Consumes: `CustomerSearch`, `SupplierSearch`, the session header built from `getPocketBase().authStore.token` (NOT `useAuth()` — it exposes no token), `POST /api/v1/repairs`, `POST /api/v1/repairs/[id]/files/photos`. - Produces: the booking screen at `/repairs-app/book-in`. - [ ] **Step 1: Write the failing test** @@ -1251,7 +1251,7 @@ git commit -m "feat(repairs-app): book a machine in, with photos" - Test: `src/app/(repairs-app)/repairs-app/[id]/repair-detail.test.tsx` **Interfaces:** -- Consumes: `nextStepFor`, `REPAIR_STATUS`, `REPAIR_STATUS_LABEL`, `REPAIR_STATUS_VARIANT` from `@/lib/repair-status`; `useAuth()`; `GET /api/v1/repairs/[id]`, `PATCH /api/v1/repairs/[id]`, `POST /api/v1/repairs/[id]/files/quote`, `POST /api/v1/repairs/[id]/generate-repair-request`. +- Consumes: `nextStepFor`, `REPAIR_STATUS`, `REPAIR_STATUS_LABEL`, `REPAIR_STATUS_VARIANT` from `@/lib/repair-status`; the session header built from `getPocketBase().authStore.token` (NOT `useAuth()` — it exposes no token); `GET /api/v1/repairs/[id]`, `PATCH /api/v1/repairs/[id]`, `POST /api/v1/repairs/[id]/files/quote`, `POST /api/v1/repairs/[id]/generate-repair-request`. - Produces: the detail screen at `/repairs-app/[id]`. - [ ] **Step 1: Write the failing test** diff --git a/src/app/(app)/repairs/[id]/page.tsx b/src/app/(app)/repairs/[id]/page.tsx index bddd5fa..c5258e3 100644 --- a/src/app/(app)/repairs/[id]/page.tsx +++ b/src/app/(app)/repairs/[id]/page.tsx @@ -104,29 +104,38 @@ function repairFileUrl(repairId: string, filename: string): string { } /** - * Fetch the repair's photos and return a filename -> blob URL map. + * Fetch the repair's photos and return a filename -> blob URL map, plus + * the filenames that could not be loaded. * * The file proxy requires a session and an `` cannot carry an * Authorization header, so the bytes are fetched with the session and * the browser renders them from a blob URL. + * + * A failed photo is reported by name rather than skipped: a silent skip + * would leave its tile reading "Loading…" for ever, so the operator + * would never learn the photo did not load. */ async function loadPhotoUrls( repairId: string, files: string[], -): Promise> { +): Promise<{ urls: Record; failed: string[] }> { const urls: Record = {}; + const failed: string[] = []; for (const name of files) { try { const res = await fetch(repairFileUrl(repairId, name), { headers: authHeaders(), }); - if (!res.ok) continue; + if (!res.ok) { + failed.push(name); + continue; + } urls[name] = URL.createObjectURL(await res.blob()); } catch { - // A photo that will not load is simply not shown. + failed.push(name); } } - return urls; + return { urls, failed }; } type RepairDetail = Repair & { @@ -201,6 +210,8 @@ export default function RepairDetailPage({ * and shown from a blob URL instead. */ const [photoUrls, setPhotoUrls] = useState>({}); + /** Photos that could not be loaded — shown as an error, not a spinner. */ + const [failedPhotos, setFailedPhotos] = useState([]); const [quoteDialogOpen, setQuoteDialogOpen] = useState(false); const [quoteDraft, setQuoteDraft] = useState({ @@ -238,7 +249,9 @@ export default function RepairDetailPage({ } const data: RepairDetail = await res.json(); setRepair(data); - setPhotoUrls(await loadPhotoUrls(data.id, data.photos ?? [])); + const photos = await loadPhotoUrls(data.id, data.photos ?? []); + setPhotoUrls(photos.urls); + setFailedPhotos(photos.failed); setEdit({ customer: data.customer ?? "", company: data.company ?? "", @@ -448,7 +461,9 @@ export default function RepairDetailPage({ fd.append("photos", p.file, p.name); const res = await fetch( `/api/v1/repairs/${repairId}/files/photos`, - { method: "POST", body: fd }, + // Multipart: the browser sets the Content-Type boundary, so + // only the session header goes here. + { method: "POST", headers: authHeaders(), body: fd }, ); if (res.ok) { uploaded++; @@ -919,6 +934,10 @@ export default function RepairDetailPage({ alt={f} className="h-full w-full object-cover" /> + ) : failedPhotos.includes(f) ? ( + + Could not load this photo + ) : ( Loading… diff --git a/src/app/api/v1/files/[collection]/[recordId]/[filename]/route.ts b/src/app/api/v1/files/[collection]/[recordId]/[filename]/route.ts index 730869c..9517124 100644 --- a/src/app/api/v1/files/[collection]/[recordId]/[filename]/route.ts +++ b/src/app/api/v1/files/[collection]/[recordId]/[filename]/route.ts @@ -122,7 +122,10 @@ export async function GET( if (collection === "repairs" || collection === "pbc_2000000004") { try { try { - assertAgentAuth(req); + // Awaited even though it is synchronous today: if it is ever + // made async, an un-awaited rejection would skip the browser + // session check below and let the request through unauthenticated. + await assertAgentAuth(req); } catch { await assertBrowserSession(req); } diff --git a/src/app/api/v1/repairs/repair-routes.test.ts b/src/app/api/v1/repairs/repair-routes.test.ts index 3c70f95..cb01904 100644 --- a/src/app/api/v1/repairs/repair-routes.test.ts +++ b/src/app/api/v1/repairs/repair-routes.test.ts @@ -3,11 +3,30 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; import { NextRequest } from "next/server"; import { createRepair, - uploadRepairFile, + createCustomerQuoteFromRepair, + generateRepairRequest, + getRepair, getRepairStatusCounts, + listCustomersForRepairForm, + listRepairs, + listSuppliersForRepairForm, + removeRepairFile, + updateRepair, + uploadRepairFile, } from "@/lib/server/repairs-service"; -import { POST as createRoute } from "./route"; -import { POST as filesRoute } from "./[id]/files/[field]/route"; +import { GET as listRoute, POST as createRoute } from "./route"; +import { + GET as getRepairRoute, + PATCH as patchRepairRoute, + DELETE as deleteRepairRoute, +} from "./[id]/route"; +import { + POST as filesRoute, + DELETE as deleteFileRoute, +} from "./[id]/files/[field]/route"; +import { POST as createQuoteRoute } from "./[id]/create-customer-quote/route"; +import { POST as generateRequestRoute } from "./[id]/generate-repair-request/route"; +import { GET as formDataRoute } from "./form-data/route"; import { GET as statusCountsRoute } from "./status-counts/route"; import { GET as fileProxyRoute } from "../files/[collection]/[recordId]/[filename]/route"; @@ -22,6 +41,9 @@ vi.mock("@/lib/server/agent-auth", () => ({ assertAgentAuth: (...args: unknown[]) => agentAuthMock(...args), })); +// Every service function the routes touch is mocked, so a route that +// skips its session check reaches a stubbed success instead of the +// network — that is what makes each 401 assertion load-bearing. vi.mock("@/lib/server/repairs-service", async () => { const actual = await vi.importActual< typeof import("@/lib/server/repairs-service") @@ -29,8 +51,16 @@ vi.mock("@/lib/server/repairs-service", async () => { return { ...actual, createRepair: vi.fn(), - uploadRepairFile: vi.fn(), + createCustomerQuoteFromRepair: vi.fn(), + generateRepairRequest: vi.fn(), + getRepair: vi.fn(), getRepairStatusCounts: vi.fn(), + listCustomersForRepairForm: vi.fn(), + listRepairs: vi.fn(), + listSuppliersForRepairForm: vi.fn(), + removeRepairFile: vi.fn(), + updateRepair: vi.fn(), + uploadRepairFile: vi.fn(), }; }); @@ -134,17 +164,53 @@ describe("repairs REST surface", () => { }); }); +/** + * Every repairs handler requires a browser session. + * + * One case per handler, and each one is load-bearing: with the session + * check removed the handler reaches its (stubbed) service and answers + * 200/201 instead of 401, so the assertion fails. The service stubs + * are therefore part of the test, not decoration. + */ describe("repairs routes require a session", () => { beforeEach(() => { vi.clearAllMocks(); sessionMock.mockRejectedValue( Object.assign(new Error("Not authenticated."), { status: 401 }), ); + + // If a handler forgets its check it must still land on a success + // status, so every service below resolves. + vi.mocked(listRepairs).mockResolvedValue({ + items: [], + totalItems: 0, + totalPages: 1, + page: 1, + perPage: 20, + } as never); + vi.mocked(getRepair).mockResolvedValue({ id: "rep1" } as never); + vi.mocked(updateRepair).mockResolvedValue({ id: "rep1" } as never); + vi.mocked(createRepair).mockResolvedValue({ id: "rep1" } as never); + vi.mocked(uploadRepairFile).mockResolvedValue({ id: "rep1" } as never); + vi.mocked(removeRepairFile).mockResolvedValue({ id: "rep1" } as never); + vi.mocked(generateRepairRequest).mockResolvedValue({ + filename: "RR-1.pdf", + } as never); + vi.mocked(createCustomerQuoteFromRepair).mockResolvedValue({ + quote: { id: "q1" }, + } as never); + vi.mocked(listCustomersForRepairForm).mockResolvedValue([] as never); + vi.mocked(listSuppliersForRepairForm).mockResolvedValue([] as never); + vi.mocked(getRepairStatusCounts).mockResolvedValue({ + total: 0, + byStatus: {}, + } as never); }); it("rejects a list request with no session", async () => { - const res = await createRoute(req("http://localhost/api/v1/repairs")); + const res = await listRoute(req("http://localhost/api/v1/repairs")); expect(res.status).toBe(401); + expect(listRepairs).not.toHaveBeenCalled(); }); it("rejects a create request with no session", async () => { @@ -159,6 +225,104 @@ describe("repairs routes require a session", () => { }), ); expect(res.status).toBe(401); + expect(createRepair).not.toHaveBeenCalled(); + }); + + it("rejects a read of one repair with no session", async () => { + const res = await getRepairRoute(req("http://localhost/api/v1/repairs/rep1"), { + params: Promise.resolve({ id: "rep1" }), + }); + expect(res.status).toBe(401); + expect(getRepair).not.toHaveBeenCalled(); + }); + + it("rejects an update of one repair with no session", async () => { + const res = await patchRepairRoute( + req("http://localhost/api/v1/repairs/rep1", { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ notes: "x" }), + }), + { params: Promise.resolve({ id: "rep1" }) }, + ); + expect(res.status).toBe(401); + expect(updateRepair).not.toHaveBeenCalled(); + }); + + it("rejects a delete of one repair with no session", async () => { + const res = await deleteRepairRoute( + req("http://localhost/api/v1/repairs/rep1", { method: "DELETE" }), + { params: Promise.resolve({ id: "rep1" }) }, + ); + expect(res.status).toBe(401); + }); + + it("rejects a file upload with no session", async () => { + const form = new FormData(); + form.append( + "photos", + new Blob([new Uint8Array([1, 2, 3])], { type: "image/jpeg" }), + "machine.jpg", + ); + const res = await filesRoute( + req("http://localhost/api/v1/repairs/rep1/files/photos", { + method: "POST", + body: form, + }), + { params: Promise.resolve({ id: "rep1", field: "photos" }) }, + ); + expect(res.status).toBe(401); + expect(uploadRepairFile).not.toHaveBeenCalled(); + }); + + it("rejects a file delete with no session", async () => { + const res = await deleteFileRoute( + req("http://localhost/api/v1/repairs/rep1/files/photos?filename=machine.jpg", { + method: "DELETE", + }), + { params: Promise.resolve({ id: "rep1", field: "photos" }) }, + ); + expect(res.status).toBe(401); + expect(removeRepairFile).not.toHaveBeenCalled(); + }); + + it("rejects a customer quote build with no session", async () => { + const res = await createQuoteRoute( + req("http://localhost/api/v1/repairs/rep1/create-customer-quote", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ items: [{ description: "Part" }] }), + }), + { params: Promise.resolve({ id: "rep1" }) }, + ); + expect(res.status).toBe(401); + expect(createCustomerQuoteFromRepair).not.toHaveBeenCalled(); + }); + + it("rejects a repair request build with no session", async () => { + const res = await generateRequestRoute( + req("http://localhost/api/v1/repairs/rep1/generate-repair-request", { + method: "POST", + }), + { params: Promise.resolve({ id: "rep1" }) }, + ); + expect(res.status).toBe(401); + expect(generateRepairRequest).not.toHaveBeenCalled(); + }); + + it("rejects the form lookups with no session", async () => { + const res = await formDataRoute(req("http://localhost/api/v1/repairs/form-data")); + expect(res.status).toBe(401); + expect(listCustomersForRepairForm).not.toHaveBeenCalled(); + expect(listSuppliersForRepairForm).not.toHaveBeenCalled(); + }); + + it("rejects the status counts with no session", async () => { + const res = await statusCountsRoute( + req("http://localhost/api/v1/repairs/status-counts"), + ); + expect(res.status).toBe(401); + expect(getRepairStatusCounts).not.toHaveBeenCalled(); }); });