security(redaction): mask credential-bearing URLs, args and headers in agent config (PEN-2747)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agents read each other's configuration through the agent-facing REST API, and that API masks credentials with its own redactor (redactAgentSecrets → redactAgentConfigPayload → sanitizeRecord) — not the packages/mcp-gateway response scrubber, which never runs here because paperclip's own MCP server is an in-process stdio bridge with no HTTP hop
• That redactor decides what to mask from the key's name, and its fallthrough returns an unrecognized key's string value verbatim, so it fails open on any spelling nobody enumerated
• Credential material carried inside a URL has no key name to catch: mcpServers.*.url matched no tier, and args — the spelling real MCP stdio configs use — was the one synonym missing from the command-args rule
• This matters more than ordinary hygiene because adapterConfig.mcpServers is exactly where an agent's k8s MCP upstream is swapped for a privileged ns-rw or admin tier, so a credential surfacing here is a privilege-escalation shape
• This pull request matches the value instead of adding four more key names, wiring the repo's already-owned URI_CREDENTIAL_RE into the structured path, adding args, and giving headers the allowlist treatment the variable map already had
• The benefit is that the next unenumerated spelling — the fifth *Url key someone adds — is covered by construction rather than by a follow-up patch

Linked Issues or Issue Description

Refs PEN-2370 (ask 3, criteria (a)+(b2) — the response-content axis, on a surface the gateway scrubber does not sit on).
Internal tracking: PEN-2747. Related internal: PEN-2620, PEN-2429.

Related PR (different module, no overlap): #1556 touches packages/mcp-gateway; this PR touches server/src/redaction.ts. No file overlap.

Underlying problem (no public issue exists; following the bug-report shape):

• What happens: redactAgentConfigPayload returns credential material in the clear when it is carried in a URL's userinfo or query component, in an args array, or in any request header not spelled Authorization.
• Why: sanitizeRecord's fallthrough (redaction.ts) hands an unrecognized key's string value back untouched. Three near-misses lined up:
  1. url is in no tier — SECRET_TIER2_STEMS spells base[-_]?url, which requires the literal base, so baseUrl matched and a plain url did not (nor endpoint, webhookUrl, serverUrl, proxyUrl).
  2. URI_CREDENTIAL_RE already existed but was wired only into redactSensitiveText, which sanitizeRecord reaches only for COMMAND_PAYLOAD_KEY_RE keys — a url value never got there.
  3. COMMAND_ARGS_PAYLOAD_KEY_RE covers commandArgs|command_args|argv but not args, the spelling real MCP stdio configs use (services/tool-access.ts). argv was masked; the identical value under args was not.
• Why it survived: the one test covering mcpServers.*.url used a credential-free fixture, so it asserted the pass-through as correct output, and expectNoPlaintextSecrets only grepped for four fixture secrets, none of them URL-borne.

No credential value was retrieved, quoted, or committed anywhere in this change. The finding was derived from source plus execution of the repo's own redaction module against synthetic values; all fixture secrets in this PR are invented strings.

What Changed

• redaction.ts — value-shaped URI masking on the structured path. New redactUriCredentialsInValue, called from sanitizeValue, applies the existing URI_CREDENTIAL_RE plus a new replacing sibling for query/fragment credential params. This covers url/endpoint/webhookUrl/serverUrl/proxyUrl in one place rather than by enumerating key names.
• Masking is surgical. Only the credential component is replaced — scheme, principal, host, port and path survive, so a credential-free URL round-trips byte-identical. Knowing which upstream and which principal an agent is pointed at is the diagnostic value these read paths exist for; blanking whole URLs would destroy it.
• args added, gated on agentConfig rather than widened into COMMAND_ARGS_PAYLOAD_KEY_RE. A bare args array in a generic event payload is not command argv (tool-call arguments, job parameters), and this module already excludes that deliberately — see the "does not treat bare args payloads as command args" case in redaction.test.ts. Inside an agent config the key is unambiguous, so the widening is scoped there and only there.
• headers masked allowlist-style, matching how the variable map is already handled. Authorization had been masked only incidentally (it contains the "auth" Tier-1 stem), leaving X-Tenant-Signature and every other vendor spelling in the clear. The exemption list is short and closed (content negotiation, cache control, user agent).
• routes/agents.ts — round-trip guard fixed. restoreRedactedAdapterValue moves from an equality test to a substring test: the URI rule masks a component, so a round-tripped value now contains the sentinel rather than equalling it. Left as equality, a caller PATCHing a previously-read config would persist a broken upstream URL — the same class of failure as a sentinel written back over live config, in a new shape.
• Tests: four credential shapes added to the shared fixture so every expectNoPlaintextSecrets case sweeps them, four new explicit assertions, and the x-safe-routing-header assertion flipped to masked.

Verification

node_modules/.bin/vitest run server/src/__tests__/agent-secret-redaction.test.ts
  → 48 passed

node_modules/.bin/vitest run server/src/__tests__/redaction.test.ts \
  server/src/__tests__/issue-comment-redaction.test.ts \
  server/src/__tests__/log-redaction.test.ts \
  server/src/__tests__/pen2370-mirror-redaction.test.ts
  → 61 passed  (confirms the unconditional sanitizeValue change does not disturb the generic event path)

node_modules/.bin/vitest run server/src/__tests__/agents-pending-approval-config.test.ts \
  server/src/__tests__/agents-service-secret-bindings.test.ts \
  server/src/__tests__/approval-payload-title-guard.test.ts \
  server/src/__tests__/approvals-service.test.ts \
  server/src/__tests__/approval-insert.test.ts
  → 70 passed  (round-trip guard + hire_agent approval payload path)

The new tests were verified to fail against the unfixed redactor, which is the check that matters here — a regression test can pass against unfixed code and prove nothing. Reverting only the two source files to their pre-fix state and re-running takes the suite from 1 failure to 11:

• the 4 new explicit cases (userinfo URL, query-borne credential, non-Authorization header, args),
• the header-allowlist assertion,
• and 6 mutating-route expectNoPlaintextSecrets sweeps that previously had no credential-bearing material to catch — which is the point of putting the fixtures on the shared object rather than in bespoke tests.

Negative direction checked too: the credential-free gbrain URL round-trips byte-identical, confirming the rule masks a credential component rather than blanking URLs.

Risks

Low-to-moderate, and the moderate part is deliberate over-masking on a read path.

• headers values outside the exemption list are now masked, including benign routing headers such as the fixture's x-safe-routing-header. This is the allowlist working as designed — a denylist over header names has no bounded vocabulary to enumerate — but it is a visible behavioural change for anyone reading an agent config expecting a routing header value. Over-masking a response is recoverable; under-masking is not.
• The round-trip guard change is the load-bearing safety fix here. Without it, the substring-vs-equality mismatch would let a masked URL be persisted back over live config. It is covered by the approval/pending-config suites above.
• redactUriCredentialsInValue runs on every sanitized string, not just agent configs. It is gated on the :// scheme separator, so prose containing token= is untouched, and a bare user@host (no :) or a credential-free https://host:8080/path (no @) cannot match. The 61 passing generic-path tests confirm no collateral change.
• No migration, no schema change, no API contract change.

Scope honesty — what this PR does NOT close. The hire_agent approval read path returns a real agent's live adapterConfig under company_scope:read (auto-allowed to every same-company agent), while GET /agents/:id requires agent_config:read. This PR closes the credential-value leak on that path — redactApprovalPayloadByType routes hire_agent through redactAgentConfigPayload, so every fix here applies there too. It does not equalize the two gates, so the residual disclosure is which upstream and which principal an agent is pointed at, not the credential. That is an authorization change in a different subsystem with a different risk profile and is tracked separately rather than bundled in here.

Model Used

Claude Opus 4.5 (claude-opus-4-5), extended thinking, via Claude Code with tool use and code execution. Model ID as reported by the running harness; 1M context.

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
• 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 redaction only, no UI surface changed
• I have updated relevant documentation to reflect my changes — the rationale is documented as module-level comments at each rule, which is where this module keeps its reasoning
• 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
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: PEN-2620
🔗 Paperclip issue: PEN-2747
🔗 Paperclip issue: PEN-2429

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The structured sanitizer now masks credential-bearing URI components by value shape rather than relying on an incomplete key-name list.
• args is scoped to agent configuration, avoiding a regression in generic event payload redaction semantics.
• Header handling uses a small benign allowlist, and the PATCH round-trip guard preserves existing values when a partially redacted URL is submitted back.
• Tests add URL userinfo, query credentials, non-Authorization headers, and MCP stdio argument coverage, including shared-fixture sweeps.

Recommended Action

1. Merge when the repository's required CI checks are green.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The structured sanitizer now masks credential-bearing URI components by value shape rather than relying on an incomplete key-name list.
• args is scoped to agent configuration, avoiding a regression in generic event payload redaction semantics.
• Header handling uses a small benign allowlist, and the PATCH round-trip guard preserves existing values when a partially redacted URL is submitted back.
• Tests add URL userinfo, query credentials, non-Authorization headers, and MCP stdio argument coverage, including shared-fixture sweeps.

Recommended Action

1. Merge when the repository's required CI checks are green.