diff --git a/docs/usage.md b/docs/usage.md index 835b7c4..47fd9a2 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -120,7 +120,7 @@ variable is not treated that way yet. | Rule | Reached sink | |---|---| -| `command-injection` | Shell or process execution (`os.system`, `subprocess` with a string, …), judged on what the input controls in the command. A string a shell runs, a string naming the program, the program itself, or an operand the program runs as a command (after the host for `ssh`, the program of `sudo`, `env`, `xargs`, `timeout`…, any argument of a batch file) is `high`. The script an interpreter runs (`bash run.sh`, `python tool.py`, its first operand when no option such as `-c` gives the code), or the module of `python -m`, is `high` and kept for review: execution is possible, and its exploitability depends on the file the input chooses. The arguments after a script or code are the script's own: options of unknown meaning, reported `medium` (a hotspot after an option of the script), refuted behind a constant prefix. Otherwise an element of an argument list run without a shell can only be read as an option: `medium`, or `high` when a documented option of the program runs a command it chooses (`git push --receive-pack=…`, `ssh -o ProxyCommand=…`). A value a documented option takes is judged by what the option does with it, whatever its prefix: run it (`sh -c`, `cmd /c`, `python -c`: `high`), keep it as data (`git commit -m`: refuted). After an option the rule does not know, the finding stays a hotspot to review. A constant prefix that keeps a standalone input from being an option (`f"v{version}"`), or an operand after `--` for a program that honours it, is refuted. Every case keeps the rule id `command-injection`, so suppressions and baselines match whatever the case. When the rule reads the command element by element and establishes what the input is there, the finding's `injection` metadata (in the JSON report, and as the only property of its SARIF result) says so: `command` when the input runs or chooses what runs (a shell string, the program, an operand run as a command, the value of an option that runs it); `option` when it can only reach the program as an option or as the value of an option not established to run it, and the message is then titled `Option injection`. When that cannot be established (an option whose effect is to review, an unknown program or element before the input, a command that is not a list or tuple display, a sink such as `os.system` the rule does not read element by element), the metadata is absent and the title stays `Command injection`. | +| `command-injection` | Shell or process execution (`os.system`, `subprocess` with a string, …), judged on what the input controls in the command. A string a shell runs, a string naming the program, the program itself, or an operand the program runs as a command (after the host for `ssh`, the program of `sudo`, `env`, `xargs`, `timeout`…, any argument of a batch file) is `high`. The script an interpreter runs (`bash run.sh`, `python tool.py`, its first operand when no option such as `-c` gives the code), or the module of `python -m`, is `high` and kept for review: execution is possible, and its exploitability depends on the file the input chooses. The arguments after a script or code are the script's own: options of unknown meaning, reported `medium` (a hotspot after an option of the script), refuted behind a constant prefix. Otherwise an element of an argument list run without a shell can only be read as an option: `medium`, or `high` when a documented option of the program runs a command it chooses (`git push --receive-pack=…`, `ssh -o ProxyCommand=…`). A value a documented option takes is judged by what the option does with it, whatever its prefix: run it (`sh -c`, `cmd /c`, `python -c`: `high`), keep it as data (`git commit -m`: refuted). After an option the rule does not know, the finding stays a hotspot to review. A constant prefix that keeps a standalone input from being an option (`f"v{version}"`), or an operand after `--` for a program that honours it, is refuted. Every case keeps the rule id `command-injection`, so suppressions and baselines match whatever the case. When the rule reads the command element by element and establishes what the input is there, the finding's `injection` metadata (in the JSON report, and as a property of its SARIF result) says so: `command` when the input runs or chooses what runs (a shell string, the program, an operand run as a command, the value of an option that runs it); `option` when it can only reach the program as an option or as the value of an option not established to run it, and the message is then titled `Option injection`. When that cannot be established (an option whose effect is to review, an unknown program or element before the input, a command that is not a list or tuple display, a sink such as `os.system` the rule does not read element by element), the metadata is absent and the title stays `Command injection`. | | `code-injection` | The code `eval` or `exec` runs; the globals and locals passed with it are data. `dangerous-eval` reports every such call, this rule the ones attacker input reaches. Also the module `importlib.import_module`, `__import__` or `runpy.run_module` imports, and the file `runpy.run_path` runs, by position or by keyword (`name`, `mod_name`, `path_name`); the package of a relative name, the globals and the run name are data. An import runs the top-level code of a module the input chooses among those that can be found, a script path the file it chooses: execution is possible, and how far it goes depends on what the input can reach, so these findings are titled `Untrusted module import` and `Untrusted script execution`. A constant package prefix (`"app.plugins." + name`) confines an import to that package: a hotspot. The environment and the command line reach these sinks as they reach `eval`. | | `sql-injection` | A database statement (`cursor.execute`, SQLAlchemy `text`, Django `raw`, …). Parameters of a parameterised query are not statements. | | `nosql-injection` | A NoSQL query, such as a MongoDB filter, given attacker input that may hold a structure: a JSON body giving `{"$ne": null}` matches any value. The input that may hold a structure is JSON, the raw bodies a `json.loads` decodes, files, WebSocket messages and standard input. Text cannot hold an operator: form fields, query parameters, headers, cookies, URL parameters, the command line, the environment, and an entry-point parameter annotated with scalars (`str`, `int`, `Optional[str]`, `Annotated[str, Query()]`, `list[str]`, …), which FastAPI validates. A Django, REST framework, aiohttp or tornado view reads a request object: its text attributes (`GET`, `POST`, `headers`, `query_params`, `query`, `match_info`, `arguments`, …) and the URL parameters after it are text, its body, files and `data` are not. Limits: a helper function the request is passed to reads it as a whole; the keys of a dict are not followed, so a string concatenated into `$where` is not reported. The analyzer ships no NoSQL sink: model plugins declare them. | @@ -356,6 +356,18 @@ in the file (`"/tool/app/api_token"`). The SARIF log gives such a result no `reg with the pointer under the location's `properties`, and the text report shows the file alone (`settings.toml: high hardcoded-credential: …`). +A finding in test code carries `context: test` in its metadata, in the JSON report and as +a property of its SARIF result. Test code is a file under a `test`, `tests`, `testing`, +`fixtures` or `__tests__` directory of the project, or a file named `test_*.py`, +`*_test.py` or `conftest.py`, judged on its path from the root of the report, not on +where the project sits. For a file checked alone, that root is the file's own directory, +as the report gives it, so a `test_*.py` module is test code either way, while a test +directory above the file is seen only when its directory is checked. The context +changes no verdict: it is there for baselines, `--fail-on` +wrappers, readers and refiner plugins, which see it. A rule may still give its own +context, as `hardcoded-credential` does with `example` for template files, and the +SARIF result carries that context too. + `root` is the directory the paths are relative to. A file's `status` is `analysed`, `ambiguous` (analysed, but another file has its module name, see above), `syntax-error` or `unreadable`; only `analysed` files count in `files_analysed`. diff --git a/src/coretrace_python/cli.py b/src/coretrace_python/cli.py index df39187..bdd53f5 100644 --- a/src/coretrace_python/cli.py +++ b/src/coretrace_python/cli.py @@ -303,7 +303,7 @@ def main(argv: list[str] | None = None) -> int: encoding="utf-8", ) else: - file_analysis = engine.analyze_file(SourceManager().load_file(args.path), plugin_roots) + file_analysis = engine.analyze_file(SourceManager().load_file(args.path), plugin_roots, root) findings, coverage = file_analysis.findings, file_analysis.coverage suppressed = file_analysis.suppressed components = () diff --git a/src/coretrace_python/engine.py b/src/coretrace_python/engine.py index 344bfbb..952845c 100644 --- a/src/coretrace_python/engine.py +++ b/src/coretrace_python/engine.py @@ -72,6 +72,7 @@ Finding, Severity, ) +from coretrace_python.findings.context import code_context from coretrace_python.findings.refutation import RefutationAnalysis from coretrace_python.findings.suppressions import partition from coretrace_python.frontend import HIRBuildError, ParseError, build_hir @@ -326,8 +327,11 @@ def check(source: SourceFile, plugin_roots: Sequence[Path]) -> tuple[Finding, .. return analyze_file(source, plugin_roots).findings -def analyze_file(source: SourceFile, plugin_roots: Sequence[Path]) -> FileAnalysis: - """One file's findings and coverage, with every plugin found under ``plugin_roots``.""" +def analyze_file(source: SourceFile, plugin_roots: Sequence[Path], root: Path | None = None) -> FileAnalysis: + """One file's findings and coverage, with every plugin found under ``plugin_roots``. + The context of its findings is judged on its path from ``root``, the root of its + report: by default its own directory, as the command line reports a file checked + alone. A source that is not a file is judged on its name as given.""" manager = _register_all(build_hir(source)) registry = load_plugins(plugin_roots, manager) @@ -339,6 +343,11 @@ def analyze_file(source: SourceFile, plugin_roots: Sequence[Path]) -> FileAnalys manager.provide(ProjectSummaries, SummaryIndex()) manager.provide(RegisteredRoutes, _routes_of(manager)) findings, supported = _check_module(manager, tuple(loaded.plugin for loaded in registry)) + if source.path is None: + path: str | None = Path(str(source.source_id)).as_posix() + else: + path = _path_from(source.path, source.path.resolve().parent if root is None else root) + findings = tuple(_situated(f, path if f.span.source_id == source.source_id else None) for f in findings) functions = len(analyzable_functions(manager.module)) coverage = Coverage((FileCoverage(str(source.source_id), "analysed", functions, len(supported)),)) kept, suppressed = partition(findings, lambda sid: source.text if sid == source.source_id else None) @@ -603,6 +612,8 @@ def analyze_project( for plugin in all_plugins: if isinstance(plugin, ProjectPlugin): findings.extend(plugin.analyze_project(context)) + # The context comes first, so that a refiner can read it. + findings = [_situated(finding, _path_from(finding.span.source_id, root)) for finding in findings] for plugin in all_plugins: if isinstance(plugin, ProjectPlugin): findings = list(apply_refinement(plugin, findings, plugin.refine(context, tuple(findings)))) @@ -687,6 +698,29 @@ def _located(path: Path, root: Path) -> str: return str(path) +def _path_from(source_id: object, root: Path) -> str | None: + """The path of the file ``source_id`` names from ``root``, the root of the report; + ``None`` for a file outside it.""" + + try: + return Path(str(source_id)).resolve().relative_to(root.resolve()).as_posix() + except ValueError: + return None + + +def _situated(finding: Finding, path: str | None) -> Finding: + """``finding`` with the context of its code in its metadata (``code_context``), judged + on ``path``, its file's path from the root of the report. A context the rule gave the + finding stays: it is the rule's (#244).""" + + if path is None or "context" in finding.metadata: + return finding + situation = code_context(path) + if situation is None: + return finding + return replace(finding, metadata={**finding.metadata, "context": situation}) + + def _sourced(finding: Finding, origins: Origins) -> Finding: """``finding`` with the plugin or advisory file its advisory comes from, when it cites one.""" diff --git a/src/coretrace_python/findings/context.py b/src/coretrace_python/findings/context.py new file mode 100644 index 0000000..864a1e8 --- /dev/null +++ b/src/coretrace_python/findings/context.py @@ -0,0 +1,32 @@ +"""Where the code of a finding lives, by its path in the project (#244): test code, which +a project runs to check itself and does not ship. The context is a fact about the file, +shown in the finding's metadata; what it changes, if anything, is the rule's decision, or +a refiner plugin's.""" + +from __future__ import annotations + +import re +from pathlib import PurePosixPath + +TEST = "test" + +_TEST_DIRECTORIES = frozenset({"test", "tests", "testing", "fixtures", "__tests__"}) +_TEST_FILE = re.compile(r"^(test_.*|.*_test|conftest)\.py$") + + +def is_test_code(path: str) -> bool: + """Whether ``path`` is test code or a fixture: under a test directory, or named like + a test module.""" + + parts = PurePosixPath(path).parts + return bool(parts) and ( + any(part in _TEST_DIRECTORIES for part in parts[:-1]) or _TEST_FILE.match(parts[-1]) is not None + ) + + +def code_context(path: str) -> str | None: + """The context a finding at ``path`` carries: ``"test"`` for test code, ``None`` for + anything else. ``path`` is relative to the project root, so where the project sits + does not count.""" + + return TEST if is_test_code(path) else None diff --git a/src/coretrace_python/plugins/secrets.py b/src/coretrace_python/plugins/secrets.py index 9a96818..5896d2d 100644 --- a/src/coretrace_python/plugins/secrets.py +++ b/src/coretrace_python/plugins/secrets.py @@ -29,6 +29,7 @@ from typing import ClassVar from coretrace_python.findings import Confidence, Finding, Severity +from coretrace_python.findings.context import is_test_code from coretrace_python.hir import nodes from coretrace_python.hir.visitors import Node, children from coretrace_python.interprocedural import discover_files @@ -100,19 +101,16 @@ def looks_like_digest(value: str, name: str | None) -> bool: return name is not None and any(word in name.lower() for word in _DIGEST_WORDS) -_TEST_DIRECTORIES = frozenset({"test", "tests", "testing", "fixtures", "__tests__"}) -_TEST_FILE = re.compile(r"^(test_.*|.*_test|conftest)\.py$") _EXAMPLE_SUFFIXES = (".example", ".sample", ".dist", ".tpl") def credential_context(path: str) -> str | None: - """``"test"`` for test code and fixtures, ``"example"`` for template configuration - files (``.env.example``), ``None`` for anything else.""" + """``"test"`` for test code and fixtures (``is_test_code``), ``"example"`` for + template configuration files (``.env.example``), ``None`` for anything else.""" - parts = Path(path).parts - if any(part in _TEST_DIRECTORIES for part in parts[:-1]) or _TEST_FILE.match(parts[-1]): + if is_test_code(Path(path).as_posix()): return "test" - if parts[-1].endswith(_EXAMPLE_SUFFIXES): + if Path(path).name.endswith(_EXAMPLE_SUFFIXES): return "example" return None diff --git a/src/coretrace_python/reporters/sarif.py b/src/coretrace_python/reporters/sarif.py index fe17ea7..17f2a97 100644 --- a/src/coretrace_python/reporters/sarif.py +++ b/src/coretrace_python/reporters/sarif.py @@ -19,9 +19,11 @@ Severity.LOW: "note", Severity.INFO: "note", } -# The kinds of injection a finding may carry, the only metadata a result exports: other -# metadata, such as evidence, may quote what the analysed code holds. +# The kinds of injection and the contexts of code a finding may carry, the only metadata +# a result exports: other metadata, such as evidence, may quote what the analysed code +# holds. _INJECTIONS = frozenset({"option", "command"}) +_CONTEXTS = frozenset({"test", "example"}) def _artifact(report: Report, path: str) -> dict[str, str]: @@ -56,9 +58,15 @@ def _result( "message": {"text": finding.message}, "locations": [location], } + properties: dict[str, str] = {} injection = finding.metadata.get("injection") if injection in _INJECTIONS: - result["properties"] = {"injection": injection} + properties["injection"] = injection + context = finding.metadata.get("context") + if context in _CONTEXTS: + properties["context"] = context + if properties: + result["properties"] = properties if suppressed: result["suppressions"] = [{"kind": "inSource"}] if baseline_state is not None: diff --git a/tests/test_code_context.py b/tests/test_code_context.py new file mode 100644 index 0000000..3c4af28 --- /dev/null +++ b/tests/test_code_context.py @@ -0,0 +1,152 @@ +"""Acceptance tests for issue #244: one classification of test code, shown on every +finding. + +A file under a ``test``, ``tests``, ``testing``, ``fixtures`` or ``__tests__`` directory of +the project, or named ``test_*.py``, ``*_test.py`` or ``conftest.py``, is test code. Every +finding in it carries ``context: test`` in its metadata, in the JSON report and as a +property of its SARIF result, judged on its path in the project, not on where the +project sits. The context changes no verdict: what a rule does with it is the rule's, +and a refiner plugin sees it. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.findings import Confidence, Finding +from coretrace_python.findings.context import code_context +from coretrace_python.reporters.sarif import render_sarif + +CHECKS = """\ +from xml.dom import minidom + +PASSWORD = "Zx81kQpLw0RtY7vB" +EXPECTED = "ok" + + +def status(): + return minidom.parseString(EXPECTED).documentElement.tagName + + +def total(): + return eval("1 + 1") +""" + + +@pytest.mark.parametrize( + ("path", "context"), + [ + ("tests/test_app.py", "test"), + ("app/tests/helpers.py", "test"), + ("test/conftest.py", "test"), + ("conftest.py", "test"), + ("app/views_test.py", "test"), + ("fixtures/users.py", "test"), + ("testing/factories.py", "test"), + ("__tests__/views.py", "test"), + ("app/views.py", None), + ("app/testing_utils.py", None), + ("app/contest.py", None), + ("requirements.txt", None), + ], +) +def test_test_code_is_recognised_by_its_path_in_the_project(path: str, context: str | None) -> None: + assert code_context(path) == context + + +def analyse(root: Path, plugin_roots: tuple[Path, ...] = ()) -> tuple[Finding, ...]: + return engine.analyze_project(root, [engine.BUNDLED_PLUGINS, *plugin_roots]).findings + + +def write(root: Path, files: dict[str, str]) -> Path: + for name, text in files.items(): + (root / name).parent.mkdir(parents=True, exist_ok=True) + (root / name).write_text(text, encoding="utf-8") + return root + + +def by_file(findings: tuple[Finding, ...], name: str) -> dict[str, Finding]: + return {f.rule_id: f for f in findings if Path(str(f.span.source_id)).name == name} + + +def test_every_finding_in_test_code_carries_the_context_and_keeps_its_verdict(tmp_path: Path) -> None: + root = write(tmp_path / "project", {"tests/test_checks.py": CHECKS, "app/checks.py": CHECKS}) + + findings = analyse(root) + in_tests, in_app = by_file(findings, "test_checks.py"), by_file(findings, "checks.py") + + assert set(in_tests) == set(in_app) == {"hardcoded-credential", "unsafe-xml", "dangerous-eval"} + assert {rule: f.metadata.get("context") for rule, f in in_tests.items()} == dict.fromkeys(in_tests, "test") + assert all("context" not in f.metadata for f in in_app.values()) + for rule in ("unsafe-xml", "dangerous-eval"): + assert (in_tests[rule].severity, in_tests[rule].confidence) == (in_app[rule].severity, in_app[rule].confidence) + # ``hardcoded-credential`` keeps the lower confidence it already gave test code. + assert in_tests["hardcoded-credential"].confidence is Confidence.LOW + assert in_app["hardcoded-credential"].confidence is Confidence.MEDIUM + + +def test_the_context_is_judged_inside_the_project_not_on_where_it_sits(tmp_path: Path) -> None: + text = "from xml.dom import minidom\n\n\ndef status(document):\n return minidom.parseString(document)\n" + root = write(tmp_path / "tests" / "project", {"app/checks.py": text}) + + findings = analyse(root) + + assert [f.rule_id for f in findings] == ["unsafe-xml"] + assert "context" not in findings[0].metadata + + +def test_the_sarif_result_carries_the_context(tmp_path: Path) -> None: + root = write(tmp_path / "project", {"tests/test_checks.py": CHECKS, "app/checks.py": CHECKS}) + analysis = engine.analyze_project(root, [engine.BUNDLED_PLUGINS]) + + results = json.loads(render_sarif(engine.report(analysis.findings, root=root)))["runs"][0]["results"] + + contexts = { + (result["locations"][0]["physicalLocation"]["artifactLocation"]["uri"], result.get("properties", {}).get("context")) + for result in results + if result["ruleId"] == "dangerous-eval" + } + assert contexts == {("tests/test_checks.py", "test"), ("app/checks.py", None)} + + +REFINER_MANIFEST = ( + 'name = "test-confidence"\nversion = "1.0.0"\nplugin_api = ">=1,<2"\nrequires = []\n' + 'provides = []\n\n[entrypoint]\nmodule = "refiner"\nclass = "TestConfidence"\n' +) +REFINER = ''' +from dataclasses import replace +from typing import ClassVar + +from coretrace_python.findings import Confidence +from coretrace_python.plugins import ProjectPlugin + + +class TestConfidence(ProjectPlugin): + name: ClassVar[str] = "test-confidence" + + def analyze_project(self, ctx): + return () + + def refine(self, ctx, findings): + return [ + replace(f, confidence=Confidence.LOW) if f.metadata.get("context") == "test" else f + for f in findings + ] +''' + + +def test_a_refiner_plugin_sees_the_context(tmp_path: Path) -> None: + plugin = tmp_path / "plugins" / "refiner" + plugin.mkdir(parents=True) + (plugin / "plugin.toml").write_text(REFINER_MANIFEST, encoding="utf-8") + (plugin / "refiner.py").write_text(REFINER, encoding="utf-8") + root = write(tmp_path / "project", {"tests/test_checks.py": CHECKS, "app/checks.py": CHECKS}) + + findings = analyse(root, (tmp_path / "plugins",)) + + assert by_file(findings, "test_checks.py")["dangerous-eval"].confidence is Confidence.LOW + assert by_file(findings, "checks.py")["dangerous-eval"].confidence is Confidence.HIGH diff --git a/tests/test_code_context_single_file.py b/tests/test_code_context_single_file.py new file mode 100644 index 0000000..19738a2 --- /dev/null +++ b/tests/test_code_context_single_file.py @@ -0,0 +1,58 @@ +"""Review of #244 (PR #251): a file checked alone carries the same context as in its +directory. + +The context is judged on the file's path from the root of the report. For a directory, +that root is the directory; for a file checked alone, it is the file's own directory, as +the command line already reports it, so a test module named ``test_*.py`` is test code +either way. A source that is not a file is judged on its name as given. +""" + +from __future__ import annotations + +import contextlib +import io +import json +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.cli import main +from coretrace_python.source import SourceManager + +EVAL = "def total(code):\n return eval(code)\n" + + +def contexts(target: Path) -> list[str | None]: + out = io.StringIO() + with contextlib.redirect_stdout(out): + main(["--check", str(target), "--format", "json"]) + return [f["metadata"].get("context") for f in json.loads(out.getvalue())["findings"] if f["rule_id"] == "dangerous-eval"] + + +@pytest.mark.parametrize(("name", "context"), [("test_checks.py", "test"), ("checks.py", None)]) +def test_a_file_checked_alone_carries_the_context_of_its_directory_run( + tmp_path: Path, name: str, context: str | None +) -> None: + (tmp_path / name).write_text(EVAL, encoding="utf-8") + + assert contexts(tmp_path / name) == contexts(tmp_path) == [context] + + +def test_the_api_judges_a_file_from_the_root_it_is_given(tmp_path: Path) -> None: + (tmp_path / "tests").mkdir() + (tmp_path / "tests" / "helpers.py").write_text(EVAL, encoding="utf-8") + source = SourceManager().load_file(tmp_path / "tests" / "helpers.py") + + from_its_directory = engine.analyze_file(source, [engine.BUNDLED_PLUGINS]).findings + from_the_project = engine.analyze_file(source, [engine.BUNDLED_PLUGINS], root=tmp_path).findings + + assert [f.metadata.get("context") for f in from_its_directory if f.rule_id == "dangerous-eval"] == [None] + assert [f.metadata.get("context") for f in from_the_project if f.rule_id == "dangerous-eval"] == ["test"] + + +@pytest.mark.parametrize(("name", "context"), [("tests/helpers.py", "test"), ("app/helpers.py", None)]) +def test_a_source_that_is_no_file_is_judged_on_its_name(name: str, context: str | None) -> None: + findings = engine.check(SourceManager().add_source(name, EVAL), [engine.BUNDLED_PLUGINS]) + + assert [f.metadata.get("context") for f in findings if f.rule_id == "dangerous-eval"] == [context]