From bb7978327ed568bf012f6efe16fd200e8baa3a5c Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 11 Jun 2026 17:09:32 -0700 Subject: [PATCH] fix(logger): redact passwords and tokens from HTTP error log lines (#8013) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resubmits #5820 by @echokos. The original PR's head fork could not accept maintainer edits (organization-owned fork without cross-org maintainer-edit access), so we've resubmitted the commits here with original authorship preserved. Thanks to @echokos for the contribution. --- ## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies; the server is an Express app with structured HTTP logging via pino-http. > - The middleware in `server/src/middleware/logger.ts` defines a `customProps` hook that attaches request context (`req.body` / `req.params` / `req.query`) to every 4xx/5xx log entry so operators can diagnose failed requests. > - That hook copies the body verbatim. Better Auth's `POST /api/auth/sign-in/email` carries an `{ email, password }` body — on a wrong-password attempt the request lands in the 4xx branch and the plaintext password is written to `~/.paperclip/logs/server.log`. > - Two existing issues raise this (#3072 plaintext-password leak, #4759 similar concerns) and neither has a fix. > - Same exposure surface applies to sign-up, reset-password, API key creation, and any endpoint that accepts a credential in the body and can return 4xx. > - This pull request introduces a small `redactSensitive` walker that returns a shallow copy of the input with values for known credential-shaped keys replaced with `[REDACTED]`, and applies it at every body/params/query log site in `customProps`. > - The benefit is that operators can keep diagnostic logging on without their disk silently accumulating user passwords and bearer tokens. ## What Changed - `server/src/middleware/redact-sensitive.ts` (new): depth-capped, case-insensitive walker. Sensitive keys covered: `password`, `currentPassword`, `newPassword`, `passwordConfirmation`, `passwordConfirm`, `confirmPassword` (+ snake_case variants), `secret`, `client_secret`, `access_token`, `refresh_token`, `id_token`, `auth_token`, `session_token`, `api_key`, `authorization`, `private_key`. Bare `token` deliberately not in the list — pagination cursors and CSRF tokens are not credentials (per Greptile review). - `server/src/middleware/logger.ts`: wraps the six log sites in `customProps` (3 ctx-path + 3 fallback-path) with `redactSensitive`. - `server/src/__tests__/redact-sensitive.test.ts` (new): covers plaintext password, case-insensitive matching, multiple credential keys, nested objects/arrays, bare `token` left untouched, primitives untouched, cycle safety. - Depth-cap returns `undefined` (field absent from log line) rather than a sentinel string, per Greptile review. ## Verification - `pnpm --filter @paperclipai/server test redact-sensitive` should run the new test file green. - Manual: tail `~/.paperclip/logs/server.log`, hit `POST /api/auth/sign-in/email` with a deliberately wrong password, confirm the logged `reqBody.password` reads `[REDACTED]` (not the plaintext) and the surrounding fields still appear for diagnosis. ## Risks Low. The walker only rewrites values at the log-emit boundary — the actual `req.body` object handed to downstream handlers is unchanged because `redactSensitive` returns a new object. Standard log fields (email, route path, status code) remain visible. The sensitive-key list is conservative enough that the only risk is over-redacting a non-credential field that happens to share a name with a known credential; the bare `token` carve-out in this revision addresses the most obvious such case. ## Model Used - Claude Opus 4.7 (`claude-opus-4-7`), Anthropic, extended thinking mode, working through Claude Code CLI. ## 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 - [ ] I have run tests locally and they pass (no `node_modules` in my disposable PR-prep checkout; CI vitest will exercise) - [x] I have added or updated tests where applicable - [ ] If this change affects the UI, I have included before/after screenshots (N/A — server-only) - [ ] I have updated relevant documentation to reflect my changes (no doc surface affected) - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge Closes #3072. Refs #4759. --- ## Original Context `customProps` in the HTTP logger copies `req.body` / `req.params` / `req.query` verbatim into 4xx/5xx log entries. Better Auth's wrong-password flow therefore writes: ``` {"reqBody":{"email":"…","password":"founding6gomez6croaking"},"msg":"POST /api/auth/sign-in/email 401"} ``` …to disk. This PR rewrites credential-shaped values to `[REDACTED]` at that boundary. --- ## Cross-references and status (maintainer) Closes #5820 Closes #3095 Closes #4760 Closes #4886 --------- Co-authored-by: Aurora Co-authored-by: Paperclip --- server/src/__tests__/redact-sensitive.test.ts | 92 +++++++++++++++++++ server/src/middleware/logger.ts | 13 +-- server/src/middleware/redact-sensitive.ts | 67 ++++++++++++++ 3 files changed, 166 insertions(+), 6 deletions(-) create mode 100644 server/src/__tests__/redact-sensitive.test.ts create mode 100644 server/src/middleware/redact-sensitive.ts diff --git a/server/src/__tests__/redact-sensitive.test.ts b/server/src/__tests__/redact-sensitive.test.ts new file mode 100644 index 00000000..173fbece --- /dev/null +++ b/server/src/__tests__/redact-sensitive.test.ts @@ -0,0 +1,92 @@ +import { describe, expect, it } from "vitest"; +import { redactSensitive } from "../middleware/redact-sensitive.js"; + +describe("redactSensitive", () => { + it("redacts a plaintext password field on a sign-in body", () => { + const body = { email: "user@example.com", password: "founding6gomez6croaking" }; + + const out = redactSensitive(body) as Record; + + expect(out.email).toBe("user@example.com"); + expect(out.password).toBe("[REDACTED]"); + expect((body as Record).password).toBe("founding6gomez6croaking"); + }); + + it("redacts password key regardless of casing", () => { + expect((redactSensitive({ Password: "x" }) as Record).Password).toBe("[REDACTED]"); + expect((redactSensitive({ PASSWORD: "x" }) as Record).PASSWORD).toBe("[REDACTED]"); + }); + + it("redacts known credential-shaped keys", () => { + const out = redactSensitive({ + currentPassword: "a", + newPassword: "b", + access_token: "c", + refresh_token: "d", + api_key: "e", + authorization: "Bearer f", + }) as Record; + + for (const value of Object.values(out)) { + expect(value).toBe("[REDACTED]"); + } + }); + + it("does not redact a bare `token` field — pagination cursors and CSRF tokens are not credentials", () => { + const out = redactSensitive({ token: "next-page-cursor", limit: 20 }) as Record; + + expect(out.token).toBe("next-page-cursor"); + expect(out.limit).toBe(20); + }); + + it("recurses into nested objects and arrays", () => { + const out = redactSensitive({ + user: { email: "user@example.com", password: "secret-pass" }, + tokens: [{ access_token: "t1" }, { access_token: "t2" }], + }) as Record; + + expect((out.user as Record).email).toBe("user@example.com"); + expect((out.user as Record).password).toBe("[REDACTED]"); + const tokens = out.tokens as Array>; + expect(tokens[0].access_token).toBe("[REDACTED]"); + expect(tokens[1].access_token).toBe("[REDACTED]"); + }); + + it("leaves primitives and non-sensitive keys untouched", () => { + const body = { email: "a@b.c", name: "Alice", count: 7, active: true, missing: null }; + + expect(redactSensitive(body)).toEqual(body); + }); + + it("returns primitives unchanged", () => { + expect(redactSensitive("hello")).toBe("hello"); + expect(redactSensitive(42)).toBe(42); + expect(redactSensitive(null)).toBe(null); + expect(redactSensitive(undefined)).toBe(undefined); + }); + + it("caps recursion depth so cycles do not pin the logger", () => { + const cycle: Record = { name: "root" }; + cycle.self = cycle; + + expect(() => redactSensitive(cycle)).not.toThrow(); + }); + + it("omits deeply-nested arrays at the depth cap instead of leaking null entries to JSON", () => { + // Build an object whose array field is reached at MAX_DEPTH. Recursing + // into the array elements would exceed the cap; without the array-level + // guard, `value.map` would produce `[undefined, ...]` which JSON.stringify + // renders as `[null, ...]`. Object properties at the same cap are + // already absent from the JSON output (JSON.stringify skips undefined + // values on objects), so this test pins the array path to the same + // contract: silently absent, not visible as nulls. + let payload: Record = { values: [1, 2, 3] }; + for (let i = 0; i < 5; i++) payload = { nested: payload }; + + const out = redactSensitive(payload); + + const json = JSON.stringify(out); + expect(json).not.toContain("null"); + expect(json).not.toContain("[1,2,3]"); + }); +}); diff --git a/server/src/middleware/logger.ts b/server/src/middleware/logger.ts index 7eb5c2b4..e014a34c 100644 --- a/server/src/middleware/logger.ts +++ b/server/src/middleware/logger.ts @@ -5,6 +5,7 @@ import { pinoHttp } from "pino-http"; import { readConfigFile } from "../config-file.js"; import { resolveDefaultLogsDir, resolveHomeAwarePath } from "../home-paths.js"; import { shouldSilenceHttpSuccessLog } from "./http-log-policy.js"; +import { redactSensitive } from "./redact-sensitive.js"; function resolveServerLogDir(): string { const envOverride = process.env.PAPERCLIP_LOG_DIR?.trim(); @@ -69,21 +70,21 @@ export const httpLogger = pinoHttp({ if (ctx) { return { errorContext: ctx.error, - reqBody: ctx.reqBody, - reqParams: ctx.reqParams, - reqQuery: ctx.reqQuery, + reqBody: redactSensitive(ctx.reqBody), + reqParams: redactSensitive(ctx.reqParams), + reqQuery: redactSensitive(ctx.reqQuery), }; } const props: Record = {}; const { body, params, query } = req as any; if (body && typeof body === "object" && Object.keys(body).length > 0) { - props.reqBody = body; + props.reqBody = redactSensitive(body); } if (params && typeof params === "object" && Object.keys(params).length > 0) { - props.reqParams = params; + props.reqParams = redactSensitive(params); } if (query && typeof query === "object" && Object.keys(query).length > 0) { - props.reqQuery = query; + props.reqQuery = redactSensitive(query); } if ((req as any).route?.path) { props.routePath = (req as any).route.path; diff --git a/server/src/middleware/redact-sensitive.ts b/server/src/middleware/redact-sensitive.ts new file mode 100644 index 00000000..4b7247a4 --- /dev/null +++ b/server/src/middleware/redact-sensitive.ts @@ -0,0 +1,67 @@ +// Redaction for HTTP log payloads. +// +// `customProps` in logger.ts copies `req.body` / `req.params` / `req.query` +// verbatim into the 4xx/5xx log lines so operators can diagnose. That means +// Better Auth's `POST /api/auth/sign-in/email` body (which has the user's +// plaintext password) and similar payloads (sign-up, reset-password, API +// keys via Authorization header equivalents) end up on disk. +// +// This walker returns a shallow copy of the input with values for sensitive +// keys replaced with the literal string "[REDACTED]". Recurses into nested +// objects/arrays. Caps depth so a hostile or accidental cycle can't pin +// the logger. + +const SENSITIVE_KEYS = new Set([ + "password", + "currentpassword", + "newpassword", + "passwordconfirmation", + "password_confirmation", + "passwordconfirm", + "password_confirm", + "confirmpassword", + "confirm_password", + "secret", + "client_secret", + "clientsecret", + "access_token", + "accesstoken", + "refresh_token", + "refreshtoken", + "id_token", + "idtoken", + "api_key", + "apikey", + "authorization", + "auth_token", + "authtoken", + "session_token", + "sessiontoken", + "private_key", + "privatekey", +]); + +const MAX_DEPTH = 6; +const REDACTED = "[REDACTED]"; + +function isSensitiveKey(key: string): boolean { + return SENSITIVE_KEYS.has(key.toLowerCase()); +} + +export function redactSensitive(value: unknown, depth = 0): unknown { + if (depth > MAX_DEPTH) return undefined; + if (value === null || typeof value !== "object") return value; + if (Array.isArray(value)) { + if (depth + 1 > MAX_DEPTH) return undefined; + return value.map((entry) => redactSensitive(entry, depth + 1)); + } + const out: Record = {}; + for (const [key, entry] of Object.entries(value as Record)) { + if (isSensitiveKey(key)) { + out[key] = REDACTED; + continue; + } + out[key] = redactSensitive(entry, depth + 1); + } + return out; +}