diff --git a/agent/sandbox/executor_manager/services/security.py b/agent/sandbox/executor_manager/services/security.py index 13a02ced2e..f0323e747a 100644 --- a/agent/sandbox/executor_manager/services/security.py +++ b/agent/sandbox/executor_manager/services/security.py @@ -26,7 +26,7 @@ class SecurePythonAnalyzer(ast.NodeVisitor): An AST-based analyzer for detecting unsafe Python code patterns. """ - DANGEROUS_IMPORTS = {"os", "subprocess", "sys", "shutil", "socket", "ctypes", "pickle", "threading", "multiprocessing", "asyncio", "http.client", "ftplib", "telnetlib"} + DANGEROUS_IMPORTS = {"os", "subprocess", "sys", "shutil", "socket", "ctypes", "pickle", "threading", "multiprocessing", "asyncio", "http.client", "ftplib", "telnetlib", "builtins"} DANGEROUS_CALLS = { "eval", @@ -77,6 +77,16 @@ class SecurePythonAnalyzer(ast.NodeVisitor): """Check for dangerous function calls.""" if isinstance(node.func, ast.Name) and node.func.id in self.DANGEROUS_CALLS: self.unsafe_items.append((f"Call: {node.func.id}", node.lineno)) + elif isinstance(node.func, ast.Attribute) and node.func.attr in self.DANGEROUS_CALLS: + # Surface the attribute-style match in the analyzer log so that + # incident response can grep for it just like the other unsafe-item + # findings; the bare append is invisible to operators. + logger.warning( + "[SafeCheck] Attribute-style dangerous call detected: %s (line %s)", + node.func.attr, + node.lineno, + ) + self.unsafe_items.append((f"Call: {node.func.attr}", node.lineno)) self.generic_visit(node) def visit_Attribute(self, node: ast.Attribute): @@ -154,9 +164,9 @@ class SecurePythonAnalyzer(ast.NodeVisitor): class SecureJavaScriptAnalyzer: DANGEROUS_PATTERNS = [ - (re.compile(r"""require\s*\(\s*['"]child_process['"]\s*\)"""), "Require: child_process"), - (re.compile(r"""require\s*\(\s*['"]fs['"]\s*\)"""), "Require: fs"), - (re.compile(r"""require\s*\(\s*['"]worker_threads['"]\s*\)"""), "Require: worker_threads"), + (re.compile(r"""require\s*\(\s*['"`]child_process['"`]\s*\)"""), "Require: child_process"), + (re.compile(r"""require\s*\(\s*['"`]fs['"`]\s*\)"""), "Require: fs"), + (re.compile(r"""require\s*\(\s*['"`]worker_threads['"`]\s*\)"""), "Require: worker_threads"), (re.compile(r"""\beval\s*\("""), "Call: eval"), (re.compile(r"""\bFunction\s*\("""), "Call: Function"), (re.compile(r"""\bprocess\s*\.\s*binding\s*\("""), "Call: process.binding"), diff --git a/agent/sandbox/tests/test_security.py b/agent/sandbox/tests/test_security.py index ed096894e4..dc8d9f8063 100644 --- a/agent/sandbox/tests/test_security.py +++ b/agent/sandbox/tests/test_security.py @@ -45,6 +45,60 @@ def test_javascript_eval_is_rejected(): assert any("eval" in issue.lower() for issue, _ in issues) +def test_javascript_child_process_template_literal_is_rejected(): + """Template literal backticks bypass single/double-quote regex patterns.""" + is_safe, issues = analyze_code_security( + "const cp = require(`child_process`); async function main() { return 'ok'; }", + SupportLanguage.NODEJS, + ) + + assert is_safe is False + assert any("child_process" in issue for issue, _ in issues) + + +def test_javascript_fs_template_literal_is_rejected(): + is_safe, issues = analyze_code_security( + "const fs = require(`fs`); async function main() { return fs.readFileSync('/etc/passwd', 'utf8'); }", + SupportLanguage.NODEJS, + ) + + assert is_safe is False + assert any("fs" in issue for issue, _ in issues) + + +def test_python_builtins_import_is_rejected(): + """builtins module gives access to eval/exec and must be blocked.""" + is_safe, issues = analyze_code_security( + "import builtins\ndef main():\n builtins.eval('1+1')", + SupportLanguage.PYTHON, + ) + + assert is_safe is False + # Pin the specific reason: rejection must come from the new ``builtins`` + # entry in ``DANGEROUS_IMPORTS``, not from some unrelated parse error. + assert any("builtins" in issue for issue, _ in issues), ( + f"expected an issue mentioning 'builtins', got {issues!r}" + ) + + +def test_python_attribute_eval_call_is_rejected(): + """Attribute-style dangerous calls (builtins.eval) must be caught.""" + is_safe, issues = analyze_code_security( + "import builtins\ndef main():\n builtins.exec('import os')", + SupportLanguage.PYTHON, + ) + + assert is_safe is False + # Pin the specific reason: rejection must come from the new + # ``ast.Attribute`` branch in ``visit_Call`` flagging the ``exec`` call, + # not from the ``import builtins`` line above. We assert ``exec`` is in at + # least one finding so the test fails if visit_Call's attribute branch is + # ever reverted. + assert any("exec" in issue for issue, _ in issues), ( + f"expected an issue mentioning 'exec', got {issues!r}" + ) + + def test_javascript_safe_code_still_passes(): is_safe, issues = analyze_code_security( "async function main(args) { return { answer: args.value ?? null }; }",