diff --git a/coworker/permissions.py b/coworker/permissions.py index 1044f37d..8df17f6a 100644 --- a/coworker/permissions.py +++ b/coworker/permissions.py @@ -27,6 +27,48 @@ def _has_shell_operators(command: str) -> bool: return any(op in command for op in _SHELL_OPERATORS) +def protected_paths() -> list[Path]: + """Files that govern the permission system itself. Nothing the agent does may write + these — in any mode, through any tool. The escalation this blocks is: approve one + ordinary-looking command, it quietly appends to the rule file, every future session is + more permissive. That happens in the DEFAULT interactive mode, so this cannot be a + property of a sandbox or of any one mode; it is a floor.""" + from .secrets import state_dir + + base = state_dir() + return [ + base / "config.toml", + base / "risk_overrides.json", + base / "workspace_trust.json", + base / "unattended.json", + base / "coworker.db", # session records carry the saved "always allow" grants + base / "secrets.json", + base / "inbox_routing.json", + ] + + +# Files INSIDE a workspace that execute on a later, innocuous-looking action. An edit here +# is a deferred command: writing `.git/hooks/pre-commit` and then running `git commit` runs +# it. They stay writable, but never WITHOUT a human — no auto-approve path may clear them. +_PROTECTED_IN_PROJECT = ( + ".git/hooks/", + ".github/workflows/", + ".gitlab-ci.yml", + ".vscode/tasks.json", + ".coworker/", # workspace policy + skills the agent would otherwise self-grant +) + + +def _is_protected_in_project(candidate: Path) -> bool: + posix = candidate.as_posix() + return any( + (f"/{marker}" in posix or posix.startswith(marker)) + if marker.endswith("/") + else posix.endswith("/" + marker) + for marker in _PROTECTED_IN_PROJECT + ) + + def _host_of(url_or_domain: str) -> str: """The lowercased host of a URL, or a bare domain as-is. `''` when there's nothing usable. Accepts both `https://docs.python.org/x` and `docs.python.org`.""" @@ -182,6 +224,19 @@ class PermissionEngine: is_egress = risk is RiskClass.EGRESS consequential = is_consequential(risk) + # SELF-PROTECTION FLOOR — runs before mode, allowlists and every auto-approve path, + # because the escalation it blocks happens in the DEFAULT mode. No verdict below can + # reach these files, and no human click in the flow can grant it either: loosening + # requires editing the files out-of-band. + if is_write or is_shell: + hit = self._touches_protected(tool_name, arguments, is_shell) + if hit is not None: + return Decision( + False, + f"refusing to modify OpenWorker's own settings: {hit}", + needs_user=False, + ) + # Discuss / plan modes: read-only. if self.mode in READ_ONLY_MODES and consequential: return Decision( @@ -191,6 +246,7 @@ class PermissionEngine: # Path scoping for writes (all modes): every path the write touches must land in a # writable root. A write whose path can't be located is not scoped-able, so it fails # closed to approval rather than slipping through auto/custom unscoped. + needs_human_for_protected = False if is_write: paths, located = write_paths(tool_name, arguments) if not located: @@ -204,11 +260,24 @@ class PermissionEngine: return Decision( False, f"path is not in a writable directory: {path}" ) + # In-project files that run on a later action (git hooks, CI configs) may be + # edited, but never by an auto-approve path — a human must see it. + if _is_protected_in_project(self._candidate(path)): + needs_human_for_protected = True # Non-consequential tools always run. if not consequential: return Decision(True, "low risk") + # A protected in-project target (git hooks, CI config) skips every auto-approve path + # below — including auto mode and the session/config allowlists — and asks. + if needs_human_for_protected: + return Decision( + False, + "this file runs automatically later — approval required", + needs_user=True, + ) + # Full access. if self.mode is Mode.AUTO: return Decision(True, "full access") @@ -289,6 +358,40 @@ class PermissionEngine: continue return False + def _touches_protected( + self, tool_name: str, arguments: dict[str, Any], is_shell: bool + ) -> Optional[str]: + """The protected settings path this call would modify, or None. + + For writes we resolve the real target. For shell we can only inspect the command + text — parser depth, so it stops accidents and casual attempts, not a determined + adversary (that needs the OS sandbox). Cheap and worth having regardless. + + Shell matching is on the FULL path only, never a bare filename: matching + `secrets.json` anywhere in a command would refuse unrelated work that merely + mentions the name. A command naming the real settings path is refused whether it + reads or writes — we cannot tell which from text, and the conservative direction is + the right one for these files. + """ + targets = [str(p) for p in protected_paths()] + if is_shell: + command = str(arguments.get("command", "")) + if not command: + return None + lowered = command.replace("\\", "/").lower() + for target in targets: + if target.replace("\\", "/").lower() in lowered: + return target + return None + paths, located = write_paths(tool_name, arguments) + if not located: + return None # unlocatable writes are already failed closed by the caller + resolved = {str(self._candidate(p)) for p in paths} + for target in targets: + if str(Path(target).resolve()) in resolved: + return target + return None + def _domain_allowed(self, url: str) -> bool: """True when the URL's host is an allowed egress destination — an exact match or a subdomain of an allowed domain (so `docs.python.org` matches `python.org`, but diff --git a/tests/test_self_protection.py b/tests/test_self_protection.py new file mode 100644 index 00000000..d58b1630 --- /dev/null +++ b/tests/test_self_protection.py @@ -0,0 +1,100 @@ +"""PR2 — the permission system protects its own settings, and in-project files that +execute later are never auto-approved. + +The escalation being blocked: approve one ordinary-looking command, it quietly appends to +`risk_overrides.json`, and every future session is more permissive. That happens in the +DEFAULT interactive mode, so the protection must be a floor — not a property of a sandbox +or of any one mode. See `ocw-context/docs/reviewed-auto-mode.md` Part 3. +""" + +from __future__ import annotations + +import pytest + +from coworker.permissions import Mode, PermissionEngine, protected_paths +from coworker.secrets import state_dir + +ALL_MODES = [Mode.DISCUSS, Mode.PLAN, Mode.INTERACTIVE, Mode.CUSTOM, Mode.AUTO] + + +def _engine(tmp_path, mode=Mode.INTERACTIVE, **kw): + # The state dir is redirected per-test by the autouse fixture in conftest, so the + # protected paths point somewhere isolated. + return PermissionEngine(workspace_root=tmp_path, mode=mode, **kw) + + +# -- the settings files --------------------------------------------------------- +@pytest.mark.parametrize("mode", ALL_MODES) +def test_write_to_settings_blocked_in_every_mode(tmp_path, mode): + eng = _engine(tmp_path, mode) + target = str(state_dir() / "risk_overrides.json") + d = eng.evaluate("write_file", {"path": target, "content": "x"}, None) + assert not d.allowed + assert not d.needs_user, "must be a hard refusal, not an approval the user can grant" + + +@pytest.mark.parametrize("mode", ALL_MODES) +def test_shell_touching_settings_blocked_in_every_mode(tmp_path, mode): + eng = _engine(tmp_path, mode) + target = str(state_dir() / "risk_overrides.json") + d = eng.evaluate("run_shell", {"command": f'echo "{{}}" > "{target}"'}, None) + assert not d.allowed and not d.needs_user + + +def test_settings_write_beats_auto_mode_and_allowlists(tmp_path): + # Every auto-approve path must lose to the floor. + target = str(state_dir() / "config.toml") + auto = _engine(tmp_path, Mode.AUTO) + assert not auto.evaluate("write_file", {"path": target, "content": "x"}, None).allowed + + custom = _engine(tmp_path, Mode.CUSTOM, auto_allow_tools={"write_file"}) + assert not custom.evaluate("write_file", {"path": target, "content": "x"}, None).allowed + + session = _engine(tmp_path) + session.allow_tool_for_session("write_file") + assert not session.evaluate("write_file", {"path": target, "content": "x"}, None).allowed + + +def test_settings_protected_via_patch_blob(tmp_path): + # The patch path is extracted from the blob, so this route is covered too. + eng = _engine(tmp_path, Mode.AUTO) + target = str(state_dir() / "workspace_trust.json") + patch = f"*** Begin Patch\n*** Update File: {target}\n@@\n-a\n+b\n*** End Patch" + assert not eng.evaluate("apply_patch", {"patch": patch}, None).allowed + + +def test_protected_paths_cover_the_grant_and_trust_stores(): + names = {p.name for p in protected_paths()} + assert {"config.toml", "risk_overrides.json", "workspace_trust.json", "coworker.db"} <= names + + +# -- in-project files that run later -------------------------------------------- +@pytest.mark.parametrize( + "rel", + [ + ".git/hooks/pre-commit", + ".github/workflows/ci.yml", + ".vscode/tasks.json", + ".coworker/config.toml", + ], +) +def test_protected_in_project_never_auto_approved(tmp_path, rel): + # Writable (inside the root) but must always reach a human, even in auto mode. + for mode in (Mode.AUTO, Mode.CUSTOM, Mode.INTERACTIVE): + eng = _engine(tmp_path, mode, auto_allow_tools={"write_file"}) + eng.allow_tool_for_session("write_file") + d = eng.evaluate("write_file", {"path": rel, "content": "x"}, None) + assert not d.allowed, f"{rel} auto-approved in {mode.value}" + assert d.needs_user, f"{rel} should ask, not hard-deny, in {mode.value}" + + +def test_ordinary_project_file_still_auto_approves(tmp_path): + # The protection must not leak onto normal edits. + eng = _engine(tmp_path, Mode.AUTO) + assert eng.evaluate("write_file", {"path": "src/app.py", "content": "x"}, None).allowed + + +def test_lookalike_paths_are_not_protected(tmp_path): + # A file merely NAMED like a hook, outside the protected dirs, is ordinary. + eng = _engine(tmp_path, Mode.AUTO) + assert eng.evaluate("write_file", {"path": "docs/pre-commit.md", "content": "x"}, None).allowed