fix(security): redact URI credentials, curl user flags, AWS keys, and PEM keys in McpArgumentSanitizer (#886)

[KhushiTrivediii]

Fixes #886

Changes

• Adds pattern matching in McpArgumentSanitizer to redact URI authority credentials, curl user flags, AWS access key IDs, and PEM private keys.
• Adds unit tests in McpGovernanceReviewTest.

[KhushiTrivediii]

All CI checks are green ✅. This is the broad fix for Issue #886 — redacts URI userinfo, curl -u/--user credentials, AWS access keys, and PEM private keys in McpArgumentSanitizer. Ready for review!

[kshivang]

Thanks @KhushiTrivediii — good catch that URI userinfo, curl -u, AWS key IDs and PEM headers were reaching the approval dialog and the operation ledger unmasked (#886).

A note on the conflict: this branch was cut from dev @ 8d35f7721 (v9.5.21), which has since been rebased forward (the #935/#695 integrations, module reorg, and the Windows persistence fix). The "conflicts" are all in files this PR does not actually change, so rather than merging the stale snapshot I rebased the branch onto current dev (bc9aa3b2f) — the contribution now sits on current dev and the PR is mergeable again. Your original commit is preserved in the ancestry.

One substantive repair (agent work, recorded separately, not credited):

• URI userinfo now reuses LogSanitizer.redactUrlUserInfo (the #640 helper) instead of a local regex. The local regex stopped at the first @ in the authority, so postgres://user:p@ss@host redacted to user:[REDACTED]@ss@host — leaving the password tail in the dialog and the ledger. The helper ends the authority at the first /,?,# and treats the last @ as the delimiter, so the whole userinfo is removed. This is exactly the reuse issue #886's suggested-fix calls for.
• PEM private keys are now masked as the whole BEGIN..END block (header-only masking left the key body on the line); truncated fragments still mask to end of line.
• curl -u/--user rule hardened: username part can't contain /,:,@ and the flag can't be mid-token, so a bare URL argument (-u http://host) is left alone while a real user:pass is masked whole.

Added regression tests pinning the last-@ case, a path @/colon, colon-in-password, and full-block PEM masking. McpGovernanceReviewTest + McpArgumentSanitizerAuthSchemeTest pass locally (5+10 tests), ktlint/detekt clean on the touched source sets. (The full ai.rever.boss.mcp.* package SIGABRTs the local test JVM in my environment and on pristine dev too, so I validated the touched classes directly; your CI's full suite is the authoritative run.)

@claude review

[kshivang]

Thanks @KhushiTrivediii — good catch that URI userinfo, curl -u, AWS key IDs and PEM headers were reaching the approval dialog and the operation ledger unmasked (#886).

Conflict note: this branch was cut from dev @ 8d35f7721 (v9.5.21), which has since been rebased forward (the #935/#695 integrations, module reorg, Windows persistence fix). The reported conflicts were all in files this PR does not actually change, so I rebased the branch onto current dev (bc9aa3b2f) instead of merging the stale snapshot — your original commit is preserved in the ancestry and the PR is mergeable again.

Substantive repair (agent work, recorded separately, not credited to you):

• URI userinfo now reuses LogSanitizer.redactUrlUserInfo (the fix(logging): redact credentials embedded in a URL's userinfo before logging #640 helper) instead of a local regex. The local regex stopped at the first @ in the authority, so postgres://user:p@ss@host redacted to user:[REDACTED]@ss@host — leaving the password tail in the dialog and the ledger. The helper ends the authority at the first /,?,# and treats the last @ as the delimiter, so the whole userinfo is removed. This is exactly the reuse issue Security: a credential inside a URI authority reaches the approval dialog and the ledger unmasked #886's suggested-fix calls for.
• PEM private keys are now masked as the whole BEGIN..END block (header-only masking left the key body on the line); truncated fragments still mask to end of line.
• curl -u/--user rule hardened: the username part can't contain /,:,@ and the flag can't be mid-token, so a bare URL argument (-u http://host) is left alone while a real user:pass is masked whole (passwords may contain colons).

Added regression tests pinning the last-@ case, a path @/colon, colon-in-password, and full-block PEM masking. McpGovernanceReviewTest + McpArgumentSanitizerAuthSchemeTest pass locally, ktlint/detekt clean on the touched source sets. (The full ai.rever.boss.mcp.* package SIGABRTs the local test JVM in my environment — on pristine dev too — so I validated the touched classes directly; your CI's full suite is the authoritative run.)

@claude review

[claude]

Claude finished @kshivang's task in 5m 22s —— View job

---

Review: McpArgumentSanitizer credential shapes

• Read the PR diff against origin/dev
• Read McpArgumentSanitizer in full + LogSanitizer.redactUserInfo/redactUrlUserInfo
• Check pattern correctness, ordering interactions, bypasses
• Check test coverage and the consuming governance surfaces
• Post review

The direction is right and the rebase repair is the correct call — redactUrlUserInfo is the helper #886's suggested fix names, and the last-@ bug it fixes was real. I hand-traced every new pattern against the existing rules; the ordering is sound (redactUrlUserInfo first cannot swallow a later shape, and the (?!//) guard genuinely keeps -u redis://… and -u http://… intact, which I verified against both new tests). Nothing here regresses McpArgumentSanitizerAuthSchemeTest — none of its fixtures carry an authority @.

What follows are gaps I'd want closed before this lands, because each one is the exact shape the PR claims to cover. These surfaces are the approval dialog and the on-disk McpOperationLedger, so a miss is a plaintext credential written to disk.

---

1. The --user= and -uadmin: forms leak — and the KDoc says they don't

McpArgumentSanitizer.kt:56 requires \s+ after the flag:

Regex("""(?i)(?<![A-Za-z0-9_-])(-u|--user)\s+([^:/\s@]+):(?!//)([^\s]*)""")

but McpArgumentSanitizer.kt:49 documents it as "(space or = form)". The = form is not implemented:

input	result
curl --user=admin:hunter2 https://x	unchanged — password in the dialog and the ledger
curl -uadmin:hunter2 https://x	unchanged

Both are valid curl (long options take --opt=value; short options take an attached argument), and -uadmin:… is the form that shows up in copy-pasted one-liners. Tracing the --user= case: at index 0 the -u alternative needs -u and sees --; at index 1 the lookbehind (?<![A-Za-z0-9_-]) sees the leading - and blocks. So there is no match at all, not a partial one.

Suggest (-u|--user)\s*[= \t]\s* style separation, or (?:\s+|=) for the long form plus an attached-argument branch for -u. At minimum the KDoc must stop claiming the = form.

Fix this →

2. An email username defeats the whole rule

[^:/\s@]+ forbids @ in the username, so curl -u alice@example.com:hunter2 matches nothing and the password survives intact. Email-as-username is the ordinary shape for Jira, Confluence, Bitbucket app passwords and most SaaS basic auth.

The @ exclusion isn't free to drop, though — I checked what it buys, and it's rsync -u user@host:/srv/data /tmp (-u is rsync's --update), which would otherwise be mangled. So this needs a narrower rule rather than a straight relaxation: allowing @ in the username while requiring the password not to begin with / would catch the email case and still leave user@host:/srv/… alone. Worth doing at least for the unambiguous --user long form, where there is no --update collision.

3. -----BEGIN PGP PRIVATE KEY BLOCK----- is not matched

McpArgumentSanitizer.kt:44 requires PRIVATE KEY----- immediately after the optional label:

-----BEGIN (?:[A-Z0-9-]+ )?PRIVATE KEY-----

GPG's armor header is -----BEGIN PGP PRIVATE KEY BLOCK----- — the trailing BLOCK means neither the BEGIN nor the END marker matches, so an exported GPG secret key passes through whole. RSA / EC / DSA / OPENSSH / ENCRYPTED are all fine. Allowing an optional BLOCK before the closing dashes covers it.

Fix this →

4. rsync -u host:/srv/data is over-masked into host:[REDACTED]

(?!//) blocks a double slash only, so a single-slash remote path is read as a password:

rsync -u host:/srv/data /tmp   ->   rsync -u host:[REDACTED] /tmp

This one matters more than an ordinary false positive because it contradicts this class's stated reason for existing (McpArgumentSanitizer.kt:13-19): a path is "the thing an operator most needs to read before approving a mutating tool call". Masking the destination of a file-copying command is the failure mode the whole class is narrower-than-LogSanitizer to avoid.

5. The \-in-authority caveat now reaches a consent surface, not just a log

LogSanitizer.redactUserInfo's own KDoc records that \ does not end an authority, so https://evil.example\@good.example/x is rendered as https://[REDACTED]@good.example/x — "hiding the host that was actually visited." That trade-off was reasoned about for logs. Here the output is what an operator reads before granting a mutating call, and under WHATWG parsing (which Chromium/JxBrowser uses) the host actually reached is evil.example. A tool argument is attacker-influenceable in exactly the way this file's governance docs assume, so an argument can now render a trusted-looking host in the approval dialog.

I don't think this blocks the PR — it's inherited from #640, not introduced here, and it's strictly better than the unredacted status quo. But it should be written down at the call site (:137-139 is the natural place), because "this helper's documented limitations were accepted for logs" is not the same acceptance as "accepted for a consent dialog." Same class of note this repo already keeps for the telemetry bus.

6. \s+ crosses newlines, and the replacement rewrites it to a space

$1 $2:[REDACTED] normalises the separator, so -u\nadmin:pass comes out as -u admin:[REDACTED] — one line where there were two. The sibling rule right above deliberately uses [ \t] for precisely this reason, and there is an existing test pinning it: McpArgumentSanitizerAuthSchemeTest.kt:61, "authorization label does not consume command text on the next line". [ \t]+ here would match the established convention, and echoing the matched separator instead of a literal space would keep the sanitized text a faithful rendering of the command being approved.

7. AWS coverage notes (minor, fail-open)

• The trailing \b means AKIA + more than 16 following uppercase/digit characters matches nothing — a malformed or concatenated token is passed through rather than masked. Real IDs are exactly 20, so this is low risk, but it fails open where the rest of the file fails closed.
• Only AKIA/ASIA of the ~10 AWS id prefixes (AROA, AIDA, AIPA, ANPA, APKA, …). Defensible scope — worth one KDoc line saying so.
• The secret access key is still exposed in the positional form: aws configure set aws_secret_access_key wJalr… has no :/=, so sensitiveAssignment never fires and sanitizeMap's key rules don't see inside a single command string. A 40-char base64 shape is too false-positive-prone to match blindly, so I'd record it as a known limitation rather than chase it.

8. Smaller items

• McpArgumentSanitizer.kt:45 — with (?s), a truncated BEGIN block will lazily scan forward to any later END anywhere in the string, swallowing unrelated text between two keys. Safe direction, worth a KDoc sentence.
• :56 — capture group 3 is never referenced by the replacement; make it \S* non-capturing. ([^\s]* → \S* while you're there.)
• Username visibility is inconsistent between the two new rules: the URI path drops the username along with the password (postgres://[REDACTED]@host) while the curl path keeps it (-u admin:[REDACTED]). Both are defensible, but an operator judging which account a call authenticates as gets an answer in one case and not the other. Fine to keep — just call it out in the KDoc so the next reader doesn't "fix" one to match the other.

9. Test structure

The 73 added lines are one test method, URI userinfo credentials, curl user flags, AWS keys, and PEM keys are redacted, carrying 13 assertions across four independent rules. This repo's convention (AGENTS.md, repeatedly) is that a mutation should fail a named test — here, breaking PEM masking fails a test whose name lists four things, and the first failing assertion masks the remaining twelve. I'd split it into four tests and put them beside McpArgumentSanitizerAuthSchemeTest, which is already the sanitizer-specific home; McpGovernanceReviewTest is otherwise about policy/approval lifecycle.

Missing fixtures, each corresponding to a finding above: --user=, -uadmin:, an email username, BEGIN PGP PRIVATE KEY BLOCK, and rsync -u host:/path. The negative cases that are present (redisPlain, pathAt, curlUrl) are the good part of this test — they're what pins the (?!//) guard.

---

Verdict

Approve-with-changes in spirit: 1, 2 and 3 are the same class of leak the PR exists to close and should be fixed here rather than followed up, since a partial credential filter is worse than a known-absent one — it invites the assumption that the dialog is clean. 4 is a genuine readability regression against this class's stated design. 5 and 6 want comments/consistency, not redesign. Everything else is minor.

One caveat on my own verification: I reviewed by reading and hand-tracing each pattern against the existing rules and the LogSanitizer implementation — I was not able to execute a scratch harness or the Gradle test task in this environment, so treat the table in finding 1 and the traces in 2–4 as careful manual analysis rather than executed output. They're mechanical enough to confirm quickly with a one-line test each, which is also the argument for adding those fixtures.
· branch fix/mcp-sanitizer-uri-credentials-and-key-shapes

[kshivang]

@claude — disposition of the findings, addressed at head ddde4fab (new commits on top of the rebased base):

Fixed here (same class of leak the PR exists to close):

1. --user= / -uadmin: forms — basicAuthFlagPattern now matches -u with a separated or attached argument and --user with a separated or = argument. Regression tests for both spellings added.
2. Email usernames — the username class now admits @ (no /, :, or whitespace); the password may itself contain : or @. The @ exclusion was only protecting rsync -u user@host:/..., which the password guard handles (see 4).
3. GPG armor — PRIVATE KEY(?: BLOCK)? in both BEGIN and END markers; -----BEGIN PGP PRIVATE KEY BLOCK----- is now masked whole (test added; public key blocks stay readable, pinned by a negative test).
4. rsync over-mask — the password may not start with / or \, so -u host:/srv/data and -u http://host stay readable while admin:pass-shaped values are still masked.
5. =/attached-form KDoc lie — gone, the rule now covers what it documents.
6. \s+ crossing newlines — separator is [ \t] (matching the authorization rule's pinned convention) and the matched separator is echoed back, so the sanitized text stays a faithful rendering.

Accepted with a note (not a redesign):
5. The \-in-authority limitation of the shared redactUrlUserInfo helper is now written down at the call site, since the output is a consent surface, not only a log. It's inherited from #640 and strictly better than unredacted.
7. AWS notes — KDoc now states the scope (AKIA/ASIA + 16 chars; other ID prefixes and the 40-char secret key deliberately not matched, with the reason). The trailing-\b fail-open on malformed tokens is documented rather than chased.
8. Username visibility — the deliberate difference (URI drops the username, curl keeps it) is pinned in the KDoc so the next reader doesn't "fix" one to match the other.
9. Test structure — the 13-assertion mega-test is split into four named tests (URI userinfo, curl user flag, AWS key ID, PEM/GPG armor) beside McpArgumentSanitizerAuthSchemeTest, each with its own fixtures including the ones you listed.

Verification: McpArgumentSanitizerAuthSchemeTest 14/14 and McpGovernanceReviewTest 4/4 green (re-run after the fixture fixes), ktlintCheck + detekt clean on the new head.

[claude]

Claude finished @kshivang's task in 7m 29s —— View job

---

Re-review of the disposition at ddde4fab

• Read the new diff against origin/dev
• Re-trace findings 1-4 and 6 against the new patterns
• Check the KDoc notes (5, 7, 8) land where claimed
• Check the four split tests and their fixtures (9)
• Check for regressions in the three sibling test files that exercise sanitizeMessage
• Post verdict

All nine dispositions hold up. I re-traced each pattern and each fixture, and also checked the three other test files that call sanitizeMessage (McpGovernanceReviewTest:61, McpOperationLedgerTest:106,264, and the ten pre-existing tests in this file) — none carries an authority @ or a -u/--user token, so the two new chain entries can't move them. McpGovernanceReviewTest is back to unchanged against dev, which confirms the mega-test really moved rather than being duplicated.

#	Claim	Verified
1	--user= / -uadmin:	✅ --user(?:[ \t]+|=) and the attached -u branch both match; two fixtures
2	Email usernames	✅ [^\s:=/]+ admits @; --user=alice@example.com:… covered too
3	GPG armor	✅ PRIVATE KEY(?: BLOCK)? in both markers, and the PGP PUBLIC KEY BLOCK negative test pins that the label group can't absorb PUBLIC
4	rsync over-mask	✅ (?![/\\]) keeps -u host:/srv/data and -u http://host readable
5	KDoc = lie	✅ gone; the KDoc now describes what the regex does
5b	\-in-authority note at the consent surface	✅ :165-171, the right place
6	\s+ newline	✅ [ \t], and the separator is echoed via $1
7	AWS scope	✅ :36-41 states prefixes, the 40-char secret omission and its reason; AKIA+17 fixture pins the fail-open
8	Username visibility	✅ :73-77, with the "do not 'fix' one to match the other" instruction
9	Test split	✅ four named tests, all the fixtures I listed, plus good negatives (redisPlain, pathAt, curlUrl, public-key block)

Group 3 is non-capturing now too. The KDoc on basicAuthFlagPattern is the best kind — it explains the (?![/\\]) guard by naming the command it protects.

---

Residual: three gaps the tightened rule opens or leaves, all in basicAuthFlagPattern

These are new or newly-reachable, not restatements. The first is the one I'd fix before merge, because it leaves a password tail on the consent surface and in the ledger, and this file already carries a test documenting that exact failure mode for a sibling rule.

A. A quoted password containing a space leaks its tail. The password branch is [^\s]*, which stops at the first space:

curl -u 'admin:hun ter' https://x
  ->  curl -u 'admin:[REDACTED] ter' https://x

ter' is readable. This is precisely the bug the existing test a quoted credential is masked for both quoting styles (:86-98) pins for authorizationHeader, whose comment spells it out: "the bare class used to take 'abc and leave def'". That rule solved it with (?:"[^"]*"|'[^']*'|[^\s&,;}]+); the new rule doesn't reuse the solution, and a space is legal in a basic-auth password, so shell-quoted -u 'user:pass phrase' is the ordinary way to pass one. Same three-branch alternation on the password would close it.

Fix this →

B. --proxy-user leaks while its own short form -U is masked. (?i) makes the -u branch match -U, which is curl's --proxy-user and carries the same user:password — good, and probably unintentional. But the long spelling isn't in the alternation, and the lookbehind (?<![A-Za-z0-9_-]) blocks a match on the -user inside it (the preceding y is in the class), so:

curl -U admin:hunter2 x          ->  curl -U admin:[REDACTED] x
curl --proxy-user admin:hunter2  ->  unchanged

One flag, two spellings, opposite outcomes. Adding --proxy-user to the alternation is a one-token change, and the KDoc's "and the equivalent spellings" then becomes true of the whole flag family.

Fix this →

C. A \-continuation between the flag and its value now leaks. Narrowing to [ \t] was right, but it makes the most common multi-line curl shape invisible:

curl -u \
  admin:hunter2 https://x

Group 2 has to match the trailing \ and then find a :, which is the newline, so there's no match at all and admin:hunter2 reaches the dialog whole. A separator alternative of [ \t]*\\\r?\n[ \t]* covers it without reopening what \s+ would: a trailing backslash is unambiguous evidence of continuation, so the rule still can't wander into an unrelated next line the way the authorization: rule's pinned convention guards against.

Relatedly, the newline fixture at :182-184 asserts the absence of masking, and its comment reads as if the only thing at stake were faithful rendering. What it actually pins is that admin:hunter2 is written to the ledger in plaintext. Keeping the fixture is fine; the comment should say that outright, the way the rest of this file's comments do.

Fix this →

Minor, no action needed beyond a KDoc line

• Over-masks that hide a fact the dialog exists to show, all in the fail-closed direction and none a leak: docker run -u 1000:1000 img -> -u 1000:[REDACTED] (hides which uid a container runs as — the most common non-curl -u X:Y there is), a relative rsync remote spec -u host:srv/data (the (?![/\\]) guard only covers absolute paths), and a single-dash -user:value flag. Requiring the username not to be all digits would take the docker case out cheaply; otherwise one KDoc sentence saying -u X:Y on a non-curl tool is masked deliberately would stop the next reader treating it as a bug.
• mysql -uroot -pSecret still passes the password through. Out of this PR's scope and genuinely hard (-p is a port flag on half the tools that have one), so worth recording beside the AWS-secret note as a known limitation rather than chased.

Verdict

The nine findings are genuinely addressed rather than papered over, and the KDoc now carries the reasoning at the density this repo's AGENTS.md asks for — the (?![/\\]) explanation and the "do not 'fix' one to match the other" note are both the kind of comment that survives a year. A is the one I'd close in this PR: it is a password tail on a consent surface, and the fix is to reuse an alternation already sitting 30 lines away in the same file. B and C are one-line pattern additions each. D/E are comments.

Caveat on my own verification, same as last round: python3 and ./gradlew are both outside this environment's allowed commands, so findings A-C are careful hand-traces against the Java regex semantics, not executed output. Each is a one-line fixture to confirm, which is also the argument for adding them.
· branch fix/mcp-sanitizer-uri-credentials-and-key-shapes

[fluck-boss]

@KhushiTrivediii thanks, this sanitizer work is useful. One thing from Claude's latest pass needs to land before merge: the new URI-credential rule can still leave a password tail visible on the consent surface and in the ledger. There's already a test in that file documenting the same failure for a sibling rule, so a matching test here should be quick. I've set this to draft. Mark it ready once that's fixed and I'll merge it.

[fluck-boss]

@KhushiTrivediii thanks for the credential-redaction work. I checked the author patch and the current PR still includes old dev history and a workflow-file edit; its merge base is 3b2ba0f9, 27 first-parent commits behind current dev. Please re-cut only the sanitizer changes and focused tests onto current dev, without the unrelated workflow/history changes. In that re-cut, pin the URI credential tail case from the earlier security review so no password tail reaches the consent surface or ledger. Marking draft pending that clean diff and exact-head CI/review.