From 82f3a3a3e4053d352cbefd054b922432434f7ecf Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 24 Sep 2026 15:49:51 -0400 Subject: [PATCH] fix(redaction): make credential redaction linear, not quadratic (#631) * fix(redaction): make URL-userinfo redaction linear, not quadratic _REDACT_URL_USERINFO could start a match at every letter of a run of scheme characters, and each attempt read to the end of the run looking for `://`. On a long unbroken run of letters or digits (a hex digest, an ID, part of a response body) that is quadratic: 1.6s for 20k characters. The display service redacts every message, stack trace and context value it publishes in the error snapshot, holding the aggregator lock, and re.sub holds the GIL for the whole call, so one such exception stalled every thread, render loop included (~0.5s measured for 20k chars of hex). It also made test_snapshot_stays_small the slowest test in the suite by far: 142s of a 383s run, 139s of it in this one regex. A match may now only start where a run of scheme characters starts (negative lookbehind). Leading digits and `+.-` are captured in group 1 so the substitution restores them, and the scheme still has to start with a letter, so what gets redacted is unchanged: old and new output were identical on 300k fuzzed inputs. 20k chars now take ~0.5ms, 200k ~6ms, and test_snapshot_stays_small takes 0.8s. test/test_redaction.py pins the exact output for schemes that begin after digits or `+.-`, and bounds 50k-character runs at 1s; against the old pattern those timing tests fail at 3-11s each. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KMXdS2S4NXTJ8ET96GymhK * fix(redaction): make Authorization-header redaction linear too _REDACT_AUTH_HEADER matched the value's opening as `\s*["\']?\s*`: two `\s*` separated only by an optional quote. With no quote, a whitespace run could be split between them in every possible way, and when no credential followed (end of text, or `,` `"` `<` ...) the engine tried them all before giving up: quadratic, 8s for `authorization:` and 20k spaces, 17s with `Proxy-Authorization:` (tried again at the inner `authorization`). Same stall as the URL pattern: re.sub holds the GIL, and the display service redacts everything it publishes. The quote and the whitespace after it are now one optional unit, `\s*(?:["\']\s*)?`, which matches the same strings with only one way to split them. Output is identical to the old pattern on 300k fuzzed inputs; 20k spaces now take ~1.6ms. A scan of all three redaction patterns over prefix/run/suffix shapes finds none left that scales superlinearly. test/test_redaction.py pins exact output for quoted, tabbed, multi-line and credential-less headers, and bounds header + 20k whitespace at 1s; against the previous pattern those fail at 8-17s each. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KMXdS2S4NXTJ8ET96GymhK --------- Co-authored-by: Claude --- CHANGELOG.md | 7 +++ src/redaction.py | 19 +++++-- test/test_redaction.py | 110 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 3 deletions(-) create mode 100644 test/test_redaction.py diff --git a/CHANGELOG.md b/CHANGELOG.md index f136d44a..c2ae2dc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,6 +120,13 @@ floor on the release that ships them): when the count is only known to the display service. - The Logs tab has a **Plugin errors** panel: per-plugin counts, repeating errors and a Clear button. +- Credential redaction in exception text (`src/redaction.py`) takes time + proportional to the text, not its square. Two patterns were quadratic: URL + `user:password@`, on a long unbroken run of letters or digits (a hex digest, + an ID), and `Authorization:` followed by a long run of whitespace. Either + used to stall every thread of the display service for up to seconds each + time the snapshot was published: about 0.5s for 20k characters of hex, 8s + for 20k spaces. What gets redacted is unchanged. ### Removed diff --git a/src/redaction.py b/src/redaction.py index b84bf1b1..a1294309 100644 --- a/src/redaction.py +++ b/src/redaction.py @@ -24,8 +24,13 @@ _REDACT_CREDENTIAL = re.compile( # silently leak the ones nobody thought of. Not covered by the generic pattern # above, whose value part stops at whitespace and so would keep the credential # once a space follows the scheme. +# +# The opening quote and the whitespace after it are one optional unit. Written +# `\s*["\']?\s*`, a whitespace run with no quote in it could be split between +# the two `\s*` in every possible way, and a header with no credential after +# it tried them all: quadratic, 8s for 20k spaces. _REDACT_AUTH_HEADER = re.compile( - r'((?:proxy-)?authorization["\']?\s*[=:]\s*["\']?\s*' + r'((?:proxy-)?authorization["\']?\s*[=:]\s*(?:["\']\s*)?' r'(?:[A-Za-z][\w.+-]*[ \t]+)?)' # optional scheme name, kept r'([^\s,"\'<>}]+)', # the credential, redacted re.IGNORECASE, @@ -34,8 +39,16 @@ _REDACT_AUTH_HEADER = re.compile( # Credentials embedded in a URL: https://user:password@host. requests quotes # the full URL in its exceptions, so this is a realistic leak. The username is # kept -- it identifies which account failed without being the secret. -_REDACT_URL_USERINFO = re.compile(r'([a-z][a-z0-9+.-]*://[^/\s:@]+:)([^/\s@]+)(@)', - re.IGNORECASE) +# +# A match may only start where a run of scheme characters starts. Unanchored, +# `[a-z][a-z0-9+.-]*://` was tried from every letter of a long run (a hex +# digest, an ID, a blob of response body), each attempt reading to the end of +# the run: quadratic, 1.6s for 20k characters, all of it holding the GIL. +# Leading digits and `+.-` sit inside group 1 so the substitution puts them +# back; the scheme proper still has to start with a letter. +_REDACT_URL_USERINFO = re.compile( + r'((? str: diff --git a/test/test_redaction.py b/test/test_redaction.py new file mode 100644 index 00000000..5f59d696 --- /dev/null +++ b/test/test_redaction.py @@ -0,0 +1,110 @@ +"""redact_credentials must stay linear in the length of its input. + +Regressions under test, both quadratic regexes in src/redaction.py: + +- The URL-userinfo pattern (`scheme://user:password@`) could start a match at + every letter of a run of scheme characters, and each attempt read to the end + of the run looking for `://`: 1.6s for a 20k-character run. +- The Authorization-header pattern had two `\\s*` separated only by an + optional quote, so a header followed by whitespace and no credential tried + every split of that whitespace between them: 8s for 20k spaces. + +The display service redacts every message, stack trace and context value it +publishes in the error snapshot, and re.sub holds the GIL throughout, so an +exception quoting a hex digest or a long ID stalled the render loop with it. +test_error_snapshot_cross_process.py's snapshot-size test spent 140s here. + +The fixed patterns have to redact exactly what the old ones did. +""" + +import time + +import pytest + +from src.redaction import redact_credentials + +# Each timed input took seconds before the fix and takes about a millisecond +# after it; the bound leaves CI plenty of headroom while still failing on a +# quadratic pattern. +_TIME_LIMIT = 1.0 + + +def _timed(text): + start = time.perf_counter() + result = redact_credentials(text) + return result, time.perf_counter() - start + + +class TestUrlUserinfo: + @pytest.mark.parametrize("text,expected", [ + ("401 for https://user:hunter2@example.com/api", + "401 for https://user:@example.com/api"), + ("HTTPS://USER:HUNTER2@EXAMPLE.COM", + "HTTPS://USER:@EXAMPLE.COM"), + ("git+ssh://deploy:hunter2@host/repo", + "git+ssh://deploy:@host/repo"), + # The scheme starts after digits or +.- in the same run. Those + # characters must survive, and the password must still go. + ("1http://user:hunter2@host", "1http://user:@host"), + ("+.-http://user:hunter2@host", "+.-http://user:@host"), + ("a1+http://user:hunter2@host", "a1+http://user:@host"), + ("see a://u:first@b and c://v:second@d", + "see a://u:@b and c://v:@d"), + ]) + def test_password_is_redacted_and_the_rest_kept(self, text, expected): + assert redact_credentials(text) == expected + + def test_a_url_without_a_password_is_untouched(self): + text = "GET https://user@example.com/path failed" + assert redact_credentials(text) == text + + +class TestAuthorizationHeader: + @pytest.mark.parametrize("text,expected", [ + ("Authorization: Bearer eyJ.SECRET.sig", "Authorization: Bearer "), + ("Proxy-Authorization: Basic dXNlcg==", "Proxy-Authorization: Basic "), + ("authorization: barecredential", "authorization: "), + # Whitespace and an opening quote around the value, in either order. + ('authorization=" Bearer tok"', 'authorization=" Bearer "'), + ("authorization: ' tok'", "authorization: ' '"), + ("authorization:\n\tBearer tok", "authorization:\n\tBearer "), + ]) + def test_credential_is_redacted_and_the_rest_kept(self, text, expected): + assert redact_credentials(text) == expected + + @pytest.mark.parametrize("text", ["authorization: ", "authorization: , next"]) + def test_a_header_without_a_credential_is_untouched(self, text): + assert redact_credentials(text) == text + + +class TestLinearTime: + @pytest.mark.parametrize("unit", ["x", "0123456789abcdef", "1a", "a+", "1"]) + def test_long_scheme_character_runs(self, unit): + text = (unit * 50_000)[:50_000] + result, elapsed = _timed(text) + assert result == text + assert elapsed < _TIME_LIMIT, f"{elapsed:.2f}s to redact {len(text)} chars of {unit!r}" + + def test_a_credential_after_a_long_run_is_still_found(self): + run = "ab12" * 10_000 + result, elapsed = _timed(f"{run} https://user:hunter2@example.com") + assert result == f"{run} https://user:@example.com" + assert elapsed < _TIME_LIMIT + + @pytest.mark.parametrize("header,whitespace", [ + ("authorization:", " "), + ("Proxy-Authorization:", "\t"), + ("authorization=", "\n"), + ]) + def test_a_header_followed_by_long_whitespace(self, header, whitespace): + text = header + whitespace * 20_000 + "," + result, elapsed = _timed(text) + assert result == text + assert elapsed < _TIME_LIMIT, ( + f"{elapsed:.2f}s to redact {header!r} and {len(text) - len(header)} more chars") + + def test_a_credential_after_long_whitespace_is_still_found(self): + gap = " " * 20_000 + result, elapsed = _timed(f"authorization:{gap}Bearer tok") + assert result == f"authorization:{gap}Bearer " + assert elapsed < _TIME_LIMIT