feat(tool-sandbox): support Git fsmonitor socket via unix_socket_bind - #1780
feat(tool-sandbox): support Git fsmonitor socket via unix_socket_bind#1780kipz wants to merge 2 commits into
Conversation
PR Review SummarySize
Affected crates
Blast radius — BroadThis PR touches: source code,documentation,configuration / policy files Updated automatically on each push to this PR. |
There was a problem hiding this comment.
nogent code review
1 high-severity security bypass, 1 bug, and 1 schema design finding.
Findings (not tied to a changed line):
- 🏗️ [LOW · design]
crates/nono-cli/data/nono-profile.schema.json:802— The newunix_socket_bindcommand-sandbox field was added toCommandSandboxConfigand documented, but is missing from the command policies schema definition incrates/nono-cli/data/nono-profile.schema.json. Update the schema to allowunix_socket_bindunder individual command sandbox policies.
Automated code + security review. CI already covers clippy, rustfmt, tests, cargo-audit and commit-lint.
| use super::dynamic_providers::expand_dynamic_tokens; | ||
| // Must canonicalize cwd to match dynamic-token providers, or a symlinked | ||
| // cwd escapes the write non-escalation downgrade. | ||
| let canonical_cwd = cwd |
There was a problem hiding this comment.
Caution
🔒 [HIGH · security] The write non-escalation downgrade logic in add_policy_unix_sockets (on both Linux and macOS) checks normalized.starts_with(&canonical_cwd). While canonical_cwd is canonicalized, normalized (returned by lexically_normalize) is not. Under a symlinked CWD (such as /tmp on macOS, which is a symlink to /private/tmp), a literal relative path like my.sock resolves to /tmp/my.sock, so normalized.starts_with(&canonical_cwd) evaluates to false. This bypasses the downgrade check, granting ReadWrite access to the parent directory and escalating write privileges outside the agent's authorized write root. Update the condition to check both: (normalized.starts_with(cwd) || normalized.starts_with(&canonical_cwd)).
| if raw.is_empty() { | ||
| return None; | ||
| } | ||
| let backlink = PathBuf::from(raw).canonicalize().ok()?; |
There was a problem hiding this comment.
Warning
🐛 [MEDIUM · bug] In verify_worktree_backlink, PathBuf::from(raw).canonicalize() is called where raw represents the gitdir backlink path. Since Git worktrees often use relative paths for backlinks, calling .canonicalize() on a relative PathBuf without prefixing it with the parent directory resolves it against the process's working directory rather than the worktree metadata root. This breaks verification for valid repositories using relative backlinks. Join raw to canonical_candidate when relative before calling .canonicalize().
Adds a unix_socket_bind command sandbox field for connect(2)/bind(2) grants on named AF_UNIX sockets, plus the @git:fsmonitor-socket token so a command running with core.fsmonitor=true can reach the per- worktree fsmonitor daemon's IPC socket without a broader filesystem grant. The token is resolved by a pure filesystem walk (no git process spawn), matching how the other git path tokens avoid trusting repo-local config. Signed-off-by: James Carnegie <me@kipz.org>
…unix_socket_bind The write non-escalation downgrade for unix_socket_bind compared a canonicalized cwd against a merely-normalized candidate path, so a literal relative socket entry under a symlinked cwd (e.g. macOS /tmp) bypassed the downgrade and got ReadWrite instead of Read. The worktree-backlink verifier canonicalized a relative gitdir backlink against the process cwd instead of the worktree metadata root, breaking verification for repos with relative backlinks. Also add the missing unix_socket_bind field to the command-policy schema. Signed-off-by: James Carnegie <me@kipz.org>
70acd69 to
c781cb3
Compare
|
Addressed the nogent findings (c781cb3):
Rebased onto latest main, |
Linked Issue
Closes #1779
Summary
Adds a
unix_socket_bindcommand-sandbox field for connect(2)/bind(2) grants on named AF_UNIX sockets, plus the@git:fsmonitor-sockettoken so a command running withcore.fsmonitor=truecan reach the per-worktree fsmonitor daemon's IPC socket without a broader filesystem grant.Test Plan
make ci(fmt, clippy -D warnings, full test suite)nono why --profile <profile> --command git --path <repo>/.git/fsmonitor--daemon.ipc --op readwrite --workdir <repo>against a real git worktree, confirming the token resolves to the exact daemon socket path and the grant is allowedChecklist
Agent Compliance Check (Required for AI/Automated PRs)