diff --git a/.flox/env/on-activate.sh b/.flox/env/on-activate.sh index baf9e18b18c6..6253f2c6bc86 100755 --- a/.flox/env/on-activate.sh +++ b/.flox/env/on-activate.sh @@ -374,27 +374,67 @@ fi # the venv bin (Step 2b), so worktrees on different branches resolve their # own pin. A failed install must not break activation; the CLI is only needed # at PR-open time and `hogli review` prints install guidance when absent. +# +# The store is an execute-later target: Step 2b links the binary into the venv +# and `hogli review` runs it in the developer's shell, outside the sandbox. The +# sandbox therefore write-denies the store (see bin/dev-sandbox.sb) and this +# trusted, unsandboxed shell performs the install itself. Skipping the sandbox +# is safe here: --ignore-scripts keeps package code from running at install +# time (npm only downloads, integrity-checks, and extracts), and the CLI runs +# unsandboxed at review time anyway. Do not sandbox the npm run instead: any +# sandbox-writable path it installs through (a TMPDIR staging dir, the shared +# ~/.npm cache) gives a concurrently sandboxed dependency a window to swap the +# binary before it is published. Every input this npm run reads is pinned away +# from sandboxed writers: the cache lives inside the write-denied store, the +# user config is disabled (and the sandbox write-denies ~/.npmrc anyway), cwd +# plus a fresh package.json keep the project-config lookup inside the store +# instead of walking up to $HOME, and npm and flock themselves are resolved +# into the immutable Nix store rather than taken from a PATH that contains +# sandbox-writable dirs. _GREPTILE_VERSION="3.4.1" _GREPTILE_STORE="$HOME/.config/posthog/tools/greptile/$_GREPTILE_VERSION" _GREPTILE_BIN="$_GREPTILE_STORE/node_modules/.bin/greptile" _GREPTILE_STAMP="$_GREPTILE_STORE/.complete" +_GREPTILE_CACHE="$_GREPTILE_STORE/.npm-cache" _install_greptile() { # Explicit `|| return`/`|| exit`: callers suppress errexit, so a failed # install would otherwise fall through and stamp the broken state. + # + # PATH here contains sandbox-writable dirs: the venv bin prepended above, and + # $FLOX_ENV/bin, which the repo's .flox/run symlink can be repointed at. A + # planted npm or flock shim there would run outside the sandbox. Resolve both + # through /usr/bin/readlink (a system binary, not PATH-resolved) and refuse + # anything that lands outside the immutable Nix store. The resolved npm's + # shebang is Nix-patched to the store node, so it does not look up node via + # PATH; the coreutils below come from the pinned system dirs. + local npm_bin flock_bin + npm_bin="$(/usr/bin/readlink -f "$(command -v npm)" 2>/dev/null)" || return 1 + flock_bin="$(/usr/bin/readlink -f "$(command -v flock)" 2>/dev/null)" || return 1 + [[ "$npm_bin" == /nix/store/* && "$flock_bin" == /nix/store/* ]] || return 1 + local PATH="/usr/bin:/bin" mkdir -p "$_GREPTILE_STORE" || return 1 + # A lock file planted as a symlink while the store was still sandbox-writable + # would make the `9>` redirection below truncate the symlink's target with + # the developer's own account. The write-deny keeps a new one from appearing. + if [[ -L "$_GREPTILE_STORE/.install.lock" ]]; then + rm -f "$_GREPTILE_STORE/.install.lock" || return 1 + fi ( # The store is shared across checkouts, so serialize concurrent # activations (fresh worktrees) installing the same version. - flock 9 || exit 1 + "$flock_bin" 9 || exit 1 if [[ ! -x "$_GREPTILE_BIN" || ! -f "$_GREPTILE_STAMP" ]]; then - if [[ "$_DEV_SANDBOX_INSTALLS" -eq 1 ]]; then - # printf %q: dev-sandbox re-parses its command string, so the path - # must survive a $HOME with spaces or quotes. - "$FLOX_ENV_PROJECT/bin/dev-sandbox" "npm install --prefix $(printf '%q' "$_GREPTILE_STORE") --no-fund --no-audit greptile@$_GREPTILE_VERSION" || exit 1 - else - npm install --prefix "$_GREPTILE_STORE" --no-fund --no-audit "greptile@$_GREPTILE_VERSION" || exit 1 - fi + cd "$_GREPTILE_STORE" || exit 1 + # Rebuild from a clean slate: npm trusts an existing node_modules tree, + # and a store from before the write-deny can hold planted files, such as + # a package.json or .npmrc symlink the writes below would follow. Remove + # every entry except the lock this subshell holds open. + find . -mindepth 1 -maxdepth 1 ! -name .install.lock -exec rm -rf {} + || exit 1 + echo '{}' > package.json || exit 1 + NPM_CONFIG_USERCONFIG=/dev/null "$npm_bin" install --cache "$_GREPTILE_CACHE" \ + --ignore-scripts --no-fund --no-audit "greptile@$_GREPTILE_VERSION" || exit 1 + rm -rf "$_GREPTILE_CACHE" [[ -x "$_GREPTILE_BIN" ]] || exit 1 touch "$_GREPTILE_STAMP" || exit 1 fi diff --git a/bin/dev-sandbox-selftest b/bin/dev-sandbox-selftest index 0d4a60883201..3f24c4130473 100755 --- a/bin/dev-sandbox-selftest +++ b/bin/dev-sandbox-selftest @@ -187,6 +187,29 @@ check_write_blocked ".git/hooks/.dev_sandbox_selftest_probe" ".git/hooks" if [[ -d "$HOME/.cargo/bin" ]]; then check_write_blocked "$HOME/.cargo/bin/.dev_sandbox_selftest_probe" "cargo bin" fi +# The machine-shared tool store: flox activation publishes the greptile CLI here +# and hogli review executes it in the developer's shell, outside Seatbelt — the +# same execute-later route as cargo bin above. Creating the dir is safe: this is +# the store's real location, and activation creates it the same way. +mkdir -p "$HOME/.config/posthog/tools" 2>/dev/null +if [[ -d "$HOME/.config/posthog/tools" ]]; then + check_write_blocked "$HOME/.config/posthog/tools/.dev_sandbox_selftest_probe" "machine tool store (greptile runs unsandboxed)" + # A rename could swap in a symlinked ancestor and move the store out from + # under the subpath deny. chmod needs the same file-write* verdict as rename + # without touching the real directories, and the bits it sets are already set. + $SANDBOX "chmod u+rwx '$HOME/.config/posthog'" >/dev/null 2>&1 + report_write_blocked $? "tool store parent node (\$HOME/.config/posthog rename swap)" + $SANDBOX "chmod u+rwx '$HOME/.config'" >/dev/null 2>&1 + report_write_blocked $? "\$HOME/.config node (rename swap)" + # Positive control: the ancestor denies are literals, not subpaths — entries + # inside ~/.config must stay writable or sandboxed tools lose their config dirs. + if $SANDBOX "echo x > '$HOME/.config/.dev_sandbox_selftest_probe'" >/dev/null 2>&1; then + pass "write allowed: entry inside ~/.config (the deny is the node, not a subpath)" + rm -f "$HOME/.config/.dev_sandbox_selftest_probe" + else + die "write blocked, should be allowed: entry inside ~/.config" + fi +fi # Homebrew's bin/sbin/Cellar: developer-owned and on PATH, same execute-later route # as cargo bin above — no git-config indirection needed to reach it. for hb_bin in /opt/homebrew/bin /usr/local/bin; do @@ -213,6 +236,14 @@ if [[ -n "$gitcommon" && -f "$gitcommon/config" ]]; then else echo " -- repo .git/config not found, skipping" fi +# npm user config: registry/proxy/cafile keys redirect where an unsandboxed npm +# (the activation's tool-store install) downloads code from — the ~/.gitconfig +# escape with npm as the trusted tool. +if [[ -f "$HOME/.npmrc" ]]; then + check_write_blocked_existing "$HOME/.npmrc" "npm user config (registry/proxy redirect)" +else + check_write_blocked "$HOME/.npmrc" "npm user config (registry/proxy redirect)" +fi # The scripts that actually run outside the sandbox. core.hooksPath points git at # .husky, so the .git/hooks deny above guards a directory git never reads once that # is set; these are the real hooks. .flox/env holds the activation scripts, and diff --git a/bin/dev-sandbox.sb b/bin/dev-sandbox.sb index 2ae4cc967d89..c2ec9bba720c 100644 --- a/bin/dev-sandbox.sb +++ b/bin/dev-sandbox.sb @@ -156,6 +156,11 @@ (literal (string-append (param "HOME") "/.gitconfig")) ; core.hooksPath / credential.helper = !cmd (literal (string-append (param "HOME") "/.gitconfig.local")) ; included by ~/.gitconfig — same escape if writable (subpath (string-append (param "HOME") "/.config/git")) ; XDG git config — same hooksPath/helper escape + ;; npm user config. Its registry, proxy, and cafile keys redirect where an + ;; unsandboxed npm (such as the activation's tool-store install above) + ;; downloads code from, which is the ~/.gitconfig escape with npm as the + ;; trusted tool. Reads were already denied with the rest of $HOME. + (literal (string-append (param "HOME") "/.npmrc")) ;; Homebrew's system-scope gitconfig, read by a brew-installed git on every ;; invocation and owned by the developer's account — unlike the Xcode CLT one, ;; which is root-owned and so already out of reach. Same hooksPath/fsmonitor @@ -183,7 +188,23 @@ (subpath "/usr/local/sbin") (subpath "/usr/local/Cellar") (subpath "/usr/local/lib") - (subpath (string-append (param "HOME") "/.cargo/bin"))) ; binaries on PATH + (subpath (string-append (param "HOME") "/.cargo/bin")) ; binaries on PATH + ;; The machine-shared tool store. The unsandboxed flox activation installs the + ;; greptile CLI here (npm --ignore-scripts, so no package code runs), links it + ;; into the venv bin, and `hogli review` executes it in the developer's shell — + ;; the same execute-later route as cargo bin above. The venv bin symlink itself + ;; cannot be denied here: sandboxed `uv sync` creates and populates the venv, so + ;; a sandboxed dependency can replace that link. `hogli review` therefore refuses + ;; a greptile that resolves outside the write-denied locations (see + ;; _TRUSTED_INSTALL_ROOTS in tools/hogli-commands/hogli_commands/review.py); + ;; this deny is what makes the store binary it does run trustworthy. + (subpath (string-append (param "HOME") "/.config/posthog/tools")) + ;; …and the ancestor nodes, so a rename cannot swap in a symlinked ~/.config or + ;; ~/.config/posthog and move the store out from under the subpath deny — the + ;; same node-swap route as the /opt/homebrew/etc denies above. Entries inside + ;; ~/.config are their own paths and stay writable. + (literal (string-append (param "HOME") "/.config/posthog")) + (literal (string-append (param "HOME") "/.config"))) ;; husky's installer writes .husky/_/ (a gitignored husky.sh plus a .gitignore) on ;; every sandboxed `pnpm install`, and it throws when that write fails, which would diff --git a/tools/hogli-commands/hogli_commands/review.py b/tools/hogli-commands/hogli_commands/review.py index 4dfcbe81fde3..8cca43b8891a 100644 --- a/tools/hogli-commands/hogli_commands/review.py +++ b/tools/hogli-commands/hogli_commands/review.py @@ -30,8 +30,11 @@ from __future__ import annotations +import os +import sys import shutil import subprocess +from pathlib import Path import click from hogli.manifest import REPO_ROOT @@ -44,6 +47,49 @@ "Re-enter the flox environment (activation installs it), " "or install it with `brew install greptileai/tap/greptile` or `npm install -g greptile`, then re-run." ) + +# PATH in an activated shell starts with the venv bin, which the dev sandbox +# can write to (the sandboxed `uv sync` populates it), so a compromised +# dependency could plant a `greptile` there that would run unsandboxed with +# the developer's account. Only execute a binary that resolves into a location +# bin/dev-sandbox.sb write-denies: the PostHog tool store, the Homebrew +# prefixes, or the immutable Nix store. Keep this list in sync with the +# `deny file-write*` block in bin/dev-sandbox.sb. +_TRUSTED_INSTALL_ROOTS: tuple[Path, ...] = ( + Path.home() / ".config" / "posthog" / "tools", + Path("/opt/homebrew/bin"), + Path("/opt/homebrew/sbin"), + Path("/opt/homebrew/Cellar"), + Path("/opt/homebrew/lib"), + Path("/usr/local/bin"), + Path("/usr/local/sbin"), + Path("/usr/local/Cellar"), + Path("/usr/local/lib"), + Path("/nix/store"), +) + +# greptile's shebang is `#!/usr/bin/env node`, and it spawns git, so its +# subprocesses resolve executables from PATH at execution time. Give them a +# PATH of write-denied and system dirs only, so a binary planted in the +# sandbox-writable venv bin (first on the activated shell's PATH) cannot run. +_TRUSTED_PATH_DIRS = ( + "/opt/homebrew/bin", + "/usr/local/bin", + "/usr/bin", + "/bin", + "/usr/sbin", + "/sbin", +) + + +def _trusted_location(path: str) -> Path | None: + """The canonical path when it sits in a write-denied install location, else None.""" + resolved = Path(path).resolve() + if any(resolved.is_relative_to(root) for root in _TRUSTED_INSTALL_ROOTS): + return resolved + return None + + _SIGNIN_HINT = ( "Run `greptile login`, or set GREPTILE_API_KEY in .env.local (see .env.local.example). " "No access? The reviewing-before-pr skill has a harness-review fallback." @@ -64,35 +110,37 @@ _CHECK_COMMIT_LIMIT = 20 -def _probe(cmd: list[str]) -> subprocess.CompletedProcess[str] | None: +def _probe(cmd: list[str], env: dict[str, str] | None) -> subprocess.CompletedProcess[str] | None: try: - return subprocess.run(cmd, capture_output=True, text=True, timeout=_PROBE_TIMEOUT_SECONDS) + return subprocess.run(cmd, capture_output=True, text=True, timeout=_PROBE_TIMEOUT_SECONDS, env=env) except (OSError, subprocess.SubprocessError): return None -def _signed_in(binary: str) -> bool: +def _signed_in(binary: str, env: dict[str, str] | None) -> bool: """False only on Greptile's explicit signed-out error; any other ``config`` failure falls through to the review call, which reports the real problem. The string match is forced: greptile exits 1 for signed-out and for ordinary failures alike.""" - result = _probe([binary, "config"]) + result = _probe([binary, "config"], env) return result is None or result.returncode == 0 or "not signed in" not in (result.stdout + result.stderr).lower() -def _branch_commits(base: str | None) -> list[str]: +def _branch_commits(base: str | None, env: dict[str, str] | None) -> list[str]: # Match change_detection's base convention: origin/master, then master for # clones without the remote ref. for ref in [base] if base is not None else ["origin/master", "master"]: - result = _probe(["git", "-C", str(REPO_ROOT), "rev-list", f"--max-count={_CHECK_COMMIT_LIMIT}", f"{ref}..HEAD"]) + result = _probe( + ["git", "-C", str(REPO_ROOT), "rev-list", f"--max-count={_CHECK_COMMIT_LIMIT}", f"{ref}..HEAD"], env + ) if result is not None and result.returncode == 0: return result.stdout.split() or ["HEAD"] return ["HEAD"] -def check(binary: str, base: str | None) -> int: - for commit in _branch_commits(base): - status = _probe([binary, "review", "status", "--commit", commit]) +def check(binary: str, base: str | None, env: dict[str, str] | None) -> int: + for commit in _branch_commits(base, env): + status = _probe([binary, "review", "status", "--commit", commit], env) if status is None: # A hung or broken probe would hang or break for every commit too. break @@ -112,14 +160,44 @@ def run(branch: str | None, instructions: str | None, force: bool, do_check: boo if binary is None: click.secho(f"Greptile CLI not found. {_INSTALL_HINT}", fg="red", err=True) return 1 - if not _signed_in(binary): + env: dict[str, str] | None = None + # The dev sandbox only exists on macOS; elsewhere these checks would just + # break legitimate installs (an npm prefix under $HOME) for no protection. + if sys.platform == "darwin": + trusted = _trusted_location(binary) + if trusted is None: + click.secho( + f"Not running greptile from {Path(binary).resolve()}: only write-protected install locations " + "are trusted (the PostHog tool store, Homebrew, or the Nix store). " + "Re-enter the flox environment to reinstall it, or use `brew install greptileai/tap/greptile`, " + "then re-run.", + fg="red", + err=True, + ) + return 1 + # Execute the canonical path, not the PATH entry which() found: that + # entry is usually the sandbox-writable venv symlink, which a sandboxed + # process could repoint between this validation and the exec. + binary = str(trusted) + node = shutil.which("node") + node_dir = None if node is None else _trusted_location(node) + if node_dir is None: + click.secho( + "node was not found in a write-protected location, and greptile runs on node. " + "Re-enter the flox environment, then re-run.", + fg="red", + err=True, + ) + return 1 + env = {**os.environ, "PATH": ":".join([str(node_dir.parent), *_TRUSTED_PATH_DIRS])} + if not _signed_in(binary, env): click.secho(f"Not signed in to Greptile. {_SIGNIN_HINT}", fg="yellow", err=True) return EX_CONFIG if do_check: - return check(binary, branch) + return check(binary, branch, env) if not force: - status = _probe([binary, "review", "status", "--commit", "HEAD"]) + status = _probe([binary, "review", "status", "--commit", "HEAD"], env) if status is not None and status.returncode == _STATUS_COMPLETED: click.secho( "HEAD already has a completed review. Showing it. Pass --force to start a new one.", @@ -130,7 +208,7 @@ def run(branch: str | None, instructions: str | None, force: bool, do_check: boo return 0 if status is not None and status.returncode == _STATUS_RUNNING: click.secho("A review for this branch is still running. Resuming it.", fg="cyan", err=True) - return subprocess.run([binary, "review", "--resume"]).returncode + return subprocess.run([binary, "review", "--resume"], env=env).returncode cmd = [binary, "review"] if branch is not None: @@ -138,7 +216,7 @@ def run(branch: str | None, instructions: str | None, force: bool, do_check: boo if instructions is not None: cmd += ["--instructions", instructions] # Inherit stdio so Greptile's own progress and interactive review view work. - return subprocess.run(cmd).returncode + return subprocess.run(cmd, env=env).returncode @click.command( diff --git a/tools/hogli-commands/hogli_commands/tests/test_review.py b/tools/hogli-commands/hogli_commands/tests/test_review.py index ad41082a2479..64e6721705ee 100644 --- a/tools/hogli-commands/hogli_commands/tests/test_review.py +++ b/tools/hogli-commands/hogli_commands/tests/test_review.py @@ -2,12 +2,14 @@ import subprocess from collections.abc import Callable, Iterator +from pathlib import Path import pytest from unittest.mock import MagicMock, patch from click.testing import CliRunner from hogli.cli import cli +from hogli_commands import review runner = CliRunner() @@ -62,6 +64,81 @@ def test_missing_cli_names_the_install_command(self, greptile_on_path: MagicMock assert result.exit_code == 1 assert "brew install greptileai/tap/greptile" in result.output + @pytest.mark.parametrize( + "platform,which_path,expected_exit", + [ + ("darwin", str(Path.home() / ".config/posthog/tools/greptile/3.4.1/node_modules/.bin/greptile"), 0), + ("darwin", "/usr/local/bin/greptile", 0), + ("darwin", "/Users/dev/posthog/.flox/cache/venv/bin/greptile", 1), + ("linux", "/home/dev/.npm-global/bin/greptile", 0), + ], + ) + @patch("hogli_commands.review.subprocess.run") + def test_darwin_runs_greptile_only_from_write_denied_locations( + self, + mock_run: MagicMock, + greptile_on_path: MagicMock, + platform: str, + which_path: str, + expected_exit: int, + ) -> None: + greptile_on_path.return_value = which_path + mock_run.side_effect = _fake_greptile(status=0) + with patch("hogli_commands.review.sys.platform", platform): + result = runner.invoke(cli, ["review"]) + assert result.exit_code == expected_exit + if expected_exit == 1: + # A planted binary must never execute, not even as an auth probe. + assert mock_run.call_args_list == [] + assert "flox" in result.output + + @patch("hogli_commands.review.subprocess.run") + def test_darwin_executes_the_resolved_path_not_the_symlink( + self, mock_run: MagicMock, greptile_on_path: MagicMock, tmp_path: Path + ) -> None: + link = tmp_path / "greptile" + link.symlink_to(_BINARY) + greptile_on_path.return_value = str(link) + mock_run.side_effect = _fake_greptile(status=0) + with patch("hogli_commands.review.sys.platform", "darwin"): + result = runner.invoke(cli, ["review"]) + assert result.exit_code == 0 + greptile_calls = [call.args[0] for call in mock_run.call_args_list if call.args[0][0] != "git"] + assert greptile_calls + # The writable symlink can be repointed after validation; only the + # canonical write-denied path may reach exec. + assert all(cmd[0] == _BINARY for cmd in greptile_calls) + + @pytest.mark.parametrize( + "node_path,expected_exit", + [ + ("/opt/homebrew/bin/node", 0), + ("/Users/dev/posthog/.flox/cache/venv/bin/node", 1), + ], + ) + @patch("hogli_commands.review.subprocess.run") + def test_darwin_pins_path_to_a_trusted_node( + self, + mock_run: MagicMock, + greptile_on_path: MagicMock, + node_path: str, + expected_exit: int, + ) -> None: + greptile_on_path.side_effect = lambda name: {"greptile": _BINARY, "node": node_path}[name] + mock_run.side_effect = _fake_greptile(status=0) + with patch("hogli_commands.review.sys.platform", "darwin"): + result = runner.invoke(cli, ["review"]) + assert result.exit_code == expected_exit + if expected_exit == 0: + assert mock_run.call_args_list + # greptile's shebang resolves node from PATH, so every subprocess + # must get the pinned PATH: the trusted node's dir, then only + # write-denied and system dirs. + pinned = ":".join([str(Path(node_path).resolve().parent), *review._TRUSTED_PATH_DIRS]) + assert all(call.kwargs["env"]["PATH"] == pinned for call in mock_run.call_args_list) + else: + assert mock_run.call_args_list == [] + @patch("hogli_commands.review.subprocess.run") def test_signed_out_exits_ex_config_without_starting_a_review(self, mock_run: MagicMock) -> None: mock_run.return_value = _proc(1, stderr="error: not signed in. Set GREPTILE_API_KEY or run `greptile login`.")