[nitininhouse]

📋 Pull Request

Description

Fixes #886. Follow-up to #837, on dev.

#640 added LogSanitizer.redactUrlUserInfo and used it to redact URI credentials before logging.
McpArgumentSanitizer is a separate sanitizer and never picked it up, so the same password is
hidden in a log entry and visible in the MCP ledger:

a database password in a log entry   ->  hidden   (since #640)
the same password in the MCP ledger  ->  visible

A credential in a URI authority has no sensitive key name, no assignment and no recognisable token
prefix, so none of the existing rules here saw it. postgres://admin:hunter2@host has a colon, but
it is not an assignment and hunter2 has no prefix to match on.

This calls the helper #640 already added rather than adding a rule of its own:

fun sanitizeMessage(text: String): String =
    LogSanitizer
        .redactUrlUserInfo(text)
        .replace(credentialShapePattern, "[REDACTED]")
        ...

curl -u user:pass is not a URI, so it keeps a small rule of its own. Its (?!//) guard is
load-bearing: redis-cli -u takes a URL, and without it -u redis://cache.example.invalid:6379
matches with redis as the user and the rest as the secret, blanking a line that holds no
credential at all.

Why reuse rather than a local rule

I wrote a local regex first, and it was worse in a way worth recording. A password containing an
@ is ordinary, and stopping at the first @ leaves its tail behind:

local regex       -> psql postgres://user:[REDACTED]@ss@db.example.invalid/app
redactUrlUserInfo -> psql postgres://[REDACTED]@db.example.invalid/app

#640's implementation already ends the authority at the first /, ? or # and treats the LAST
@ in it as the delimiter, which its own KDoc calls out. A second implementation here would be a
second dialect of URI parsing to keep in step, for no benefit. The source carries a comment saying
so, so it does not get quietly reintroduced.

Behaviour note

The helper masks the whole userinfo, so the approval dialog shows postgres://[REDACTED]@prod-db
rather than keeping the username visible. That matches the logging path, and one convention seems
better than two. Say the word if you would rather the MCP surface keep the user.

Validation

McpArgumentSanitizerUriCredentialTest, 9 tests. Three exist because of specific mistakes:

• a password containing an @ is removed whole, tail included (the case above)
• redis-cli -u redis://cache.example.invalid:6379 is left alone, so a URI with a port and no
  credentials is not read as user cache and password 6379
• https://api.example.invalid/a:b@c/d is left alone, since a colon and an @ in a path are
  not userinfo

The rest pin that every common scheme is covered, that an ordinary URL is untouched, that the host
stays readable, that redaction stops at the end of a line, and that the behaviour merged in #837
still holds.

Sample hosts are RFC 2606 .invalid domains on purpose. A fixture shaped like a real provider host
trips GitHub secret scanning on every clone and fork, even with a fabricated credential.

Full :composeApp:desktopTest --tests "ai.rever.boss.mcp.*" and :composeApp:ktlintCheck green
locally. Environment: macOS 15.3 arm64, JDK 17.0.20.1 (Temurin), Gradle 9.7.1.

Deliberately out of scope

#886 also records AWS AKIA… keys and PEM private key blocks going through unmasked. Those extend
credentialShapePattern rather than reusing a URI helper, so they are a different mechanism. Happy
to follow up separately, or to fold them in if you would rather review one change.

🔄 Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
• 🔧 Configuration change (changes to build scripts, CI/CD, or project configuration)
• 📚 Documentation (documentation only changes)
• 🧹 Refactoring (code change that neither fixes a bug nor adds a feature)
• ⚡ Performance improvement (code change that improves performance)
• 🧪 Tests (adding missing tests or correcting existing tests)

📦 Version Impact

• No version change needed
• Patch version (bug fixes, small improvements) - x.x.+1
• Minor version (new features, enhancements) - x.+1.0
• Major version (breaking changes) - +1.0.0

✅ Testing Checklist

🧪 Local Testing

• I have tested this change locally on my development machine
• All existing tests pass with my changes
• I have added new tests for new functionality (if applicable)
• I have verified the fix/feature works as intended

🖥️ Platform Testing

• 🍎 macOS - Tested and working
• 🪟 Windows - Tested and working
• 🐧 Linux - Tested and working
• 📱 Mobile (iOS/Android) - Tested if applicable

One call and one regex in common code with no platform-specific behaviour, and the tests are plain
JVM unit tests, so the macOS run should carry across. I do not have Windows or Linux hardware to
confirm that.

[kshivang]

@claude review

[claude]

Claude finished @kshivang's task in 4m 10s —— View job

---

Review: redact URI userinfo in MCP arguments

• Read the diff against origin/dev
• Read McpArgumentSanitizer in full, LogSanitizer.redactUrlUserInfo / findAuthorityEnd / endsAuthority, and the sibling McpArgumentSanitizerAuthSchemeTest
• Check rule ordering, regex edge cases, and the ledger/dialog call path
• Post review

---

The core call is right. Reusing LogSanitizer.redactUrlUserInfo rather than writing a second URI dialect is the correct trade, the PR body's user:p@ss@host worked example is the evidence for it, and the helper is a linear scan rather than a regex so it costs nothing on the hot path. Ordering is also right: the helper runs first, and since it only ever inserts the literal [REDACTED] (no sensitive keyword, no :/=), it cannot confuse the four rules that follow. .invalid fixtures and failure-mode-naming test names are both good.

My findings are all in basicAuthFlag - the one piece of this PR that is a new local rule rather than reuse - plus one consequence of the reuse worth writing down.

1. [^\s]+ swallows a shell separator, hiding a command from the approval dialog (medium)

McpArgumentSanitizer.kt:120. Every other value-consuming rule in this file deliberately stops at a separator - sensitiveAssignment and authorizationHeader both use [^\s&,;}]+, bearer uses [^\s"',;}]+. The new rule uses bare [^\s]+, so:

curl -u admin:hunter2;rm -rf /important/data
  ->  curl -u [REDACTED] -rf /important/data

The credential is gone, but so is ;rm. This surface is not a log: an operator reads the sanitized string and then presses Approve, so a redaction that can absorb ;rm, &&curl, or |sh is worse than a partially-leaked password tail. The sibling test file already pins this exact class for the authorization rule (authorization label does not consume command text on the next line), and the file's own KDoc is explicit that readability of the command is the reason this sanitizer is narrower than LogSanitizer.sanitizeMap.

Suggest matching the existing class ([^\s&,;}]+, possibly plus |) and adding a test that both the separator and the following command survive. The trade is deliberate and already made three times in this file - a password containing & leaks its tail rather than hiding a command.

Fix this →

2. No quoted-value alternative, so a password with a space leaks its tail (medium-low)

Same line. sensitiveAssignment and authorizationHeader both carry (?:"[^"]*"|'[^']*'|…); basicAuthFlag does not, so:

curl -u 'admin:hun ter2' https://api.example.invalid
  ->  curl -u [REDACTED] ter2' https://api.example.invalid

This is the failure mode a quoted credential is masked for both quoting styles in the sibling file exists to prevent - and note its comment there, that a whole-string contains check passes while the credential is still readable, so the test for this needs to assert on the tail specifically. The quote sits before the user here, so the fix is an alternation over the whole argument rather than over the value:

Regex("""(?i)(--user|-u)([ \t]+)(?:"[^"]*:[^"]*"|'[^']*:[^']*'|[^\s:/@]+:(?!//)[^\s&,;}]+)""")

Fix this →

3. (?i) and no left boundary redact things that are not credentials (low)

docker run -u 1000:1000 alpine becomes docker run -u [REDACTED] alpine. No leak, but it blanks operator-relevant detail in the dialog this file's KDoc says must stay readable, and -u user:group is an ordinary docker/podman invocation. Case-insensitivity is separately defensible (curl -U is --proxy-user, genuinely a credential), so this is mostly about the missing left boundary - a (?<![\w-]) guard would at least stop foo-u a:b matching. Worth a line in the KDoc if you keep it as-is, since (?!//) already has one and the reader will wonder why this case was not also considered.

4. The helper's documented \ limitation lands harder here than in a log (low, but record it)

redactUserInfo's own KDoc says \ does not end an authority, so https://evil.example\@good.example/x is rendered https://[REDACTED]@good.example/x while WHATWG parsing actually loads evil.example. In a log that is a documented inaccuracy. On this surface it is a spoofing vector: the operator approves a call against a host that is not the host the redaction showed them, and the redaction is what hid it. Same for the protocol-relative //user:pass@host case, which the helper leaves alone entirely and nothing in this file catches.

I would not block on it - the PR's reuse argument is still the right one, and a divergent local copy would be worse. But since this PR is what makes the logging helper load-bearing for a human security decision, it deserves either a sentence in the new KDoc at :124 or a follow-up issue, so the next reader does not have to rediscover it from plugin-logging.

Nits

• Stale KDoc link. McpArgumentSanitizerUriCredentialTest.kt:15 references [McpArgumentSanitizer.uriUserInfo], which does not exist - a leftover from the local-regex draft, and it names the mechanism this PR deliberately did not use. Should be [LogSanitizer.redactUrlUserInfo].
• Host inconsistency in the new KDoc. McpArgumentSanitizer.kt:115 says //cache.internal:6379 two lines after the example host cache.example.invalid. Given the PR body's point about .invalid fixtures, worth making consistent.
• No em-dashes in either file, per AGENTS.md. The one line over 140 chars is the command helper and it carries @Suppress("MaxLineLength"), matching the sibling test.

On the behaviour question you raised

Masking the username along with the password is the right call. Two conventions for one string is the thing that made #640 necessary here in the first place, the host stays readable so the operator can still tell which database is being reached, and a username is routinely the credential in these URIs anyway (x-access-token, default, an access key id).

Verification

I read the code and traced the cases above by hand; I was not able to run :composeApp:desktopTest or ktlintCheck in this environment (Gradle is outside my allowed commands here), so I am relying on your local green run for those. Nothing I found should affect the 9 tests as written - findings 1-3 are gaps in coverage rather than failures of the existing assertions.
· branch fix/uri-credential-redaction

[fluck-boss]

Thanks so much for this contribution, @nitininhouse - the work here is genuinely appreciated, and it's clear real care went into it.

This overlaps with #1019 ("test(mcp): state the argument sanitizer's contract as a property, and close the "), which is further along in review and will carry this change forward. To keep review attention focused, I'm closing this one in favor of #1019 - please do review and follow that PR, and you're very welcome to pick up any part of it that still needs help or take on another open issue.

Your contribution credit is noted here, and anything unique in this branch that #1019 misses is still welcome as a follow-up. Thanks again for contributing to BossConsole!