diff --git a/README.md b/README.md index d6dd3ce83e..277f15a1b6 100644 --- a/README.md +++ b/README.md @@ -245,6 +245,16 @@ Skill trust uses the HCS-28 baseline adapter IDs, weights, and denominator rules +For parseable Python files, the `eval` check inspects syntax: bare builtin references, +`builtins.eval` (including import aliases), and `.eval(...)` calls with arguments +remain findings. A no-argument method call such as PyTorch's +[`Module.eval()`](https://docs.pytorch.org/docs/stable/generated/torch.nn.Module.html#torch.nn.Module.eval) +does not by itself indicate dynamic code execution. Python comments and string +literals are not treated as `eval` calls; expressions inside f-strings are inspected. +If Python parsing fails, the conservative text check remains in effect. JavaScript +and TypeScript checks are unchanged. This rule is a heuristic, not type inference +or a proof that an arbitrary method implementation is safe. + ## CLI Usage ```bash diff --git a/src/codex_plugin_scanner/checks/code_quality.py b/src/codex_plugin_scanner/checks/code_quality.py index eb77b48cd6..7657c65dce 100644 --- a/src/codex_plugin_scanner/checks/code_quality.py +++ b/src/codex_plugin_scanner/checks/code_quality.py @@ -2,6 +2,7 @@ from __future__ import annotations +import ast import re from pathlib import Path @@ -79,6 +80,51 @@ def _has_shell_injection_pattern(content: str) -> bool: return False +def _python_eval_bindings(tree: ast.AST) -> tuple[set[str], set[str]]: + eval_names = {"eval"} + builtins_names = {"builtins", "__builtins__"} + for node in ast.walk(tree): + if isinstance(node, ast.Import): + builtins_names.update(alias.asname or alias.name for alias in node.names if alias.name == "builtins") + elif isinstance(node, ast.ImportFrom) and node.module == "builtins" and node.level == 0: + eval_names.update(alias.asname or alias.name for alias in node.names if alias.name == "eval") + return eval_names, builtins_names + + +def _has_python_eval(content: str) -> bool: + """Distinguish Python eval usage from no-argument methods such as Module.eval().""" + try: + tree = ast.parse(content.removeprefix("\ufeff")) + except (SyntaxError, ValueError, RecursionError): + # Malformed files or syntax newer than this interpreter keep the text check. + return bool(EVAL_RE.search(content)) + + eval_names, builtins_names = _python_eval_bindings(tree) + for node in ast.walk(tree): + # Keep builtin references conservative: aliases and partial application can + # execute code without spelling eval(...) at the eventual call site. + if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Load) and node.id in eval_names: + return True + if ( + isinstance(node, ast.Attribute) + and isinstance(node.ctx, ast.Load) + and node.attr == "eval" + and isinstance(node.value, ast.Name) + and node.value.id in builtins_names + ): + return True + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "eval" + and (node.args or node.keywords) + ): + # Unknown receivers with any payload, including *args/**kwargs, retain + # the finding. No receiver name or dependency import is an allowlist. + return True + return False + + def check_no_eval(plugin_dir: Path, files: tuple[Path, ...] | None = None) -> CheckResult: findings: list[str] = [] for fpath in _find_code_files(plugin_dir, files): @@ -86,7 +132,8 @@ def check_no_eval(plugin_dir: Path, files: tuple[Path, ...] | None = None) -> Ch content = fpath.read_text(encoding="utf-8", errors="ignore") except OSError: continue - if EVAL_RE.search(content): + has_eval = _has_python_eval(content) if fpath.suffix == ".py" else bool(EVAL_RE.search(content)) + if has_eval: findings.append(f"{fpath.relative_to(plugin_dir)}: eval()") if FUNCTION_RE.search(content): findings.append(f"{fpath.relative_to(plugin_dir)}: new Function()") diff --git a/tests/test_code_quality.py b/tests/test_code_quality.py index 4056699e2a..02b275a6a4 100644 --- a/tests/test_code_quality.py +++ b/tests/test_code_quality.py @@ -1,5 +1,6 @@ """Tests for code quality checks.""" +import ast import tempfile from pathlib import Path @@ -11,6 +12,7 @@ check_no_shell_injection, run_code_quality_checks, ) +from codex_plugin_scanner.models import Severity FIXTURES = Path(__file__).parent / "fixtures" @@ -46,6 +48,117 @@ def test_ignores_symlinked_code_files_outside_root(self): r = check_no_eval(root) assert r.passed is True + @pytest.mark.parametrize( + "source", + ( + "model.eval()", + "self.model.eval()", + "self.cross_model.eval()\nself.fusion.eval()", + "proposal.eval()\nranker.eval()", + "gen.model.eval()\npolicy.model.eval()", + "model.eval(\n # switch to inference mode\n)", + "(model.eval)()", + "model.eval().to(device)", + "models[index].eval()", + "load_model().eval()", + "模型.eval()", + "\ufeffmodel.eval()", + "# eval(source) is intentionally unused\nmodel.eval()", + 'description = "eval(source)"\nmodel.eval()', + 'def infer():\n """Avoid eval(source)."""\n model.eval()', + "class Model:\n def eval(self):\n return self\nModel().eval()", + ), + ) + def test_python_inference_methods_are_not_dynamic_execution(self, tmp_path: Path, source: str): + (tmp_path / "inference.py").write_text(source, encoding="utf-8") + + result = check_no_eval(tmp_path) + + assert result.passed is True + assert result.points == result.max_points == 5 + assert result.findings == () + + @pytest.mark.parametrize( + "source", + ( + "eval(source)", + "eval()", + "(eval)(source)", + "evaluate = eval\nevaluate(source)", + "builtins.eval(source)", + "builtins.eval()", + "__builtins__.eval(source)", + "import builtins as b\nb.eval(source)", + "import builtins as b\nb.eval()", + "import builtins as b\nevaluate = b.eval\nevaluate(source)", + "from builtins import eval as evaluate\nevaluate(source)", + "model.eval(source)", + "model.eval(source=source)", + "model.eval(*args)", + "model.eval(**kwargs)", + "model.eval()\neval(source)", + "model.eval(\n source\n)", + 'message = f"result: {eval(source)}"', + "class Model:\n def eval(self):\n return eval(source)\nModel().eval()", + "import functools\nmodel.eval = functools.partial(eval, source)\nmodel.eval()", + ), + ) + def test_python_dynamic_eval_still_fails(self, tmp_path: Path, source: str): + (tmp_path / "runner.py").write_text(source, encoding="utf-8") + + result = check_no_eval(tmp_path) + + assert result.passed is False + assert result.points == 0 + assert len(result.findings) == 1 + finding = result.findings[0] + assert finding.rule_id == "DANGEROUS_DYNAMIC_EXECUTION" + assert finding.severity is Severity.HIGH + assert finding.file_path == "runner.py" + + @pytest.mark.parametrize("source", ("if :\n eval(source)", "model.eval(\n", "\x00eval(source)")) + def test_unparseable_python_keeps_conservative_text_detection(self, tmp_path: Path, source: str): + (tmp_path / "invalid.py").write_text(source, encoding="utf-8") + + assert check_no_eval(tmp_path).passed is False + + @pytest.mark.parametrize("error", (SyntaxError, ValueError, RecursionError)) + def test_python_parser_failure_retains_text_detection( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, error: type[Exception] + ): + def cannot_parse(_source: str): + raise error("parser unavailable for this source") + + (tmp_path / "runner.py").write_text("eval(source)", encoding="utf-8") + monkeypatch.setattr(ast, "parse", cannot_parse) + + result = check_no_eval(tmp_path) + + assert result.passed is False + assert result.findings[0].severity is Severity.HIGH + + @pytest.mark.parametrize("extension", (".js", ".ts", ".jsx", ".tsx", ".mjs", ".cjs")) + @pytest.mark.parametrize("source", ("eval(source)", "window.eval(source)", "model.eval()", "new Function(source)")) + def test_javascript_and_typescript_dynamic_execution_detection_is_unchanged( + self, tmp_path: Path, extension: str, source: str + ): + (tmp_path / f"runner{extension}").write_text(source, encoding="utf-8") + + result = check_no_eval(tmp_path) + + assert result.passed is False + assert result.findings[0].severity is Severity.HIGH + + def test_preselected_python_files_use_the_same_detection(self, tmp_path: Path): + safe = tmp_path / "inference.py" + unsafe = tmp_path / "runner.py" + safe.write_text("model.eval()", encoding="utf-8") + unsafe.write_text("eval(source)", encoding="utf-8") + + assert check_no_eval(tmp_path, files=(safe,)).passed is True + result = check_no_eval(tmp_path, files=(safe, unsafe)) + assert [finding.file_path for finding in result.findings] == ["runner.py"] + class TestCheckNoShellInjection: def test_passes_clean_dir(self): @@ -65,11 +178,7 @@ def test_ignores_template_literal_near_spawn_switch_label(self): with tempfile.TemporaryDirectory() as tmpdir: root = Path(tmpdir) (root / "dispatcher.ts").write_text( - "const message = `failed ${reason}`;\n" - "switch (kind) {\n" - " case 'spawn':\n" - " return message;\n" - "}\n", + "const message = `failed ${reason}`;\nswitch (kind) {\n case 'spawn':\n return message;\n}\n", encoding="utf-8", ) @@ -137,8 +246,7 @@ def test_detects_one_hop_variable_passed_to_required_node_child_process(self): with tempfile.TemporaryDirectory() as tmpdir: root = Path(tmpdir) (root / "runner.js").write_text( - "const cmd = `echo ${userInput}`;\n" - 'require("node:child_process").exec(cmd);\n', + 'const cmd = `echo ${userInput}`;\nrequire("node:child_process").exec(cmd);\n', encoding="utf-8", ) @@ -152,8 +260,7 @@ def test_detects_typed_interpolated_template_variable_in_typescript(self): with tempfile.TemporaryDirectory() as tmpdir: root = Path(tmpdir) (root / "runner.ts").write_text( - "export const command: string = `echo ${userInput}`;\n" - "child_process.exec(command);\n", + "export const command: string = `echo ${userInput}`;\nchild_process.exec(command);\n", encoding="utf-8", ) @@ -168,8 +275,7 @@ def test_detects_typescript_template_assertion_suffixes(self, suffix: str): with tempfile.TemporaryDirectory() as tmpdir: root = Path(tmpdir) (root / "runner.ts").write_text( - f"const cmd = `echo ${{userInput}}` {suffix};\n" - "child_process.exec(cmd);\n", + f"const cmd = `echo ${{userInput}}` {suffix};\nchild_process.exec(cmd);\n", encoding="utf-8", )