fix(security): Make the redaction patterns linear without length caps

intent(security): the third review round showed the previous commit traded one defect for three — length caps created false negatives, and both the trailing punctuation restore and its regex were themselves broken decision(userinfo-match): express the userinfo as a repetition of `<chunk>@` instead of a greedy `[^/\s]*@`. Both find the same last '@', but the repetition is unambiguous — one '@' per iteration — so it is linear without needing a length cap, and a 1000-character JWT credential is redacted again rejected(length-caps): capping the userinfo at 256 characters silently returned long credentials verbatim; JWT-style tokens routinely exceed that rejected(punctuation-restore): re-appending trailing punctuation after the placeholder leaked the tail of values like `token=secret:...`, restored a punctuation-only credential in full, and was itself quadratic. The closing quote of a log line is now absorbed instead — losing a character of the message is the correct side of that trade constraint(host-lookahead): the scp-style lookahead stays bounded at 256, which is above the 253-character hostname limit in RFC 1035, because it re-runs on every backtrack and is the one remaining quadratic risk learned(perf-testing): a single absolute timing threshold does not distinguish linear from quadratic; the test now compares two input sizes and asserts the growth ratio Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-19 06:00:49 +08:00 · 2026-08-04 23:20:04 +09:00
parent 163af053df
commit 91c32321a8
2 changed files with 59 additions and 44 deletions
@@ -28,47 +28,45 @@ const CREDENTIAL_QUERY_KEYS = [
  'token',
 ];

-// Upper bounds on the credential-bearing spans. A real userinfo or host is far
-// shorter than these, but the quantifiers must be bounded: the patterns below
-// backtrack across every '@' in a token, and an unbounded search combined with
-// the lookahead is quadratic in the token length, which an untrusted caller (an
-// MCP client passing `remote`) could use to stall the event loop.
-const MAX_USERINFO = 256;
+// A hostname cannot exceed 253 characters (RFC 1035), so bounding the lookahead
+// below costs nothing real. It has to be bounded: the userinfo repetition
+// backtracks once per '@' in the token, and re-running an unbounded lookahead on
+// each of those attempts is quadratic in the token length — which an untrusted
+// caller (an MCP client passing `remote`) could use to stall the event loop.
 const MAX_HOST = 256;

-// `scheme://userinfo@host`. The userinfo runs to the last '@' before the path
-// begins. All of it is replaced, not just the password half: the credential is
-// often the username alone (`https://<token>@github.com/...`).
+// The span from the start of the userinfo through the last '@' before the path
+// begins, written as a repetition of `<chunk>@` rather than a greedy
+// `[^/\s]*@`. Both find the same last '@', but the repetition is unambiguous —
+// each iteration consumes exactly one '@' — so it cannot backtrack quadratically
+// on a token containing many of them.
 //
 // Only '/' and whitespace terminate the search, deliberately not '?' or '#': a
 // password containing either of those unencoded is malformed but still a real
 // secret, and stopping early would leave it in the clear. The cost is that a
 // path-less URL with an '@' inside its query gets over-redacted, which is a far
 // better failure than printing a credential.
-const SCHEME_USERINFO_PATTERN = new RegExp(`([a-z][a-z0-9+.-]{0,31}://)[^/\\s]{0,${MAX_USERINFO}}@`, 'gi');
+const USERINFO = '(?:[^@/\\s]*@)+';
+
+// `scheme://userinfo@host`. All of the userinfo is replaced, not just the
+// password half: the credential is often the username alone
+// (`https://<token>@github.com/...`).
+const SCHEME_USERINFO_PATTERN = new RegExp(`([a-z][a-z0-9+.-]{0,31}://)${USERINFO}`, 'gi');

 // scp-style `user:password@host:path`, which carries no scheme. Redacted only
 // when the userinfo contains a ':', so ordinary SSH remotes stay readable:
 // in `git@github.com:owner/repo` the username is a fixed literal, never a secret,
-// and authentication happens out of band via the SSH key. Matching to the last
-// '@' in the token keeps a password that itself contains '@' from surviving, and
-// the trailing lookahead requires the `host:path` that every scp-style remote
-// has, so unrelated text like `failed at 12:30@example.com` is left alone.
-const SCP_USERINFO_PATTERN = new RegExp(
-  `(^|\\s)[^/@\\s:]{1,${MAX_USERINFO}}:[^/\\s]{0,${MAX_USERINFO}}@(?=[^\\s:]{1,${MAX_HOST}}:)`,
-  'g',
-);
+// and authentication happens out of band via the SSH key. The trailing lookahead
+// requires the `host:path` that every scp-style remote has, so unrelated text
+// like `failed at 12:30@example.com` is left alone.
+const SCP_USERINFO_PATTERN = new RegExp(`(^|\\s)[^/@\\s:]+:${USERINFO}(?=[^\\s:]{1,${MAX_HOST}}:)`, 'g');

 // The value runs to the end of the parameter. Sub-delimiters such as ',' and '('
 // are legal unencoded in a query value, so stopping at them would leave most of a
-// credential in the clear — the trailing punctuation that log output wraps a URL
-// in is restored afterwards instead.
-const CREDENTIAL_QUERY_PATTERN = new RegExp(`([?&])(${CREDENTIAL_QUERY_KEYS.join('|')})=([^&#\\s]*)`, 'gi');
-
-// Punctuation that ends a quoted or parenthesised URL in a log line. It is put
-// back after the placeholder so redaction does not eat the rest of the message;
-// a credential that genuinely ends with one of these loses only that character.
-const TRAILING_PUNCTUATION_PATTERN = /['"`)\]}>,;:.]+$/;
+// credential in the clear. Redaction therefore absorbs any punctuation that
+// closes the URL in a log line (a trailing quote, say) — losing a character of
+// the surrounding message is the acceptable side of this trade.
+const CREDENTIAL_QUERY_PATTERN = new RegExp(`([?&])(${CREDENTIAL_QUERY_KEYS.join('|')})=[^&#\\s]*`, 'gi');

 /**
 * Replaces credentials embedded in a URL with a placeholder.
@@ -78,11 +76,7 @@ export const redactUrl = (value: string): string =>
  value
    .replace(SCHEME_USERINFO_PATTERN, `$1${REDACTED}@`)
    .replace(SCP_USERINFO_PATTERN, `$1${REDACTED}@`)
-    .replace(
-      CREDENTIAL_QUERY_PATTERN,
-      (_match, separator, key, paramValue) =>
-        `${separator}${key}=${REDACTED}${TRAILING_PUNCTUATION_PATTERN.exec(paramValue)?.[0] ?? ''}`,
-    );
+    .replace(CREDENTIAL_QUERY_PATTERN, `$1$2=${REDACTED}`);

 /**
 * Extracts an error's message with any embedded credentials redacted.
@@ -109,26 +109,47 @@ describe('urlRedact', () => {
      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`;
+    test('should redact a userinfo longer than any hostname', () => {
+      // JWT-style credentials run well past a few hundred characters. A length
+      // cap here would hand back the whole secret.
+      const longToken = 'a'.repeat(1000);

-      const start = process.hrtime.bigint();
-      redactUrl(adversarial);
-      const elapsedMs = Number(process.hrtime.bigint() - start) / 1e6;
-
-      expect(elapsedMs).toBeLessThan(100);
+      expect(redactUrl(`https://${longToken}@example.com/repo.git`)).toBe('https://***@example.com/repo.git');
    });

-    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.
+    test('should absorb punctuation that closes the URL in a log line', () => {
+      // Documented trade-off: the value runs to whitespace, so the closing quote
+      // is swallowed. Treating quotes as terminators instead would leave a
+      // credential that merely starts with one in the clear.
      expect(redactUrl("fatal: unable to access 'https://example.com/r?token=s3cr3t': HTTP 401")).toBe(
-        "fatal: unable to access 'https://example.com/r?token=***': HTTP 401",
+        "fatal: unable to access 'https://example.com/r?token=*** HTTP 401",
      );
    });

+    test.each([
+      ['many @ in one token', (n: number) => `u:${'a@'.repeat(n / 2)}host`],
+      ['long credential query value', (n: number) => `https://example.com/r?token=${':'.repeat(n)}a`],
+    ])('should scale linearly on adversarial input: %s', (_label, build) => {
+      // These shapes were quadratic in earlier revisions: the userinfo match
+      // backtracks once per '@', and an MCP client controls `remote` with no
+      // length limit, so super-linear growth here stalls the event loop.
+      const measure = (n: number): number => {
+        const input = build(n);
+        const start = process.hrtime.bigint();
+        redactUrl(input);
+        return Number(process.hrtime.bigint() - start) / 1e6;
+      };
+
+      measure(20_000); // warm up the JIT so the first timing is not an outlier
+      const small = measure(20_000);
+      const large = measure(80_000);
+
+      // 4x the input. Linear predicts ~4x; quadratic predicts ~16x. The generous
+      // ceiling keeps this from flaking on a loaded CI machine while still
+      // failing loudly on a return to quadratic behaviour.
+      expect(large).toBeLessThan(Math.max(small, 1) * 8);
+    });
+
    test('should redact every URL when text embeds more than one', () => {
      const text = `tried https://a:${PASSWORD}@one.example.com/x.git then https://c:${PASSWORD}@two.example.com/y.git`;
      expect(redactUrl(text)).toBe('tried https://***@one.example.com/x.git then https://***@two.example.com/y.git');