diff --git a/src/coretrace_python/plugins/secrets.py b/src/coretrace_python/plugins/secrets.py index f4cd1e1..9a96818 100644 --- a/src/coretrace_python/plugins/secrets.py +++ b/src/coretrace_python/plugins/secrets.py @@ -2,7 +2,11 @@ Secrets are string literals of the PyHIR. ``literals`` walks every string literal with the name it is bound to (assignment target, keyword argument, dictionary key) and the -enclosing function; ``SecretDetector`` reports at most one finding per literal: a +enclosing function; a string expression whose parts are all constant (``+``, +``sep.join``, an f-string) is one literal, followed by its pieces (#228). +``SecretDetector`` reports at most one finding per literal, and one per such expression, +the strongest of its whole value and its pieces, at the whole expression, or its pieces +on their own when the whole value is no secret: a provider pattern first (``hardcoded-secret``), then a credential-like name with a real value (``hardcoded-credential``), then a high-entropy token on its own (``high-entropy-string``). Messages and metadata carry a redacted preview, never the @@ -20,7 +24,7 @@ from bisect import bisect_right from collections import Counter from collections.abc import Iterator, Sequence -from dataclasses import dataclass +from dataclasses import dataclass, replace from pathlib import Path from typing import ClassVar @@ -35,6 +39,8 @@ # A value, the name it is bound to, where it is, and its enclosing function. A value of # a JSON or TOML file whose line cannot be established is at a file location. Literal = tuple[str, str | None, Location, str | None] +# A literal and, for a string expression whose parts are all constant, its pieces. +LiteralGroup = tuple[Literal, tuple[Literal, ...]] _HEX = re.compile(r"^[0-9a-fA-F]+$") _TOKEN = re.compile(r"^[A-Za-z0-9+/=_-]+$") @@ -131,17 +137,36 @@ def shannon_entropy(text: str) -> float: def literals(module: nodes.Module) -> Iterator[Literal]: - """Every string literal of the module as ``(value, bound name, span, function)``.""" + """Every string literal of the module as ``(value, bound name, span, function)``, a + string expression whose parts are all constant followed by its pieces.""" + + for literal, pieces in literal_groups(module): + yield literal + yield from pieces + + +def literal_groups(module: nodes.Module) -> Iterator[LiteralGroup]: + """Every string literal of the module with, for a string expression whose parts are + all constant, the literals it is made of, bound to no name.""" for statement in module.body: yield from _walk(statement, None, None) -def _walk(node: Node, name: str | None, function: str | None) -> Iterator[Literal]: +def _walk(node: Node, name: str | None, function: str | None) -> Iterator[LiteralGroup]: if isinstance(node, nodes.Constant): if isinstance(node.value, str): - yield node.value, name, node.span, function + yield (node.value, name, node.span, function), () return + if isinstance(node, (nodes.BinaryOp, nodes.FormattedString, nodes.Call)): + folded = _folded(node) + if folded is not None: + # Folded once at the outermost expression: ``a + b`` of ``a + b + c`` is no + # value of its own. A chain that does not fold is tried again at each level, + # quadratic in its length, which the frontend bounds (about 1000 terms). + pieces = tuple((value, None, span, function) for value, span in _pieces(node)) + yield (folded, name, node.span, function), pieces + return if isinstance(node, nodes.Function): for child in children(node): yield from _walk(child, None, node.name) @@ -184,6 +209,56 @@ def _walk(node: Node, name: str | None, function: str | None) -> Iterator[Litera yield from _walk(child, None, function) +def _folded(node: Node) -> str | None: + """The string ``node`` evaluates to when all its parts are constant: a string, ``+`` + of strings, an f-string whose parts are all constant (its conversions and format + specifications ignored), ``sep.join`` of a list or tuple of strings with a constant + separator; None otherwise.""" + + parts = _parts(node) + if parts is None: + return node.value if isinstance(node, nodes.Constant) and isinstance(node.value, str) else None + folded = [_folded(part) for part in parts] + if None in folded: + return None + texts = [text for text in folded if text is not None] + if isinstance(node, nodes.Call): + return texts[0].join(texts[1:]) + return "".join(texts) + + +def _parts(node: Node) -> tuple[Node, ...] | None: + """What a string expression ``_folded`` reads is made of, the separator of a + ``join`` first; None for any other node.""" + + if isinstance(node, nodes.BinaryOp) and node.operator == "add": + return node.left, node.right + if isinstance(node, nodes.FormattedString): + return node.parts + if ( + isinstance(node, nodes.Call) + and isinstance(node.callee, nodes.Attribute) + and node.callee.name == "join" + and len(node.arguments) == 1 + and not node.keywords + and isinstance(node.arguments[0], (nodes.List, nodes.Tuple)) + ): + return node.callee.value, *node.arguments[0].elements + return None + + +def _pieces(node: Node) -> Iterator[tuple[str, SourceSpan]]: + """The strings a folded expression is made of, in order, with their spans.""" + + parts = _parts(node) + if parts is None: + assert isinstance(node, nodes.Constant) and isinstance(node.value, str) + yield node.value, node.span + return + for part in parts: + yield from _pieces(part) + + def _is_environment_lookup(call: nodes.Call) -> bool: callee = call.callee return ( @@ -402,6 +477,13 @@ def _value_end(lines: list[str], quoted: dict[str, list[int]], number: int, inde return last +_CONFIDENCE_ORDER = (Confidence.LOW, Confidence.MEDIUM, Confidence.HIGH) + + +def _strength(finding: Finding) -> tuple[int, int]: + return finding.severity.rank, _CONFIDENCE_ORDER.index(finding.confidence) + + def redacted(value: str) -> str: preview = value[:4] if len(value) > 8 else value[:1] return f"{preview}… ({len(value)} characters)" @@ -423,10 +505,17 @@ class SecretDetector(Plugin): def analyze(self, ctx: PluginContext) -> Sequence[Finding]: findings: list[Finding] = [] - for value, name, span, function in literals(ctx.module): - finding = self.judge(value, name, span, function) - if finding is not None: - findings.append(finding) + for literal, pieces in literal_groups(ctx.module): + found = self.judge(*literal) + parts = [finding for finding in (self.judge(*piece) for piece in pieces) if finding is not None] + if found is None: + findings.extend(parts) + else: + # A piece may be the stronger finding: two pieces that touch leave the + # whole value matching no provider pattern. It is reported at the whole, + # so that a baseline digests every line of it. + strongest = max((found, *parts), key=_strength) + findings.append(strongest if strongest is found else replace(strongest, span=found.span)) return findings def judge(self, value: str, name: str | None, span: Location, function: str | None) -> Finding | None: diff --git a/tests/test_folded_secret_literals.py b/tests/test_folded_secret_literals.py new file mode 100644 index 0000000..8c771f3 --- /dev/null +++ b/tests/test_folded_secret_literals.py @@ -0,0 +1,124 @@ +"""Acceptance tests for issue #228: a secret built from several string literals. + +A string expression whose parts are all constant (``+`` of strings, ``sep.join`` of a +list or tuple of strings with a constant separator, an f-string whose parts are all +constant) is one value, folded once at the outermost expression. It yields one finding, +the strongest of the whole value and of its pieces, at the span of the whole +expression, so the baseline digests every line of it; when the whole value is no +secret, its pieces are judged on their own, as before. An expression with a part that +is not constant is not folded as a whole; its literals are judged as before. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.cli import main +from coretrace_python.source import SourceManager + +PLUGINS = Path(__file__).resolve().parent.parent / "src" / "coretrace_python" / "bundled" +SECRET_RULES = ("hardcoded-secret", "hardcoded-credential", "high-entropy-string") +KEY = ( + 'KEY = ("-----BEGIN RSA PRIVATE KEY-----\\n" +\n' + ' "ZZZZowIBAAKCAQ\\n" +\n' + ' "-----END RSA PRIVATE KEY-----")\n' +) + + +def secrets(text: str) -> list[tuple[str, int, int | None]]: + findings = engine.check(SourceManager().add_source("app/keys.py", text), [PLUGINS]) + return sorted( + (f.rule_id, f.span.start_line, f.span.end_line) + for f in findings + if f.rule_id in SECRET_RULES + ) + + +@pytest.mark.parametrize( + ("text", "expected"), + [ + (KEY, [("hardcoded-secret", 1, 3)]), + ('PASSWORD = "Zx81kQpL" + "w0RtY7vB" + "n3MsD9cF"\n', [("hardcoded-credential", 1, 1)]), + ('TOKEN = "ghp_" + "a1B2a1B2a1B2a1B2a1B2a1B2a1B2a1B2a1B2"\n', [("hardcoded-secret", 1, 1)]), + ( + ( + 'KEY = "\\n".join([\n' + ' "-----BEGIN RSA PRIVATE KEY-----",\n' + ' "ZZZZowIBAAKCAQ",\n' + ' "-----END RSA PRIVATE KEY-----",\n' + "])\n" + ), + [("hardcoded-secret", 1, 5)], + ), + ('TOKEN = f"ghp_" "a1B2a1B2a1B2a1B2a1B2a1B2a1B2a1B2a1B2"\n', [("hardcoded-secret", 1, 1)]), + ], +) +def test_a_secret_built_from_constant_pieces_is_judged_whole( + text: str, expected: list[tuple[str, int, int | None]] +) -> None: + assert secrets(text) == expected + + +def test_an_expression_with_a_part_that_is_not_constant_is_not_folded() -> None: + text = ( + "import os\n\n" + 'TOKEN = "ghp_a1B2a1B2a1B2a1B2a1B2a1B2a1B2a1B2a1B2" + os.environ["SUFFIX"]\n' + 'PREFIX = "ghp_" + os.environ["REST"]\n' + ) + + assert secrets(text) == [("hardcoded-secret", 3, 3)] + + +def test_a_changed_body_of_a_folded_secret_is_new( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + root = tmp_path / "proj" + root.mkdir() + (root / "keys.py").write_text(KEY, encoding="utf-8") + monkeypatch.chdir(tmp_path) + baseline = tmp_path / "baseline.json" + assert main(["--check", "proj", "--baseline", str(baseline)]) == 0 + (root / "keys.py").write_text(KEY.replace("ZZZZ", "AAAA"), encoding="utf-8") + capsys.readouterr() + + assert main(["--check", "proj", "--baseline", str(baseline)]) == 1 + assert capsys.readouterr().out.startswith("keys.py:1:8: high hardcoded-secret") + + +def test_a_piece_is_still_judged_when_the_whole_value_is_no_secret() -> None: + """``"Token: " + token`` is no opaque token as a whole; its token piece still is.""" + + text = 'X = "Token: " + "Zx81kQpLw0RtY7vBn3MsD9cF2hJ6gK4aQ"\n' + + assert secrets(text) == [("high-entropy-string", 1, 1)] + + +@pytest.mark.parametrize( + ("text", "expected"), + [ + # The pieces touch, so the whole matches no provider pattern; a piece does. + ( + 'CREDS = "AKIAIOSFODNN7EXAMPLE" + "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"\n', + [("hardcoded-secret", 1, 1)], + ), + ( + 'api_token = "".join(["v1", "ghp_7Hq2Lm9XvB4nR8sT1kW6yZ3cF5gJ0dPaQeUi"])\n', + [("hardcoded-secret", 1, 1)], + ), + ], +) +def test_the_strongest_finding_of_the_whole_or_a_piece_is_reported_at_the_whole( + text: str, expected: list[tuple[str, int, int | None]] +) -> None: + assert secrets(text) == expected + + +def test_a_part_of_a_chain_is_no_value_of_its_own() -> None: + """``a + b + c`` folds once: ``a + b`` never exists, and is not judged.""" + + text = 'commit_sha = "9f8e7d6c5b4a39281706" + "f5e4d3c2b1a09f8e7d6c5b" + "4a392817"\n' + + assert secrets(text) == []