fix(security): Bound the redaction patterns and stop truncating query values

intent(security): a second review round found that the previous commit's fix for
over-redaction had introduced a worse problem, plus a denial-of-service vector
decision(query-values): revert to consuming the whole parameter value and restore
any trailing punctuation afterwards, instead of treating it as a terminator —
',', '(' and quotes are legal unencoded in a query value, so stopping at them
left most of a credential in the clear to protect a cosmetic detail
constraint(regex-bounds): the userinfo patterns backtrack across every '@' in a
token, so unbounded quantifiers plus the host lookahead are quadratic; a 64KB
input took ~1s and MCP's `remote` argument is untrusted and unbounded, giving an
event-loop stall. Bounded to 256 chars, which is far above any real userinfo and
brings the same input to ~0.2ms
learned(tradeoffs): the earlier choice traded a security property for diagnostic
readability, which is the wrong direction — the readability fix now happens after
redaction rather than by narrowing what gets redacted

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Kazuki Yamada
2026-08-04 23:12:02 +09:00
parent d5e5614e4d
commit 163af053df
2 changed files with 48 additions and 10 deletions
+20
View File
@@ -102,6 +102,26 @@ describe('urlRedact', () => {
);
});
test('should redact a query value that starts with a URL sub-delimiter', () => {
// ',' and '(' are legal unencoded in a query value, so treating them as
// terminators would leave almost the whole credential in the clear.
expect(redactUrl('https://example.com/r?token=,secret')).toBe('https://example.com/r?token=***');
expect(redactUrl('https://example.com/r?token=(secret)plus')).toBe('https://example.com/r?token=***');
});
test('should stay linear on adversarial input', () => {
// The userinfo patterns backtrack across every '@' in a token. Unbounded,
// this input is quadratic and can stall the event loop of an MCP server
// whose `remote` argument comes from an untrusted client.
const adversarial = `u:${'a@'.repeat(32_000)}host`;
const start = process.hrtime.bigint();
redactUrl(adversarial);
const elapsedMs = Number(process.hrtime.bigint() - start) / 1e6;
expect(elapsedMs).toBeLessThan(100);
});
test('should not swallow the diagnostic text that surrounds a redacted query value', () => {
// The quote and the trailing status are part of git's message, not the token.
expect(redactUrl("fatal: unable to access 'https://example.com/r?token=s3cr3t': HTTP 401")).toBe(