From fe412933ead4e10d01b21a9ec0feed10abd845ef Mon Sep 17 00:00:00 2001 From: Flea Flicker <22+gb_flea@noreply.git.farh.net> Date: Tue, 9 Jun 2026 06:17:42 +0000 Subject: [PATCH] GRO-2294: Route Optimization security hardening (geocode-batch limit cap + redact settings secret) (#193) --- .mcp.json | 11 +++ UAT_PLAYBOOK.md | 5 +- src/__tests__/geocodeBatchLimit.test.ts | 89 ++++++++++++++++++++++++ src/__tests__/settings.test.ts | 91 +++++++++++++++++++++++++ src/routes/clients.ts | 11 ++- src/routes/settings.ts | 16 ++++- trigger-uat-1779751324.txt | 0 7 files changed, 219 insertions(+), 4 deletions(-) create mode 100644 .mcp.json create mode 100644 src/__tests__/geocodeBatchLimit.test.ts create mode 100644 src/__tests__/settings.test.ts create mode 100644 trigger-uat-1779751324.txt diff --git a/.mcp.json b/.mcp.json new file mode 100644 index 0000000..6efc1ca --- /dev/null +++ b/.mcp.json @@ -0,0 +1,11 @@ +{ + "mcpServers": { + "gitea": { + "type": "http", + "url": "https://git-mcp.farh.net/mcp", + "headers": { + "Authorization": "Bearer ${GITEA_TOKEN}" + } + } + } +} diff --git a/UAT_PLAYBOOK.md b/UAT_PLAYBOOK.md index cf0a541..48082de 100644 --- a/UAT_PLAYBOOK.md +++ b/UAT_PLAYBOOK.md @@ -133,6 +133,7 @@ Geocoding turns a client's street address into `latitude`/`longitude` + `geocode | TC-API-2.11 | Geocode endpoint is manager-only | As **groomer** or **receptionist**, `POST /api/clients/{id}/geocode` | 403 Forbidden (role not permitted) | | TC-API-2.12 | Batch geocode un-geocoded clients | As manager, `POST /api/clients/geocode-batch?limit=10` on a DB with un-geocoded clients | 200 OK; body `{ provider, processed, geocoded, unresolved, errors, remaining, outcomes[] }`. `processed` ≤ 10; `remaining` reflects un-geocoded clients beyond this batch. Re-run while `remaining > 0` to finish (throttled to provider rate limit) | | TC-API-2.13 | Batch geocode — invalid limit | As manager, `POST /api/clients/geocode-batch?limit=0` (or non-numeric) | 400 `{ error: "limit must be a positive integer" }` | +| TC-API-2.13a | Batch geocode — `?limit` cap enforced (GRO-2294) | As manager, `POST /api/clients/geocode-batch?limit=100000` on a DB with un-geocoded clients | 200 OK; the request is **clamped to the documented max of 500** — `processed` ≤ 500 (never the raw 100000). A fractional `?limit` (e.g. `49.9`) is floored to `49`. Confirms a manager cannot hold one synchronous request open / accrue unbounded Google API cost via an oversized limit | | TC-API-2.14 | Batch geocode — manager-only | As groomer/receptionist, `POST /api/clients/geocode-batch` | 403 Forbidden | | TC-API-2.15 | Auto-geocode on create | As manager/receptionist, `POST /api/clients` with a valid `address` | 201 Created; response includes a `geocoding` object (`status: "geocoded"` for a resolvable address) and the persisted client carries `latitude`/`longitude`/`geocodedAt`. Creating without an address succeeds with no `geocoding` field | | TC-API-2.16 | Auto-geocode on address update | As manager/receptionist, `PATCH /api/clients/{id}` changing `address` to a new valid value | 200 OK; response includes a `geocoding` object and refreshed coordinates. Patching unrelated fields (e.g. `name`) does NOT re-geocode (no `geocoding` field) | @@ -165,6 +166,8 @@ Geocoding turns a client's street address into `latitude`/`longitude` + `geocode | 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.19d | Get pet profile summary — owner-bypass writes audit row (GRO-2063) | Same setup as TC-API-3.19a (sign in as `uat-customer@groombook.dev`, establish a portal session for the customer's own clientId, call `GET /api/pets/{ownPetId}/profile-summary` with `X-Impersonation-Session-Id: {sessionId}` and a 200 OK response). Then call `GET /api/impersonation/sessions/{sessionId}/audit-log` and confirm there is exactly one entry with `action === "read_profile_summary"`, `pageVisited` matching the profile-summary path, and `metadata` containing `petId` and `actorStaffId` for the customer. Repeat TC-API-3.19b (cross-tenant attempt) and confirm NO new `read_profile_summary` row was written for the cross-tenant attempt. | 200 OK on the profile-summary call AND an audit log entry is present with the correct shape (defense-in-depth audit row; bypass attempts against other clients must NOT log) | +| TC-UAT-2 | Groomer accesses linked pet profile summary (GRO-2100) | Sign in as `uat-groomer@groombook.dev`; `GET /api/pets/c0000001-0000-0000-0000-000000000002/profile-summary` (UAT Pup Alpha — linked via deterministic completed appointment `a0000001-0000-0000-0000-000000000001`, service `b0000001-…-0001` "Bath & Brush", `startTime` ~7 days ago) | 200 OK, `recentGroomingHistory[]` non-empty (>=1 entry), `visitCount >= 1`, `upcomingAppointment` null (the seeded appointment is in the past) | +| TC-UAT-3 | Groomer blocked from unlinked pet profile summary (GRO-2100) | Sign in as `uat-groomer@groombook.dev`; `GET /api/pets/c0000001-0000-0000-0000-000000000003/profile-summary` (UAT Pup Beta — intentionally UNLINKED; no appointment row references this pet's clientId+groomerId combo) | 403 Forbidden (RBAC `groomer` role lacks the appointment-linkage grant for this pet). NOTE: if 404 is returned instead of 403, file a separate RBAC defect (not against the seed) — see GRO-2100 verification note | | 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"}` | @@ -329,7 +332,7 @@ This means: | # | Scenario | Steps | Expected | |---|----------|-------|----------| -| TC-API-13.1 | Get business settings | GET /api/admin/settings | 200 OK, business settings returned | +| TC-API-13.1 | Get business settings | GET /api/admin/settings | 200 OK, business settings returned. Response body **must NOT include `googleMapsApiKey`** — the encrypted secret is redacted from the projection (GRO-2294, defense-in-depth); non-secret fields (`businessName`, colors, `routeOptimizationProvider`, etc.) are still present | | TC-API-13.2 | Update business settings | PATCH /api/admin/settings with updated values | 200 OK, settings updated | | TC-API-13.3 | Upload logo | POST /api/admin/settings/logo/upload with file | 200 OK, logo uploaded and stored | | TC-API-13.4 | View logo | GET /api/admin/settings/logo | 200 OK, logo image returned | diff --git a/src/__tests__/geocodeBatchLimit.test.ts b/src/__tests__/geocodeBatchLimit.test.ts new file mode 100644 index 0000000..8731c02 --- /dev/null +++ b/src/__tests__/geocodeBatchLimit.test.ts @@ -0,0 +1,89 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { Hono } from "hono"; + +// ─── Mocks ────────────────────────────────────────────────────────────────── +// GRO-2294: the POST /clients/geocode-batch handler must clamp ?limit to the +// documented maximum (500) before invoking the geocoding service. We mock the +// service to capture the exact limit the route forwards. + +const geocodeUngeocodedClients = vi.fn(async () => ({ + totalRemaining: 0, + processed: 0, + geocoded: 0, + failed: 0, + remaining: 0, +})); + +vi.mock("../services/clientGeocoding.js", () => ({ + geocodeUngeocodedClients, + geocodeClient: vi.fn(), + resolveClientGeocodingProvider: vi.fn(), +})); + +vi.mock("@groombook/db", () => { + const tableProxy = (name: string) => + new Proxy( + { _name: name }, + { get: (_t, p) => (p === "_name" ? name : { table: name, column: p }) } + ); + return { + getDb: () => ({}), + clients: tableProxy("clients"), + appointments: tableProxy("appointments"), + and: vi.fn(), + eq: vi.fn(), + or: vi.fn(), + exists: vi.fn(), + }; +}); + +const { clientsRouter } = await import("../routes/clients.js"); + +const app = new Hono(); +app.route("/clients", clientsRouter); + +function postBatch(query: string) { + return app.request(`/clients/geocode-batch${query}`, { method: "POST" }); +} + +describe("POST /clients/geocode-batch — ?limit cap (GRO-2294)", () => { + beforeEach(() => { + geocodeUngeocodedClients.mockClear(); + }); + + it("defaults to 50 when no ?limit is supplied", async () => { + const res = await postBatch(""); + expect(res.status).toBe(200); + expect(geocodeUngeocodedClients).toHaveBeenCalledWith(expect.anything(), 50); + }); + + it("passes through a value within the cap", async () => { + const res = await postBatch("?limit=120"); + expect(res.status).toBe(200); + expect(geocodeUngeocodedClients).toHaveBeenCalledWith(expect.anything(), 120); + }); + + it("clamps an over-cap value to 500", async () => { + const res = await postBatch("?limit=100000"); + expect(res.status).toBe(200); + expect(geocodeUngeocodedClients).toHaveBeenCalledWith(expect.anything(), 500); + }); + + it("floors a fractional value before clamping", async () => { + const res = await postBatch("?limit=49.9"); + expect(res.status).toBe(200); + expect(geocodeUngeocodedClients).toHaveBeenCalledWith(expect.anything(), 49); + }); + + it("rejects a non-positive limit with 400", async () => { + const res = await postBatch("?limit=0"); + expect(res.status).toBe(400); + expect(geocodeUngeocodedClients).not.toHaveBeenCalled(); + }); + + it("rejects a non-numeric limit with 400", async () => { + const res = await postBatch("?limit=abc"); + expect(res.status).toBe(400); + expect(geocodeUngeocodedClients).not.toHaveBeenCalled(); + }); +}); diff --git a/src/__tests__/settings.test.ts b/src/__tests__/settings.test.ts new file mode 100644 index 0000000..c878999 --- /dev/null +++ b/src/__tests__/settings.test.ts @@ -0,0 +1,91 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { Hono } from "hono"; + +// ─── Mocks ────────────────────────────────────────────────────────────────── +// GRO-2294: GET /api/admin/settings must not return the encrypted +// googleMapsApiKey ciphertext, on either the existing-row or auto-create branch. + +let selectRows: Record[] = []; +let insertReturning: Record[] = []; + +function makeChainable(data: unknown[]): unknown { + const arr = [...data]; + const chain = new Proxy(arr, { + get(target, prop) { + if (prop === "where" || prop === "orderBy" || prop === "limit") { + return () => chain; + } + // @ts-expect-error proxy passthrough + return target[prop]; + }, + }); + return chain; +} + +vi.mock("@groombook/db", () => { + const businessSettings = new Proxy( + { _name: "business_settings" }, + { get: (_t, p) => (p === "_name" ? "business_settings" : { column: p }) } + ); + return { + getDb: () => ({ + select: () => ({ from: () => makeChainable(selectRows) }), + insert: () => ({ + values: () => ({ returning: () => insertReturning }), + }), + }), + businessSettings, + eq: vi.fn(), + }; +}); + +vi.mock("../lib/s3.js", () => ({ + getPresignedUploadUrl: vi.fn(), + deleteObject: vi.fn(), + putObject: vi.fn(), + getObject: vi.fn(), +})); + +const { settingsRouter } = await import("../routes/settings.js"); + +const app = new Hono(); +app.route("/settings", settingsRouter); + +const FULL_ROW = { + id: "settings-uuid-1", + businessName: "GroomBook", + primaryColor: "#4f8a6f", + accentColor: "#8b7355", + routeOptimizationProvider: "google", + googleMapsApiKey: "ENCRYPTED::super-secret-ciphertext", + createdAt: new Date(), + updatedAt: new Date(), +}; + +describe("GET /settings — googleMapsApiKey redaction (GRO-2294)", () => { + beforeEach(() => { + selectRows = []; + insertReturning = []; + }); + + it("omits googleMapsApiKey from an existing settings row", async () => { + selectRows = [{ ...FULL_ROW }]; + const res = await app.request("/settings", { method: "GET" }); + expect(res.status).toBe(200); + const body = (await res.json()) as Record; + expect(body).not.toHaveProperty("googleMapsApiKey"); + // Non-secret fields are still returned. + expect(body.businessName).toBe("GroomBook"); + expect(body.routeOptimizationProvider).toBe("google"); + }); + + it("omits googleMapsApiKey from the auto-create branch", async () => { + selectRows = []; + insertReturning = [{ ...FULL_ROW, id: "settings-uuid-new" }]; + const res = await app.request("/settings", { method: "GET" }); + expect(res.status).toBe(200); + const body = (await res.json()) as Record; + expect(body).not.toHaveProperty("googleMapsApiKey"); + expect(body.id).toBe("settings-uuid-new"); + }); +}); diff --git a/src/routes/clients.ts b/src/routes/clients.ts index e7ac65c..328ed31 100644 --- a/src/routes/clients.ts +++ b/src/routes/clients.ts @@ -12,6 +12,12 @@ import { export const clientsRouter = new Hono(); +// Batch-geocode bounds (GRO-2294): default 50, hard cap 500. The cap bounds how +// long one synchronous request stays open and the per-request external API cost +// when routeOptimizationProvider = "google". +const GEOCODE_BATCH_DEFAULT_LIMIT = 50; +const GEOCODE_BATCH_MAX_LIMIT = 500; + type ClientRow = typeof clients.$inferSelect; /** @@ -185,12 +191,15 @@ clientsRouter.post("/:clientId/geocode", async (c) => { clientsRouter.post("/geocode-batch", async (c) => { const db = getDb(); const limitRaw = c.req.query("limit"); - let limit = 50; + let limit = GEOCODE_BATCH_DEFAULT_LIMIT; if (limitRaw !== undefined) { limit = Number(limitRaw); if (!Number.isFinite(limit) || limit <= 0) { return c.json({ error: "limit must be a positive integer" }, 400); } + // Clamp to the documented maximum to bound synchronous request duration + // and (for the Google provider) per-request external API cost. + limit = Math.min(Math.floor(limit), GEOCODE_BATCH_MAX_LIMIT); } const summary = await geocodeUngeocodedClients(db, limit); return c.json(summary); diff --git a/src/routes/settings.ts b/src/routes/settings.ts index 3b931db..8529135 100644 --- a/src/routes/settings.ts +++ b/src/routes/settings.ts @@ -7,6 +7,17 @@ import { requireSuperUser } from "../middleware/rbac.js"; export const settingsRouter = new Hono(); +type BusinessSettingsRow = typeof businessSettings.$inferSelect; + +// Strip the encrypted googleMapsApiKey ciphertext from settings responses +// (GRO-2294, defense-in-depth). The secret is never needed client-side; it is +// only written via the dedicated provider-config endpoint. +function redactSettings(row: BusinessSettingsRow) { + const rest: Partial = { ...row }; + delete rest.googleMapsApiKey; + return rest; +} + // GET /api/admin/settings — return current business settings settingsRouter.get("/", async (c) => { const db = getDb(); @@ -14,9 +25,10 @@ settingsRouter.get("/", async (c) => { if (!row) { // Auto-create default settings if none exist const [created] = await db.insert(businessSettings).values({}).returning(); - return c.json(created); + if (!created) throw new Error("Failed to create default settings"); + return c.json(redactSettings(created)); } - return c.json(row); + return c.json(redactSettings(row)); }); const hexColorRegex = /^#[0-9a-fA-F]{6}$/; diff --git a/trigger-uat-1779751324.txt b/trigger-uat-1779751324.txt new file mode 100644 index 0000000..e69de29