diff --git a/coworker/permissions.py b/coworker/permissions.py index 82477f7b..9903dcc9 100644 --- a/coworker/permissions.py +++ b/coworker/permissions.py @@ -8,6 +8,7 @@ from __future__ import annotations +import re import shlex from dataclasses import dataclass, field from enum import Enum @@ -24,6 +25,24 @@ def _has_shell_operators(command: str) -> bool: return any(op in command for op in _SHELL_OPERATORS) + +# `C:\dir` / `C:/dir` - a drive-qualified path, which carries no separator of its own. +_WIN_DRIVE = re.compile(r"\A[A-Za-z]:[\\/]") + + +def _looks_like_path(token: str) -> bool: + """Whether an argument names a filesystem location rather than a pattern, a + subcommand or a value. Intentionally narrow: a bare word like `status`, `install` or + `'*.py'` is not treated as a path, so only arguments that genuinely point somewhere + are scope-checked.""" + return ( + token in (".", "..") + or token.startswith(("/", "~", "./", "../", "\\")) + or "/" in token + or "\\" in token + or bool(_WIN_DRIVE.match(token)) + ) + from .risk import ( # re-exported for back-compat (manager.py imports WRITE_TOOLS) SHELL_TOOL, WRITE_TOOLS, @@ -150,7 +169,12 @@ def evaluate( # interactive / custom: allowlists. if is_shell: command = str(arguments.get("command", "")) - if self._command_allowed(command): + # Being on the allowlist says the PROGRAM is safe to run unattended. It says + # nothing about where that program is pointed, and `read_file` already refuses + # to leave the granted roots ("path escapes the workspace"), so auto-running + # `cat ~/.ssh/id_rsa` would hand back through the shell exactly what the file + # tool declines to read. Keep the same boundary on both paths. + if self._command_allowed(command) and not self._path_outside_roots(command): return Decision(True, "command on allowlist") if command and command in self.session_allow_commands: return Decision(True, "command allowed for session") @@ -191,6 +215,34 @@ def _candidate(self, path: str) -> Path: p = Path(path).expanduser() return p.resolve() if p.is_absolute() else (self.workspace_root / p).resolve() + def _path_outside_roots(self, command: str) -> Optional[str]: + """The first argument of `command` naming a location outside every granted root, + or None if nothing does. + + Only argument tokens that actually look like paths are considered, so patterns + and subcommands (`'*.py'`, `status`, `install`) are ignored. A relative path is + resolved against the workspace and therefore stays inside it unless it climbs + out with `..`; `_under_root` resolves symlinks, so a link planted in the + workspace does not launder a target outside it. + """ + try: + argv = shlex.split(command) + except ValueError: + return None # unparsable; _command_allowed already refuses these + for token in argv[1:]: + if token.startswith("-"): + # `--file=/etc/passwd` carries the path on the right of the `=`; a bare + # flag carries none. + _, sep, value = token.partition("=") + if not sep or not value: + continue + token = value + if not _looks_like_path(token): + continue + if not self._under_root(token): + return token + return None + def _under_root(self, path: str) -> bool: candidate = self._candidate(path) for rp, _ in self._resolved_roots(): diff --git a/tests/test_permissions_risk.py b/tests/test_permissions_risk.py index c41e1141..0becfe14 100644 --- a/tests/test_permissions_risk.py +++ b/tests/test_permissions_risk.py @@ -131,6 +131,85 @@ def test_allowlist_prefix_is_argv_boundary(tmp_path): assert eng.evaluate("run_shell", {"command": "lsof"}, None).needs_user +def _scoped_engine(tmp_path): + """Workspace + a second granted (read-only) root, with reader commands allowlisted.""" + ws = tmp_path / "ws" + (ws / "src").mkdir(parents=True) + (ws / "README.md").write_text("hello\n") + (ws / "src" / "app.py").write_text("x = 1\n") + docs = tmp_path / "docs" + docs.mkdir() + (docs / "notes.md").write_text("notes\n") + return ws, docs, PermissionEngine( + workspace_root=ws, + allowed_commands=["ls", "cat", "pwd", "grep", "find", "head", "wc", "pytest"], + roots=[{"path": ws, "writable": True}, {"path": docs, "writable": False}], + ) + + +@pytest.mark.parametrize( + "command", + [ + "cat /etc/passwd", + "cat ~/.ssh/id_rsa", + "cat ../../../etc/passwd", + "head -n 5 /etc/hosts", + "wc -l /etc/passwd", + "grep -r AKIA /Users", + "ls /", + "pytest --collect-only /tmp/evil_test.py", + "grep --file=/etc/passwd pattern", # path hidden on the right of `=` + ], +) +def test_allowlisted_reader_does_not_auto_run_outside_the_roots(tmp_path, command): + # `read_file` already refuses to leave the granted roots ("path escapes the + # workspace"). An allowlisted `cat` must not hand back through the shell what the + # file tool declines to read. + _, _, eng = _scoped_engine(tmp_path) + d = eng.evaluate("run_shell", {"command": command}, None) + assert not d.allowed and d.needs_user, command + + +def test_symlink_out_of_the_workspace_does_not_launder_the_read(tmp_path): + ws, _, eng = _scoped_engine(tmp_path) + secret = tmp_path / "secret" + secret.mkdir() + (secret / "id_rsa").write_text("SECRET\n") + (ws / "link").symlink_to(secret) + # The argument is workspace-relative, but it resolves outside the roots. + d = eng.evaluate("run_shell", {"command": "cat link/id_rsa"}, None) + assert not d.allowed and d.needs_user + + +@pytest.mark.parametrize( + "command", + [ + "ls -la", + "pwd", + "grep -n TODO README.md", + "grep -rn TODO src/", + "find . -name '*.py'", + "find . -type f -maxdepth 2", + "cat README.md", + "cat ./src/app.py", + "wc -l src/app.py", + "pytest -q", + ], +) +def test_reads_inside_the_roots_still_auto_run(tmp_path, command): + # Patterns and subcommands are not paths, and a relative path stays inside the + # workspace unless it climbs out, so ordinary use is untouched. + _, _, eng = _scoped_engine(tmp_path) + assert eng.evaluate("run_shell", {"command": command}, None).allowed, command + + +def test_absolute_paths_inside_a_granted_root_still_auto_run(tmp_path): + ws, docs, eng = _scoped_engine(tmp_path) + assert eng.evaluate("run_shell", {"command": f"cat {ws}/README.md"}, None).allowed + # A second granted root is readable too, not just the primary workspace. + assert eng.evaluate("run_shell", {"command": f"cat {docs}/notes.md"}, None).allowed + + def test_shell_commands_not_auto_allowed_by_default(tmp_path): # There is no generally safe executable: these examples cover code execution, # environment disclosure, reads outside the workspace, and helper execution.