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 3559466..784d918 100644 --- a/src/coretrace_python/plugins/secrets.py +++ b/src/coretrace_python/plugins/secrets.py @@ -178,13 +178,19 @@ 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; + punctuation that ends a sentence around it (``(see ?token=...)``) is not part of it.""" 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.rstrip(".,;:!?)")): + return True + return False DEFAULT_PATTERNS: tuple[SecretPattern, ...] = ( @@ -198,10 +204,21 @@ 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, 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}|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. 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/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, diff --git a/tests/test_url_secret_placeholders.py b/tests/test_url_secret_placeholders.py new file mode 100644 index 0000000..7dcf036 --- /dev/null +++ b/tests/test_url_secret_placeholders.py @@ -0,0 +1,66 @@ +"""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:http://localhost:8080@0.0.0.0:80", + "(the ?token=...)", + ], +) +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", + "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: + (finding,) = url_secrets(value) + + assert finding.metadata["provider"] == "url"