security(logger): contain credential-bearing config in 4xx/5xx request-body logs (PEN-2843)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agents are defined by an adapterConfig, whose env map holds the credentials that agent runs with — provider API keys, base URLs, tokens
• The HTTP logger copies the entire request body into the log line on every 4xx/5xx so operators can diagnose failures, passing it through redactSensitive first
• That redactor is an exact-match denylist of key names (password, access_token, api_key, …). It does not contain env, adapterConfig, runtimeConfig or mcpServers, and it cannot contain an agent's actual variable names because those are arbitrary
• PATCH /agents/:id and the hire_agent approval path both accept adapterConfig in the request body, so any validation error, 403 or conflict on those routes wrote live credential values verbatim to stdout and to the on-disk log file
• This pull request stops treating the problem as "which key names are secret" and instead names the few containers whose contents are secret by construction, masking every scalar leaf beneath them at any depth regardless of name
• The benefit is that a credential variable added to an agent's env next month is covered without anyone editing this file — a property a name denylist cannot have — while key names survive so the 4xx log still records which variables were set

Linked Issues or Issue Description

• Refs PEN-2843 (door ops(opencode-k8s): bump to a1e0a0c (activates auto-compact) #11 on the PEN-2370 credential-disclosure series)
• Refs PEN-2370 — ask 1 ("return names with values elided") and ask 3 ("fix the invariant, not the instance")

No GitHub issue exists; the finding is tracked in Paperclip. Summary, following the bug template:

What happened. customProps in server/src/middleware/logger.ts logs reqBody on every response >= 400. redactSensitive masks only keys present in an exact-match SENSITIVE_KEYS set. env, adapterConfig, runtimeConfig and mcpServers are absent from it, and a real variable name such as OPENAI_API_KEY does not match either (the set holds the literals api_key / apikey; it does not substring-match). A request to a route that accepts adapterConfig — PATCH /agents/:id, or the hire_agent approval payload — which returns any 4xx therefore emits those values in the clear.

Expected. Credential material never reaches a log line.

Impact. Log output is readable by anything with log access, including the read-only Kubernetes grant's pods_log. No attacker is required: a routine validation rejection is sufficient.

Not reproduced against live data. No pod was probed and no pod log was read — reading it is the exposure. The finding is established from source and pinned by unit tests over synthetic values.

What Changed

• server/src/middleware/redact-sensitive.ts
  • Added SENSITIVE_CONTAINER_KEYS — adapterConfig, runtimeConfig, env, mcpServers, headers — keys whose entire subtree is credential material by construction.
  • Added redactContainer, which masks every scalar leaf beneath such a key at any depth while preserving structure and key names, so the log still says which variables were set.
  • Non-object container values (scalars, JSON strings) are masked outright rather than returned; arrays are mapped rather than recursed past. These are the two shapes that have bypassed walkers on this series before.
  • Reuses the existing MAX_DEPTH cap, so the cycle and depth-cap contracts are unchanged.
• server/src/__tests__/redact-sensitive.test.ts — six tests covering value-elision-with-name-preservation, deep nesting, the array shape, the string shape, mcpServers/headers values the scalar rules miss, and an over-reach guard.

Verification

cd server
npx vitest run src/__tests__/redact-sensitive.test.ts   # 16/16
npx vitest run src/__tests__/redact-sensitive.test.ts \
  src/__tests__/redaction.test.ts src/__tests__/log-redaction.test.ts \
  src/__tests__/pen2370-mirror-redaction.test.ts \
  src/__tests__/agent-secret-redaction.test.ts          # 126/126
npx tsc --noEmit                                        # clean

Fail-first confirmed. 5 of the 6 new tests fail on unfixed master, with the planted secret visible in the failure text — e.g. expected '{"adapterConfig":[{"env":{"K":"array-secret"}}]}' not to contain 'array-secret'.

The 6th is an over-reach guard (environment, configVersion, envoy must stay untouched) and correctly passes on both trees.

One test is worth calling out because it initially proved nothing: the mcpServers/headers case first used a url key and an Authorization header, and passed on unfixed code — it was being carried by the existing URL-part stripper and the existing authorization denylist entry, not by the new rule. It was rewritten to use a token in a path segment under a non-urlish key and a vendor-specific header name, so it now genuinely exercises the container path and fails without the fix.

Blast radius. middleware/redact-sensitive.ts has exactly one production importer, middleware/logger.ts. This changes log output only; no response path is affected.

Risks

Low, and bounded to log output.

• Diagnostic loss. Values under the five container keys no longer appear in 4xx/5xx logs. Key names are deliberately preserved, which keeps the diagnostic that matters (which variables were set) and matches PEN-2370 ask 1. Values under those keys were never safe to log.
• Over-reach. Matching is exact on the lower-cased key, so environment, envoy and configVersion are unaffected; pinned by a test.
• headers is the broadest entry. Included deliberately — credential-bearing MCP headers are door fix(agents): drop redacted env sentinel ("***") on PATCH/POST round-trips #8's material — and it only ever affects a headers object inside a logged request body.
• No schema, migration, API contract or response-shape change.

Model Used

• Claude (Anthropic), claude-opus-5[1m] — 1M context, extended thinking, agentic tool use (repository read/write, test execution, GitHub API).

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above — the only open PR matching redaction/logging terms is fix(security): correct false claim in sensitive-env-guard.ts about adapter parity #1091, which touches packages/plugins/sandbox-providers/kubernetes/src/sensitive-env-guard.ts, a different file in a different package; no overlap
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, server-side logging only
• I have updated relevant documentation to reflect my changes — the rule and its rationale are documented in-file, where the next editor of the denylist will read them
• I have considered and documented any risks above
• All Paperclip CI gates are green — verified at head a63ff7bd3: 20 SUCCESS, 0 failing, 1 SKIPPED (Storybook visual regression); mergeStateStatus CLEAN
• Greptile is 5/5 — not applicable on this repository. Greptile does not review Blockcast/paperclip; the sibling PRs in this same series (security(mcp-gateway): let a grant enumerate the tools it exposes (PEN-2735) #1573, fix(approvals): gate hire_agent embedded agent config on agent_config:read #1574, security(portability): redact agent credentials on the company export path (PEN-2778) #1578) each merged with allyblockcast as the only reviewer. Ally's review at this exact head returned 0 Critical / 0 Important / 0 Suggestions.
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: PEN-2843
🔗 Paperclip issue: PEN-2370

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a63ff7b

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The redaction policy correctly handles credential-bearing containers by exact key, preserving diagnostic names while masking scalar leaves.
• Array and scalar/string-shaped containers are covered explicitly, and the implementation retains the existing depth cap behavior.
• Tests cover deep nesting, bypass-prone shapes, and look-alike keys to constrain overreach.

Recommended Action

1. No Critical or Important issues found. The App-authored PR is reviewed as a formal comment at the exact head.