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(),