From 6a399f9045ee49ccbf8a3bd9b1622a5a1f0703df Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 16:21:59 +0000 Subject: [PATCH 1/3] fix(api): unify auth 401 envelope and cap document list offset (audit H1, H3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two self-contained API-contract-hygiene fixes from the 2026-07-14 audit (Wave H), each offline-verifiable with no schema or provider dependency. H1 (S4/S5) — unify the JSON error envelope for auth failures. `unauthorizedResponse` returned a bare `{ error }` 401 without the stable `code`/`message` every other API failure carries. It now routes through the shared `jsonError` helper, so a 401 body is `{ error, message, code: "authentication_required" }`. `jsonError` gains an optional `{ log }` flag (default true); the auth helper passes `log: false` so a routine unauthenticated request is not recorded as a server-side error. H3 (S7) — cap the document list `offset` at 10_000 (matching the jobs list) instead of 1_000_000, so a deep-offset request cannot force PostgREST to skip through a million rows as a slow-query/DoS lever. `queryInteger` clamps, so oversized offsets are pinned to the cap. Tests: extend tests/http-error-response.test.ts (auth envelope shape + `log` opt-out suppresses the error log without changing the payload) and tests/api-validation-contract.test.ts (offset above 10k clamps to 10k, range pinned). Verification: focused vitest for the touched surfaces (29) plus the auth/route-adjacent suites (155) pass; typecheck + eslint + prettier clean on all changed files. (The repo-wide `verify:cheap` typecheck fails only on pre-existing missing optional test dev deps — @testing-library/*, @axe-core/playwright — absent in this container and unrelated to this change.) Deferred from Wave H with reasons: H2 (admin-route rate limits — ripples into many test mocks that would 503 without a rate-limit row), H4 (upload Content-Length admission — undici/proxy requests legitimately omit Content-Length, so a hard 411 would break chunked uploads and the existing tests), H5 (authed-summarize public scope — retrieval path, gated on the live golden eval). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_011Zbpyexer9cLgxhrB61RCU --- src/app/api/documents/route.ts | 4 ++- src/lib/http.ts | 6 ++-- src/lib/supabase/auth.ts | 13 +++++---- tests/api-validation-contract.test.ts | 13 +++++++++ tests/http-error-response.test.ts | 42 ++++++++++++++++++++++++++- 5 files changed, 68 insertions(+), 10 deletions(-) diff --git a/src/app/api/documents/route.ts b/src/app/api/documents/route.ts index cb3350718..5524e7b7b 100644 --- a/src/app/api/documents/route.ts +++ b/src/app/api/documents/route.ts @@ -111,7 +111,9 @@ function projectPublicFields>(row: T, columns: const documentListQuerySchema = z.object({ limit: queryInteger({ fallback: 100, min: 1, max: 200 }), - offset: queryInteger({ fallback: 0, min: 0, max: 1_000_000 }), + // Cap the offset at 10k (matching the jobs list) so a deep-offset request cannot force + // PostgREST to skip through a million rows as a slow-query/DoS lever. `queryInteger` clamps. + offset: queryInteger({ fallback: 0, min: 0, max: 10_000 }), q: z.string().optional().default("").transform(safeSearchTerm), status: z .string() diff --git a/src/lib/http.ts b/src/lib/http.ts index 82ee6613c..7daeaa6b8 100644 --- a/src/lib/http.ts +++ b/src/lib/http.ts @@ -55,12 +55,14 @@ function logSafeError(error: unknown, status: number) { }); } -export function jsonError(error: unknown, status = 500) { +export function jsonError(error: unknown, status = 500, options?: { log?: boolean }) { const responseStatus = error instanceof PublicApiError ? error.status : status; const message = publicErrorMessage(error, responseStatus); const code = publicErrorCode(error, responseStatus); const requestId = error instanceof PublicApiError ? error.details?.requestId : undefined; - logSafeError(error, responseStatus); + // Expected client-auth failures (e.g. an unauthenticated request) can opt out of the + // error-level log so a routine 401 does not read as a server fault in the logs. + if (options?.log ?? true) logSafeError(error, responseStatus); return NextResponse.json( { error: message, diff --git a/src/lib/supabase/auth.ts b/src/lib/supabase/auth.ts index 49b6bee29..d27d31a9f 100644 --- a/src/lib/supabase/auth.ts +++ b/src/lib/supabase/auth.ts @@ -1,7 +1,7 @@ import { createServerClient, parseCookieHeader } from "@supabase/ssr"; -import { NextResponse } from "next/server"; -import { createAdminClient } from "@/lib/supabase/admin"; import { env } from "@/lib/env"; +import { jsonError } from "@/lib/http"; +import { createAdminClient } from "@/lib/supabase/admin"; type AdminClient = ReturnType; @@ -78,10 +78,11 @@ export class AuthenticationError extends Error { export function unauthorizedResponse(error?: AuthenticationError) { void error; - return NextResponse.json( - { error: "Authentication required." }, - { status: 401, headers: { "Cache-Control": "private, no-store" } }, - ); + // Route through the shared error envelope so 401s carry a stable `code`/`message` + // like every other API failure (and inherit its `Cache-Control: private, no-store`). + // `log: false` keeps a routine unauthenticated request from being recorded as a + // server-side error. + return jsonError(new AuthenticationError(), 401, { log: false }); } /** diff --git a/tests/api-validation-contract.test.ts b/tests/api-validation-contract.test.ts index 41743422c..5caff8e53 100644 --- a/tests/api-validation-contract.test.ts +++ b/tests/api-validation-contract.test.ts @@ -288,6 +288,19 @@ describe("API validation contracts", () => { expect(client.calls[1].range).toEqual({ from: 0, to: 99 }); }); + it("caps the document list offset at 10k so a deep-offset request cannot force a full scan", async () => { + const client = createSupabaseMock(() => ok([], 0)); + mockRuntime(client); + const { GET } = await import("../src/app/api/documents/route"); + + const response = await GET(authenticatedRequest("/api/documents?limit=200&offset=5000000&includeMeta=false")); + const body = await payload(response); + + expect(response.status).toBe(200); + expect(body.pagination).toMatchObject({ limit: 200, offset: 10_000 }); + expect(client.calls[0].range).toEqual({ from: 10_000, to: 10_199 }); + }); + it("treats empty document-detail chunk as absent and clamps page/chunk windows", async () => { const client = createSupabaseMock((call) => { if (call.table === "documents" && call.maybeSingle) { diff --git a/tests/http-error-response.test.ts b/tests/http-error-response.test.ts index 5c544d600..e42f58261 100644 --- a/tests/http-error-response.test.ts +++ b/tests/http-error-response.test.ts @@ -1,5 +1,7 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { jsonError, PublicApiError } from "../src/lib/http"; +import { logger } from "../src/lib/logger"; +import { unauthorizedResponse } from "../src/lib/supabase/auth"; describe("jsonError public payload", () => { it("keeps public error payloads stable without exposing stack or internal causes", async () => { @@ -84,3 +86,41 @@ describe("jsonError public payload", () => { expect(emptyBody.code).toBe("request_failed"); }); }); + +describe("jsonError logging opt-out", () => { + afterEach(() => vi.restoreAllMocks()); + + it("logs by default but stays silent when log is disabled, without changing the payload", async () => { + const errorSpy = vi.spyOn(logger, "error").mockImplementation(() => {}); + + const logged = await jsonError(new PublicApiError("nope", 500, { code: "boom" })).json(); + expect(errorSpy).toHaveBeenCalledTimes(1); + + errorSpy.mockClear(); + const silent = await jsonError(new PublicApiError("nope", 500, { code: "boom" }), 500, { log: false }).json(); + expect(errorSpy).not.toHaveBeenCalled(); + expect(silent).toEqual(logged); + }); +}); + +describe("unauthorizedResponse envelope", () => { + afterEach(() => vi.restoreAllMocks()); + + it("returns the shared error envelope with a stable authentication_required code", async () => { + const response = unauthorizedResponse(); + const body = await response.json(); + + expect(response.status).toBe(401); + expect(body).toEqual({ + error: "Authentication required.", + message: "Authentication required.", + code: "authentication_required", + }); + }); + + it("does not record a routine unauthenticated request as a server error", () => { + const errorSpy = vi.spyOn(logger, "error").mockImplementation(() => {}); + unauthorizedResponse(); + expect(errorSpy).not.toHaveBeenCalled(); + }); +}); From 1ad80196576e5ce9432475298e29bdcb3d7ef57c Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:32:31 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=93=9D=20CodeRabbit=20Chat:=20Add=20G?= =?UTF-8?q?enerated=20Unit=20Tests=20for=20PR=20Changes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/api-validation-contract.test.ts | 35 ++++++++++++ tests/http-error-response.test.ts | 77 ++++++++++++++++++++++++++- 2 files changed, 111 insertions(+), 1 deletion(-) diff --git a/tests/api-validation-contract.test.ts b/tests/api-validation-contract.test.ts index 5caff8e53..ca4f248a6 100644 --- a/tests/api-validation-contract.test.ts +++ b/tests/api-validation-contract.test.ts @@ -301,6 +301,41 @@ describe("API validation contracts", () => { expect(client.calls[0].range).toEqual({ from: 10_000, to: 10_199 }); }); + it("passes through a document list offset at or under the 10k cap unchanged", async () => { + const client = createSupabaseMock(() => ok([], 0)); + mockRuntime(client); + const { GET } = await import("../src/app/api/documents/route"); + + const underCapResponse = await GET( + authenticatedRequest("/api/documents?limit=50&offset=9999&includeMeta=false"), + ); + const underCapBody = await payload(underCapResponse); + const atCapResponse = await GET( + authenticatedRequest("/api/documents?limit=50&offset=10000&includeMeta=false"), + ); + const atCapBody = await payload(atCapResponse); + + expect(underCapResponse.status).toBe(200); + expect(underCapBody.pagination).toMatchObject({ limit: 50, offset: 9999 }); + expect(client.calls[0].range).toEqual({ from: 9999, to: 10_048 }); + expect(atCapResponse.status).toBe(200); + expect(atCapBody.pagination).toMatchObject({ limit: 50, offset: 10_000 }); + expect(client.calls[1].range).toEqual({ from: 10_000, to: 10_049 }); + }); + + it("clamps a document list offset that is just past the 10k cap down to exactly 10k", async () => { + const client = createSupabaseMock(() => ok([], 0)); + mockRuntime(client); + const { GET } = await import("../src/app/api/documents/route"); + + const response = await GET(authenticatedRequest("/api/documents?limit=50&offset=10001&includeMeta=false")); + const body = await payload(response); + + expect(response.status).toBe(200); + expect(body.pagination).toMatchObject({ limit: 50, offset: 10_000 }); + expect(client.calls[0].range).toEqual({ from: 10_000, to: 10_049 }); + }); + it("treats empty document-detail chunk as absent and clamps page/chunk windows", async () => { const client = createSupabaseMock((call) => { if (call.table === "documents" && call.maybeSingle) { diff --git a/tests/http-error-response.test.ts b/tests/http-error-response.test.ts index e42f58261..61da5cc9c 100644 --- a/tests/http-error-response.test.ts +++ b/tests/http-error-response.test.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { jsonError, PublicApiError } from "../src/lib/http"; import { logger } from "../src/lib/logger"; -import { unauthorizedResponse } from "../src/lib/supabase/auth"; +import { AuthenticationError, unauthorizedResponse } from "../src/lib/supabase/auth"; describe("jsonError public payload", () => { it("keeps public error payloads stable without exposing stack or internal causes", async () => { @@ -101,6 +101,57 @@ describe("jsonError logging opt-out", () => { expect(errorSpy).not.toHaveBeenCalled(); expect(silent).toEqual(logged); }); + + it("still logs when an options object is passed without an explicit log key", async () => { + const errorSpy = vi.spyOn(logger, "error").mockImplementation(() => {}); + + jsonError(new PublicApiError("nope", 500, { code: "boom" }), 500, {}); + + expect(errorSpy).toHaveBeenCalledTimes(1); + }); + + it("logs when log is explicitly enabled", async () => { + const errorSpy = vi.spyOn(logger, "error").mockImplementation(() => {}); + + jsonError(new PublicApiError("nope", 500, { code: "boom" }), 500, { log: true }); + + expect(errorSpy).toHaveBeenCalledTimes(1); + }); + + it("suppresses logging for a plain (non-PublicApiError) error when log is disabled", async () => { + const errorSpy = vi.spyOn(logger, "error").mockImplementation(() => {}); + + const response = jsonError(new Error("boom"), 503, { log: false }); + const body = await response.json(); + + expect(errorSpy).not.toHaveBeenCalled(); + expect(response.status).toBe(503); + expect(body).toEqual({ + error: "Request failed.", + message: "Request failed.", + code: "internal_error", + }); + }); + + it("does not affect the response status, headers, or requestId when logging is disabled", async () => { + vi.spyOn(logger, "error").mockImplementation(() => {}); + + const error = new PublicApiError("Search failed safely.", 503, { + code: "search_unavailable", + requestId: "req_456", + }); + const response = jsonError(error, 500, { log: false }); + const body = await response.json(); + + expect(response.status).toBe(503); + expect(response.headers.get("Cache-Control")).toBe("private, no-store"); + expect(body).toEqual({ + error: "Search failed safely.", + message: "Search failed safely.", + code: "search_unavailable", + requestId: "req_456", + }); + }); }); describe("unauthorizedResponse envelope", () => { @@ -123,4 +174,28 @@ describe("unauthorizedResponse envelope", () => { unauthorizedResponse(); expect(errorSpy).not.toHaveBeenCalled(); }); + + it("ignores a caller-supplied AuthenticationError and always returns the generic message", async () => { + const response = unauthorizedResponse(new AuthenticationError("Session expired for user 12345")); + const body = await response.json(); + + expect(response.status).toBe(401); + expect(body).toEqual({ + error: "Authentication required.", + message: "Authentication required.", + code: "authentication_required", + }); + }); + + it("sets a private, no-store Cache-Control header so a 401 is never cached or shared", () => { + const response = unauthorizedResponse(); + + expect(response.headers.get("Cache-Control")).toBe("private, no-store"); + }); + + it("does not log even when a caller-supplied AuthenticationError is provided", () => { + const errorSpy = vi.spyOn(logger, "error").mockImplementation(() => {}); + unauthorizedResponse(new AuthenticationError("some internal detail")); + expect(errorSpy).not.toHaveBeenCalled(); + }); }); From 7aff93f698f4def9e4818281660acc03728a36c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 17:35:50 +0000 Subject: [PATCH 3/3] style: prettier-format the generated document-offset tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit's auto-generated unit tests (commit 1ad8019) added valuable edge-case coverage for the H3 offset cap and the H1 jsonError `log` flag, but tests/api-validation-contract.test.ts was not prettier-formatted, failing the CI format:check gate. Formatting only — no test logic changed; all 35 tests in the two touched files pass. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_011Zbpyexer9cLgxhrB61RCU --- tests/api-validation-contract.test.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/api-validation-contract.test.ts b/tests/api-validation-contract.test.ts index ca4f248a6..e5251971f 100644 --- a/tests/api-validation-contract.test.ts +++ b/tests/api-validation-contract.test.ts @@ -306,13 +306,9 @@ describe("API validation contracts", () => { mockRuntime(client); const { GET } = await import("../src/app/api/documents/route"); - const underCapResponse = await GET( - authenticatedRequest("/api/documents?limit=50&offset=9999&includeMeta=false"), - ); + const underCapResponse = await GET(authenticatedRequest("/api/documents?limit=50&offset=9999&includeMeta=false")); const underCapBody = await payload(underCapResponse); - const atCapResponse = await GET( - authenticatedRequest("/api/documents?limit=50&offset=10000&includeMeta=false"), - ); + const atCapResponse = await GET(authenticatedRequest("/api/documents?limit=50&offset=10000&includeMeta=false")); const atCapBody = await payload(atCapResponse); expect(underCapResponse.status).toBe(200);