Skip to content

🛡️ Sentinel: [CRITICAL] Fix environment leakage and sandbox bypass in unit test gate#329

Open
heidi-dang wants to merge 1 commit into
feat/bootstrap-scaffoldfrom
sentinel/fix-unit-test-gate-leak-10674563983628842377
Open

🛡️ Sentinel: [CRITICAL] Fix environment leakage and sandbox bypass in unit test gate#329
heidi-dang wants to merge 1 commit into
feat/bootstrap-scaffoldfrom
sentinel/fix-unit-test-gate-leak-10674563983628842377

Conversation

@heidi-dang
Copy link
Copy Markdown
Owner

🛡️ Sentinel Security Fix

🚨 Severity: CRITICAL
💡 Vulnerability: The scripts/03_unit_test_gate.py script was executing generated code samples while inheriting the entire environment of the parent process. This exposed sensitive credentials (e.g., OPENAI_API_KEY, GITHUB_TOKEN) to potentially untrusted code. Furthermore, the script's code-wrapping logic was flawed, causing SyntaxError on multi-line blocks, and it lacked protection against several common Python sandbox bypasses.

🎯 Impact: Maliciously generated code could exfiltrate secrets or perform unauthorized actions using the pipeline's environment.

🔧 Fix:

  1. Environment Isolation: Modified test_python_code to pass a strictly filtered environment (allowlist: PATH, PYTHONPATH, LANG, PYTHONIOENCODING) to the subprocess.
  2. Code Wrapping Fix: Used textwrap.indent to ensure user code is correctly indented within the execution template, fixing a recurring SyntaxError.
  3. Regex Hardening: Updated DANGEROUS_PATTERNS to block importlib, __builtins__, __globals__, __subclasses__, and os.environ.
  4. Stability Fixes: Resolved a NameError in heidi_engine/telemetry.py and a FileNotFoundError in save_jsonl when writing to the current directory.

✅ Verification:

  • Verified environment isolation with custom scripts (secrets no longer visible to executed code).
  • Verified that code with if/for/def blocks now executes correctly.
  • Verified that obfuscated bypass attempts are now caught by the enhanced regex.
  • All 21 existing unit tests passed.
  • ruff linting checks passed.

PR created automatically by Jules for task 10674563983628842377 started by @heidi-dang

… unit test gate

🚨 Severity: CRITICAL
💡 Vulnerability: The scripts/03_unit_test_gate.py utility inherited the full environment, exposing secrets to executed code. Code wrapping logic was also broken, and bypass patterns were missing.
🎯 Impact: Potential exfiltration of sensitive API keys via generated code execution.
🔧 Fix: Filtered environment variables, fixed code indentation in template, and hardened dangerous pattern detection.
✅ Verification: Confirmed via reproduction scripts and full test suite.
@google-labs-jules
Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements security hardening measures and code refinements. It addresses environment variable leakage in the unit test gate by filtering subprocess environments and expands the detection of dangerous Python patterns. Additionally, it removes a thread-safe state cache in the telemetry engine and improves directory handling in JSONL saving utilities. Review feedback suggests further strengthening the security regex to include the builtins module and variants of dangerous os functions, while also noting a redundancy in the environment allowlist.

Comment on lines +71 to +72
r"\bimport\s+[^#\n]*\b(os|subprocess|sys|shutil|socket|requests|urllib|pathlib|pickle|pty|code|bdb|pdb|multiprocessing|threading|tempfile|ftplib|smtplib|telnetlib|http|xmlrpc|importlib|pkgutil|pydoc)\b",
r"\bfrom\s+(os|subprocess|sys|shutil|socket|requests|urllib|pathlib|pickle|pty|code|bdb|pdb|multiprocessing|threading|tempfile|ftplib|smtplib|telnetlib|http|xmlrpc|importlib|pkgutil|pydoc)\b",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The list of dangerous imports should include builtins. While you have blocked access to the __builtins__ attribute, an attacker could still use import builtins to access sensitive functions like eval or exec if they can bypass the other regex patterns (e.g., via builtins.__dict__).

Suggested change
r"\bimport\s+[^#\n]*\b(os|subprocess|sys|shutil|socket|requests|urllib|pathlib|pickle|pty|code|bdb|pdb|multiprocessing|threading|tempfile|ftplib|smtplib|telnetlib|http|xmlrpc|importlib|pkgutil|pydoc)\b",
r"\bfrom\s+(os|subprocess|sys|shutil|socket|requests|urllib|pathlib|pickle|pty|code|bdb|pdb|multiprocessing|threading|tempfile|ftplib|smtplib|telnetlib|http|xmlrpc|importlib|pkgutil|pydoc)\b",
r"\bimport\s+[^#\n]*\b(os|subprocess|sys|shutil|socket|requests|urllib|pathlib|pickle|pty|code|bdb|pdb|multiprocessing|threading|tempfile|ftplib|smtplib|telnetlib|http|xmlrpc|importlib|pkgutil|pydoc|builtins)\b",
r"\bfrom\s+(os|subprocess|sys|shutil|socket|requests|urllib|pathlib|pickle|pty|code|bdb|pdb|multiprocessing|threading|tempfile|ftplib|smtplib|telnetlib|http|xmlrpc|importlib|pkgutil|pydoc|builtins)\b",

r"\b__subclasses__\b",
# Dangerous module functions
r"\bos\.(system|popen|spawn|remove|unlink|rmdir|mkdir|chmod|chown|kill|exec|fork|pipe)\b",
r"\bos\.(system|popen|spawn|remove|unlink|rmdir|mkdir|chmod|chown|kill|exec|fork|pipe|environ)\b",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The regex for dangerous os functions is incomplete and easily bypassed.

  1. It misses environb (the bytes version of environ), getenv, and putenv.
  2. The use of \b around exec and spawn fails to catch common variants like os.execv, os.execve, os.spawnv, etc., as \b does not match the transition between a word character and another word character (like v).
Suggested change
r"\bos\.(system|popen|spawn|remove|unlink|rmdir|mkdir|chmod|chown|kill|exec|fork|pipe|environ)\b",
r"\bos\.(system|popen|spawn\w*|remove|unlink|rmdir|mkdir|chmod|chown|kill|exec\w*|fork|pipe|environb?|getenv|putenv)\b",

Comment on lines +270 to +272
allowed_env_keys = {"PATH", "PYTHONPATH", "LANG", "PYTHONIOENCODING"}
allowed_env = {k: os.environ[k] for k in allowed_env_keys if k in os.environ}
allowed_env["PYTHONPATH"] = temp_dir
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Including PYTHONPATH in allowed_env_keys is redundant because it is immediately overwritten by temp_dir on line 272. For clarity and to avoid confusion about whether the parent's PYTHONPATH is being preserved, it should be removed from the allowlist if the intention is to strictly isolate the execution to the temporary directory.

Suggested change
allowed_env_keys = {"PATH", "PYTHONPATH", "LANG", "PYTHONIOENCODING"}
allowed_env = {k: os.environ[k] for k in allowed_env_keys if k in os.environ}
allowed_env["PYTHONPATH"] = temp_dir
allowed_env_keys = {"PATH", "LANG", "PYTHONIOENCODING"}
allowed_env = {k: os.environ[k] for k in allowed_env_keys if k in os.environ}
allowed_env["PYTHONPATH"] = temp_dir

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant