diff --git a/apps/server/src/routes/rpc/adapter.test.ts b/apps/server/src/routes/rpc/adapter.test.ts index 7ca83d40..72e7e134 100644 --- a/apps/server/src/routes/rpc/adapter.test.ts +++ b/apps/server/src/routes/rpc/adapter.test.ts @@ -97,6 +97,16 @@ describe("toConnectError", () => { const err = captureThrow(() => toConnectError(original)); expect(err).toBe(original); }); + + // Unclassified errors propagate raw so `errorInterceptor` — the only + // layer holding the request id — can log and redact them in one place. + // The redaction itself is covered in `interceptors/__tests__/error.test.ts`. + test("rethrows an unclassified error untouched", () => { + const drizzleish = new Error("Failed query: insert into ..."); + const err = captureThrow(() => toConnectError(drizzleish)); + expect(err).toBe(drizzleish); + expect(err).not.toBeInstanceOf(ConnectError); + }); }); /** diff --git a/apps/server/src/routes/rpc/adapter.ts b/apps/server/src/routes/rpc/adapter.ts index 5ce3da0e..9e364a93 100644 --- a/apps/server/src/routes/rpc/adapter.ts +++ b/apps/server/src/routes/rpc/adapter.ts @@ -29,7 +29,8 @@ export function toServiceCtx(rpcCtx: RpcContext): ServiceContext { * Map any error thrown by a service call to a `ConnectError`. Preserves the * existing Connect error surface — granular reasons carried by the caller's * per-handler error helpers (in `errors.ts`) still bypass this mapper since - * they throw `ConnectError` directly. + * they throw `ConnectError` directly. Errors it can't classify propagate + * untouched to `errorInterceptor`, which logs and redacts them. */ export function toConnectError(err: unknown): never { if (err instanceof ConnectError) throw err; @@ -59,6 +60,8 @@ export function toConnectError(err: unknown): never { throw new ConnectError(err.message, Code.Internal); } } - const message = err instanceof Error ? err.message : "Unknown error"; - throw new ConnectError(message, Code.Internal); + // Unclassified: rethrow raw so `errorInterceptor` handles it. Only the + // interceptor holds the `RpcContext`, so it's the only layer that can put + // the request id in both the log line and the client's message. + throw err; } diff --git a/apps/server/src/routes/rpc/interceptors/__tests__/error.test.ts b/apps/server/src/routes/rpc/interceptors/__tests__/error.test.ts new file mode 100644 index 00000000..8997a44f --- /dev/null +++ b/apps/server/src/routes/rpc/interceptors/__tests__/error.test.ts @@ -0,0 +1,87 @@ +import { Code, ConnectError, type Interceptor } from "@connectrpc/connect"; +import { describe, expect, test } from "@openstatus/test-utils"; + +import { RPC_CONTEXT_KEY } from "../auth"; +import { errorInterceptor } from "../error"; + +type NextFn = Parameters[0]; +type RpcRequest = Parameters[0]; + +function mockNextReject(error: unknown): NextFn { + return (() => Promise.reject(error)) as unknown as NextFn; +} + +function createMockRequest(opts?: { requestId?: string }): RpcRequest { + const contextValues = new Map(); + if (opts?.requestId) { + contextValues.set(RPC_CONTEXT_KEY, { requestId: opts.requestId }); + } + return { + service: { typeName: "openstatus.status_page.v1.StatusPageService" }, + method: { name: "AddMonitorComponent" }, + message: {}, + header: new Headers(), + contextValues: { get: (key: unknown) => contextValues.get(key) }, + } as unknown as RpcRequest; +} + +async function captureReject(fn: () => Promise): Promise { + try { + await fn(); + } catch (err) { + return err; + } + throw new Error("expected fn to reject, but it resolved"); +} + +// A drizzle `DrizzleQueryError.message` is the rendered SQL plus the bound +// params — the exact shape that leaked to a Terraform user via the RPC API. +const DRIZZLE_ERROR = new Error( + 'Failed query: insert into "page_component" ("id", "workspace_id", "page_id") ' + + 'values (null, ?, ?) returning "id"\nparams: 13049,5354,monitor', +); + +describe("errorInterceptor", () => { + test("redacts an unclassified error to an opaque Internal", async () => { + const err = (await captureReject(() => + errorInterceptor()(mockNextReject(DRIZZLE_ERROR))( + createMockRequest({ requestId: "req-1" }), + ), + )) as ConnectError; + + expect(err).toBeInstanceOf(ConnectError); + expect(err.code).toBe(Code.Internal); + expect(err.rawMessage).not.toContain("page_component"); + expect(err.rawMessage).not.toContain("params:"); + expect(err.rawMessage).not.toContain("insert into"); + }); + + test("carries the request id so logs and client share a correlation id", async () => { + const err = (await captureReject(() => + errorInterceptor()(mockNextReject(DRIZZLE_ERROR))( + createMockRequest({ requestId: "req-correlate-me" }), + ), + )) as ConnectError; + + expect(err.rawMessage).toContain("req-correlate-me"); + }); + + test("falls back to a bare message when there is no RPC context", async () => { + const err = (await captureReject(() => + errorInterceptor()(mockNextReject(DRIZZLE_ERROR))(createMockRequest()), + )) as ConnectError; + + expect(err.rawMessage).toBe("Internal server error"); + }); + + test("passes an existing ConnectError through unchanged", async () => { + const original = new ConnectError("Monitor not found", Code.NotFound); + const err = await captureReject(() => + errorInterceptor()(mockNextReject(original))( + createMockRequest({ requestId: "req-2" }), + ), + ); + + expect(err).toBe(original); + }); +}); diff --git a/apps/server/src/routes/rpc/interceptors/error.ts b/apps/server/src/routes/rpc/interceptors/error.ts index 82e44817..85a270a8 100644 --- a/apps/server/src/routes/rpc/interceptors/error.ts +++ b/apps/server/src/routes/rpc/interceptors/error.ts @@ -23,6 +23,20 @@ const ERROR_CODE_MAP: Record = { INTERNAL_SERVER_ERROR: Code.Internal, }; +/** + * Opaque `Internal` error for anything we didn't classify. The request id is + * the only detail that crosses the wire — it's the handle support needs to + * find the real cause in the logs. + */ +export function internalError(requestId?: string): ConnectError { + return new ConnectError( + requestId + ? `Internal server error (request id: ${requestId})` + : "Internal server error", + Code.Internal, + ); +} + /** * Error mapping interceptor for ConnectRPC. * Converts OpenStatusApiError to ConnectError with appropriate codes. @@ -68,10 +82,10 @@ export function errorInterceptor(): Interceptor { requestId: rpcCtx?.requestId, }); - throw new ConnectError( - error instanceof Error ? error.message : "Internal server error", - Code.Internal, - ); + // Never forward the raw message: drizzle's `DrizzleQueryError` embeds + // the full SQL and bound params, which would leak schema and other + // rows' ids to the API client. + throw internalError(rpcCtx?.requestId); } }; }