From 74b7f87f4c6cfc72595ea866f24a7f27591a8730 Mon Sep 17 00:00:00 2001 From: Yif-Yang <29210256+Yif-Yang@users.noreply.github.com> Date: Sun, 26 Jul 2026 17:04:05 +0000 Subject: [PATCH] fix: close remaining generated-code secret leaks --- .../envs/spreadsheetbench/codegen_agent.py | 61 ++++-- skillopt/envs/spreadsheetbench/executor.py | 119 +++++++---- .../spreadsheetbench/prompts/react_system.md | 3 +- skillopt/envs/spreadsheetbench/react_agent.py | 34 ++-- skillopt_sleep/staging.py | 166 ++++++++++++++- tests/test_executor_env_isolation.py | 125 +++++++++++- tests/test_react_agent_no_shell.py | 11 + tests/test_staging_redaction_azure.py | 191 ++++++++++++++++++ 8 files changed, 622 insertions(+), 88 deletions(-) diff --git a/skillopt/envs/spreadsheetbench/codegen_agent.py b/skillopt/envs/spreadsheetbench/codegen_agent.py index b67a5089..32a8d679 100644 --- a/skillopt/envs/spreadsheetbench/codegen_agent.py +++ b/skillopt/envs/spreadsheetbench/codegen_agent.py @@ -258,40 +258,57 @@ def _build_codex_task( def _build_codex_driver() -> str: return ( + "import os\n" "import pathlib\n" "import re\n" + "import shutil\n" "import subprocess\n" - "import sys\n\n" + "import sys\n" "import tempfile\n\n" 'INPUT_PATH = "input.xlsx"\n' 'OUTPUT_PATH = "output.xlsx"\n' "code = pathlib.Path('solution.py').read_text(encoding='utf-8')\n" "code = re.sub(r'^\\s*(INPUT_PATH|OUTPUT_PATH)\\s*=\\s*.+$', '', code, flags=re.MULTILINE)\n" "# Write patched code to a temporary file and run it in a clean subprocess\n" - "# (avoids exec/compile of untrusted LLM-generated code in the current process).\n" - "_patched = pathlib.Path('_driver_runner.py')\n" - "_patched.write_text(\n" - " f'INPUT_PATH = {INPUT_PATH!r}\\nOUTPUT_PATH = {OUTPUT_PATH!r}\\n' + code,\n" - " encoding='utf-8',\n" - ")\n" - "import os as _os\n" - "_safe_env = {\n" - " 'PATH': _os.environ.get('PATH', '/usr/bin:/bin'),\n" - " 'HOME': str(pathlib.Path('_driver_runner.py').parent.resolve()),\n" - " 'TMPDIR': tempfile.gettempdir(),\n" - "}\n" - "if _os.name == 'nt':\n" - " _safe_env['SYSTEMROOT'] = _os.environ.get('SYSTEMROOT', '')\n" - " _safe_env['TEMP'] = tempfile.gettempdir()\n" - " _safe_env['TMP'] = tempfile.gettempdir()\n" - "_safe_env = {k: v for k, v in _safe_env.items() if v}\n" + "# with a scrubbed environment. This avoids in-process exec/compile but is\n" + "# not a filesystem, process, or network sandbox.\n" + "_work_dir = str(pathlib.Path.cwd())\n" + "_temp_dir = tempfile.mkdtemp(prefix='skillopt-generated-')\n" "try:\n" + " _patched = pathlib.Path(_temp_dir) / 'runner.py'\n" + " _safe_env = {\n" + " 'PATH': os.environ.get('PATH') or os.defpath,\n" + " 'HOME': _work_dir,\n" + " 'TMPDIR': _temp_dir,\n" + " }\n" + " for _key in (\n" + " 'PYTHONPATH', 'PYTHONHOME', 'VIRTUAL_ENV',\n" + " 'LD_LIBRARY_PATH', 'DYLD_LIBRARY_PATH',\n" + " 'LANG', 'LANGUAGE', 'LC_ALL', 'LC_CTYPE',\n" + " 'PYTHONIOENCODING', 'PYTHONUTF8',\n" + " 'SYSTEMDRIVE', 'PATHEXT', 'COMSPEC',\n" + " ):\n" + " if os.environ.get(_key):\n" + " _safe_env[_key] = os.environ[_key]\n" + " if os.name == 'nt':\n" + " _safe_env['SYSTEMROOT'] = (\n" + " os.environ.get('SYSTEMROOT')\n" + " or os.environ.get('SystemRoot')\n" + " or os.environ.get('WINDIR', '')\n" + " )\n" + " _safe_env['USERPROFILE'] = _work_dir\n" + " _safe_env['TEMP'] = _temp_dir\n" + " _safe_env['TMP'] = _temp_dir\n" + " _safe_env['APPDATA'] = _temp_dir\n" + " _safe_env['LOCALAPPDATA'] = _temp_dir\n" + " _safe_env = {k: v for k, v in _safe_env.items() if v}\n" + " _patched.write_text(\n" + " f'INPUT_PATH = {INPUT_PATH!r}\\nOUTPUT_PATH = {OUTPUT_PATH!r}\\n' + code,\n" + " encoding='utf-8',\n" + " )\n" " _res = subprocess.run([sys.executable, str(_patched)], capture_output=True, text=True, env=_safe_env)\n" "finally:\n" - " try:\n" - " _patched.unlink()\n" - " except OSError:\n" - " pass\n" + " shutil.rmtree(_temp_dir, ignore_errors=True)\n" "if _res.returncode != 0:\n" " print(_res.stdout, end='')\n" " print(_res.stderr, end='')\n" diff --git a/skillopt/envs/spreadsheetbench/executor.py b/skillopt/envs/spreadsheetbench/executor.py index 09b78807..0996f242 100644 --- a/skillopt/envs/spreadsheetbench/executor.py +++ b/skillopt/envs/spreadsheetbench/executor.py @@ -28,14 +28,70 @@ r'^\s*(INPUT_PATH|OUTPUT_PATH)\s*=\s*.+$', re.MULTILINE ) +_GENERATED_CODE_ENV_PASSTHROUGH = ( + # Preserve interpreter/import behavior without inheriting API/cloud keys. + "PYTHONPATH", + "PYTHONHOME", + "VIRTUAL_ENV", + "LD_LIBRARY_PATH", + "DYLD_LIBRARY_PATH", + # Keep text I/O deterministic for non-ASCII spreadsheet content. + "LANG", + "LANGUAGE", + "LC_ALL", + "LC_CTYPE", + "PYTHONIOENCODING", + "PYTHONUTF8", + # Needed by executable lookup and CPython on some Windows installations. + "SYSTEMDRIVE", + "PATHEXT", + "COMSPEC", +) + def _strip_path_assignments(code: str) -> str: """Remove INPUT_PATH/OUTPUT_PATH assignments from user code.""" return _PATH_ASSIGN_RE.sub("", code) +def generated_code_env(work_dir: str, temp_dir: str) -> dict[str, str]: + """Return the minimal environment for LLM-generated spreadsheet Python. + + This prevents direct inheritance of parent-process credentials. It is not a + filesystem, process, or network sandbox. + """ + private_dir = os.path.abspath(work_dir or os.getcwd()) + private_temp = os.path.abspath(temp_dir) + safe_env = { + "PATH": os.environ.get("PATH") or os.defpath, + "HOME": private_dir, + "TMPDIR": private_temp, + } + for key in _GENERATED_CODE_ENV_PASSTHROUGH: + value = os.environ.get(key) + if value: + safe_env[key] = value + if os.name == "nt": + system_root = ( + os.environ.get("SYSTEMROOT") + or os.environ.get("SystemRoot") + or os.environ.get("WINDIR") + or "" + ) + safe_env.update({ + "SYSTEMROOT": system_root, + "USERPROFILE": private_dir, + "TEMP": private_temp, + "TMP": private_temp, + "APPDATA": private_temp, + "LOCALAPPDATA": private_temp, + }) + return {key: value for key, value in safe_env.items() if value} + + def run_generated_code(code: str, input_path: str, output_path: str, timeout: int | None = 120) -> tuple[bool, str]: - os.makedirs(os.path.dirname(output_path), exist_ok=True) + output_dir = os.path.dirname(os.path.abspath(output_path)) + os.makedirs(output_dir, exist_ok=True) cleaned = _strip_path_assignments(code) indented = textwrap.indent(cleaned, " ") script = RUNNER_TEMPLATE.format( @@ -43,41 +99,28 @@ def run_generated_code(code: str, input_path: str, output_path: str, timeout: in output_path=output_path, user_code_indented=indented, ) - with tempfile.NamedTemporaryFile("w", suffix=".py", delete=False) as f: - f.write(script) - tmp = f.name - # Build a minimal environment so generated code does not directly inherit - # API keys, cloud credentials, or other parent-process environment values. - # This is environment isolation, not a filesystem or network sandbox. - import platform as _platform - _safe_env: dict[str, str] = { - "PATH": os.environ.get("PATH", "/usr/bin:/bin"), - "HOME": os.path.dirname(output_path), - "TMPDIR": tempfile.gettempdir(), - } - if _platform.system() == "Windows": - _safe_env["SYSTEMROOT"] = os.environ.get("SYSTEMROOT", "") - _safe_env["TEMP"] = tempfile.gettempdir() - _safe_env["TMP"] = tempfile.gettempdir() - # Omit platform-specific entries that are absent in the parent. - _safe_env = {k: v for k, v in _safe_env.items() if v} - try: - proc = subprocess.run( - [sys.executable, tmp], - capture_output=True, - text=True, - timeout=timeout if timeout and timeout > 0 else None, - env=_safe_env, - ) - if proc.returncode != 0: - return False, (proc.stdout + "\n" + proc.stderr).strip() - if not os.path.exists(output_path): - return False, "output file was not created" - return True, "" - except subprocess.TimeoutExpired: - return False, f"timeout after {timeout}s" - finally: + # Keep the runner and scratch files out of the result directory. Environment + # scrubbing prevents direct credential inheritance; it is not a filesystem, + # process, or network sandbox. + with tempfile.TemporaryDirectory( + prefix="skillopt-generated-", ignore_cleanup_errors=True + ) as temp_dir: + runner = os.path.join(temp_dir, "runner.py") + with open(runner, "w", encoding="utf-8") as f: + f.write(script) + safe_env = generated_code_env(output_dir, temp_dir) try: - os.unlink(tmp) - except OSError: - pass + proc = subprocess.run( + [sys.executable, runner], + capture_output=True, + text=True, + timeout=timeout if timeout and timeout > 0 else None, + env=safe_env, + ) + if proc.returncode != 0: + return False, (proc.stdout + "\n" + proc.stderr).strip() + if not os.path.exists(output_path): + return False, "output file was not created" + return True, "" + except subprocess.TimeoutExpired: + return False, f"timeout after {timeout}s" diff --git a/skillopt/envs/spreadsheetbench/prompts/react_system.md b/skillopt/envs/spreadsheetbench/prompts/react_system.md index afbbffb5..27987e11 100644 --- a/skillopt/envs/spreadsheetbench/prompts/react_system.md +++ b/skillopt/envs/spreadsheetbench/prompts/react_system.md @@ -2,7 +2,8 @@ You are an expert spreadsheet manipulation agent. {critical_rules}{skill_section}## Tools You have two tools: -- `bash` -- execute any shell command and receive its output. +- `bash` -- run a Python command whose executable is `python` or `python3`; + arbitrary shell commands are blocked. - `write_file` -- write content to a file (path, content). Use this for solution.py. ## Protocol diff --git a/skillopt/envs/spreadsheetbench/react_agent.py b/skillopt/envs/spreadsheetbench/react_agent.py index 2ddbc301..61faa1d6 100644 --- a/skillopt/envs/spreadsheetbench/react_agent.py +++ b/skillopt/envs/spreadsheetbench/react_agent.py @@ -12,9 +12,11 @@ import shlex import subprocess import sys +import tempfile from skillopt.model import chat_target_messages from skillopt.prompts import load_prompt +from skillopt.envs.spreadsheetbench.executor import generated_code_env # ── Tool schemas ───────────────────────────────────────────────────────────── @@ -23,13 +25,13 @@ "function": { "name": "bash", "description": ( - "Execute a bash command and receive stdout+stderr (truncated to 4000 chars). " - "Use Python to read / write Excel files." + "Run a Python command (python/python3 only) and receive stdout+stderr " + "(truncated to 4000 chars)." ), "parameters": { "type": "object", "properties": { - "cmd": {"type": "string", "description": "Bash command to execute."} + "cmd": {"type": "string", "description": "Python command to execute."} }, "required": ["cmd"], }, @@ -40,13 +42,13 @@ "type": "function", "name": "bash", "description": ( - "Execute a bash command and receive stdout+stderr (truncated to 4000 chars). " - "Use Python to read / write Excel files." + "Run a Python command (python/python3 only) and receive stdout+stderr " + "(truncated to 4000 chars)." ), "parameters": { "type": "object", "properties": { - "cmd": {"type": "string", "description": "Bash command to execute."} + "cmd": {"type": "string", "description": "Python command to execute."} }, "required": ["cmd"], }, @@ -279,14 +281,18 @@ def _run_bash(cmd: str, work_dir: str, timeout: int = 60) -> str: "use Python to manipulate spreadsheets]" ) parts[0] = sys.executable - proc = subprocess.run( - parts, - shell=False, - capture_output=True, - text=True, - timeout=timeout, - cwd=work_dir, - ) + with tempfile.TemporaryDirectory( + prefix="skillopt-generated-", ignore_cleanup_errors=True + ) as temp_dir: + proc = subprocess.run( + parts, + shell=False, + capture_output=True, + text=True, + timeout=timeout, + cwd=work_dir, + env=generated_code_env(work_dir, temp_dir), + ) out = (proc.stdout + proc.stderr).strip() except subprocess.TimeoutExpired: return f"[timeout after {timeout}s]" diff --git a/skillopt_sleep/staging.py b/skillopt_sleep/staging.py index 705ef2c7..12201ec9 100644 --- a/skillopt_sleep/staging.py +++ b/skillopt_sleep/staging.py @@ -16,6 +16,89 @@ from skillopt_sleep.types import SleepReport +# A secret value may be quoted, braced (ODBC-style), or an unquoted scalar. +# Accept EOF as the terminator for quoted/braced values because diagnostics are +# often truncated precisely where a failing client was printing a credential. +# Doubled quote/brace characters are the escape convention used by SQL/ODBC. +_UNQUOTED_SECRET_VALUE = ( + r'''(?:[^\s"';&,)\]}]|[)\]}]+(?=[^\s"';&,)\]}]))+''' +) +_SECRET_VALUE = ( + r'''(?:"(?:\\(?:[^\r\n]|(?=\r?\n|$))|""|[^"\\\r\n])*''' + r'''(?:"|(?=\r?\n|$))''' + r'''|'(?:\\(?:[^\r\n]|(?=\r?\n|$))|''|[^'\\\r\n])*''' + r'''(?:'|(?=\r?\n|$))''' + r'''|\{(?:\\(?:[^\r\n]|(?=\r?\n|$))|}}|[^}\\\r\n])*''' + r'''(?:}|(?=\r?\n|$))''' + r'''|''' + _UNQUOTED_SECRET_VALUE + r''')''' +) + +# Match both short labels (``token=``) and environment/connection-string names +# whose final component identifies a credential (``AZURE_CLIENT_SECRET=``). +_SECRET_NAME_BODY = ( + r"(?:(?:[A-Za-z0-9]+[_-])*(?:" + r"api[_-]?key|access[_-]?token|refresh[_-]?token|token|" + r"password|passwd|secret|secret[_-]?key|secret[_-]?access[_-]?key|" + r"shared[_-]?access[_-]?key|private[_-]?key" + r")|[A-Za-z0-9]*(?:" + r"apikey|accesstoken|refreshtoken|clientsecret|secretkey|" + r"secretaccesskey|sharedaccesskey|privatekey" + r"))" +) +_SECRET_ASSIGNMENT_NAME = ( + r"(?(?[\"'])" + + r"(?:" + _SECRET_NAME_BODY + r"|pwd|accountkey)" + + r"(?P=key_quote)\s*:\s*)" + + r"(?P" + _SECRET_VALUE + r")" +) + +_REDACTED_MARKER = re.compile(r"^\[REDACTED(?:_[A-Z_]+)?\]$") +_SECRET_MAPPING_KEY_SUFFIXES = ( + "apikey", + "accesstoken", + "refreshtoken", + "token", + "password", + "passwd", + "clientsecret", + "secret", + "secretkey", + "secretaccesskey", + "sharedaccesskey", + "privatekey", + "accountkey", +) + + +def _redact_json_assignment(match: re.Match[str]) -> str: + """Keep JSON-like value quotes while replacing their complete contents.""" + value = match.group("value") + quote = ( + value[:1] if value[:1] in {'"', "'"} else match.group("key_quote") + ) + return f"{match.group('prefix')}{quote}[REDACTED]{quote}" + + +def _is_secret_mapping_key(key: Any) -> bool: + """Recognize credential-bearing dict keys without flagging token budgets.""" + if not isinstance(key, str): + return False + stripped = key.strip() + # PWD is conventionally the non-secret process working directory. Mixed or + # lower-case ``Pwd`` remains a common database-password field. + if stripped == "PWD": + return False + compact = re.sub(r"[^a-z0-9]", "", stripped.casefold()) + return compact in {"pwd", "sig", "authorization"} or compact.endswith( + _SECRET_MAPPING_KEY_SUFFIXES + ) + # Secret patterns scrubbed from any free-text we persist to the staging dir # (diagnostics, reports). Kept here so every on-disk artifact shares one # redaction pass; harvest_codex reuses these for session text too. @@ -31,26 +114,70 @@ # the "Authorization:" prefix. (re.compile(r"\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\b"), "[REDACTED_JWT]"), - (re.compile(r"(?i)(Authorization:\s*Bearer\s+)[^\s\"']+"), r"\1[REDACTED]"), - (re.compile(r"(?i)(Authorization:\s*Basic\s+)[^\s\"']+"), r"\1[REDACTED]"), + ( + re.compile( + r'''(?i)(Authorization:\s*Bearer\s+)''' + r'''(?!\[REDACTED(?:_[A-Z_]+)?\])''' + + _SECRET_VALUE + ), + r"\1[REDACTED]", + ), + ( + re.compile( + r'''(?i)(Authorization:\s*Basic\s+)''' + r'''(?!\[REDACTED(?:_[A-Z_]+)?\])''' + + _SECRET_VALUE + ), + r"\1[REDACTED]", + ), # Connection-string passwords. Handle quoted values (which may contain # semicolons) before the generic name=value rule below, and retain the key # plus all non-secret connection-string fields for useful diagnostics. ( re.compile( - r'''(?i)(\bPassword\s*=\s*)(?:"[^"]+"|'[^']+'|[^;"'\s&]+)''' + r'''(?i)(\bPassword\s*=\s*)''' + r'''(?!\[REDACTED(?:_[A-Z_]+)?\])''' + + _SECRET_VALUE ), r"\1[REDACTED_DB_PASS]", ), + # ODBC commonly abbreviates Password as Pwd. Keep the conventional + # all-uppercase PWD working-directory variable intact. ( re.compile( - r"(?i)\b(api[_-]?key|token|password|secret)\b" - r"(\s*[:=]\s*)(?!\[REDACTED(?:_[A-Z_]+)?\])[^\s\"';&]+" + r"((?&...) - (re.compile(r"(?i)\bsig=[A-Za-z0-9%+/]{10,}"), "[REDACTED_SAS_SIG]"), + ( + re.compile(r"(?i)(\bsig\s*=\s*)[A-Za-z0-9%+/]{10,}"), + r"\1[REDACTED_SAS_SIG]", + ), # Azure Storage account keys (base64, typically 88 chars) - (re.compile(r"(?i)AccountKey=[A-Za-z0-9+/=]{20,}"), "[REDACTED_STORAGE_KEY]"), + ( + re.compile( + r'''(?i)(\bAccountKey\s*=\s*)''' + r'''(?!\[REDACTED(?:_[A-Z_]+)?\])''' + + _SECRET_VALUE + ), + r"\1[REDACTED_STORAGE_KEY]", + ), ) @@ -77,14 +214,23 @@ def redact_secrets(value: Any) -> Any: scalars pass through unchanged. """ if isinstance(value, str): - out = value + out = _JSON_SECRET_ASSIGNMENT.sub(_redact_json_assignment, value) for pattern, replacement in _SECRET_PATTERNS: out = pattern.sub(replacement, out) return out if isinstance(value, list): return [redact_secrets(v) for v in value] if isinstance(value, dict): - return {k: redact_secrets(v) for k, v in value.items()} + redacted = {} + for key, item in value.items(): + if _is_secret_mapping_key(key): + if isinstance(item, str) and _REDACTED_MARKER.fullmatch(item): + redacted[key] = item + else: + redacted[key] = "[REDACTED]" + else: + redacted[key] = redact_secrets(item) + return redacted return value diff --git a/tests/test_executor_env_isolation.py b/tests/test_executor_env_isolation.py index 9a8d4776..0c721ce3 100644 --- a/tests/test_executor_env_isolation.py +++ b/tests/test_executor_env_isolation.py @@ -7,11 +7,16 @@ """ from __future__ import annotations +import os +import shutil import subprocess import sys +import tempfile +from types import SimpleNamespace from skillopt.envs.spreadsheetbench.codegen_agent import _build_codex_driver -from skillopt.envs.spreadsheetbench.executor import run_generated_code +from skillopt.envs.spreadsheetbench.executor import generated_code_env, run_generated_code +from skillopt.envs.spreadsheetbench.react_agent import _run_bash # User code that records whether a given env var is visible to the child. @@ -48,16 +53,66 @@ def test_path_still_available_to_generated_code(tmp_path) -> None: assert out.read_text(encoding="utf-8") == "YES" +def test_non_secret_python_environment_is_preserved(tmp_path, monkeypatch) -> None: + monkeypatch.setenv("PYTHONPATH", "/safe/development/path") + monkeypatch.setenv("LANG", "C.UTF-8") + monkeypatch.setenv("AZURE_OPENAI_API_KEY", "must-not-leak") + + env = generated_code_env(str(tmp_path), str(tmp_path / "scratch")) + + assert env["PYTHONPATH"] == "/safe/development/path" + assert env["LANG"] == "C.UTF-8" + assert "AZURE_OPENAI_API_KEY" not in env + + +def test_installed_spreadsheet_dependency_is_importable(tmp_path) -> None: + probe = ( + "import openpyxl\n" + "with open(OUTPUT_PATH, 'w', encoding='utf-8') as _f:\n" + " _f.write('YES')\n" + ) + out = tmp_path / "out.txt" + + ok, err = run_generated_code(probe, str(tmp_path / "in.xlsx"), str(out)) + + assert ok, err + assert out.read_text(encoding="utf-8") == "YES" + + +def test_generated_scratch_directory_is_private_and_cleaned(tmp_path) -> None: + probe = ( + "import tempfile\n" + "scratch = tempfile.NamedTemporaryFile(delete=False)\n" + "scratch.close()\n" + "with open(OUTPUT_PATH, 'w', encoding='utf-8') as _f:\n" + " _f.write(scratch.name)\n" + ) + out = tmp_path / "out.txt" + + ok, err = run_generated_code(probe, str(tmp_path / "in.xlsx"), str(out)) + + assert ok, err + scratch_path = out.read_text(encoding="utf-8") + assert os.path.dirname(scratch_path) != str(tmp_path) + assert not os.path.exists(scratch_path) + + def test_codex_driver_scrubs_env_sets_tempdir_and_cleans_runner( tmp_path, monkeypatch ) -> None: monkeypatch.setenv("SUPER_SECRET_TOKEN", "do-not-inherit") + monkeypatch.setenv("PYTHONPATH", "/safe/development/path") + sentinel = tmp_path / "_driver_runner.py" + sentinel.write_text("do not overwrite", encoding="utf-8") (tmp_path / "solution.py").write_text( "import os\n" "with open(OUTPUT_PATH, 'w', encoding='utf-8') as f:\n" " f.write('|'.join([\n" " os.environ.get('SUPER_SECRET_TOKEN', 'ABSENT'),\n" " 'TMPDIR' if os.environ.get('TMPDIR') else 'NO_TMPDIR',\n" + " 'PRIVATE_TMP' if os.environ.get('TMPDIR') != os.getcwd() else 'BAD_TMP',\n" + " 'PRIVATE_HOME' if os.environ.get('HOME') == os.getcwd() else 'BAD_HOME',\n" + " 'DEV_PATH' if os.environ.get('PYTHONPATH') == '/safe/development/path' else 'NO_DEV_PATH',\n" " ]))\n", encoding="utf-8", ) @@ -73,5 +128,69 @@ def test_codex_driver_scrubs_env_sets_tempdir_and_cleans_runner( ) assert proc.returncode == 0, proc.stdout + proc.stderr - assert (tmp_path / "output.xlsx").read_text(encoding="utf-8") == "ABSENT|TMPDIR" - assert not (tmp_path / "_driver_runner.py").exists() + assert ( + tmp_path / "output.xlsx" + ).read_text(encoding="utf-8") == ( + "ABSENT|TMPDIR|PRIVATE_TMP|PRIVATE_HOME|DEV_PATH" + ) + assert sentinel.read_text(encoding="utf-8") == "do not overwrite" + + +def test_generated_code_tempdirs_tolerate_windows_cleanup_races( + tmp_path, monkeypatch +) -> None: + cleanup_modes = [] + + def simulated_windows_rmtree(cls, name, ignore_errors=False, repeated=False): + del cls, repeated + cleanup_modes.append(ignore_errors) + if not ignore_errors: + raise PermissionError(32, "directory is still in use", name) + shutil.rmtree(name, ignore_errors=True) + + monkeypatch.setattr( + tempfile.TemporaryDirectory, + "_rmtree", + classmethod(simulated_windows_rmtree), + ) + monkeypatch.setattr( + "skillopt.envs.spreadsheetbench.executor.subprocess.run", + lambda *args, **kwargs: SimpleNamespace(returncode=0, stdout="", stderr=""), + ) + out = tmp_path / "out.txt" + out.write_text("created", encoding="utf-8") + + ok, err = run_generated_code("pass", "input.xlsx", str(out)) + + assert ok, err + assert cleanup_modes == [True] + + +def test_react_tempdir_tolerates_windows_cleanup_races(tmp_path, monkeypatch) -> None: + cleanup_modes = [] + + def simulated_windows_rmtree(cls, name, ignore_errors=False, repeated=False): + del cls, repeated + cleanup_modes.append(ignore_errors) + if not ignore_errors: + raise PermissionError(32, "directory is still in use", name) + shutil.rmtree(name, ignore_errors=True) + + monkeypatch.setattr( + tempfile.TemporaryDirectory, + "_rmtree", + classmethod(simulated_windows_rmtree), + ) + monkeypatch.setattr( + "skillopt.envs.spreadsheetbench.react_agent.subprocess.run", + lambda *args, **kwargs: SimpleNamespace( + returncode=0, stdout="success", stderr="" + ), + ) + + assert _run_bash("python -c pass", str(tmp_path)) == "success" + assert cleanup_modes == [True] + + +def test_codex_driver_uses_cleanup_tolerant_tempdir() -> None: + assert "shutil.rmtree(_temp_dir, ignore_errors=True)" in _build_codex_driver() diff --git a/tests/test_react_agent_no_shell.py b/tests/test_react_agent_no_shell.py index 267bed46..c7d1f2d3 100644 --- a/tests/test_react_agent_no_shell.py +++ b/tests/test_react_agent_no_shell.py @@ -29,6 +29,17 @@ def test_similarly_named_executable_is_blocked(tmp_path) -> None: assert "blocked" in out.lower() +def test_python_child_does_not_inherit_parent_secrets( + tmp_path, monkeypatch +) -> None: + monkeypatch.setenv("SUPER_SECRET_TOKEN", "must-not-leak") + out = _run_bash( + 'python -c "import os; print(os.environ.get(\'SUPER_SECRET_TOKEN\', \'ABSENT\'))"', + str(tmp_path), + ) + assert out == "ABSENT" + + def test_shell_metacharacters_not_interpreted(tmp_path) -> None: # With shell=False the ';' and following tokens become arguments to python, # not a second shell command, so the marker file must NOT be created. diff --git a/tests/test_staging_redaction_azure.py b/tests/test_staging_redaction_azure.py index 6db50925..d7fe8a08 100644 --- a/tests/test_staging_redaction_azure.py +++ b/tests/test_staging_redaction_azure.py @@ -23,6 +23,15 @@ def test_storage_account_key_redacted() -> None: assert "aB3dEfGhIjKlMnOpQrStUvWx" not in out +def test_quoted_or_spaced_storage_account_keys_redacted() -> None: + for conn in ( + 'AccountKey = "aB3dEfGhIjKlMnOpQrStUvWx=="', + "AccountKey = {aB3dEfGhIjKlMnOpQrStUvWx==}", + ): + out = redact_secrets(conn) + assert out == "AccountKey = [REDACTED_STORAGE_KEY]" + + def test_connection_string_password_redacted() -> None: conn = "Server=db;Password=Sup3rSecret!;Database=app" out = redact_secrets(conn) @@ -37,12 +46,194 @@ def test_quoted_connection_string_password_redacted() -> None: assert out == "Server=db;Password=[REDACTED_DB_PASS];Database=app" +def test_braced_connection_string_password_redacted() -> None: + conn = "Server=db;Password={top;secret;value};Database=app" + out = redact_secrets(conn) + assert out == "Server=db;Password=[REDACTED_DB_PASS];Database=app" + + +def test_escaped_connection_string_values_are_fully_redacted() -> None: + for conn in ( + 'Server=db;Password="top""secret";Database=app', + "Server=db;Password={top}}secret};Database=app", + ): + out = redact_secrets(conn) + assert out == "Server=db;Password=[REDACTED_DB_PASS];Database=app" + + def test_generic_secret_redaction_preserves_following_fields() -> None: text = "token=top-secret&request=42;status=failed" out = redact_secrets(text) assert out == "token=[REDACTED]&request=42;status=failed" +def test_quoted_generic_secrets_redacted() -> None: + for text in ( + 'token="opaquevalue123"', + "api_key='opaquevalue123'", + "secret={opaque;value;123}", + ): + out = redact_secrets(text) + assert "opaque" not in out + assert "[REDACTED]" in out + + +def test_truncated_quoted_secrets_fail_closed() -> None: + for text in ( + 'token="opaquevalue123', + "api_key='opaquevalue123", + 'Password="Sup3rSecret!', + 'Authorization: Bearer "opaquevalue123', + ): + out = redact_secrets(text) + assert "opaquevalue123" not in out + assert "Sup3rSecret" not in out + assert "[REDACTED" in out + + multiline = 'token="opaquevalue123\nrequest failed' + assert redact_secrets(multiline) == "token=[REDACTED]\nrequest failed" + + +def test_backslash_escaped_quoted_secrets_are_fully_redacted() -> None: + for text in ( + 'token="top\\"secret";next=ok', + "api_key='top\\'secret';next=ok", + 'Authorization: Bearer "top\\"secret" next', + ): + out = redact_secrets(text) + assert "top" not in out + assert "secret" not in out.casefold() + assert "next" in out + + +def test_environment_style_secret_names_are_redacted() -> None: + for text in ( + "AZURE_CLIENT_SECRET=abcdefghijklmnopqrstuvwxyz", + "AZURE_OPENAI_API_KEY=abcdefghijklmnopqrstuvwxyz", + "access_token=abcdefghijklmnopqrstuvwxyz", + "refresh_token=abcdefghijklmnopqrstuvwxyz", + "SharedAccessKey=abcdefghijklmnopqrstuvwxyz", + "AWS_SECRET_ACCESS_KEY=abcdefghijklmnopqrstuvwxyz", + ): + out = redact_secrets(text) + assert "abcdefghijklmnopqrstuvwxyz" not in out, text + assert "[REDACTED]" in out + + +def test_secret_key_and_camel_case_names_are_redacted() -> None: + for text in ( + "SECRET_KEY=abcdefghijklmnopqrstuvwxyz", + "clientSecret=abcdefghijklmnopqrstuvwxyz", + "serviceAccessToken=abcdefghijklmnopqrstuvwxyz", + ): + out = redact_secrets(text) + assert "abcdefghijklmnopqrstuvwxyz" not in out, text + assert "[REDACTED]" in out + + +def test_process_pwd_is_preserved_but_odbc_pwd_is_redacted() -> None: + assert redact_secrets("PWD=/home/user/project") == "PWD=/home/user/project" + assert redact_secrets("Pwd=abcdefghijklmnopqrstuvwxyz") == ( + "Pwd=[REDACTED_DB_PASS]" + ) + connection = "Driver={ODBC Driver};UID=sa;PWD=hunter2;Database=app" + assert redact_secrets(connection) == ( + "Driver={ODBC Driver};UID=sa;PWD=[REDACTED_DB_PASS];Database=app" + ) + + +def test_long_alphabetic_bare_token_is_redacted() -> None: + assert redact_secrets("token abcdefghijklmnopqrstuvwxyz") == ( + "token [REDACTED]" + ) + + +def test_delimited_bare_secrets_are_redacted() -> None: + assert redact_secrets("token abc123def, retrying") == ( + "token [REDACTED], retrying" + ) + assert redact_secrets("password s3cret-value; retrying") == ( + "password [REDACTED]; retrying" + ) + + +def test_assignment_redaction_preserves_closing_delimiters() -> None: + assert redact_secrets("retry(token=abc123def)") == ( + "retry(token=[REDACTED])" + ) + assert redact_secrets("values[api_key=abc123def]") == ( + "values[api_key=[REDACTED]]" + ) + assert redact_secrets("token=ab}cd") == "token=[REDACTED]" + assert redact_secrets("token=ab}}cd") == "token=[REDACTED]" + assert redact_secrets("f(g(token=abc123def))") == ( + "f(g(token=[REDACTED]))" + ) + assert redact_secrets("[[api_key=abc123def]]") == ( + "[[api_key=[REDACTED]]]" + ) + + +def test_json_style_secret_assignments_remain_well_formed() -> None: + text = ( + '{"token":"opaquevalue123",' + '"api_key":"otheropaque456",' + '"AZURE_CLIENT_SECRET":"thirdopaque789"}' + ) + out = redact_secrets(text) + assert "opaque" not in out + assert out == ( + '{"token":"[REDACTED]",' + '"api_key":"[REDACTED]",' + '"AZURE_CLIENT_SECRET":"[REDACTED]"}' + ) + + +def test_mapping_keys_drive_recursive_redaction() -> None: + payload = { + "token": "opaquevalue123", + "nested": { + "access_token": "nestedopaque456", + "password": "Sup3rSecret!", + "token_budget": 42, + }, + "PWD": "/home/user/project", + } + + out = redact_secrets(payload) + + assert out["token"] == "[REDACTED]" + assert out["nested"]["access_token"] == "[REDACTED]" + assert out["nested"]["password"] == "[REDACTED]" + assert out["nested"]["token_budget"] == 42 + assert out["PWD"] == "/home/user/project" + + +def test_redaction_is_idempotent_across_supported_secret_forms() -> None: + samples = ( + "token=[REDACTED]", + "Password=[REDACTED_DB_PASS]", + "Pwd=[REDACTED_DB_PASS]", + "AccountKey=[REDACTED_STORAGE_KEY]", + "Authorization: Bearer [REDACTED]", + "retry(token=abc123def)", + "values[api_key=abc123def]", + '{"token":"opaquevalue123"}', + ) + for sample in samples: + once = redact_secrets(sample) + assert redact_secrets(once) == once, sample + + +def test_ordinary_security_prose_is_not_redacted() -> None: + for text in ( + "token budget exceeded", + "password reset failed", + "The token count is 42", + ): + assert redact_secrets(text) == text + + def test_recurses_into_containers() -> None: payload = {"logs": ["ok", "AccountKey=aB3dEfGhIjKlMnOpQrStUvWx=="]} out = redact_secrets(payload)