diff --git a/.server-changes/sanitize-auth-failure-messages.md b/.server-changes/sanitize-auth-failure-messages.md new file mode 100644 index 0000000000..a29d08a4d2 --- /dev/null +++ b/.server-changes/sanitize-auth-failure-messages.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: fix +--- + +Stop API auth failures from leaking the auth controller's raw error string (e.g. internal DB connection errors) to clients; return a fixed status-derived message instead. diff --git a/apps/webapp/app/services/routeBuilders/apiBuilder.server.ts b/apps/webapp/app/services/routeBuilders/apiBuilder.server.ts index 0c661a7e68..425905c9cf 100644 --- a/apps/webapp/app/services/routeBuilders/apiBuilder.server.ts +++ b/apps/webapp/app/services/routeBuilders/apiBuilder.server.ts @@ -4,6 +4,7 @@ import { ActionFunctionArgs, json, LoaderFunctionArgs } from "@remix-run/server- import { fromZodError } from "zod-validation-error"; import { apiCors } from "~/utils/apiCors"; import { logger } from "../logger.server"; +import { sanitizeAuthFailure } from "./publicAuthError"; import { rbac } from "../rbac.server"; import type { RbacAbility, RbacResource } from "@trigger.dev/rbac"; import { @@ -64,7 +65,14 @@ async function authenticateRequestForApiBuilder( // Plugin auth distinguishes 401 (who are you?) from 403 (you're not // allowed) — e.g. a suspended account or IP block returns 403. // Forwarding the status preserves that semantic for client retry logic. - return { ok: false, status: result.status, error: result.error }; + // + // Never forward the controller's `error` string: a controller can + // conflate an infra failure with an auth rejection (e.g. an + // unreachable DB throws a Prisma error carrying the prod RDS hostname, + // which the plugin returns as the auth error). Log it server-side and + // return a fixed status-derived message instead. See sanitizeAuthFailure. + logger.warn("API bearer auth failed", { status: result.status, error: result.error }); + return { ok: false, ...sanitizeAuthFailure(result) }; } // Plugins return the full AuthenticatedEnvironment shape directly — no @@ -554,9 +562,14 @@ export function createLoaderPATApiRoute< const ctx = contextFn ? await contextFn(parsedParams, request) : {}; const patAuth = await rbac.authenticatePat(request, ctx); if (!patAuth.ok) { + // Same provenance rule as bearer auth: never forward the + // controller's raw `error` string to the client. Log it + // server-side, return a fixed status-derived message. + logger.warn("API PAT auth failed", { status: patAuth.status, error: patAuth.error }); + const safe = sanitizeAuthFailure(patAuth); return await wrapResponse( request, - json({ error: patAuth.error }, { status: patAuth.status }), + json({ error: safe.error }, { status: safe.status }), corsStrategy !== "none" ); } diff --git a/apps/webapp/app/services/routeBuilders/publicAuthError.ts b/apps/webapp/app/services/routeBuilders/publicAuthError.ts new file mode 100644 index 0000000000..eced7800a0 --- /dev/null +++ b/apps/webapp/app/services/routeBuilders/publicAuthError.ts @@ -0,0 +1,39 @@ +/** + * Client-safe auth-failure messages. + * + * Auth controllers (the OSS RBAC fallback and the cloud RBAC plugin) return + * an `error` string on failure. That string is NOT safe to forward to the + * client: a controller can conflate an infrastructure failure with an auth + * rejection — e.g. when the database is unreachable the plugin's key lookup + * throws a Prisma error ("Can't reach database server at ") which it returns as the auth `error`. The apiBuilder then put + * that string into the response body, and the SDK surfaced it verbatim in + * the customer's run view via `TriggerApiError`, leaking internal infra + * detail. + * + * The client only ever needs to know *that* auth failed and *which kind* + * (401 = who are you / 403 = not allowed) so its retry logic can branch. + * It never needs the controller's prose. So we derive the message purely + * from the status and drop the controller's string (logged server-side at + * the call site). This makes leakage impossible regardless of what any + * current or future controller returns — there is no path by which a raw + * internal string reaches the client. + */ +export type AuthFailureStatus = 401 | 403; + +export function publicAuthError(status: AuthFailureStatus): string { + return status === 403 ? "Forbidden" : "Invalid credentials"; +} + +/** + * Replace a controller auth failure's `error` with the status-derived + * client-safe message, preserving the status. The original `error` is + * intentionally discarded here — callers log it server-side before + * sanitizing so full detail is retained in logs. + */ +export function sanitizeAuthFailure(failure: { status: AuthFailureStatus; error: string }): { + status: AuthFailureStatus; + error: string; +} { + return { status: failure.status, error: publicAuthError(failure.status) }; +} diff --git a/apps/webapp/test/api-auth.e2e.test.ts b/apps/webapp/test/api-auth.e2e.test.ts index 31e365d6d4..0dd3d1e6f7 100644 --- a/apps/webapp/test/api-auth.e2e.test.ts +++ b/apps/webapp/test/api-auth.e2e.test.ts @@ -68,6 +68,21 @@ describe("API bearer auth — baseline behavior", () => { const body = await res.json(); expect(body).toHaveProperty("error"); }); + + it("401 body carries only a fixed status-derived message, not the controller's string", async () => { + // The auth controller's own `error` string must never reach the client — + // a controller can conflate an infra failure with an auth rejection and + // leak internal detail (e.g. a Prisma "Can't reach database server" error + // carrying the prod RDS hostname). The body is derived purely from status. + // `/api/v1/runs` uses the apiBuilder/RBAC bridge (createLoaderApiRoute), + // which is the path that surfaced the prod leak via trigger(). + const res = await server.webapp.fetch("/api/v1/runs", { + headers: { Authorization: "Bearer tr_dev_completely_invalid_key_xyz_not_real" }, + }); + const body = (await res.json()) as { error?: string }; + expect(res.status).toBe(401); + expect(body.error).toBe("Invalid credentials"); + }); }); describe("JWT bearer auth — baseline behavior", () => { diff --git a/apps/webapp/test/publicAuthError.test.ts b/apps/webapp/test/publicAuthError.test.ts new file mode 100644 index 0000000000..5a2e884689 --- /dev/null +++ b/apps/webapp/test/publicAuthError.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from "vitest"; +import { publicAuthError, sanitizeAuthFailure } from "../app/services/routeBuilders/publicAuthError.js"; + +describe("publicAuthError", () => { + it("returns a fixed client-safe message for 401", () => { + expect(publicAuthError(401)).toBe("Invalid credentials"); + }); + + it("returns a fixed client-safe message for 403", () => { + expect(publicAuthError(403)).toBe("Forbidden"); + }); +}); + +describe("sanitizeAuthFailure", () => { + it("preserves the status so client retry logic still works", () => { + expect(sanitizeAuthFailure({ status: 401, error: "Invalid API key" }).status).toBe(401); + expect(sanitizeAuthFailure({ status: 403, error: "Unauthorized" }).status).toBe(403); + }); + + it("never echoes the controller's raw error string", () => { + // The exact production leak: the RBAC plugin conflated an infra failure + // with an auth rejection and returned the raw Prisma message — carrying + // the prod RDS hostname — as its `error` string. The SDK then surfaced + // it verbatim in the customer's run view via TriggerApiError. + const leaked = + "Invalid `prisma.project.findFirst()` invocation:\n\nCan't reach database server at " + + "`trigger-app-prod-database.cluster-cghdbxygvjc4.us-east-1.rds.amazonaws.com:5432`"; + + // Sanity: the fixture is genuinely leaky. + expect(leaked).toContain("rds.amazonaws.com"); + + const sanitized = sanitizeAuthFailure({ status: 401, error: leaked }); + + expect(sanitized.error).toBe("Invalid credentials"); + expect(sanitized.error).not.toContain("prisma"); + expect(sanitized.error).not.toContain("rds.amazonaws.com"); + expect(sanitized.error).not.toContain("database server"); + }); +});