From c73b4f4ed8fd9735ae41624f070e035bd33a82c6 Mon Sep 17 00:00:00 2001 From: Hugo Date: Sun, 4 Oct 2026 12:23:01 +0200 Subject: [PATCH 1/3] fix(secrets): check URL secrets for placeholders and host:port addresses The url provider pattern reported help text such as ?token=... and proxy mode specifications such as udp://127.0.0.1:1234@127.0.0.1:0 as credentials. A provider pattern may now name its secret group, which goes through the placeholder check credentials already get; a query value ends at a character no URL holds unescaped, and a host:port pair before @ is an address. Closes #209 --- src/coretrace_python/plugins/secrets.py | 22 +++++++-- tests/test_url_secret_placeholders.py | 62 +++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 tests/test_url_secret_placeholders.py diff --git a/src/coretrace_python/plugins/secrets.py b/src/coretrace_python/plugins/secrets.py index 3559466..6a109da 100644 --- a/src/coretrace_python/plugins/secrets.py +++ b/src/coretrace_python/plugins/secrets.py @@ -178,13 +178,18 @@ def _bound_name(target: nodes.Target) -> str | None: @dataclass(frozen=True) class SecretPattern: - """A provider-specific secret format.""" + """A provider-specific secret format. When the regex names a ``secret`` group, that + group is the secret itself, and a match whose secret is a placeholder is not one.""" provider: str regex: str def matches(self, text: str) -> bool: - return re.search(self.regex, text) is not None + for match in re.finditer(self.regex, text): + secret = match.groupdict().get("secret") + if secret is None or not is_placeholder(secret): + return True + return False DEFAULT_PATTERNS: tuple[SecretPattern, ...] = ( @@ -198,10 +203,19 @@ def matches(self, text: str) -> bool: SecretPattern("jwt", r"\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\b"), SecretPattern("sendgrid", r"\bSG\.[A-Za-z0-9_-]{22}\.[A-Za-z0-9_-]{43}\b"), SecretPattern("twilio", r"\bSK[0-9a-fA-F]{32}\b"), + # A password in the user information. A host and a port before the ``@`` are an + # address, as in proxy specifications (``udp://127.0.0.1:1234@0.0.0.0:0``). + SecretPattern( + "url", + r"://(?!(?:\d{1,3}(?:\.\d{1,3}){3}|[A-Za-z0-9-]+(?:\.[A-Za-z0-9-]+)+):\d+@)" + r"[^/\s:@'\"]+:(?P[^/\s@'\"]+)@", + ), + # A secret in a query parameter, up to a character no URL holds unescaped (RFC 3986): + # prose around it, such as Markdown quotes, is not part of it. SecretPattern( "url", - r"://[^/\s:@'\"]+:[^/\s@'\"]+@" - r"|[?&](?:password|passwd|pwd|token|api_key|apikey|secret|access_key)=[^&\s'\"]{3,}", + r"[?&](?:password|passwd|pwd|token|api_key|apikey|secret|access_key)=" + r"(?P[^&\s'\"`<>{}|\\^]{3,})", ), ) diff --git a/tests/test_url_secret_placeholders.py b/tests/test_url_secret_placeholders.py new file mode 100644 index 0000000..d960b09 --- /dev/null +++ b/tests/test_url_secret_placeholders.py @@ -0,0 +1,62 @@ +"""Acceptance tests for issue #209: credentials in URLs, not their placeholders. + +The ``url`` provider pattern finds a password in the user information of a URL and a +secret in a query parameter. The secret it matches goes through the placeholder check +that credentials already get: ``?token=...``, ``?token=`` or ``?token={token}`` in +help text are not secrets. A ``host:port@`` pair is an address, as in proxy mode +specifications (``reverse:udp://127.0.0.1:1234@127.0.0.1:0``), not a user and a password. +Real credentials in URLs are still reported. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.findings import Finding +from coretrace_python.source import SourceManager + +PLUGINS = Path(__file__).resolve().parent.parent / "src" / "coretrace_python" / "bundled" + + +def url_secrets(value: str) -> list[Finding]: + findings = engine.check( + SourceManager().add_source("app/cli.py", f"TEXT = {value!r}\n"), [PLUGINS] + ) + return [f for f in findings if f.rule_id == "hardcoded-secret"] + + +@pytest.mark.parametrize( + "value", + [ + "--upload needs --token (the ?token=... mitmweb prints at startup)", + "The token is the `?token=...` in the URL mitmweb prints on startup.", + "open http://127.0.0.1:8081/?token= in a browser", + "open http://127.0.0.1:8081/?token={token} in a browser", + "https://host.example/api?password=password", + "https://user:@db.example/app", + "reverse:udp://127.0.0.1:1234@127.0.0.1:0", + "reverse:tcp://example.com:8080@0.0.0.0:80", + ], +) +def test_placeholders_and_addresses_are_not_url_secrets(value: str) -> None: + assert url_secrets(value) == [] + + +@pytest.mark.parametrize( + "value", + [ + "https://alice:s3cr3tP4ss@example.com/", + "postgres://admin:hunter2@db.example/app", + "https://admin:1234@example.com/", + "https://host.example/api?token=a1b2c3d4e5f6", + "see ?token=... then https://host.example/api?api_key=a1b2c3d4e5f6", + "call `https://host.example/api?token=a1b2c3d4e5f6` to sync", + ], +) +def test_real_credentials_in_urls_are_still_reported(value: str) -> None: + (finding,) = url_secrets(value) + + assert finding.metadata["provider"] == "url" From 1d579d7f336d52a35cd9b52f21f7413cfda6330a Mon Sep 17 00:00:00 2001 From: Hugo Date: Sun, 4 Oct 2026 13:03:28 +0200 Subject: [PATCH 2/3] test(regression): drop the URL placeholders no longer reported healthchecks' gotify tests use ?token=abc, a three-character placeholder, and full-stack-fastapi-template's .env names its database password through ${POSTGRES_PASSWORD}. Neither is a hardcoded secret since the url pattern checks its secret for placeholders. Exactly these four entries are removed. Refs #209 --- .../expected/full-stack-fastapi-template.json | 6 ------ tests/regression/expected/healthchecks.json | 18 ------------------ 2 files changed, 24 deletions(-) diff --git a/tests/regression/expected/full-stack-fastapi-template.json b/tests/regression/expected/full-stack-fastapi-template.json index 1fc03bd..7c2b7c6 100644 --- a/tests/regression/expected/full-stack-fastapi-template.json +++ b/tests/regression/expected/full-stack-fastapi-template.json @@ -24,12 +24,6 @@ "rule": "hardcoded-credential", "function": null }, - { - "file": ".env", - "line": 18, - "rule": "hardcoded-secret", - "function": null - }, { "file": "backend/app/api/deps.py", "line": 1, diff --git a/tests/regression/expected/healthchecks.json b/tests/regression/expected/healthchecks.json index 658f82a..f3c361b 100644 --- a/tests/regression/expected/healthchecks.json +++ b/tests/regression/expected/healthchecks.json @@ -220,24 +220,6 @@ "rule": "hardcoded-credential", "function": "test_it_updates_channel" }, - { - "file": "hc/integrations/gotify/tests/test_notify.py", - "line": 50, - "rule": "hardcoded-secret", - "function": "test_it_works" - }, - { - "file": "hc/integrations/gotify/tests/test_notify.py", - "line": 93, - "rule": "hardcoded-secret", - "function": "test_it_handles_subpath" - }, - { - "file": "hc/integrations/gotify/tests/test_notify.py", - "line": 105, - "rule": "hardcoded-secret", - "function": "test_it_handles_subpath_with_missing_slash" - }, { "file": "hc/integrations/matrix/tests/test_notify.py", "line": 15, From 8e3af8a4000260217bef89369597953c8a56277b Mon Sep 17 00:00:00 2001 From: Hugo Date: Sun, 4 Oct 2026 13:18:45 +0200 Subject: [PATCH 3/3] fix(secrets): keep URL passwords holding @ and dotted user names Review of the url pattern change: a password holding an unescaped @ was cut at it and dropped as a placeholder, and a dotted user name with a numeric password was taken for a host and port. The password now runs to the last @, only an IP address or localhost before :port@ is an address, and sentence punctuation after a placeholder is ignored. Document the secret group for plugin authors. Refs #209 --- docs/plugins.md | 4 ++++ src/coretrace_python/plugins/secrets.py | 15 +++++++++------ tests/test_url_secret_placeholders.py | 6 +++++- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/docs/plugins.md b/docs/plugins.md index a14ad1f..5d34663 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -255,6 +255,10 @@ class InternalSecrets(SecretDetector): credential_names: ClassVar[tuple[str, ...]] = (*DEFAULT_CREDENTIAL_NAMES, "acme_key") ``` +A pattern whose regex names a `secret` group (`(?P...)`) is judged on that group: +a match whose secret is a placeholder (`...`, ``, `${VAR}`, fewer than four +characters) is not reported, as with credential names. + `entropy_threshold`, `hex_entropy_threshold` and `minimum_length` tune the high-entropy rule. Findings carry a redacted preview only. diff --git a/src/coretrace_python/plugins/secrets.py b/src/coretrace_python/plugins/secrets.py index 6a109da..784d918 100644 --- a/src/coretrace_python/plugins/secrets.py +++ b/src/coretrace_python/plugins/secrets.py @@ -179,7 +179,8 @@ def _bound_name(target: nodes.Target) -> str | None: @dataclass(frozen=True) class SecretPattern: """A provider-specific secret format. When the regex names a ``secret`` group, that - group is the secret itself, and a match whose secret is a placeholder is not one.""" + group is the secret itself, and a match whose secret is a placeholder is not one; + punctuation that ends a sentence around it (``(see ?token=...)``) is not part of it.""" provider: str regex: str @@ -187,7 +188,7 @@ class SecretPattern: def matches(self, text: str) -> bool: for match in re.finditer(self.regex, text): secret = match.groupdict().get("secret") - if secret is None or not is_placeholder(secret): + if secret is None or not is_placeholder(secret.rstrip(".,;:!?)")): return True return False @@ -203,12 +204,14 @@ def matches(self, text: str) -> bool: SecretPattern("jwt", r"\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\b"), SecretPattern("sendgrid", r"\bSG\.[A-Za-z0-9_-]{22}\.[A-Za-z0-9_-]{43}\b"), SecretPattern("twilio", r"\bSK[0-9a-fA-F]{32}\b"), - # A password in the user information. A host and a port before the ``@`` are an - # address, as in proxy specifications (``udp://127.0.0.1:1234@0.0.0.0:0``). + # A password in the user information, up to the last ``@`` before the host, since a + # password may hold one. An IP address or ``localhost`` and a port before the ``@`` + # are an address, as in proxy specifications (``udp://127.0.0.1:1234@0.0.0.0:0``); + # a dotted name is not, since user names such as ``john.doe`` have dots. SecretPattern( "url", - r"://(?!(?:\d{1,3}(?:\.\d{1,3}){3}|[A-Za-z0-9-]+(?:\.[A-Za-z0-9-]+)+):\d+@)" - r"[^/\s:@'\"]+:(?P[^/\s@'\"]+)@", + r"://(?!(?:\d{1,3}(?:\.\d{1,3}){3}|localhost):\d+@)" + r"[^/\s:@'\"]+:(?P[^/\s'\"]+)@", ), # A secret in a query parameter, up to a character no URL holds unescaped (RFC 3986): # prose around it, such as Markdown quotes, is not part of it. diff --git a/tests/test_url_secret_placeholders.py b/tests/test_url_secret_placeholders.py index d960b09..7dcf036 100644 --- a/tests/test_url_secret_placeholders.py +++ b/tests/test_url_secret_placeholders.py @@ -38,7 +38,8 @@ def url_secrets(value: str) -> list[Finding]: "https://host.example/api?password=password", "https://user:@db.example/app", "reverse:udp://127.0.0.1:1234@127.0.0.1:0", - "reverse:tcp://example.com:8080@0.0.0.0:80", + "reverse:http://localhost:8080@0.0.0.0:80", + "(the ?token=...)", ], ) def test_placeholders_and_addresses_are_not_url_secrets(value: str) -> None: @@ -54,6 +55,9 @@ def test_placeholders_and_addresses_are_not_url_secrets(value: str) -> None: "https://host.example/api?token=a1b2c3d4e5f6", "see ?token=... then https://host.example/api?api_key=a1b2c3d4e5f6", "call `https://host.example/api?token=a1b2c3d4e5f6` to sync", + "mongodb://admin:P@ssw0rd123@mongo:27017/app", + "https://john.doe:123456@example.com/", + "postgres://postgres.abcdefghij:20240917@aws-0-eu-west-1.pooler.supabase.com:6543/postgres", ], ) def test_real_credentials_in_urls_are_still_reported(value: str) -> None: