Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,16 @@ Skill trust uses the HCS-28 baseline adapter IDs, weights, and denominator rules

<a id="quality-suite-commands"></a>

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
Expand Down
49 changes: 48 additions & 1 deletion src/codex_plugin_scanner/checks/code_quality.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from __future__ import annotations

import ast
import re
from pathlib import Path

Expand Down Expand Up @@ -79,14 +80,60 @@ 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):
try:
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()")
Expand Down
128 changes: 117 additions & 11 deletions tests/test_code_quality.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Tests for code quality checks."""

import ast
import tempfile
from pathlib import Path

Expand All @@ -11,6 +12,7 @@
check_no_shell_injection,
run_code_quality_checks,
)
from codex_plugin_scanner.models import Severity

FIXTURES = Path(__file__).parent / "fixtures"

Expand Down Expand Up @@ -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):
Expand All @@ -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",
)

Expand Down Expand Up @@ -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",
)

Expand All @@ -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",
)

Expand All @@ -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",
)

Expand Down
Loading