From bb9bf0ba682a926b62e84d727a220ebf28e92d16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 22 Aug 2026 13:23:48 +0200 Subject: [PATCH] Refuse what these four routes used to absorb in silence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four surfaces answered a bad request by quietly substituting a good one. Each fix comes with the test that catches it, and each test was confirmed red against the old behaviour before the fix went back in. `PATCH /api/user/ai-provider` inspected its body key by key with `typeof` guards, so a numeric `model` or a boolean `baseUrl` failed every guard, produced no entry in the update, and the write went ahead without it — a 200 that says "saved" for a field that was discarded. A body of nothing but such keys came back as "No valid fields" naming none of them. The real schema now lives in `src/lib/validations/ai-provider.ts`, the route runs one `safeParse`, and a mismatch is the multi-issue 422 every sibling answers with. The accepted field set is unchanged, and the object is deliberately not strict: an unknown key has always been ignored here and still is. The one thing the schema has to carry that no other does is that this body holds plaintext credentials, so no rule may interpolate the value it rejected — noted where the next `.refine()` would go. `?slice=` on `/api/analytics` was compared against one literal and fell through to the DEFAULT body on anything else, so `?slice=summary` answered 200 after running the heaviest chain on the surface. Invisible and expensive at once. Closed enum, 422 on anything outside it. `?metricType=` and `?limit=` on `/api/personal-records` were parsed defensively, which read as robustness and behaved as silence — and in one direction it was worse than silence: dropping an unrecognised `metricType` WIDENS the read to every metric, so a typo returned more of the record than was asked for. Both refuse now, and the 500 ceiling refuses rather than clamps, because serving a different number of rows than were requested is the same quiet substitution. No caller in the tree sends a value the new rules reject. `POST /api/ingest/medication` bucketed on the client address, so a household running a chat bridge, a button and a home-automation rule — the normal deployment for this route — put all three in one 60/min cell and let the chattiest starve the rest. It already resolves a per-caller identity two lines further down and threw it away; the bucket is that token now. A request that resolves no token keeps the per-IP bucket it always had, through the shared anonymous helper, so a flood is still capped and a broken proxy chain collapses into the tight global cell rather than a shared "unknown" one. Its 429 goes through `apiError` and carries `meta` like every other refusal. Two older tests pinned the defects by name — "drops an unknown metricType silently" and "falls back to default on garbage ?limit" — and one pinned a throw where the rest of the surface returns an envelope. All three are rewritten with the reason beside them. The generated YAML is deliberately not in this commit. --- src/app/api/analytics/__tests__/route.test.ts | 46 ++++++ src/app/api/analytics/route.ts | 32 +++- .../ingest/medication/__tests__/route.test.ts | 151 +++++++++++++++++- src/app/api/ingest/medication/route.ts | 80 ++++++++-- .../personal-records/__tests__/route.test.ts | 58 +++++-- src/app/api/personal-records/route.ts | 62 +++---- .../user/ai-provider/__tests__/route.test.ts | 110 ++++++++++++- src/app/api/user/ai-provider/route.ts | 141 +++++++--------- src/lib/openapi/routes/analytics.ts | 11 +- src/lib/openapi/routes/awards.ts | 12 +- src/lib/openapi/routes/ingest.ts | 6 +- src/lib/openapi/routes/profile.ts | 90 ++--------- src/lib/validations/ai-provider.ts | 90 ++++++++++- src/lib/validations/analytics.ts | 29 ++++ src/lib/validations/measurement.ts | 19 +++ 15 files changed, 682 insertions(+), 255 deletions(-) create mode 100644 src/lib/validations/analytics.ts diff --git a/src/app/api/analytics/__tests__/route.test.ts b/src/app/api/analytics/__tests__/route.test.ts index 18c2f8552..fb423dea4 100644 --- a/src/app/api/analytics/__tests__/route.test.ts +++ b/src/app/api/analytics/__tests__/route.test.ts @@ -450,6 +450,52 @@ describe("GET /api/analytics", () => { // slice critical path). }); +/** + * `?slice=` is a closed set, and the cost of it not being one was the point. + * + * The route compared the raw string against `"summaries"` and fell through to + * the DEFAULT body on anything else, so `?slice=summary` answered 200 after + * running the heaviest chain on the surface. A client could sit on that typo + * indefinitely and see nothing but a slow, correct-looking response — the + * failure mode was invisible and expensive at the same time. + */ +describe("GET /api/analytics — the slice parameter is a closed set", () => { + const call = (url: string) => + (GET as unknown as (request: Request) => Promise)( + new Request(url), + ); + + it.each(["summary", "SUMMARIES", "default", ""])( + "refuses ?slice=%s rather than serving the expensive default body", + async (value) => { + const res = await call( + `http://localhost/api/analytics?slice=${encodeURIComponent(value)}`, + ); + expect(res.status).toBe(422); + const body = (await res.json()) as { + meta?: { errorCode?: string }; + details?: { issues?: Array<{ path: string }> }; + }; + expect(body.meta?.errorCode).toBe("analytics.invalid_query"); + expect(body.details?.issues?.map((i) => i.path)).toContain("slice"); + }, + ); + + it("still serves the default body when the parameter is absent", async () => { + const res = await call("http://localhost/api/analytics"); + expect(res.status).toBe(200); + }); + + it("still serves the slim body for the one legal value", async () => { + const res = await call("http://localhost/api/analytics?slice=summaries"); + expect(res.status).toBe(200); + const body = (await res.json()) as { data: Record }; + // The slim slice is recognisable by what it does NOT carry. + expect(body.data).not.toHaveProperty("healthScore"); + expect(body.data).toHaveProperty("summaries"); + }); +}); + /** * v1.34.5 — the blood-pressure score's basis has to ARRIVE, not merely exist. * diff --git a/src/app/api/analytics/route.ts b/src/app/api/analytics/route.ts index ea430bc31..4af449575 100644 --- a/src/app/api/analytics/route.ts +++ b/src/app/api/analytics/route.ts @@ -1,7 +1,8 @@ import { prisma } from "@/lib/db"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; -import { apiSuccess } from "@/lib/api-response"; +import { apiSuccess, returnAllZodIssues } from "@/lib/api-response"; +import { analyticsQuerySchema } from "@/lib/validations/analytics"; import { NO_STORE_BUT_BFCACHE } from "@/lib/http/cache-headers"; import { cachedSwr, caches, type ServerCache } from "@/lib/cache/server-cache"; import { summarize, type DataPoint } from "@/lib/analytics/trends"; @@ -46,13 +47,17 @@ export const dynamic = "force-dynamic"; * tests instantiate `new Request("http://localhost/api/analytics")` * and cast through). Falls back to the raw `URL(request.url)` parse * so the slim slice branch is reachable from both call shapes. + * + * Returns the object the query schema parses: an absent parameter is an + * absent KEY, not an empty string, so `slice` stays optional rather than + * failing the enum on every unparameterised call. */ -function readSliceParam(request: Request | undefined): string | null { - if (!request) return null; +function readQueryParams(request: Request | undefined): Record { + if (!request) return {}; try { - return new URL(request.url).searchParams.get("slice"); + return Object.fromEntries(new URL(request.url).searchParams); } catch { - return null; + return {}; } } @@ -60,6 +65,21 @@ export const GET = apiHandler(async (request?: Request) => { const { user } = await requireAuth(); annotate({ action: { name: "analytics.get" } }); + // A `slice` outside the closed set is refused rather than served the + // default body. Falling through was the expensive failure: `?slice=summary` + // ran the full 30-query chain and answered 200, so a client could sit on a + // typo indefinitely while paying for the heaviest read on the surface. + const parsedQuery = analyticsQuerySchema.safeParse(readQueryParams(request)); + if (!parsedQuery.success) { + annotate({ + action: { name: "analytics.get" }, + meta: { outcome: "invalid_query" }, + }); + return returnAllZodIssues(parsedQuery.error, 422, { + errorCode: "analytics.invalid_query", + }); + } + // v1.4.33 C1 — slim summaries slice for the dashboard tile strip. // The default path runs 30+ chunked findMany reads to feed // `summarize()` per type; under `?slice=summaries` the route @@ -72,7 +92,7 @@ export const GET = apiHandler(async (request?: Request) => { // behind a hover-prefetch from the Coach drawer + the correlation // tile's `` boundary. Consumers that need only the headline // tile values land on this branch and skip the heavy chain. - if (readSliceParam(request) === "summaries") { + if (parsedQuery.data.slice === "summaries") { // v1.4.34 IW-G — read-through the analytics cache keyed on // (userId, slice). The slim slice is the dashboard tile strip's hot // path; multiple dashboard mounts inside a 60-second TTL all hit a diff --git a/src/app/api/ingest/medication/__tests__/route.test.ts b/src/app/api/ingest/medication/__tests__/route.test.ts index cee53c4a1..43cba330c 100644 --- a/src/app/api/ingest/medication/__tests__/route.test.ts +++ b/src/app/api/ingest/medication/__tests__/route.test.ts @@ -32,6 +32,7 @@ vi.mock("@/lib/auth/audit", () => ({ vi.mock("@/lib/auth/hmac", () => ({ hashToken: (s: string) => `hash(${s})` })); vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn(), + checkAuthSurfaceRateLimit: vi.fn(), rateLimitHeaders: () => ({}), })); vi.mock("@/lib/app-settings", () => ({ @@ -62,16 +63,19 @@ vi.mock("next/headers", () => ({ import { POST } from "../route"; import { prisma } from "@/lib/db"; -import { checkRateLimit } from "@/lib/rate-limit"; +import { checkAuthSurfaceRateLimit, checkRateLimit } from "@/lib/rate-limit"; import { isApiGloballyEnabled } from "@/lib/app-settings"; import { invalidateUserMedications } from "@/lib/cache/invalidate"; -function postReq(body: unknown): NextRequest { +function postReq( + body: unknown, + bearer: string | null = "hlk_test_token", +): NextRequest { return new NextRequest("http://localhost/api/ingest/medication", { method: "POST", headers: { "content-type": "application/json", - authorization: "Bearer hlk_test_token", + ...(bearer === null ? {} : { authorization: `Bearer ${bearer}` }), }, body: JSON.stringify(body), }); @@ -80,6 +84,10 @@ function postReq(body: unknown): NextRequest { beforeEach(() => { vi.resetAllMocks(); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true } as never); + vi.mocked(checkAuthSurfaceRateLimit).mockResolvedValue({ + allowed: true, + ip: "203.0.113.7", + } as never); vi.mocked(isApiGloballyEnabled).mockResolvedValue(true); vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ id: "tok-1", @@ -190,6 +198,10 @@ describe("POST /api/ingest/medication — slot attribution + convergence (v1.16. function wireHappyPath() { vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true } as never); + vi.mocked(checkAuthSurfaceRateLimit).mockResolvedValue({ + allowed: true, + ip: "203.0.113.7", + } as never); vi.mocked(isApiGloballyEnabled).mockResolvedValue(true); vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ id: "tok-1", @@ -433,3 +445,136 @@ describe("POST /api/ingest/medication — idempotency-key stability floor", () = expect(prisma.medicationIntakeEvent.create).toHaveBeenCalledTimes(1); }); }); + +/** + * A bridge is identified by its TOKEN, not by the address it dials from. + * + * The bucket used to be `ingest:${ip}`, so a household running a chat bridge, + * a button and a home-automation rule — the normal deployment for this route, + * not an edge case — put all three in one 60/min cell and let the chattiest + * of them starve the other two. What makes it a defect rather than a + * trade-off is that the route already resolves a per-caller identity two + * lines further down and threw it away. + * + * The fallback matters as much as the fix: a request that resolves no token + * has no identity but its address, and must stay capped on it. Keying the + * failure path on anything shared — or on nothing — would hand an + * unauthenticated flood a free pass, or collapse every anonymous caller in + * the world into one cell, which is worse than what it replaces. + */ +describe("POST /api/ingest/medication — the rate-limit bucket", () => { + it("keys the bucket on the token, not on the client IP", async () => { + vi.mocked(prisma.medication.findFirst).mockResolvedValue(null as never); + await POST( + postReq({ medicationName: "Metformin", idempotencyKey: "k-stable-001" }), + ); + + expect(vi.mocked(checkRateLimit)).toHaveBeenCalledTimes(1); + const [key, limit, windowMs] = vi.mocked(checkRateLimit).mock.calls[0]; + expect(key).toBe("ingest:token:tok-1"); + expect(limit).toBe(60); + expect(windowMs).toBe(60_000); + // The address must not appear in the key at all — that is the whole + // point. A NAT'd sibling on the same address gets its own cell. + expect(key).not.toContain("203.0.113"); + }); + + it("gives two tokens on one address two independent buckets", async () => { + vi.mocked(prisma.medication.findFirst).mockResolvedValue(null as never); + vi.mocked(prisma.apiToken.findUnique) + .mockResolvedValueOnce({ + id: "tok-bridge", + userId: "user-1", + permissions: ["*"], + revoked: false, + expiresAt: null, + lastUsedAt: null, + } as never) + .mockResolvedValueOnce({ + id: "tok-button", + userId: "user-1", + permissions: ["*"], + revoked: false, + expiresAt: null, + lastUsedAt: null, + } as never); + + await POST(postReq({ medicationName: "A", idempotencyKey: "k-aaaa-001" })); + await POST(postReq({ medicationName: "B", idempotencyKey: "k-bbbb-001" })); + + const keys = vi.mocked(checkRateLimit).mock.calls.map(([k]) => k); + expect(keys).toEqual([ + "ingest:token:tok-bridge", + "ingest:token:tok-button", + ]); + expect(new Set(keys).size).toBe(2); + }); + + it("refuses an exhausted token bucket with the standard envelope", async () => { + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: false, + remaining: 0, + resetAt: Date.now() + 30_000, + } as never); + + const res = await POST( + postReq({ medicationName: "Metformin", idempotencyKey: "k-stable-002" }), + ); + expect(res.status).toBe(429); + const body = (await res.json()) as { + data: null; + error: string; + meta?: { errorCode?: string }; + }; + expect(body.data).toBeNull(); + // The 429 was hand-rolled through `NextResponse.json` and carried no + // `meta`, so a client branching on `errorCode` could not see it. + expect(body.meta?.errorCode).toBe("ingest.rate_limited"); + expect(prisma.medication.findFirst).not.toHaveBeenCalled(); + }); + + it("falls back to the per-IP bucket when no token is presented", async () => { + const res = await POST( + postReq( + { medicationName: "Metformin", idempotencyKey: "k-anon-0001" }, + null, + ), + ); + expect(res.status).toBe(401); + // The anonymous path must still be capped, and on the address — the only + // identity such a request has. + expect(vi.mocked(checkAuthSurfaceRateLimit)).toHaveBeenCalledTimes(1); + const [, prefix, limit, windowMs] = vi.mocked(checkAuthSurfaceRateLimit) + .mock.calls[0]; + expect(prefix).toBe("ingest:ip"); + expect(limit).toBe(60); + expect(windowMs).toBe(60_000); + // No token resolved, so no token bucket was charged. + expect(vi.mocked(checkRateLimit)).not.toHaveBeenCalled(); + }); + + it("falls back to the per-IP bucket when the token does not resolve", async () => { + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue(null as never); + const res = await POST( + postReq({ medicationName: "Metformin", idempotencyKey: "k-bogus-001" }), + ); + expect(res.status).toBe(401); + expect(vi.mocked(checkAuthSurfaceRateLimit)).toHaveBeenCalledTimes(1); + expect(vi.mocked(checkRateLimit)).not.toHaveBeenCalled(); + }); + + it("caps an invalid-token flood before it can answer 401 forever", async () => { + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue(null as never); + vi.mocked(checkAuthSurfaceRateLimit).mockResolvedValue({ + allowed: false, + remaining: 0, + resetAt: Date.now() + 30_000, + ip: "203.0.113.7", + } as never); + + const res = await POST( + postReq({ medicationName: "Metformin", idempotencyKey: "k-flood-001" }), + ); + expect(res.status).toBe(429); + }); +}); diff --git a/src/app/api/ingest/medication/route.ts b/src/app/api/ingest/medication/route.ts index 4e1845184..2d45fc4bd 100644 --- a/src/app/api/ingest/medication/route.ts +++ b/src/app/api/ingest/medication/route.ts @@ -11,7 +11,12 @@ import { sanitiseZodIssues, } from "@/lib/api-response"; import { hashToken } from "@/lib/auth/hmac"; -import { checkRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; +import { + checkAuthSurfaceRateLimit, + checkRateLimit, + rateLimitHeaders, + type RateLimitResult, +} from "@/lib/rate-limit"; import { externalIntakeSchema } from "@/lib/validations/medication"; import { unstableExternalIdMeta, @@ -26,13 +31,49 @@ import { import { consumeForIntake } from "@/lib/medications/inventory/consumption"; import { invalidateUserMedications } from "@/lib/cache/invalidate"; import { DEFAULT_TIMEZONE } from "@/lib/tz/format"; -import { NextRequest, NextResponse } from "next/server"; +import { NextRequest } from "next/server"; + +const RATE_LIMIT = 60; +const RATE_WINDOW_MS = 60 * 1000; + +/** The 429, with the standard headers and the standard error envelope. */ +function tooManyRequests(rl: RateLimitResult) { + const response = apiError("Rate limit exceeded", 429, { + errorCode: "ingest.rate_limited", + }); + for (const [key, value] of Object.entries(rateLimitHeaders(rl))) { + response.headers.set(key, value); + } + return response; +} + +/** + * Refuse a caller that presented no usable token, capped on the way out. + * + * The bucket is the caller's ADDRESS, because that is the only identity an + * unauthenticated request has. `checkAuthSurfaceRateLimit` is the shared + * helper for exactly this shape: it keys on the trusted-proxy IP normally, + * and when the proxy chain does not match the configured hop count it routes + * every anonymous caller into one tight global bucket instead of the + * free-for-all that a shared `"unknown"` cell would be. + */ +async function refuseUnauthenticated(request: NextRequest, message: string) { + const rl = await checkAuthSurfaceRateLimit( + request, + "ingest:ip", + RATE_LIMIT, + RATE_WINDOW_MS, + ); + if (!rl.allowed) return tooManyRequests(rl); + return apiError(message, 401); +} /** * External medication ingest endpoint. * Auth: Bearer token (hashed and looked up in api_tokens table). * Idempotent via idempotencyKey. - * Rate limit: 60 requests per minute per IP. + * Rate limit: 60 requests per minute per TOKEN (per client IP for a request + * that presents no valid one). */ export const POST = apiHandler(async (request: NextRequest) => { annotate({ action: { name: "ingest.medication" } }); @@ -43,19 +84,10 @@ export const POST = apiHandler(async (request: NextRequest) => { const ip = getClientIp(request); - // Rate limiting: 60 requests per minute per IP - const rl = await checkRateLimit(`ingest:${ip}`, 60, 60 * 1000); - if (!rl.allowed) { - return NextResponse.json( - { data: null, error: "Rate limit exceeded" }, - { status: 429, headers: rateLimitHeaders(rl) }, - ); - } - // Extract bearer token const authHeader = request.headers.get("authorization"); if (!authHeader?.startsWith("Bearer ")) { - return apiError("Authorization header required", 401); + return refuseUnauthenticated(request, "Authorization header required"); } const token = authHeader.slice(7); @@ -74,13 +106,31 @@ export const POST = apiHandler(async (request: NextRequest) => { }); if (!apiToken || apiToken.revoked) { - return apiError("Invalid or revoked token", 401); + return refuseUnauthenticated(request, "Invalid or revoked token"); } if (apiToken.expiresAt && apiToken.expiresAt < new Date()) { - return apiError("Token expired", 401); + return refuseUnauthenticated(request, "Token expired"); } + // A bridge is identified by its TOKEN, not by the address it dials from. + // The bucket used to be `ingest:${ip}`, so every integration behind one + // NAT shared a single 60/min cell and one chatty bridge starved the rest — + // and a household running a chat bridge, a button and a home-automation + // rule is the normal deployment here, not an edge case. + // + // The identity is only known after the lookup, so the lookup now runs + // before the limiter: one indexed single-row read ahead of what was + // already an upsert against `rate_limits`. A caller whose token does not + // resolve is still capped on the per-IP bucket it was always capped on, + // one step earlier than it reaches anything expensive. + const rl = await checkRateLimit( + `ingest:token:${apiToken.id}`, + RATE_LIMIT, + RATE_WINDOW_MS, + ); + if (!rl.allowed) return tooManyRequests(rl); + if ( !apiToken.permissions.includes("*") && !apiToken.permissions.includes("medication:ingest") diff --git a/src/app/api/personal-records/__tests__/route.test.ts b/src/app/api/personal-records/__tests__/route.test.ts index 60f6b4932..30ce136d4 100644 --- a/src/app/api/personal-records/__tests__/route.test.ts +++ b/src/app/api/personal-records/__tests__/route.test.ts @@ -82,17 +82,24 @@ describe("GET /api/personal-records", () => { }); }); - it("drops an unknown metricType silently (loose-typed filter contract)", async () => { + // Was "drops an unknown metricType silently (loose-typed filter contract)". + // That test pinned the defect rather than the contract: dropping the filter + // does not degrade gracefully, it WIDENS the read to every metric, so a + // typo returned more of the record than the caller asked for and the 200 + // said nothing about it. + it("refuses an unknown metricType instead of widening the read", async () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); const res = await GET( req("http://localhost/api/personal-records?metricType=BOGUS"), ); - expect(res.status).toBe(200); - expect(prisma.personalRecord.findMany).toHaveBeenCalledWith({ - where: { userId: "user-1" }, - orderBy: { achievedAt: "desc" }, - take: 100, - }); + expect(res.status).toBe(422); + const body = (await res.json()) as { + meta?: { errorCode?: string }; + details?: { issues?: Array<{ path: string }> }; + }; + expect(body.meta?.errorCode).toBe("personal_records.invalid_query"); + expect(body.details?.issues?.map((i) => i.path)).toContain("metricType"); + expect(prisma.personalRecord.findMany).not.toHaveBeenCalled(); }); it("honours an explicit ?limit param", async () => { @@ -108,10 +115,10 @@ describe("GET /api/personal-records", () => { }); }); - it("clamps ?limit to the project-wide ceiling of 500", async () => { + it("serves the ceiling itself", async () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); const res = await GET( - req("http://localhost/api/personal-records?limit=999999"), + req("http://localhost/api/personal-records?limit=500"), ); expect(res.status).toBe(200); expect(prisma.personalRecord.findMany).toHaveBeenCalledWith({ @@ -121,19 +128,36 @@ describe("GET /api/personal-records", () => { }); }); - it("falls back to default on garbage ?limit (defence-in-depth)", async () => { + // Was "clamps ?limit to the project-wide ceiling of 500". Silently serving + // 500 rows to a caller that asked for 999 999 is the same class of quiet + // substitution as the default-fallback below: the answer differs from the + // question and nothing says so. + it("refuses a ?limit above the ceiling instead of clamping", async () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); const res = await GET( - req("http://localhost/api/personal-records?limit=abc"), + req("http://localhost/api/personal-records?limit=999999"), ); - expect(res.status).toBe(200); - expect(prisma.personalRecord.findMany).toHaveBeenCalledWith({ - where: { userId: "user-1" }, - orderBy: { achievedAt: "desc" }, - take: 100, - }); + expect(res.status).toBe(422); + expect(prisma.personalRecord.findMany).not.toHaveBeenCalled(); }); + // Was "falls back to default on garbage ?limit (defence-in-depth)". A + // fallback that cannot be observed is not defence, it is a typo the caller + // never learns about. + it.each(["abc", "-1", "0", "2.5"])( + "refuses a garbage ?limit=%s", + async (value) => { + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + const res = await GET( + req(`http://localhost/api/personal-records?limit=${value}`), + ); + expect(res.status).toBe(422); + const body = (await res.json()) as { meta?: { errorCode?: string } }; + expect(body.meta?.errorCode).toBe("personal_records.invalid_query"); + expect(prisma.personalRecord.findMany).not.toHaveBeenCalled(); + }, + ); + it("seeded record round-trips through the response", async () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); const fakeRecord = { diff --git a/src/app/api/personal-records/route.ts b/src/app/api/personal-records/route.ts index 0900e3cf7..6b7793b1b 100644 --- a/src/app/api/personal-records/route.ts +++ b/src/app/api/personal-records/route.ts @@ -7,10 +7,18 @@ * v1.5 iOS-Swift app can build its query path against a stable * contract from day one. * - * Query params: + * Query params (validated against `listPersonalRecordsSchema`; a bad value + * is a 422, not a shrug): * - metricType: optional MeasurementType filter (e.g. ?metricType=VO2_MAX) * - limit: optional pagination cap (default 100, max 500) * + * Both were parsed defensively until this release — an unknown `metricType` + * was dropped and a garbage `limit` reverted to the default. That read as + * robustness and behaved as silence, and in one direction it was worse than + * silence: dropping the filter WIDENS the read to every metric, so a typo + * returned more rows than the caller asked for and nothing said so. The 500 + * ceiling refuses rather than clamps, for the same reason. + * * Response envelope (matches the project-wide `apiSuccess` contract): * { data: PersonalRecord[], error: null } */ @@ -18,49 +26,31 @@ import { prisma } from "@/lib/db"; import type { NextRequest } from "next/server"; import { apiHandler, requireRecordAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; -import { apiSuccess } from "@/lib/api-response"; -import { measurementTypeEnum } from "@/lib/validations/measurement"; -import type { MeasurementType } from "@/generated/prisma/client"; +import { apiSuccess, returnAllZodIssues } from "@/lib/api-response"; +import { listPersonalRecordsSchema } from "@/lib/validations/measurement"; export const dynamic = "force-dynamic"; -// v1.4.25 W10 reconcile (Sr-M4) — pagination bounds. Matches the -// `listMeasurementsSchema` ceiling (max 500) so every ingest-and-read -// endpoint on the route surface shares the same upper bound. Today -// the worker hasn't populated any rows so even an uncapped findMany is -// harmless, but a power user with multi-year Apple Health history could -// accumulate 50+ PRs per metric × 14 PR-trackable metrics once the -// worker lands. Add the cap now so the v1.5 iOS app's query path is -// bounded from day one. -const DEFAULT_LIMIT = 100; -const MAX_LIMIT = 500; - export const GET = apiHandler(async (request: NextRequest) => { const { user } = await requireRecordAuth("read", "measurements"); annotate({ action: { name: "personalRecords.list" } }); - const { searchParams } = new URL(request.url); - const metricTypeParam = searchParams.get("metricType"); - const limitParam = searchParams.get("limit"); - - // Defensive parse — drop unknown values rather than 400 so the - // caller's loosely-typed filter doesn't take the page down. - const metricType: MeasurementType | null = - metricTypeParam && measurementTypeEnum.safeParse(metricTypeParam).success - ? (metricTypeParam as MeasurementType) - : null; - - // Limit clamp: parse → clamp [1, MAX_LIMIT] → default fallback. A - // garbage value (`?limit=abc`, `?limit=-1`, `?limit=999999`) silently - // clamps to the default rather than 400-ing — same defence-in-depth - // stance the metricType parse takes. - let limit = DEFAULT_LIMIT; - if (limitParam != null) { - const parsed = Number(limitParam); - if (Number.isInteger(parsed) && parsed > 0) { - limit = Math.min(parsed, MAX_LIMIT); - } + const parsed = listPersonalRecordsSchema.safeParse( + Object.fromEntries(new URL(request.url).searchParams), + ); + if (!parsed.success) { + annotate({ + action: { name: "personalRecords.list" }, + meta: { + outcome: "validation_failed", + issue_count: parsed.error.issues.length, + }, + }); + return returnAllZodIssues(parsed.error, 422, { + errorCode: "personal_records.invalid_query", + }); } + const { metricType, limit } = parsed.data; const records = await prisma.personalRecord.findMany({ where: { diff --git a/src/app/api/user/ai-provider/__tests__/route.test.ts b/src/app/api/user/ai-provider/__tests__/route.test.ts index 62019f04d..a1421a944 100644 --- a/src/app/api/user/ai-provider/__tests__/route.test.ts +++ b/src/app/api/user/ai-provider/__tests__/route.test.ts @@ -178,10 +178,22 @@ describe("PATCH /api/user/ai-provider — v1.22 fields", () => { ); }); + // Was `.rejects.toThrow(/between 10 and 600/)`. The route threw an + // HttpError for this one field while every other bad value was skipped in + // silence; it runs one Zod parse now, so the refusal is the standard + // multi-issue 422 that every sibling route answers with. it("rejects an out-of-range response timeout", async () => { - await expect( - patch(patchRequest({ responseTimeoutSeconds: 5 })), - ).rejects.toThrow(/between 10 and 600/); + const res = await patch(patchRequest({ responseTimeoutSeconds: 5 })); + expect(res.status).toBe(422); + const body = (await res.json()) as { + meta?: { errorCode?: string }; + details?: { issues?: Array<{ path: string; message?: string }> }; + }; + expect(body.meta?.errorCode).toBe("ai_provider.invalid"); + const issue = body.details?.issues?.find( + (i) => i.path === "responseTimeoutSeconds", + ); + expect(issue?.message).toMatch(/between 10 and 600/); expect(vi.mocked(prisma.user.update)).not.toHaveBeenCalled(); }); @@ -268,6 +280,98 @@ describe("PATCH /api/user/ai-provider — the gateway's own columns (#470)", () }); }); +/** + * A wrongly-typed field must be refused, not skipped. + * + * The route inspected the body key by key with `typeof` guards. A value of + * the wrong type failed every guard and simply produced no entry in the + * update object, so the write went ahead WITHOUT it and answered 200 — the + * caller was told the save succeeded while the field they set was discarded. + * A body of nothing but such keys fell through to "No valid fields", naming + * none of them, which is the same silence with a worse message. + */ +describe("PATCH /api/user/ai-provider — wrongly-typed fields are named", () => { + const patch = PATCH as (req: Request) => Promise; + + it.each([ + ["model", 42], + ["baseUrl", true], + ["compatModel", { nested: "object" }], + ["anthropicKey", 12345], + ["openaiKey", ["array"]], + ["responseTimeoutSeconds", "120"], + ])("refuses a %s of the wrong type and names its path", async (key, bad) => { + const res = await patch(patchRequest({ [key]: bad })); + expect(res.status).toBe(422); + const body = (await res.json()) as { + meta?: { errorCode?: string }; + details?: { issues?: Array<{ path: string }> }; + }; + expect(body.meta?.errorCode).toBe("ai_provider.invalid"); + expect(body.details?.issues?.map((i) => i.path)).toContain(key); + expect(vi.mocked(prisma.user.update)).not.toHaveBeenCalled(); + }); + + it("refuses the whole write when one field of several is wrongly typed", async () => { + vi.mocked(isPublicUrl).mockReturnValue(true); + const res = await patch( + patchRequest({ provider: "OPENAI", model: 42, openaiKey: "sk-real" }), + ); + expect(res.status).toBe(422); + // Partial application would have persisted the provider and dropped the + // model, leaving the account in a state the caller never asked for. + expect(vi.mocked(prisma.user.update)).not.toHaveBeenCalled(); + }); + + it("refuses an unrecognised provider and names its path", async () => { + const res = await patch(patchRequest({ provider: "GEMINI" })); + expect(res.status).toBe(422); + const body = (await res.json()) as { + details?: { issues?: Array<{ path: string }> }; + }; + expect(body.details?.issues?.map((i) => i.path)).toContain("provider"); + }); + + it("never echoes a rejected credential back in the error body", async () => { + const res = await patch( + patchRequest({ anthropicKey: 12345, openaiKey: "sk-super-secret" }), + ); + expect(res.status).toBe(422); + expect(JSON.stringify(await res.json())).not.toContain("sk-super-secret"); + }); + + it("still answers 'No valid fields' for a body of nothing it knows", async () => { + const res = await patch(patchRequest({ somethingElse: "value" })); + expect(res.status).toBe(422); + const body = (await res.json()) as { + error: string; + meta?: { errorCode?: string }; + }; + expect(body.error).toBe("No valid fields"); + expect(body.meta?.errorCode).toBe("ai_provider.no_fields"); + expect(vi.mocked(prisma.user.update)).not.toHaveBeenCalled(); + }); + + it("keeps accepting the three-state contract on a known field", async () => { + vi.mocked(isPublicUrl).mockReturnValue(true); + await patch(patchRequest({ model: " gpt-4o ", anthropicKey: "" })); + expect(vi.mocked(prisma.user.update)).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + aiModel: "gpt-4o", + aiAnthropicKeyEncrypted: null, + }), + }), + ); + // An omitted key is untouched, not cleared. + const data = vi.mocked(prisma.user.update).mock.calls[0][0].data as Record< + string, + unknown + >; + expect(data).not.toHaveProperty("aiOpenaiKeyEncrypted"); + }); +}); + describe("GET /api/user/ai-provider — the gateway's fields (#470)", () => { it("reports the base URL and model, and the key as presence only", async () => { vi.mocked(prisma.user.findUnique).mockResolvedValue({ diff --git a/src/app/api/user/ai-provider/route.ts b/src/app/api/user/ai-provider/route.ts index 4fc45e32a..524a32d01 100644 --- a/src/app/api/user/ai-provider/route.ts +++ b/src/app/api/user/ai-provider/route.ts @@ -1,25 +1,21 @@ import { NextRequest } from "next/server"; -import { apiHandler, requireAuth, HttpError } from "@/lib/api-handler"; +import { apiHandler, requireAuth } from "@/lib/api-handler"; import { prisma } from "@/lib/db"; -import { apiSuccess, apiError, safeJson } from "@/lib/api-response"; +import { + apiSuccess, + apiError, + returnAllZodIssues, + safeJson, +} from "@/lib/api-response"; import { isPublicUrl } from "@/lib/validations/notifications"; import { isLocalAiHostAllowed } from "@/lib/ai/local-host-allowlist"; import { encrypt, decrypt } from "@/lib/crypto"; import { resolveProviderAvailability } from "@/lib/ai/provider"; import { annotate } from "@/lib/logging/context"; +import { aiProviderPatchSchema } from "@/lib/validations/ai-provider"; export const dynamic = "force-dynamic"; -const ALLOWED = new Set([ - "OPENAI", - "ANTHROPIC", - "LOCAL", - // v1.33.1 (#470) — the OpenAI-compatible gateway (LiteLLM / OpenRouter / - // vLLM). Its base URL, key and model live in their own three columns. - "OPENAI_COMPATIBLE", - "CHATGPT_OAUTH", -]); - export const GET = apiHandler(async () => { const { user } = await requireAuth(); annotate({ action: { name: "user.ai-provider.get" } }); @@ -78,39 +74,47 @@ export const PATCH = apiHandler(async (request: NextRequest) => { const { user } = await requireAuth(); annotate({ action: { name: "user.ai-provider.update" } }); - const { data: body, error } = await safeJson>( - request, - { maxBytes: 64 * 1024 }, - ); + const { data: rawBody, error } = await safeJson(request, { + maxBytes: 64 * 1024, + }); if (error) return error; + // Every accepted field states its type in the schema, so a wrongly-typed + // value is a named issue rather than a key quietly dropped on the floor. + // The multi-issue envelope is safe to return even though this body carries + // plaintext credentials: Zod's type and range messages name the received + // TYPE, never the received value. See the note in the validations module + // before adding a `.refine()` here. + const parsed = aiProviderPatchSchema.safeParse(rawBody); + if (!parsed.success) { + annotate({ + action: { name: "user.ai-provider.update" }, + meta: { + outcome: "validation_failed", + issue_count: parsed.error.issues.length, + }, + }); + return returnAllZodIssues(parsed.error, 422, { + errorCode: "ai_provider.invalid", + }); + } + const body = parsed.data; + const updates: Record = {}; if (body.provider !== undefined) { - if (body.provider === null || body.provider === "") { - updates.aiProvider = null; - } else if ( - typeof body.provider === "string" && - ALLOWED.has(body.provider) - ) { - updates.aiProvider = body.provider; - } else { - throw new HttpError(422, "Invalid provider"); - } + updates.aiProvider = + body.provider === null || body.provider === "" ? null : body.provider; } if (body.model !== undefined) { - if (body.model === null || body.model === "") { - updates.aiModel = null; - } else if (typeof body.model === "string") { - updates.aiModel = body.model.trim() || null; - } + updates.aiModel = body.model === null ? null : body.model.trim() || null; } if (body.baseUrl !== undefined) { if (body.baseUrl === null || body.baseUrl === "") { updates.aiBaseUrl = null; - } else if (typeof body.baseUrl === "string") { + } else { const trimmed = body.baseUrl.trim(); // SSRF guard: by default reject private/internal hostnames so a // compromised user account cannot point the server at the cloud @@ -137,7 +141,7 @@ export const PATCH = apiHandler(async (request: NextRequest) => { if (body.compatBaseUrl !== undefined) { if (body.compatBaseUrl === null || body.compatBaseUrl === "") { updates.aiCompatBaseUrl = null; - } else if (typeof body.compatBaseUrl === "string") { + } else { const trimmed = body.compatBaseUrl.trim(); // Same SSRF floor as the Local provider: a public host always, a // private one only when the operator allowlisted it. Gateways on a LAN @@ -154,72 +158,49 @@ export const PATCH = apiHandler(async (request: NextRequest) => { } if (body.compatModel !== undefined) { - if (body.compatModel === null || body.compatModel === "") { - updates.aiCompatModel = null; - } else if (typeof body.compatModel === "string") { - updates.aiCompatModel = body.compatModel.trim() || null; - } + updates.aiCompatModel = + body.compatModel === null ? null : body.compatModel.trim() || null; } if (body.compatKey !== undefined) { - if (body.compatKey === null || body.compatKey === "") { - updates.aiCompatKeyEncrypted = null; - } else if (typeof body.compatKey === "string") { - updates.aiCompatKeyEncrypted = encrypt(body.compatKey.trim()); - } + updates.aiCompatKeyEncrypted = + body.compatKey === null || body.compatKey === "" + ? null + : encrypt(body.compatKey.trim()); } if (body.anthropicKey !== undefined) { - if (body.anthropicKey === null || body.anthropicKey === "") { - updates.aiAnthropicKeyEncrypted = null; - } else if (typeof body.anthropicKey === "string") { - updates.aiAnthropicKeyEncrypted = encrypt(body.anthropicKey.trim()); - } + updates.aiAnthropicKeyEncrypted = + body.anthropicKey === null || body.anthropicKey === "" + ? null + : encrypt(body.anthropicKey.trim()); } if (body.localKey !== undefined) { - if (body.localKey === null || body.localKey === "") { - updates.aiLocalKeyEncrypted = null; - } else if (typeof body.localKey === "string") { - updates.aiLocalKeyEncrypted = encrypt(body.localKey.trim()); - } + updates.aiLocalKeyEncrypted = + body.localKey === null || body.localKey === "" + ? null + : encrypt(body.localKey.trim()); } if (body.openaiKey !== undefined) { - if (body.openaiKey === null || body.openaiKey === "") { - updates.aiOpenaiKeyEncrypted = null; - } else if (typeof body.openaiKey === "string") { - updates.aiOpenaiKeyEncrypted = encrypt(body.openaiKey.trim()); - } + updates.aiOpenaiKeyEncrypted = + body.openaiKey === null || body.openaiKey === "" + ? null + : encrypt(body.openaiKey.trim()); } // ── v1.22 (#89) — response timeout (seconds) ────────────────── + // The 10–600 bounds are the schema's; an out-of-range value never reaches + // here. if (body.responseTimeoutSeconds !== undefined) { - if (body.responseTimeoutSeconds === null) { - updates.aiResponseTimeoutSeconds = null; - } else if ( - typeof body.responseTimeoutSeconds === "number" && - Number.isInteger(body.responseTimeoutSeconds) - ) { - // Sane bounds: 10 s floor (a stricter value just trips the timeout) and - // 600 s ceiling (a generous cap for a slow self-hosted backend). - if ( - body.responseTimeoutSeconds < 10 || - body.responseTimeoutSeconds > 600 - ) { - throw new HttpError( - 422, - "Response timeout must be between 10 and 600 seconds", - ); - } - updates.aiResponseTimeoutSeconds = body.responseTimeoutSeconds; - } else { - throw new HttpError(422, "Invalid response timeout"); - } + updates.aiResponseTimeoutSeconds = body.responseTimeoutSeconds; } if (Object.keys(updates).length === 0) { - throw new HttpError(422, "No valid fields"); + return apiError("No valid fields", 422, { + errorCode: "ai_provider.no_fields", + }); } // When the provider switches away from LOCAL, drop any stored diff --git a/src/lib/openapi/routes/analytics.ts b/src/lib/openapi/routes/analytics.ts index 6caa1c8a9..38369fb21 100644 --- a/src/lib/openapi/routes/analytics.ts +++ b/src/lib/openapi/routes/analytics.ts @@ -27,7 +27,7 @@ import { derivedProvenance, glucoseClinicalSchema, } from "./insights/schemas"; -import { dataEnvelope, stdResponses } from "./shared"; +import { dataEnvelope, errorEnvelope, stdResponses } from "./shared"; // ── The per-type summary the tile strip renders ────────────────────── @@ -512,7 +512,7 @@ export const analyticsPaths: NonNullable = { tags: ["Analytics"], summary: "The dashboard analytics aggregate", description: - "The web dashboard's aggregate read, in two shapes chosen by `?slice=`.\n\n`?slice=summaries` returns the slim tile-strip body from two SQL passes. ANY other value — including an absent, empty or misspelt one — returns the thick default body; the parameter is matched for the exact string `summaries` and is never rejected, so `?slice=summary` silently gets the expensive path.\n\nBoth shapes are served through a stale-while-revalidate cache. A cached body can be up to a minute old, and the default slice's cache key includes the reader's locale because the correlation `interpretation` sentences are narrated per reader. `Cache-Control: private, max-age=0, must-revalidate` on both — bfcache-friendly rather than the framework's stock `no-store`.\n\nThis GET is not free of side effects, and a caller polling it should know: it records a Health Score row for the scored instant, syncs accepted correlation hypotheses into the pattern ledger (which is what mints the `patternId` the dismissal endpoint takes), and kicks a background rollup-freshness refresh. The refresh is fire-and-forget by design — awaiting it stalled the event loop for tens of seconds on large accounts — so a value can be up to about a minute behind a measurement that just landed, and the next read has it.\n\nAuth is a cookie session or a wildcard (`*`) Bearer token; a narrow-scope token is refused. NOT delegable: it resolves the caller's own record, so a switched session still reads its own analytics.", + "The web dashboard's aggregate read, in two shapes chosen by `?slice=`.\n\n`?slice=summaries` returns the slim tile-strip body from two SQL passes; omitting the parameter returns the thick default body. Any OTHER value is refused with 422 (`meta.errorCode` = `analytics.invalid_query`). It used to fall through to the default instead, which made `?slice=summary` answer 200 after running the heaviest chain on the surface — the typo was invisible and expensive at the same time.\n\nBoth shapes are served through a stale-while-revalidate cache. A cached body can be up to a minute old, and the default slice's cache key includes the reader's locale because the correlation `interpretation` sentences are narrated per reader. `Cache-Control: private, max-age=0, must-revalidate` on both — bfcache-friendly rather than the framework's stock `no-store`.\n\nThis GET is not free of side effects, and a caller polling it should know: it records a Health Score row for the scored instant, syncs accepted correlation hypotheses into the pattern ledger (which is what mints the `patternId` the dismissal endpoint takes), and kicks a background rollup-freshness refresh. The refresh is fire-and-forget by design — awaiting it stalled the event loop for tens of seconds on large accounts — so a value can be up to about a minute behind a measurement that just landed, and the next read has it.\n\nAuth is a cookie session or a wildcard (`*`) Bearer token; a narrow-scope token is refused. NOT delegable: it resolves the caller's own record, so a switched session still reads its own analytics.", parameters: [ { name: "slice", @@ -520,7 +520,7 @@ export const analyticsPaths: NonNullable = { required: false, schema: { type: "string", enum: ["summaries"] }, description: - "`summaries` selects the slim tile-strip body. Any other value falls through to the default body rather than erroring.", + "`summaries` selects the slim tile-strip body. Omit it for the default body. Any other value is a 422 — it is not treated as an omission.", }, ], responses: { @@ -537,6 +537,11 @@ export const analyticsPaths: NonNullable = { }, }, ...stdResponses, + "422": { + description: + "`slice` carried a value outside the closed set. `meta.errorCode` = `analytics.invalid_query`, with the offending path under `details.issues`.", + content: { "application/json": { schema: errorEnvelope } }, + }, }, }, }, diff --git a/src/lib/openapi/routes/awards.ts b/src/lib/openapi/routes/awards.ts index 39b33f90b..a442b6076 100644 --- a/src/lib/openapi/routes/awards.ts +++ b/src/lib/openapi/routes/awards.ts @@ -19,6 +19,7 @@ import { import { MODULE_DISABLED_DESCRIPTION, dataEnvelope, + errorEnvelope, recordRefusal, stdResponses, } from "./shared"; @@ -158,7 +159,7 @@ export const awardsPaths: NonNullable = { tags: ["Awards"], summary: "List detected personal bests", description: - "The record's personal bests, newest achievement first. Read-only — rows come from the detection worker and no endpoint creates or edits one.\n\nBoth query parameters are parsed defensively and NEVER 422: an unrecognised `metricType` is dropped (the response is then unfiltered rather than empty), and a `limit` that is not a positive integer falls back to the default rather than being rejected. A caller cannot tell a typo from a deliberate omission, so validate on your side.\n\nDelegable on a `measurements` grant.", + "The record's personal bests, newest achievement first. Read-only — rows come from the detection worker and no endpoint creates or edits one.\n\nBoth query parameters are validated and a bad value is a 422. They were forgiving until this release, and in one direction that was worse than merely quiet: dropping an unrecognised `metricType` WIDENED the read to every metric, so a typo returned more rows than the caller asked for. The `limit` ceiling refuses rather than clamps for the same reason.\n\nDelegable on a `measurements` grant.", parameters: [ { name: "metricType", @@ -166,7 +167,7 @@ export const awardsPaths: NonNullable = { required: false, schema: { type: "string" }, description: - "A `MeasurementType` to filter to. A value outside the enum is IGNORED, not refused.", + "A `MeasurementType` to filter to. A value outside the enum is refused with 422 — it is not dropped, because dropping it would widen the read rather than narrow it.", }, { name: "limit", @@ -174,7 +175,7 @@ export const awardsPaths: NonNullable = { required: false, schema: { type: "integer", minimum: 1, maximum: 500, default: 100 }, description: - "Page cap, default 100, clamped at 500. A non-integer, zero or negative value silently reverts to 100; a value above 500 clamps down.", + "Page cap, default 100, ceiling 500. A non-integer, a zero, a negative value or anything above 500 is refused with 422 rather than corrected — serving a different number of rows than were asked for is another difference nobody is told about.", }, ], responses: { @@ -191,6 +192,11 @@ export const awardsPaths: NonNullable = { }, }, ...stdResponses, + "422": { + description: + "`metricType` or `limit` carried an unacceptable value. `meta.errorCode` = `personal_records.invalid_query`, with the offending path under `details.issues`.", + content: { "application/json": { schema: errorEnvelope } }, + }, }, }, }, diff --git a/src/lib/openapi/routes/ingest.ts b/src/lib/openapi/routes/ingest.ts index d8dad6ebc..fb2cfb643 100644 --- a/src/lib/openapi/routes/ingest.ts +++ b/src/lib/openapi/routes/ingest.ts @@ -32,7 +32,7 @@ export const ingestPaths: NonNullable = { tags: ["Medications"], summary: "Record a dose from an external integration", description: - "Logs a medication intake from outside the app — a chat bridge, a home-automation rule, a button on a shelf.\n\n**Auth is Bearer only.** A cookie session does not authenticate here: the route reads the `Authorization` header itself, hashes it and looks the token up, so a browser with a valid login and no token gets 401. The token must carry either the `*` wildcard or the `medication:ingest` scope — AND, once the named medication resolves, either `*` or that medication's own `medication:{id}:ingest` scope. The second check runs after the lookup, which is why a token scoped to one medication gets 404 for a name it may not touch only when the name does not exist, and 403 when it does.\n\n**The operator can switch this off.** With the global API kill switch disabled every request is refused with 403 before the token is even read.\n\n**Replay is by `idempotencyKey`, and the status says which happened.** A key already stored for this token's owner returns the existing row with **200**; a fresh dose returns **201**. Nothing else distinguishes them — same body shape, same fields — so branch on the status code.\n\n**A dose is attributed to its scheduled slot, not stamped ad-hoc.** When the take falls inside a schedule's window band it converges onto that slot's canonical row (closing the pending reminder rather than leaving it open beside a duplicate ad-hoc take); otherwise it lands as a standalone row with `scheduledFor = takenAt`. Only a genuine pending-to-taken transition consumes inventory, so a re-post that converges onto an already-taken slot does not decrement stock twice.\n\nRate-limited to 60 requests per minute **per client IP**, not per token — several bridges behind one NAT share the bucket. The limit is applied before the token is read, so an unauthenticated flood is capped too.", + "Logs a medication intake from outside the app — a chat bridge, a home-automation rule, a button on a shelf.\n\n**Auth is Bearer only.** A cookie session does not authenticate here: the route reads the `Authorization` header itself, hashes it and looks the token up, so a browser with a valid login and no token gets 401. The token must carry either the `*` wildcard or the `medication:ingest` scope — AND, once the named medication resolves, either `*` or that medication's own `medication:{id}:ingest` scope. The second check runs after the lookup, which is why a token scoped to one medication gets 404 for a name it may not touch only when the name does not exist, and 403 when it does.\n\n**The operator can switch this off.** With the global API kill switch disabled every request is refused with 403 before the token is even read.\n\n**Replay is by `idempotencyKey`, and the status says which happened.** A key already stored for this token's owner returns the existing row with **200**; a fresh dose returns **201**. Nothing else distinguishes them — same body shape, same fields — so branch on the status code.\n\n**A dose is attributed to its scheduled slot, not stamped ad-hoc.** When the take falls inside a schedule's window band it converges onto that slot's canonical row (closing the pending reminder rather than leaving it open beside a duplicate ad-hoc take); otherwise it lands as a standalone row with `scheduledFor = takenAt`. Only a genuine pending-to-taken transition consumes inventory, so a re-post that converges onto an already-taken slot does not decrement stock twice.\n\nRate-limited to 60 requests per minute **per token**. A household running a chat bridge, a button and a home-automation rule gets three independent allowances rather than three shares of one, which is what the previous per-IP bucket gave them. A request that presents no valid token falls back to a per-IP bucket with the same ceiling, so an unauthenticated flood is still capped.", requestBody: { required: true, content: { "application/json": { schema: externalIntakeRequest } }, @@ -64,7 +64,7 @@ export const ingestPaths: NonNullable = { ...stdResponses, "401": { description: - "No `Authorization: Bearer …` header, or a token that is unknown, revoked or past its expiry. The three cases are not distinguished on the wire.", + "No `Authorization: Bearer …` header, or a token that is unknown, revoked or past its expiry. The three cases are not distinguished on the wire, and all four are charged to the per-IP bucket rather than to any token.", content: { "application/json": { schema: errorEnvelope } }, }, "403": { @@ -92,7 +92,7 @@ export const ingestPaths: NonNullable = { }, "429": { description: - "More than 60 requests in a minute from this client IP. Carries the standard rate-limit headers. Note the body is the bare `{ data: null, error }` pair without `meta`.", + "More than 60 requests in a minute for this token — or, when no valid token was presented, from this client IP. `meta.errorCode` = `ingest.rate_limited`, plus the standard `X-RateLimit-Remaining` / `X-RateLimit-Reset` headers.", content: { "application/json": { schema: errorEnvelope } }, }, }, diff --git a/src/lib/openapi/routes/profile.ts b/src/lib/openapi/routes/profile.ts index a8c96953b..45455bc1c 100644 --- a/src/lib/openapi/routes/profile.ts +++ b/src/lib/openapi/routes/profile.ts @@ -15,7 +15,7 @@ import { MODULE_KEYS } from "@/lib/modules/registry"; import { SCORE_PILLAR_IDS } from "@/lib/analytics/score/types"; import { savedReportProfileSchema } from "@/lib/report-selection/profile-shape"; import { - AI_PROVIDER_KINDS, + aiProviderPatchSchema, aiTestOverrideSchema, } from "@/lib/validations/ai-provider"; import { @@ -518,81 +518,15 @@ const aiProviderResponse = z "The calling user's AI-provider configuration plus the effective availability (`aiAvailable` + `managedBy`). Reports key presence + a masked preview only — never a plaintext key.", }); -// The PATCH body is documented here rather than imported from a validations -// module because there is nothing to import: the route hand-rolls its parse -// field by field instead of running a Zod `safeParse`, so the shape below is -// read off that code. Two consequences a client needs to be told, because no -// generated schema would carry them: a key of the wrong TYPE (a number -// `model`, a boolean `baseUrl`) is silently ignored rather than refused, and -// a body containing only such keys fails as "No valid fields" rather than -// naming the offender. -const aiProviderPatchRequest = z - .object({ - provider: z - .enum(AI_PROVIDER_KINDS) - .nullable() - .optional() - .describe( - 'The provider to select. `null` or `""` clears it. Any other unrecognised value is a 422 — this is the one field whose bad value is refused rather than skipped.', - ), - model: z - .string() - .nullable() - .optional() - .describe('Trimmed; `null`, `""` or whitespace-only clears it.'), - baseUrl: z - .string() - .nullable() - .optional() - .describe( - 'Custom base URL for the LOCAL provider. `null` or `""` clears it. A non-empty value runs the SSRF floor: a private or internal host is refused with 422 unless the operator allowlisted that exact host. NOTE this column is shared — switching `provider` away from LOCAL in the same request clears it automatically, so a stored cloud key can never be sent to a host configured for something else.', - ), - compatBaseUrl: z - .string() - .nullable() - .optional() - .describe( - "Base URL of the OpenAI-compatible gateway. Its own column, deliberately not shared with `baseUrl`. Same SSRF floor.", - ), - compatModel: z.string().nullable().optional(), - compatKey: z - .string() - .nullable() - .optional() - .describe( - 'Bearer for the gateway; encrypted at rest. `null` or `""` clears it. Never echoed back — the read reports presence only.', - ), - anthropicKey: z - .string() - .nullable() - .optional() - .describe('Encrypted at rest. `null` or `""` clears it.'), - localKey: z - .string() - .nullable() - .optional() - .describe('Encrypted at rest. `null` or `""` clears it.'), - openaiKey: z - .string() - .nullable() - .optional() - .describe('Encrypted at rest. `null` or `""` clears it.'), - responseTimeoutSeconds: z - .number() - .int() - .min(10) - .max(600) - .nullable() - .optional() - .describe( - "Per-user AI response timeout in seconds, 10–600. `null` restores the built-in default (~120 s). A number outside the range, or a non-integer, is a 422.", - ), - }) - .meta({ - id: "AiProviderPatchRequest", - description: - "Partial update of the calling user's AI-provider configuration. An omitted key is untouched; an explicit `null` (or empty string, on the string fields) clears the column. At least one recognised key must be present. Key material is write-only: it is encrypted on the way in and never returned.", - }); +// The PATCH body is the runtime validator, imported rather than restated. +// It used to be described here instead, because the route hand-rolled its +// parse field by field and there was nothing to import; the route runs +// `safeParse` now, so the published shape and the parse are one object. +const aiProviderPatchRequest = aiProviderPatchSchema.meta({ + id: "AiProviderPatchRequest", + description: + "Partial update of the calling user's AI-provider configuration. An omitted key is left untouched; `null` (or an empty string, on the string fields) clears the column. At least one recognised key must be present, or the write is refused. Key material is write-only: it is encrypted on the way in and never returned. Unknown keys are ignored rather than refused — the object is deliberately not strict, so a client sending a field this server does not know yet is not broken by it.", +}); const aiTestRequest = aiTestOverrideSchema.meta({ id: "AiTestRequest", @@ -767,7 +701,7 @@ export const profilePaths: NonNullable = { tags: ["Auth"], summary: "Update the calling user's AI-provider configuration", description: - "Partial update — an omitted key is left untouched, an explicit `null` (or an empty string, on the string fields) clears the column. Key material is write-only: it is encrypted at rest on the way in and the read never gives it back, only presence plus a four-character preview.\n\nOne implicit write is worth knowing about: switching `provider` to anything other than LOCAL, without naming `baseUrl` in the same request, CLEARS the stored base URL. The column is shared across providers, so leaving it would send a cloud key to a host that was configured for a self-hosted backend.\n\nA base URL — for LOCAL or for the OpenAI-compatible gateway — runs the SSRF floor: a private or internal host is refused unless the operator allowlisted that exact hostname on this instance.\n\nThe response is a bare `{ updated: true }` acknowledgement. It does not echo the resolved configuration, so a client that needs the new state re-reads the GET.\n\nUnlike its neighbours this route does not run a Zod parse: it inspects the body field by field, so a key of the WRONG TYPE is skipped in silence rather than refused, and a body of nothing but such keys comes back as 422 'No valid fields' without naming any of them.", + "Partial update — an omitted key is left untouched, an explicit `null` (or an empty string, on the string fields) clears the column. Key material is write-only: it is encrypted at rest on the way in and the read never gives it back, only presence plus a four-character preview.\n\nOne implicit write is worth knowing about: switching `provider` to anything other than LOCAL, without naming `baseUrl` in the same request, CLEARS the stored base URL. The column is shared across providers, so leaving it would send a cloud key to a host that was configured for a self-hosted backend.\n\nA base URL — for LOCAL or for the OpenAI-compatible gateway — runs the SSRF floor: a private or internal host is refused unless the operator allowlisted that exact hostname on this instance.\n\nThe response is a bare `{ updated: true }` acknowledgement. It does not echo the resolved configuration, so a client that needs the new state re-reads the GET.\n\nA wrongly-typed value is refused, not skipped. Until this release the route inspected the body field by field with `typeof` guards, so a numeric `model` or a boolean `baseUrl` was dropped in silence and the write went ahead without it; a body of nothing but such keys came back as 'No valid fields' naming none of them. It runs the standard Zod parse now and answers the multi-issue 422 with the offending path.", requestBody: { required: true, content: { "application/json": { schema: aiProviderPatchRequest } }, @@ -796,7 +730,7 @@ export const profilePaths: NonNullable = { ...stdResponses, "422": { description: - "Nothing was written. Either no recognised field was present ('No valid fields'), or `provider` named a value outside the five, or `responseTimeoutSeconds` was not an integer in 10–600, or a base URL pointed at an internal or private host the operator has not allowlisted. Single-message — there is no per-issue list on this route.", + "Nothing was written. A body that failed validation carries the multi-issue list under `details.issues` with `meta.errorCode` = `ai_provider.invalid` — a wrongly-typed field, a `provider` outside the five, or a `responseTimeoutSeconds` that is not an integer in 10–600 all land there and name their path. Two single-message cases stay outside that list because they are not shape failures: no recognised field was present at all (`meta.errorCode` = `ai_provider.no_fields`), and a base URL pointing at an internal or private host the operator has not allowlisted.", content: { "application/json": { schema: errorEnvelope } }, }, }, diff --git a/src/lib/validations/ai-provider.ts b/src/lib/validations/ai-provider.ts index 7126a398b..7b51bcffe 100644 --- a/src/lib/validations/ai-provider.ts +++ b/src/lib/validations/ai-provider.ts @@ -1,14 +1,18 @@ /** - * Request schema for `POST /api/ai/test` — the AI connection probe. + * Request schemas for the AI-provider surfaces: the configuration write + * (`PATCH /api/user/ai-provider`) and the connection probe + * (`POST /api/ai/test`). * - * Lives outside the route file so the OpenAPI registry can import it (a route - * module may only export handlers plus the Next.js route config), which keeps - * the published contract and the runtime parser one object. + * They live outside the route files so the OpenAPI registry can import them (a + * route module may only export handlers plus the Next.js route config), which + * keeps the published contract and the runtime parser one object. * - * The body carries UNSAVED credentials on purpose: the settings surface tests - * a selection before it is persisted. Nothing here is written to the user row - * — `resolveProviderForTest` layers the override over the stored config for - * the duration of the one probe and drops it. + * Both bodies carry plaintext credentials, which puts one constraint on every + * rule below: a validation message must never interpolate the value it + * rejected. Zod's built-in type, enum and range messages name the received + * TYPE and never the received value, so the multi-issue 422 these feed is safe + * to return. A `.refine()` with a message that quotes its input would break + * that, and would put a key in an error body. */ import { z } from "zod/v4"; @@ -23,6 +27,76 @@ export const AI_PROVIDER_KINDS = [ export const aiProviderKindSchema = z.enum(AI_PROVIDER_KINDS); +/** + * A field the caller may set, clear, or leave alone. + * + * Three states, and the difference matters on every column here: an OMITTED + * key leaves the stored value untouched, `null` or `""` clears it, and a + * non-empty string replaces it. The empty string is not a synonym for "no + * change" anywhere on this surface — the settings forms send `null` and `""` + * interchangeably to mean "clear", and both have always cleared. + */ +const clearableString = z.string().nullable(); + +/** + * `PATCH /api/user/ai-provider` — a partial update of the provider config. + * + * The route used to inspect the body key by key with `typeof` guards, which + * meant a wrongly-typed value (a numeric `model`, a boolean `baseUrl`) was + * SKIPPED rather than refused: the write went ahead without it, and a body of + * nothing but such keys came back as "No valid fields" without naming one. + * This schema is what closes that — every accepted field now states its type, + * and a mismatch is a named issue in the standard multi-issue 422. + * + * The accepted field set is exactly what the route already accepted; this + * describes the existing contract rather than changing it. In particular the + * object is deliberately NOT `.strict()`: an unrecognised key has always been + * ignored here, and refusing one now would break a client sending a field this + * server does not know yet. Unknown keys are dropped, as before. + */ +export const aiProviderPatchSchema = z.object({ + /** + * The selected provider. `null` / `""` clears it. Unlike every other field + * this one has always REFUSED an unrecognised value rather than skipping it, + * and still does. + */ + provider: z + .union([aiProviderKindSchema, z.literal("")]) + .nullable() + .optional(), + /** Trimmed on write; whitespace-only clears, like `""`. */ + model: clearableString.optional(), + /** + * Custom base URL for the LOCAL provider. Trimmed, then run through the + * SSRF floor at the route — a private host is refused there, not here, + * because the allowlist is operator state rather than a shape rule. + */ + baseUrl: clearableString.optional(), + /** The OpenAI-compatible gateway's own base URL. Same SSRF floor. */ + compatBaseUrl: clearableString.optional(), + compatModel: clearableString.optional(), + /** Encrypted at rest on write; never echoed back by the read. */ + compatKey: clearableString.optional(), + anthropicKey: clearableString.optional(), + localKey: clearableString.optional(), + openaiKey: clearableString.optional(), + /** + * Per-user AI response timeout in seconds. `null` restores the built-in + * default. The 10 s floor is below what any real backend beats and the + * 600 s ceiling is generous for a slow self-hosted one; outside that range + * the value is refused rather than clamped, so a typo is visible. + */ + responseTimeoutSeconds: z + .number() + .int() + .min(10, "Response timeout must be between 10 and 600 seconds") + .max(600, "Response timeout must be between 10 and 600 seconds") + .nullable() + .optional(), +}); + +export type AiProviderPatchInput = z.infer; + export const aiTestOverrideSchema = z .object({ provider: aiProviderKindSchema.optional().nullable(), diff --git a/src/lib/validations/analytics.ts b/src/lib/validations/analytics.ts new file mode 100644 index 000000000..b714d7bc1 --- /dev/null +++ b/src/lib/validations/analytics.ts @@ -0,0 +1,29 @@ +/** + * Query validation for `GET /api/analytics`. + * + * One parameter, one legal value, and the point of writing it down is that + * the route used to compare the raw string against `"summaries"` and fall + * through to the thick default body on anything else. `?slice=summary` then + * ran the whole 30-query chain and answered as if nothing were wrong, which + * is the most expensive way this surface can absorb a typo. + * + * Lives outside the route file so the OpenAPI registry can import it (a route + * module may only export handlers plus the Next.js route config). + */ +import { z } from "zod/v4"; + +/** The slices `GET /api/analytics` knows how to serve. */ +export const ANALYTICS_SLICES = ["summaries"] as const; + +export const analyticsSliceEnum = z.enum(ANALYTICS_SLICES); + +export const analyticsQuerySchema = z.object({ + /** + * `summaries` selects the slim tile-strip body. Omitted selects the thick + * default body. Any other value is refused — falling through to the + * default would hide the typo behind the more expensive read. + */ + slice: analyticsSliceEnum.optional(), +}); + +export type AnalyticsQuery = z.infer; diff --git a/src/lib/validations/measurement.ts b/src/lib/validations/measurement.ts index 3cd8ac1bd..104043716 100644 --- a/src/lib/validations/measurement.ts +++ b/src/lib/validations/measurement.ts @@ -725,6 +725,25 @@ export const updateMeasurementSchema = z.object({ .optional(), }); +/** + * Query params for `GET /api/personal-records`. + * + * Both fields used to be parsed defensively at the route: an unrecognised + * `metricType` was dropped (which WIDENED the read to every metric rather + * than narrowing it, so a typo returned more data than asked for) and a + * garbage `limit` reverted to the default. Neither told the caller anything, + * so a typo was indistinguishable from an omission. Both are refused now. + * + * The 500 ceiling matches every other paginated read on the surface, and it + * refuses rather than clamps for the same reason: silently serving 500 rows + * to a caller that asked for 999 999 is another difference nobody is told + * about. + */ +export const listPersonalRecordsSchema = z.object({ + metricType: measurementTypeEnum.optional(), + limit: z.coerce.number().int().min(1).max(500).optional().default(100), +}); + export const listMeasurementsSchema = z .object({ type: measurementTypeEnum.optional(),