From c56aa689acdb4ed87c41aedabf7c2ac2af009519 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 5 Nov 2026 11:14:00 -0800 Subject: [PATCH] fix(server): say where secret-sensitive requests fail in Sentry (#19040) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The server sends unexpected 5xx errors to Sentry through the shared Express error handler > - Some routes carry credentials: chat setup, provider webhooks, MCP, company observability settings, Muse, avatars, and similar routes > - On these routes the handler replaces the error with a generic `Secret-sensitive request failed` error, because the original name and message can contain secrets > - That replacement also removes every useful fact. The Sentry event does not show the route, the status, the error type, or the throw site, so a maintainer cannot find the cause > - This pull request adds a small fixed-shape record to that generic error. The record says where the failure came from, and it never copies request data or error text > - The benefit is that the next event on these routes points to a route and a code location, while the privacy rule stays the same ## Linked Issues or Issue Description **What happened?** A hosted staging server reported one event with the message `Secret-sensitive request failed`. The event had only the stack of the sanitizer itself. It had no route, no status, no error type, and no frame from the original error. The trace had no spans and no logs. A maintainer cannot find the root cause from this event. **Expected behavior** The event keeps the generic message, so no secret leaves the process. The event also tells the maintainer which secret-sensitive route family failed, the HTTP method and status, the error type, a safe error code when one exists, the matched route definition, and the original throw site. **Steps to reproduce** 1. Make a route handler throw on a secret-sensitive path, for example `PUT /api/companies/:companyId/observability`. 2. Enable the backend Sentry DSN. 3. Look at the captured event. It shows only `Secret-sensitive request failed` and the sanitizer stack. **Paperclip version or commit** `master` at e107a757556d3c74dcb5594b5eca9639cd3951e3. I searched open and closed pull requests for `Secret-sensitive`. I did not find a pull request that changes these diagnostics. ## What Changed - `server/src/middleware/http-log-policy.ts`: add `secretSensitiveRouteFamily()`. It returns a fixed label for the policy branch that matched, or `null`. `isSecretSensitiveHttpRequest()` now calls it. The matching rules do not change; the combined `/mcp/runner` and avatar pattern is split into two branches with the same rules. - `server/src/middleware/secret-sensitive-request-error.ts` (new): `SecretSensitiveRequestError`. It keeps the generic message, `name: "Error"`, its own stack, and default grouping. It adds frozen diagnostics: - `routeFamily`: from the closed list above. - `method`: one of the standard HTTP methods, or `other`. - `status`: the 5xx `HttpError` status, or 500. - `errorKind`: `http_error`, `type_error`, `range_error`, `reference_error`, `syntax_error`, `error`, or `non_error`. - `errnoCode`: only a key of `os.constants.errno`. `databaseErrorCode`: only a five-character SQLSTATE. Both are read from own data properties of the error and at most three causes. Getters are not called. - `routePattern`: the matched Express route definition (`req.route.path`), only when it is a string in a narrow character set. A RegExp route becomes `null`. - `originFrames`: up to eight complete V8 frames from the original stack. The extractor first removes the exact `name: message` header that V8 writes. It rebuilds the header like `Error.prototype.toString` from data properties only and requires `stack` to start with it, so a multiline message can never pass as a frame. If the header cannot be identified, no frame is kept. Each remaining line must match a strict frame pattern: a code-shaped function name and a location that may contain spaces, Windows drive or UNC paths, or a percent-encoded `file:///` URL. - `server/src/middleware/error-handler.ts`: the sanitizer now returns this error. The HTTP response, the request log context, and Telemetry do not change. - `server/src/sentry.ts`: `captureException` sends these diagnostics as `secret_route_family`, `http_method`, `http_status`, `error_kind`, optional `errno_code` and `db_error_code` tags, and a `secret_sensitive_request` context. Run tags for agent requests stay. The fingerprint stays `{{ default }}`. The module header documents the new event fields next to the other Sentry privacy notes. - `server/src/__tests__/secret-sensitive-request-error.test.ts` (new): unit tests for each bounded field, the header-first frame extraction (multiline-message canary, unidentifiable header, paths with spaces and Windows paths), the route-family policy, and an end-to-end test through a real Express app, the error handler, and the Sentry gate. ## Verification - `cd server && pnpm vitest run src/__tests__/secret-sensitive-request-error.test.ts src/__tests__/http-log-policy.test.ts src/__tests__/sentry.test.ts src/__tests__/error-handler.test.ts src/__tests__/agent-commentary.integration.test.ts src/__tests__/agent-profile-avatar.test.ts`: 6 files, 108 tests passed, 5 skipped. - Red before green: with the old `sentry.ts` and the new test, the end-to-end test fails because the event has no diagnostics tags. With this change, it passes. The three frame-extraction tests added for review feedback fail against the previous extractor and pass now. - The end-to-end test puts a canary credential in the query string, the JSON body, the error name, the error message, a cause message, and `HttpError.details`. It checks that no Sentry argument and no reported stack contains the canary. It also checks that the HTTP response stays `{ "error": "Internal server error" }` with the original status. - The existing tests in `error-handler.test.ts` for chat setup and observability credentials still pass without change. They check that the reported error, the error context, and Telemetry carry no canary. - Server typecheck with `PAPERCLIP_TYPECHECK_LOW_MEMORY=1 node scripts/typecheck-layers.mjs`: no error in server files. My sandbox had only the server dependency subset installed, so the layers also reported missing-module errors in `cli/`, `ui/`, and `packages/`. These errors are not related to this change. CI runs the full check. - Note for local runs: an empty `ANTHROPIC_BASE_URL` makes `sentry.test.ts` fail at import on `master` too. Unset it before you run the suite. ## Risks - Low risk. Sentry stays opt-in, and the change affects only events from secret-sensitive routes. - Privacy: the new values are closed labels, bounded numbers, source-code route definitions, Node errno names, SQLSTATE codes, and V8 frames that match a strict pattern after the message header is removed. A frame contains a function name and a file location. In theory, code can give a function a name from data. The frame pattern accepts only identifier-like names of at most 120 characters, and the event never includes the error name or message. - If code changes an error's `message` or `name` after its stack was formatted, the header no longer matches and the event has no origin frames. This fails closed. - `req.route` holds the last route that the router dispatched. If a later middleware fails after a route calls `next()`, `routePattern` can name that earlier route. The route family is still correct because it comes from the URL policy. - Grouping does not change. The reported error keeps the same type, message, and sanitizer stack, so existing Sentry issues keep collecting events. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Provider: Anthropic. Model: Claude Opus 5.5 (`claude-opus-5-5`), used through Claude Code with tool use (shell, file edits, test runs). Extended reasoning was not explicitly enabled. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .../secret-sensitive-request-error.test.ts | 270 ++++++++++++++++++ server/src/middleware/error-handler.ts | 19 +- server/src/middleware/http-log-policy.ts | 59 +++- .../secret-sensitive-request-error.ts | 166 +++++++++++ server/src/sentry.ts | 30 ++ 5 files changed, 522 insertions(+), 22 deletions(-) create mode 100644 server/src/__tests__/secret-sensitive-request-error.test.ts create mode 100644 server/src/middleware/secret-sensitive-request-error.ts diff --git a/server/src/__tests__/secret-sensitive-request-error.test.ts b/server/src/__tests__/secret-sensitive-request-error.test.ts new file mode 100644 index 00000000000..67427f83005 --- /dev/null +++ b/server/src/__tests__/secret-sensitive-request-error.test.ts @@ -0,0 +1,270 @@ +import express from "express"; +import request from "supertest"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { HttpError } from "../errors.js"; +import { + isSecretSensitiveHttpRequest, + secretSensitiveRouteFamily, +} from "../middleware/http-log-policy.js"; +import { SecretSensitiveRequestError } from "../middleware/secret-sensitive-request-error.js"; + +const facts = { routeFamily: "chat_endpoint_setup", method: "POST", routePath: "/:id/setup" } as const; + +describe("SecretSensitiveRequestError", () => { + it("keeps the generic message and its own stack", () => { + const original = Object.assign(new Error("echoed canary-token"), { name: "canary-token" }); + + const error = new SecretSensitiveRequestError(original, facts); + + expect(error.name).toBe("Error"); + expect(error.message).toBe("Secret-sensitive request failed"); + expect(error.stack).not.toContain("canary-token"); + expect(error).not.toHaveProperty("cause"); + expect(JSON.stringify(error.diagnostics)).not.toContain("canary-token"); + }); + + it("records the route family, method, status, kind and route definition", () => { + const error = new SecretSensitiveRequestError(new HttpError(503, "provider said canary-token"), facts); + + expect(error.diagnostics).toMatchObject({ + routeFamily: "chat_endpoint_setup", + method: "POST", + status: 503, + errorKind: "http_error", + errnoCode: null, + databaseErrorCode: null, + routePattern: "/:id/setup", + }); + }); + + it.each([ + [new TypeError("x"), "type_error"], + [new RangeError("x"), "range_error"], + [new ReferenceError("x"), "reference_error"], + [new SyntaxError("x"), "syntax_error"], + [new Error("x"), "error"], + ["thrown string", "non_error"], + ])("labels %s as %s", (original, kind) => { + expect(new SecretSensitiveRequestError(original, facts).diagnostics.errorKind).toBe(kind); + }); + + it("uses closed or code-shaped values and drops everything else", () => { + const original = Object.assign(new Error("x"), { code: "canary-token" }); + const error = new SecretSensitiveRequestError(original, { + routeFamily: "provider_webhook", + method: "canary-token", + routePath: /canary-token/, + }); + + expect(error.diagnostics).toMatchObject({ + method: "other", + status: 500, + errnoCode: null, + databaseErrorCode: null, + routePattern: null, + }); + expect(new SecretSensitiveRequestError(new Error("x"), { ...facts, routePath: "/:id?token=canary-token" }) + .diagnostics.routePattern).toBeNull(); + }); + + it("reads errno and SQLSTATE codes from own data properties in the cause chain", () => { + const database = Object.assign(new Error("duplicate key canary-token"), { code: "23505" }); + const network = Object.assign(new Error("connect failed", { cause: database }), { code: "ECONNREFUSED" }); + const wrapper = new Error("wrapped", { cause: network }); + + expect(new SecretSensitiveRequestError(wrapper, facts).diagnostics).toMatchObject({ + errnoCode: "ECONNREFUSED", + databaseErrorCode: "23505", + }); + + const getterCode = new Error("x"); + Object.defineProperty(getterCode, "code", { get: () => "ECONNRESET" }); + expect(new SecretSensitiveRequestError(getterCode, facts).diagnostics.errnoCode).toBeNull(); + }); + + it("keeps well-formed original stack frames and drops the message and injected lines", () => { + const original = new Error("provider echoed canary-token\nsecond message line canary-token"); + original.stack = [ + "Error: provider echoed canary-token", + "second message line canary-token", + " at SlackSetup.verify (/app/server/dist-runtime/server.mjs:120:7)", + " at async Promise.all (index 0)", + " at canary-token (https://example.test/?token=canary-token:1:2)", + " at /app/node_modules/.pnpm/router@2.2.0/node_modules/router/lib/layer.js:152:17", + " at process.processTicksAndRejections (node:internal/process/task_queues:105:5)", + ].join("\n"); + + expect(new SecretSensitiveRequestError(original, facts).diagnostics.originFrames).toEqual([ + "at SlackSetup.verify (/app/server/dist-runtime/server.mjs:120:7)", + "at /app/node_modules/.pnpm/router@2.2.0/node_modules/router/lib/layer.js:152:17", + "at process.processTicksAndRejections (node:internal/process/task_queues:105:5)", + ]); + }); + + it("drops frame-shaped lines inside a multiline message", () => { + const original = Object.assign( + new Error("provider failed\n at canaryToken (/app/a.js:1:1)\n at /canary-token/b.js:2:2"), + { name: "ProviderError" }, + ); + + const frames = new SecretSensitiveRequestError(original, facts).diagnostics.originFrames; + + expect(frames.length).toBeGreaterThan(0); + expect(frames[0]).toContain("secret-sensitive-request-error.test.ts"); + expect(JSON.stringify(frames)).not.toMatch(/canary/i); + }); + + it("keeps no frame when the message header cannot be identified", () => { + const changed = new Error("first canary-token\n at canaryToken (/app/a.js:1:1)"); + void changed.stack; + changed.message = "replaced"; + const replaced = new Error("x"); + replaced.stack = " at canaryToken (/app/a.js:1:1)\n at f (/app/b.js:2:2)"; + const getterName = new Error("x"); + Object.defineProperty(getterName, "name", { get: () => "Error" }); + + for (const original of [changed, replaced, getterName]) { + expect(new SecretSensitiveRequestError(original, facts).diagnostics.originFrames).toEqual([]); + } + }); + + it("keeps frames whose paths contain spaces or Windows drives", () => { + const original = new Error("x"); + original.stack = [ + "Error: x", + " at verify (/home/Jane Doe/paperclip/server.js:10:2)", + " at C:\\Users\\Jane Doe\\paperclip\\server\\dist\\index.js:3:4", + " at Layer.handle (file:///C:/Users/Jane%20Doe/paperclip/layer.js:5:6)", + " at Router.handle (\\\\build-host\\share\\router.js:7:8)", + ].join("\n"); + + expect(new SecretSensitiveRequestError(original, facts).diagnostics.originFrames).toEqual([ + "at verify (/home/Jane Doe/paperclip/server.js:10:2)", + "at C:\\Users\\Jane Doe\\paperclip\\server\\dist\\index.js:3:4", + "at Layer.handle (file:///C:/Users/Jane%20Doe/paperclip/layer.js:5:6)", + "at Router.handle (\\\\build-host\\share\\router.js:7:8)", + ]); + }); + + it("caps origin frames", () => { + const original = new Error("x"); + original.stack = ["Error: x", ...Array.from({ length: 20 }, (_, i) => ` at f${i} (/app/a.js:${i + 1}:1)`)].join("\n"); + + expect(new SecretSensitiveRequestError(original, facts).diagnostics.originFrames).toHaveLength(8); + }); +}); + +describe("secretSensitiveRouteFamily", () => { + it("names the policy branch with a fixed label", () => { + for (const [method, url, family] of [ + ["POST", "/api/chat-webhooks/endpoint-1/slack", "provider_webhook"], + ["GET", "/api/muse/v1/commands", "muse"], + ["GET", "/mcp/files/file-1", "mcp_files"], + ["POST", "/api/companies/company/agent-commentary", "agent_commentary"], + ["PUT", "/api/companies/company/observability", "agent_observability"], + ["POST", "/MCP/RUNNER/call", "mcp_runner"], + ["POST", "/api/companies/company/agents/agent/avatar", "agent_avatar"], + ["GET", "/api/chat-slack/oauth/callback?code=private", "chat_slack_oauth"], + ["POST", "/api/chat-endpoints/id/slack/install", "chat_endpoint_slack"], + ["POST", "/api/companies/company/voice-sessions", "voice_sessions"], + ["POST", "/mcp/oauth/token", "mcp_oauth"], + ["POST", "/api/mcp/tools", "api_mcp"], + ["POST", "/api/chat-endpoints/id/setup-secret", "chat_endpoint_setup"], + ] as const) { + expect(secretSensitiveRouteFamily(method, url)).toBe(family); + expect(isSecretSensitiveHttpRequest(method, url)).toBe(true); + } + }); + + it("returns null wherever the request is not secret-sensitive", () => { + for (const [method, url] of [ + ["GET", "/api/chat-endpoints/id/setup"], + ["POST", "/api/chat-endpoints/id/test"], + ["GET", "/api/mcp/tools"], + [undefined, "/api/mcp/tools"], + ["POST", undefined], + ] as const) { + expect(secretSensitiveRouteFamily(method, url)).toBeNull(); + expect(isSecretSensitiveHttpRequest(method, url)).toBe(false); + } + }); +}); + +describe("secret-sensitive Sentry capture", () => { + const BACKEND_DSN_ENV = "SENTRY_DSN_BACKEND"; + const originalBackendDsn = process.env[BACKEND_DSN_ENV]; + + afterEach(() => { + if (originalBackendDsn === undefined) delete process.env[BACKEND_DSN_ENV]; + else process.env[BACKEND_DSN_ENV] = originalBackendDsn; + vi.doUnmock("@sentry/node"); + vi.doUnmock("../peer-version-check.js"); + }); + + it("reports where the request failed without its private values", async () => { + // `@sentry/node` is optional and not installed in CI; open the gate with a fake. + process.env[BACKEND_DSN_ENV] = "https://fixture@example.com/1"; + const captureException = vi.fn((_error: unknown, _hint?: unknown) => "event-id"); + vi.doMock("@sentry/node", () => ({ + getClient: () => ({ on: vi.fn() }), + init: vi.fn(), + captureException, + close: vi.fn(async () => true), + httpIntegration: vi.fn((options: unknown) => ({ name: "Http", ...(options as object) })), + onUnhandledRejectionIntegration: vi.fn((options: unknown) => ({ name: "OnUnhandledRejection", ...(options as object) })), + })); + vi.doMock("../peer-version-check.js", () => ({ checkExactPeerVersions: () => ({ ok: true }) })); + vi.resetModules(); + const { sentryReady } = await import("../sentry.js"); + const { errorHandler } = await import("../middleware/error-handler.js"); + const { HttpError: FreshHttpError } = await import("../errors.js"); + await sentryReady; + const app = express(); + app.use(express.json()); + app.put("/api/companies/:companyId/observability", () => { + const database = Object.assign(new Error("duplicate canary-api-key"), { code: "23505" }); + throw Object.assign(new TypeError("provider echoed canary-api-key", { cause: database }), { + name: "canary-api-key", + code: "ECONNREFUSED", + }); + }); + app.post("/api/chat-endpoints/:id/setup", () => { + throw new FreshHttpError(503, "provider echoed canary-api-key", { token: "canary-api-key" }); + }); + app.use(errorHandler); + + const res = await request(app) + .put("/api/companies/company-1/observability?token=canary-api-key") + .send({ credentials: { apiKey: "canary-api-key" } }); + const setupRes = await request(app).post("/api/chat-endpoints/endpoint-1/setup").send({ botToken: "canary-api-key" }); + + expect(res.status).toBe(500); + expect(res.body).toEqual({ error: "Internal server error" }); + expect(setupRes.status).toBe(503); + expect(setupRes.body).toEqual({ error: "Internal server error" }); + expect(captureException).toHaveBeenCalledTimes(2); + const [reported, hint] = captureException.mock.calls[0] as [Error, { tags: Record; contexts: Record>; fingerprint: string[] }]; + expect(reported).toMatchObject({ name: "Error", message: "Secret-sensitive request failed" }); + expect(hint.fingerprint).toEqual(["{{ default }}"]); + expect(hint.tags).toEqual({ + secret_route_family: "agent_observability", + http_method: "PUT", + http_status: "500", + error_kind: "type_error", + errno_code: "ECONNREFUSED", + db_error_code: "23505", + }); + expect(hint.contexts.secret_sensitive_request).toMatchObject({ + routeFamily: "agent_observability", + routePattern: "/api/companies/:companyId/observability", + }); + const frames = hint.contexts.secret_sensitive_request!.originFrames as string[]; + expect(frames.some((frame) => frame.includes("secret-sensitive-request-error.test.ts"))).toBe(true); + expect(captureException.mock.calls[1]![1]).toMatchObject({ + tags: { secret_route_family: "chat_endpoint_setup", http_method: "POST", http_status: "503", error_kind: "http_error" }, + contexts: { secret_sensitive_request: { routePattern: "/api/chat-endpoints/:id/setup" } }, + }); + expect(JSON.stringify(captureException.mock.calls)).not.toContain("canary-api-key"); + expect(String(reported.stack)).not.toContain("canary-api-key"); + }); +}); diff --git a/server/src/middleware/error-handler.ts b/server/src/middleware/error-handler.ts index 71058769f4c..3463d122a2f 100644 --- a/server/src/middleware/error-handler.ts +++ b/server/src/middleware/error-handler.ts @@ -8,7 +8,11 @@ import { getTelemetryClient } from "../telemetry.js"; import { captureException } from "../sentry.js"; import { COMPANY_IMPORT_API_PATH } from "../routes/company-import-paths.js"; import { logger } from "./logger.js"; -import { isSecretSensitiveHttpRequest } from "./http-log-policy.js"; +import { + isSecretSensitiveHttpRequest, + secretSensitiveRouteFamily, +} from "./http-log-policy.js"; +import { SecretSensitiveRequestError } from "./secret-sensitive-request-error.js"; import { collectSensitiveStringValues, redactSensitiveValueOccurrences, @@ -69,12 +73,13 @@ function attachErrorContext( } function sanitizeSecretSensitiveError(req: Request, error: Error): Error { - if (!isSecretSensitiveHttpRequest(req.method, req.originalUrl)) return error; - const sanitized = new Error("Secret-sensitive request failed"); - // Both `name` and `message` are attacker/provider-controlled properties on - // JavaScript errors. Do not preserve either on a credential-bearing route. - sanitized.name = "Error"; - return sanitized; + const routeFamily = secretSensitiveRouteFamily(req.method, req.originalUrl); + if (!routeFamily) return error; + return new SecretSensitiveRequestError(error, { + routeFamily, + method: req.method, + routePath: (req.route as { path?: unknown } | undefined)?.path, + }); } function sanitizeSecretSensitiveResponse( diff --git a/server/src/middleware/http-log-policy.ts b/server/src/middleware/http-log-policy.ts index 69f0b3cb945..ef98af0e0ab 100644 --- a/server/src/middleware/http-log-policy.ts +++ b/server/src/middleware/http-log-policy.ts @@ -38,11 +38,30 @@ function normalizePath(url: string): string { return pathname.length > 0 ? pathname : "/"; } -const SECRET_SENSITIVE_HTTP_PATHS = [ - /^\/api\/companies\/[^/]+\/voice-sessions(?:\/|$)/, - /^\/mcp\/(?:oauth|paperclip)(?:\/|$)/, - /^\/api\/mcp(?:\/|$)/, - /^\/api\/chat-endpoints\/[^/]+\/setup(?:-secret)?(?:\/|$)/, +/** + * A fixed label for the policy branch that made a request secret-sensitive. + * Labels come from this closed list, never from the request URL. + */ +export type SecretSensitiveRouteFamily = + | "provider_webhook" + | "muse" + | "mcp_files" + | "agent_commentary" + | "agent_observability" + | "mcp_runner" + | "agent_avatar" + | "chat_slack_oauth" + | "chat_endpoint_slack" + | "voice_sessions" + | "mcp_oauth" + | "api_mcp" + | "chat_endpoint_setup"; + +const SECRET_SENSITIVE_HTTP_PATHS: ReadonlyArray = [ + ["voice_sessions", /^\/api\/companies\/[^/]+\/voice-sessions(?:\/|$)/], + ["mcp_oauth", /^\/mcp\/(?:oauth|paperclip)(?:\/|$)/], + ["api_mcp", /^\/api\/mcp(?:\/|$)/], + ["chat_endpoint_setup", /^\/api\/chat-endpoints\/[^/]+\/setup(?:-secret)?(?:\/|$)/], ]; const SECRET_SENSITIVE_HTTP_METHODS = new Set(["POST", "PUT", "PATCH"]); @@ -106,20 +125,30 @@ export function isSecretSensitiveHttpRequest( method: string | undefined, url: string | undefined, ): boolean { - if (isPrivateWebhookHttpRequest(method, url) || isPrivateMuseHttpRequest(url)) return true; - if (url && normalizePath(url).startsWith("/mcp/files/")) return true; - if (isPrivateAgentCommentaryHttpRequest(url)) return true; + return secretSensitiveRouteFamily(method, url) !== null; +} + +/** The policy branch that makes a request secret-sensitive, or `null`. */ +export function secretSensitiveRouteFamily( + method: string | undefined, + url: string | undefined, +): SecretSensitiveRouteFamily | null { + if (isPrivateWebhookHttpRequest(method, url)) return "provider_webhook"; + if (isPrivateMuseHttpRequest(url)) return "muse"; + if (url && normalizePath(url).startsWith("/mcp/files/")) return "mcp_files"; + if (isPrivateAgentCommentaryHttpRequest(url)) return "agent_commentary"; // Custom OTLP header values are arbitrary credentials, including on rejected // methods or malformed subpaths. Named-field redaction cannot identify them. - if (isPrivateAgentObservabilityHttpRequest(url)) return true; + if (isPrivateAgentObservabilityHttpRequest(url)) return "agent_observability"; // Avatar uploads and dedicated Dot MCP calls may contain private image bytes. - if (url && /^(?:\/mcp\/runner(?:\/|$)|\/api\/companies\/[^/]+\/agents\/[^/]+\/avatar(?:\/|$))/i.test(normalizePath(url).replace(/^https?:\/\/[^/]*/i, ""))) return true; - if (!method || !url) return false; - if (/^\/api\/chat-slack\/(?:managed\/)?oauth(?:\/|$)/i.test(normalizePath(url).replace(/^https?:\/\/[^/]*/i, ""))) return true; - if (/^\/api\/chat-endpoints\/[^/]+\/slack(?:\/|$)/i.test(normalizePath(url).replace(/^https?:\/\/[^/]*/i, ""))) return true; - if (!SECRET_SENSITIVE_HTTP_METHODS.has(method.toUpperCase())) return false; + if (url && /^\/mcp\/runner(?:\/|$)/i.test(normalizePath(url).replace(/^https?:\/\/[^/]*/i, ""))) return "mcp_runner"; + if (url && /^\/api\/companies\/[^/]+\/agents\/[^/]+\/avatar(?:\/|$)/i.test(normalizePath(url).replace(/^https?:\/\/[^/]*/i, ""))) return "agent_avatar"; + if (!method || !url) return null; + if (/^\/api\/chat-slack\/(?:managed\/)?oauth(?:\/|$)/i.test(normalizePath(url).replace(/^https?:\/\/[^/]*/i, ""))) return "chat_slack_oauth"; + if (/^\/api\/chat-endpoints\/[^/]+\/slack(?:\/|$)/i.test(normalizePath(url).replace(/^https?:\/\/[^/]*/i, ""))) return "chat_endpoint_slack"; + if (!SECRET_SENSITIVE_HTTP_METHODS.has(method.toUpperCase())) return null; const pathname = normalizePath(url); - return SECRET_SENSITIVE_HTTP_PATHS.some((pattern) => pattern.test(pathname)); + return SECRET_SENSITIVE_HTTP_PATHS.find(([, pattern]) => pattern.test(pathname))?.[0] ?? null; } export function shouldSilenceHttpSuccessLog( diff --git a/server/src/middleware/secret-sensitive-request-error.ts b/server/src/middleware/secret-sensitive-request-error.ts new file mode 100644 index 00000000000..be717a96495 --- /dev/null +++ b/server/src/middleware/secret-sensitive-request-error.ts @@ -0,0 +1,166 @@ +import os from "node:os"; +import { HttpError } from "../errors.js"; +import type { SecretSensitiveRouteFamily } from "./http-log-policy.js"; + +const HTTP_METHODS = new Set(["GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"]); +const MAX_CAUSE_DEPTH = 4; +const MAX_ORIGIN_FRAMES = 8; +// Express route definitions are source code, not request data. Anything +// outside this character set (for example a RegExp route) is dropped. +const ROUTE_PATTERN = /^[A-Za-z0-9_/:.*?(){}-]{1,200}$/; +// PostgreSQL SQLSTATE: five upper-case alphanumerics with at least one digit. +const SQLSTATE_PATTERN = /^(?=.*\d)[0-9A-Z]{5}$/; +// One V8 frame whose function name and location contain only code-shaped +// characters. Locations may contain spaces and Windows drive or UNC paths. +// Frames that do not match are dropped, never truncated. +const FRAME_LOCATION = String.raw`(?:file:\/\/\/?)?(?:node:)?(?:[A-Za-z]:[\\/])?[A-Za-z0-9_./\\@%+ -]{1,300}:\d{1,7}:\d{1,7}`; +const STACK_FRAME_PATTERN = new RegExp( + String.raw`^at (?:(?:async |new )?[A-Za-z0-9_$.<>[\]]{1,120} \(${FRAME_LOCATION}\)|${FRAME_LOCATION})$`, +); + +export type SecretSensitiveErrorKind = + | "http_error" + | "type_error" + | "range_error" + | "reference_error" + | "syntax_error" + | "error" + | "non_error"; + +/** + * Fixed-shape facts about a failed secret-sensitive request. Every value is a + * closed label, a bounded number, or text taken from source code. None is + * copied from the error message, error name, request URL, body, or headers. + */ +export interface SecretSensitiveRequestDiagnostics { + routeFamily: SecretSensitiveRouteFamily; + method: string; + status: number; + errorKind: SecretSensitiveErrorKind; + errnoCode: string | null; + databaseErrorCode: string | null; + routePattern: string | null; + originFrames: string[]; +} + +export interface SecretSensitiveRequestFacts { + routeFamily: SecretSensitiveRouteFamily; + method: string | undefined; + /** The matched Express route definition, when the router recorded one. */ + routePath: unknown; +} + +/** + * The only error a credential-bearing route reports. It keeps the generic + * message and its own stack, so Sentry grouping does not change, and adds + * bounded diagnostics that say where the original failure came from. + */ +export class SecretSensitiveRequestError extends Error { + readonly diagnostics: SecretSensitiveRequestDiagnostics; + + constructor(original: unknown, facts: SecretSensitiveRequestFacts) { + super("Secret-sensitive request failed"); + // Both `name` and `message` are attacker/provider-controlled properties on + // JavaScript errors. Do not preserve either on a credential-bearing route. + this.name = "Error"; + this.diagnostics = Object.freeze({ + routeFamily: facts.routeFamily, + method: normalizeMethod(facts.method), + status: original instanceof HttpError && isServerErrorStatus(original.status) ? original.status : 500, + errorKind: errorKindOf(original), + errnoCode: findOwnCode(original, (code) => Object.hasOwn(os.constants.errno, code)), + databaseErrorCode: findOwnCode(original, (code) => SQLSTATE_PATTERN.test(code)), + routePattern: normalizeRoutePattern(facts.routePath), + originFrames: originFramesOf(original), + }); + } +} + +function normalizeMethod(method: string | undefined): string { + const upper = typeof method === "string" ? method.toUpperCase() : ""; + return HTTP_METHODS.has(upper) ? upper : "other"; +} + +function isServerErrorStatus(status: unknown): status is number { + return typeof status === "number" && Number.isInteger(status) && status >= 500 && status <= 599; +} + +function errorKindOf(error: unknown): SecretSensitiveErrorKind { + if (error instanceof HttpError) return "http_error"; + if (error instanceof TypeError) return "type_error"; + if (error instanceof RangeError) return "range_error"; + if (error instanceof ReferenceError) return "reference_error"; + if (error instanceof SyntaxError) return "syntax_error"; + return error instanceof Error ? "error" : "non_error"; +} + +/** Read `code` from the error and its causes as own data properties only. */ +function findOwnCode(error: unknown, accept: (code: string) => boolean): string | null { + let current: unknown = error; + for (let depth = 0; depth < MAX_CAUSE_DEPTH && current && typeof current === "object"; depth++) { + const code = Object.getOwnPropertyDescriptor(current, "code"); + if (code && "value" in code && typeof code.value === "string" && accept(code.value)) return code.value; + const cause = Object.getOwnPropertyDescriptor(current, "cause"); + current = cause && "value" in cause ? cause.value : null; + } + return null; +} + +function normalizeRoutePattern(routePath: unknown): string | null { + return typeof routePath === "string" && ROUTE_PATTERN.test(routePath) ? routePath : null; +} + +/** + * Keep the original throw site without its message. V8 starts `stack` with the + * `name: message` header, and that header can span several lines. Frames are + * read only after the exact header is removed; if the header cannot be + * identified, no frame is kept. + */ +function originFramesOf(error: unknown): string[] { + if (!(error instanceof Error)) return []; + let stack: unknown; + try { + // V8 exposes `stack` as an accessor; a replaced getter must not break reporting. + stack = error.stack; + } catch { + return []; + } + if (typeof stack !== "string") return []; + const header = stackHeaderOf(error); + if (header === null || !stack.startsWith(`${header}\n`)) return []; + const frames: string[] = []; + for (const line of stack.slice(header.length + 1).split("\n")) { + const frame = line.trim(); + if (!STACK_FRAME_PATTERN.test(frame)) continue; + frames.push(frame); + if (frames.length === MAX_ORIGIN_FRAMES) break; + } + return frames; +} + +/** + * The header V8 writes when it formats `stack`, built like + * `Error.prototype.toString`. Getters and non-string values return `null`. + */ +function stackHeaderOf(error: Error): string | null { + const name = dataPropertyOf(error, "name"); + const message = dataPropertyOf(error, "message"); + if (name === null || message === null) return null; + const resolvedName = name === undefined ? "Error" : name; + const resolvedMessage = message === undefined ? "" : message; + if (resolvedName === "") return resolvedMessage; + if (resolvedMessage === "") return resolvedName; + return `${resolvedName}: ${resolvedMessage}`; +} + +/** Look up a string data property on the object or its prototypes without calling getters. */ +function dataPropertyOf(target: object, key: string): string | undefined | null { + for (let current: object | null = target; current; current = Object.getPrototypeOf(current)) { + const descriptor = Object.getOwnPropertyDescriptor(current, key); + if (!descriptor) continue; + if (!("value" in descriptor)) return null; + if (descriptor.value === undefined) return undefined; + return typeof descriptor.value === "string" ? descriptor.value : null; + } + return undefined; +} diff --git a/server/src/sentry.ts b/server/src/sentry.ts index 0fe3360c47b..abc577a8d09 100644 --- a/server/src/sentry.ts +++ b/server/src/sentry.ts @@ -40,6 +40,18 @@ // and request isolation need it) and turns the breadcrumb off with the // integration's own `breadcrumbs` option. // +// A failed secret-sensitive request (chat setup, provider webhooks, MCP, +// company observability settings, and similar routes) reports only the +// generic `Secret-sensitive request failed` error, with its own stack and +// default grouping. `SecretSensitiveRequestError` adds `secret_route_family`, +// `http_method`, `http_status` and `error_kind` tags, optional `errno_code` +// and `db_error_code` tags (a Node errno name or a SQLSTATE), and a +// `secret_sensitive_request` context. The context holds the matched Express +// route definition and up to eight complete V8 frames from the original +// stack. Every value is a closed label, a bounded number, or a code-shaped +// string; anything else is dropped. The event never carries the request +// URL, query, body, headers, or the original error name or message. +// // `onUnhandledRejectionIntegration` defaults to `mode: "warn"`, which // registers a `process.on("unhandledRejection")` listener. Node cancels its // own crash-on-unhandled-rejection behavior when any listener is registered. @@ -62,6 +74,7 @@ import os from "node:os"; import type { HeartbeatRunStatus } from "@paperclipai/shared"; import { AdapterStopTimeoutError } from "./services/adapter-stop-timeout.js"; import { CloudPortfolioError } from "./services/cloud-portfolio-error.js"; +import { SecretSensitiveRequestError } from "./middleware/secret-sensitive-request-error.js"; import { storageFailureDiagnosticsFor } from "./storage/failure-diagnostics.js"; import type { RunFailureDiagnostics } from "./services/run-failure-diagnostics.js"; import { readBuildCommit } from "./build-commit.js"; @@ -139,6 +152,23 @@ export function captureException(error: unknown, reference?: SentryRunReference) contexts: { cloud_portfolio: { ...CloudPortfolioError.diagnosticsFor(error) } }, fingerprint: ["{{ default }}"], }); + } else if (error instanceof SecretSensitiveRequestError) { + // The sanitized error keeps its generic message, stack and default + // grouping. Only its fixed-shape diagnostics are added. + const diagnostics = error.diagnostics; + sentryHandle.captureException(error, { + tags: { + ...tags, + secret_route_family: diagnostics.routeFamily, + http_method: diagnostics.method, + http_status: String(diagnostics.status), + error_kind: diagnostics.errorKind, + ...(diagnostics.errnoCode ? { errno_code: diagnostics.errnoCode } : {}), + ...(diagnostics.databaseErrorCode ? { db_error_code: diagnostics.databaseErrorCode } : {}), + }, + contexts: { secret_sensitive_request: { ...diagnostics, originFrames: [...diagnostics.originFrames] } }, + fingerprint: ["{{ default }}"], + }); } else if (storageFailure) { // The original error keeps its default grouping; only the normalized // record is added, never the provider's request or response.