fix(runtime): block sandbox escape via intermediate-ancestor symlink on writes to non-existent dirs#6299
Conversation
…on writes to non-existent dirs resolve_sandbox_path_ext is the single resolver behind file_write, apply_patch, and the network-reachable web_fetch_to_file. When the target's parent directory did not exist it took a string-rebase shortcut: it stripped the workspace prefix and rejoined the suffix onto the canonical root, so the returned path still contained any existing intermediate-ancestor symlink while passing the starts_with check at the string level. The leaf-symlink guard (#5141) only inspects the final path component, so a path like link/newdir/file.txt — where link is a symlink to a directory outside the workspace and newdir does not exist yet — resolved to <workspace>/link/newdir/file.txt, passed the check, and then had the caller's create_dir_all + write follow link straight out of the jail (overwriting e.g. ~/.ssh/authorized_keys or ~/.librefang/config.toml). The non-existent-parent branch now walks up to the deepest ancestor that actually exists and canonicalizes that, resolving every ancestor symlink (including a symlinked workspace root such as macOS /tmp -> /private/tmp), then appends the still-non-existent, symlink-free suffix. An escaping ancestor is rejected by the existing starts_with check while legitimate deep-mkdir writes — including symlinks that stay inside the workspace — keep working. Adds regression tests for the relative, absolute, and named-workspace escape variants plus the inside-workspace-symlink positive case.
github-actions
Bot
added
size/M
houko
left a comment
houko
left a comment
There was a problem hiding this comment.
Security fix looks correct. The deepest-existing-ancestor walk + canonicalize approach properly closes the ancestor-symlink escape in the non-existent-parent branch, the positive (inside_workspace_still_resolves) test confirms no over-blocking, and the CHANGELOG entry follows the one-sentence-per-line rule.
Two findings — see inline comments for the first:
1. Multi-line comment blocks (inline, workspace_sandbox.rs)
Both the implementation block (~20 lines) and the per-test intro blocks (~7 lines each) violate CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max."
The WHY is non-obvious enough to warrant a comment; the rule just caps it at one line.
Details and suggested replacements are in the inline notes above.
2. Same-crate deferral: ProviderCooldown wiring (librefang-runtime)
The "Out of scope" section defers the ProviderCooldown circuit-breaker wiring with "both call sites pass None."
CLAUDE.md: "Fix what you found — don't punt it to a 'follow-up'. The bar to defer: would fixing it require touching a different crate or domain than the one you're already in? If no, fix it in this PR. If yes, surface the question to the human reviewer with the concrete trade-off and ask before deferring; don't decide unilaterally."
Both call sites are inside librefang-runtime — the same crate this PR already touches.
The correct action is either to fix it here or to explicitly present the trade-off to the reviewer and ask before deferring.
The other three out-of-scope items (PATCH /api/agents/{id}/config, upsert_secret, memory_config_patch) touch different crates, so those deferrals are legitimate.
Generated by Claude Code
| // Parent doesn't exist yet. We must NOT string-strip the workspace | ||
| // root and rejoin onto the canonical root: an EXISTING intermediate | ||
| // ancestor (between the root and the non-existent parent) may itself | ||
| // be a symlink pointing outside every allowed root. The leaf guard | ||
| // above only inspects the FINAL component, and the `parent.exists()` | ||
| // branch above only canonicalizes the immediate parent — so a path | ||
| // like `link/newdir/file.txt`, where `link` is a symlink to an | ||
| // outside directory and `newdir` does not exist, would be rebased to | ||
| // `<canon_root>/link/newdir/file.txt`, pass the `starts_with` check | ||
| // at the string level, and then have the caller's `create_dir_all` + | ||
| // write follow `link` straight out of the jail. | ||
| // | ||
| // For an absolute candidate whose ancestor is one of the additional | ||
| // roots, rebase onto that canonical root. Otherwise rebase onto the | ||
| // canonical primary workspace root. This is safe because: | ||
| // 1. We already rejected `..` components. | ||
| // 2. The relative suffix is appended to a canonical root and no | ||
| // symlinks can exist in the (non-existent) subtree. | ||
| let mut rebased: Option<PathBuf> = None; | ||
| if path.is_absolute() { | ||
| for root in additional_roots { | ||
| if let Ok(rel) = candidate.strip_prefix(root) { | ||
| rebased = Some(root.join(rel)); | ||
| break; | ||
| } | ||
| // Resolve it safely instead: walk up to the deepest ancestor that | ||
| // actually exists and canonicalize THAT. Canonicalization resolves | ||
| // every ancestor symlink (including a symlinked workspace root such | ||
| // as macOS `/tmp -> /private/tmp`), so the `starts_with` check below | ||
| // sees the real on-disk location and rejects an escaping ancestor. | ||
| // The remaining suffix is symlink-free by construction — it does not | ||
| // exist on disk yet — so appending it cannot reintroduce an escape. | ||
| // `..` components were already rejected at the top of the function. |
There was a problem hiding this comment.
CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max."
This 20-line block is the WHY (non-obvious security invariant), so a comment is justified — but the rule caps it at one line regardless of context.
Suggested condensed form:
// Walk to the deepest existing ancestor and canonicalize it: any existing
// intermediate symlink is then caught by the starts_with check below (#6299).The detailed attack description lives in the PR body and CHANGELOG, where prose length is unrestricted.
Generated by Claude Code
| // ATTACK: an EXISTING intermediate component is a symlink pointing | ||
| // outside the workspace, and the leaf's parent does NOT exist yet — so | ||
| // the resolver lands in the non-existent-parent branch. Before the fix | ||
| // that branch string-rebased onto the canonical root and passed the | ||
| // `starts_with` check, after which `tool_file_write`'s `create_dir_all` | ||
| // + write followed the symlink out of the jail. After the fix the | ||
| // deepest existing ancestor is canonicalized and the escape is caught. |
There was a problem hiding this comment.
Same CLAUDE.md rule applies inside tests: one short // line max per comment block.
The function name (test_intermediate_ancestor_symlink_escape_blocked) and the assert! message already document what's being tested.
The same applies to the comment blocks in the other three new test functions.
Suggested: remove these intro blocks entirely, or collapse each to a single line referencing the PR, e.g.:
// Regression for #6299: intermediate-ancestor symlink, non-existent-parent branch.Generated by Claude Code
The fix dropped the literal additional-root rebase in the non-existent-parent branch in favor of canonicalizing the deepest existing ancestor. The existing positive test only covered an inside-workspace symlink under the primary root, leaving the additional-root path — the most likely over-block regression from that change — unguarded. Adds test_new_nested_file_under_additional_root_resolves: a new file in a not-yet-existing subdir addressed through an additional (named-workspace) root must still resolve inside that root.
1 participant
Summary
Closes a workspace-sandbox escape in
librefang-runtime'sresolve_sandbox_path_ext— the single path resolver behind thefile_write,apply_patch, andweb_fetch_to_filetools.When the write target's parent directory did not exist, the resolver took a string-rebase shortcut: it stripped the
workspace_rootprefix from the candidate and rejoined the suffix onto the canonical root.The returned path therefore still contained any existing intermediate-ancestor symlink, yet passed the
starts_with(canon_root)check at the string level.The leaf-symlink guard added in #5141 only inspects the final path component (
symlink_metadata(candidate)), and theparent.exists()branch only canonicalizes the immediate parent — so an ancestor symlink slipped through.Exploit
With a symlink
link -> /outsidepresent in the workspace (operator setup, a named-workspace mount, or created by the agent in exec mode), a write tolink/newdir/file.txtwherenewdirdoes not exist:candidate.exists()followslink->/outside/newdir/file.txt-> missing ->false, so it enters the non-existent-parent branch.symlink_metadata(candidate)returnsErr(the leaf and its parent don't exist), so nothing is rejected.parent(link/newdir) doesn't exist, so the code rebases to<canon_workspace>/link/newdir/file.txtandstarts_withpasses.tool_file_writethen runscreate_dir_all(parent)+ write on that path, the OS followslink, and the bytes land at/outside/newdir/file.txt— outside the jail.web_fetch_to_filemakes this reachable from a network-driven agent turn with no shell access (fetch remote bytes, write them through the ancestor symlink), enabling overwrite of files such as~/.ssh/authorized_keys,~/.librefang/config.toml, or cron files.Fix
The non-existent-parent branch now walks up to the deepest ancestor that actually exists, canonicalizes that (resolving every ancestor symlink, including a symlinked workspace root such as macOS
/tmp -> /private/tmp), and appends the still-non-existent, symlink-free suffix.An escaping ancestor is then rejected by the existing
starts_withcheck, while legitimate deep-mkdirwrites — including symlinks that stay inside the workspace — keep working unchanged.This brings the non-existent-parent branch in line with the
parent.exists()branch, which already canonicalizes and is safe.Tests
Adds four
#[cfg(unix)]regression tests inworkspace_sandbox.rs:test_intermediate_ancestor_symlink_escape_blocked— relative path through a symlinked ancestor with a non-existent leaf parent is rejected, and the escape target dir is not created.test_intermediate_ancestor_symlink_escape_blocked_absolute— same via an absolute path.test_intermediate_ancestor_symlink_escape_blocked_via_additional_root— same when addressed through a named-workspace (additional_roots) prefix.test_intermediate_symlink_to_inside_workspace_still_resolves— positive case: an inside-workspace symlink with a non-existent subdir still resolves (no over-blocking).All pre-existing
workspace_sandboxtests (leaf-symlink #5141 cases,additional_rootscases, the non-existent-parent happy path) are unaffected.Verification
This host has no native Rust toolchain and no working container runtime (Docker Desktop is not installed; the
dockershim is a dangling symlink), so localcargo/ Docker verification was not possible — CI is the build/test gate here.The change is one self-contained function in one crate; I hand-checked types, borrows, and rustfmt shape, and traced every new and pre-existing test through the new branch.
Reviewers / CI run:
cargo test -p librefang-runtime workspace_sandbox.Out of scope
This PR is intentionally scoped to the single sandbox-resolver fix.
A repo-wide audit in the same session surfaced other independent findings in different crates/domains — the
ProviderCooldowncircuit breaker is never wired intolibrefang-runtime's retry path (both call sites passNone);PATCH /api/agents/{id}/configsilently dropsapi_key_env/base_urlthat its OpenAPI schema advertises;upsert_secretrejects newlines in the value but not the key;memory_config_patchcan panic on a non-table[memory]config.These will be handled as separate issues / follow-up PRs per the one-PR-one-domain policy.