diff --git a/CHANGELOG.md b/CHANGELOG.md index f136d44a..4a40a394 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,6 +120,12 @@ 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. +- Redacting `user:password@` from URLs in exception text (`src/redaction.py`) + takes time proportional to the text, not its square. A plugin error quoting + a long unbroken run of letters or digits (a hex digest, an ID) 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. What gets redacted is + unchanged. ### Removed diff --git a/src/redaction.py b/src/redaction.py index b84bf1b1..2d674be5 100644 --- a/src/redaction.py +++ b/src/redaction.py @@ -34,8 +34,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..8c0befb6 --- /dev/null +++ b/test/test_redaction.py @@ -0,0 +1,63 @@ +"""redact_credentials must stay linear in the length of its input. + +Regression under test: 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 `://`. A 20k-character run took +1.6s; 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 anchored pattern has to redact exactly what the old one did, including a +scheme that begins after digits or `+.-` in the same run. +""" + +import time + +import pytest + +from src.redaction import redact_credentials + + +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 TestLinearTime: + # Each of these took seconds before the fix (letters ~10s at this size) + # and takes about a millisecond after it; the bound leaves CI plenty of + # headroom while still failing on a quadratic pattern. + @pytest.mark.parametrize("unit", ["x", "0123456789abcdef", "1a", "a+", "1"]) + def test_long_scheme_character_runs(self, unit): + text = (unit * 50_000)[:50_000] + start = time.perf_counter() + assert redact_credentials(text) == text + elapsed = time.perf_counter() - start + assert elapsed < 1.0, 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 + text = f"{run} https://user:hunter2@example.com" + start = time.perf_counter() + assert redact_credentials(text) == f"{run} https://user:@example.com" + assert time.perf_counter() - start < 1.0