diff --git a/apps/aggregator/src/routes/xrpc/router.ts b/apps/aggregator/src/routes/xrpc/router.ts index b5cf5bb28e..4c3f219fe7 100644 --- a/apps/aggregator/src/routes/xrpc/router.ts +++ b/apps/aggregator/src/routes/xrpc/router.ts @@ -76,13 +76,13 @@ function applyCorsHeaders(headers: Headers): void { } /** - * Generic 500 for an unexpected pre-dispatch failure (e.g. a D1 error resolving - * the labeler policy). Logs the internal error for operators but returns the - * router's own opaque envelope, so no internal detail (SQL text, stack) reaches - * the client. Matches the body the router itself produces for a handler throw. + * Generic 500 for an unexpected failure (e.g. a D1 error). Logs the internal + * error under `context` for operators but returns the router's own opaque + * envelope, so no internal detail (SQL text, stack) reaches the client. + * Matches the body the router itself produces for a handler throw. */ -function internalErrorResponse(err: unknown): Response { - console.error("[aggregator] xrpc policy resolution failed", { +function internalErrorResponse(err: unknown, context: string): Response { + console.error(`[aggregator] xrpc ${context} failed`, { error: err instanceof Error ? (err.stack ?? err.message) : String(err), }); return new InternalServerError({ @@ -90,6 +90,22 @@ function internalErrorResponse(err: unknown): Response { }).toResponse(); } +/** + * Wrap an unexpected dispatch failure as an error Response carrying CORS + + * `no-store`. An unwrapped throw escapes to workerd's bare 500, dropping both + * — and on the cacheable `sync.getRecord` path it would leave a takedown- + * relevant error under a public cache header. `XRPCError` keeps its typed + * envelope (e.g. a malformed accept-labelers header); anything else is opaque. + */ +function wrapDispatchError(err: unknown, context: string): Response { + const errorResponse = + err instanceof XRPCError ? err.toResponse() : internalErrorResponse(err, context); + const headers = new Headers(errorResponse.headers); + headers.set("cache-control", NO_STORE); + applyCorsHeaders(headers); + return new Response(errorResponse.body, { status: errorResponse.status, headers }); +} + /** * Dispatch any `/xrpc/*` request. Returns null when the path isn't an * XRPC route (caller falls through to other route matching). @@ -108,7 +124,12 @@ export async function handleXrpc(env: Env, request: Request): Promise { }); }); +describe("sync.getRecord — unexpected blob-fetch failure", () => { + // A D1 error inside the CAR passthrough (here: `publishers` is gone) must + // take the CORS + `no-store` wrapper rather than escape to workerd's bare + // 500 — and must NOT inherit the cacheable success header. Capture the + // table's schema so the shared beforeEach (`DELETE FROM publishers`) keeps + // working after a test drops it. + let publishersSchema: string; + beforeAll(async () => { + const row = await testEnv.DB.prepare( + "SELECT sql FROM sqlite_master WHERE type='table' AND name='publishers'", + ).first<{ sql: string }>(); + publishersSchema = row!.sql; + }); + afterEach(async () => { + const exists = await testEnv.DB.prepare( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name='publishers'", + ).first(); + if (!exists) await testEnv.DB.prepare(publishersSchema).run(); + }); + + it("wraps a D1 failure in a 500 carrying CORS + no-store and no leaked internals", async () => { + // The publisher-profile blob read runs `SELECT record_blob FROM publishers`; + // dropping the table makes that throw a non-XRPC D1 error. + await testEnv.DB.prepare("DROP TABLE publishers").run(); + + const res = await SELF.fetch( + `https://test/xrpc/com.atproto.sync.getRecord?did=${DID_A}&collection=${NSID.publisherProfile}&rkey=self`, + ); + expect(res.status).toBe(500); + expect(res.headers.get("cache-control")).toBe("private, no-store"); + expect(res.headers.get("access-control-allow-origin")).toBe("*"); + const body = (await res.json()) as { error: string; message?: string }; + expect(body.error).toBe("InternalServerError"); + // No internal detail (SQL text, table name, stack) leaks to the client. + expect(JSON.stringify(body)).not.toMatch(/publishers|no such table|SQL|SELECT/i); + }); +}); + describe("getPublisherVerification", () => { it("returns the verification claims naming a DID as subject, newest first", async () => { await seedVerification({