feat(shell): fold the selected session directory into the jail roots - #375
feat(shell): fold the selected session directory into the jail roots#375ytallo wants to merge 2 commits into
Conversation
A console-selected working directory arrives as the per-call base_dir, but
the coder jail only permitted paths inside the statically configured
host_roots — so any selected directory outside them was rejected with C215
("path is outside every allowed root").
PathResolver::session_scoped adds the selected base_dir into the effective
allowed roots for that call (via with_session_root), so every downstream
check treats it as a first-class root: containment in resolve/resolve_in,
the non-accessible denylist (which relativises through containing_root),
is_root, and session_root. It is a no-op when base_dir is absent, already
inside a configured root, or uncanonicalizable.
Safe to widen on: base_dir is stamped by the harness control plane, never
the model, so only operator-chosen directories grow the jail — and
resolve_in still scopes access to the session directory. Wired into every
base_dir-bearing coder dispatch closure.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 26 skipped (no docs/).
Four for four. Nicely done. |
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesSession-Scoped Path Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shell/src/code/path.rs (1)
786-828: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add a no-op assertion for a non-canonicalizable
base_dir.The test covers outside-jail widening, denylist enforcement, inside-jail no-op, and
Noneno-op, but not the third documented no-op (lines 472-474): abase_dirthat cannot be canonicalized should return the resolver unchanged so the handler's ownresolve_inproduces the precise error. A one-line assert pins that branch.💚 Suggested addition
// A base_dir that cannot be canonicalized is a no-op (the handler's own // resolve_in then surfaces the precise error). assert_eq!( r.session_scoped(Some("/this/does/not/exist/xyz123")).roots().len(), r.roots().len() );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shell/src/code/path.rs` around lines 786 - 828, Add a no-op assertion for the non-canonicalizable base_dir branch in session_scoped. The current test in PathResolver::session_scoped covers outside-jail widening, denylist behavior, inside-root no-op, and None no-op, but not the case where the selected base_dir cannot be canonicalized. Add an assertion that a clearly invalid path leaves the resolver unchanged so resolve_in can surface the precise error from the handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shell/src/code/path.rs`:
- Around line 485-496: Treat the session_scoped(base_dir) input as untrusted and
do not use caller-provided request data to widen the jail. Update session_scoped
in Path so it only derives the session root from server-side/control-plane
metadata, or explicitly reject non-control-plane base_dir values before calling
canonicalize_wire, containing_root, or with_session_root; keep the clone-only
behavior for untrusted/absent inputs.
---
Nitpick comments:
In `@shell/src/code/path.rs`:
- Around line 786-828: Add a no-op assertion for the non-canonicalizable
base_dir branch in session_scoped. The current test in
PathResolver::session_scoped covers outside-jail widening, denylist behavior,
inside-root no-op, and None no-op, but not the case where the selected base_dir
cannot be canonicalized. Add an assertion that a clearly invalid path leaves the
resolver unchanged so resolve_in can surface the precise error from the handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97e63f65-9fba-4b6c-bace-9e441d5c2b53
📒 Files selected for processing (2)
shell/src/code/functions/mod.rsshell/src/code/path.rs
| pub fn session_scoped(self: &Arc<Self>, base_dir: Option<&str>) -> Arc<Self> { | ||
| let Some(bd) = base_dir else { | ||
| return self.clone(); | ||
| }; | ||
| let Ok(base_canon) = self.canonicalize_wire(bd, Path::new(bd)) else { | ||
| return self.clone(); | ||
| }; | ||
| if self.containing_root(&base_canon).is_some() { | ||
| return self.clone(); | ||
| } | ||
| Arc::new(self.with_session_root(base_canon)) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Where is base_dir defined on the input structs and how is it deserialized?
rg -nP -C3 '\bbase_dir\b' --type=rust shell/src/code/functions | rg -nP -C3 '(struct|serde|deserialize|Deserialize|Option<String>)'
# 2. Find the input struct definitions carrying base_dir.
ast-grep run --pattern 'struct $S { $$$ base_dir: $T, $$$ }' --lang rust shell/src
# 3. Is base_dir ever overwritten/injected server-side (control plane / workspace injection)
# rather than taken verbatim from the deserialized model request?
rg -nP -C3 '\bbase_dir\s*[:=]' --type=rust shell/src | rg -nvP '//'
# 4. How requests reach register_function / new_async — is the arg payload model-authored?
rg -nP -C4 'new_async|register_function' --type=rust shell/src/code/functions/mod.rsRepository: iii-hq/workers
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== functions/mod.rs outline =="
ast-grep outline shell/src/code/functions/mod.rs --view expanded | sed -n '1,220p'
echo
echo "== functions/types.rs outline =="
ast-grep outline shell/src/functions/types.rs --view expanded | sed -n '1,260p'
echo
echo "== request structs with base_dir in code/functions =="
rg -n -C4 'pub base_dir: Option<String>|base_dir: req\.base_dir\.as_deref\(\)|base_dir:' shell/src/code/functions/*.rs shell/src/functions/types.rs
echo
echo "== registration / schema plumbing in functions/mod.rs =="
sed -n '1,260p' shell/src/code/functions/mod.rs
echo
echo "== top of functions/types.rs =="
sed -n '1,240p' shell/src/functions/types.rsRepository: iii-hq/workers
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shell/src/code/functions/mod.rs around register_all =="
sed -n '220,460p' shell/src/code/functions/mod.rs
echo
echo "== shell/src/code/functions/mod.rs around catalog/spec =="
sed -n '170,240p' shell/src/code/functions/mod.rs
echo
echo "== all use sites of req.base_dir.as_deref() in code/functions =="
rg -n -C2 'req\.base_dir\.as_deref\(\)' shell/src/code/functions
echo
echo "== all JsonSchema/Deserialize request structs with base_dir =="
rg -n -C3 '#\[derive\([^]]*Deserialize[^]]*\)' shell/src/code/functions/*.rs shell/src/functions/types.rs | sed -n '1,220p'Repository: iii-hq/workers
Length of output: 29640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shell/src/code/path.rs around session root logic =="
sed -n '430,540p' shell/src/code/path.rs
echo
echo "== shell/src/code/path.rs around resolve_in / require_writable_in =="
sed -n '340,430p' shell/src/code/path.rs
echo
echo "== shell/src/code/path.rs around canonicalize_wire / containing_root =="
sed -n '140,220p' shell/src/code/path.rsRepository: iii-hq/workers
Length of output: 12784
Treat base_dir as untrusted input before widening the jail. base_dir is part of the deserialized tool request schema and is passed straight into session_scoped(), so the allowed roots expand from caller-supplied data. Move it to server-side metadata or reject non-control-plane values before cloning the resolver.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shell/src/code/path.rs` around lines 485 - 496, Treat the
session_scoped(base_dir) input as untrusted and do not use caller-provided
request data to widen the jail. Update session_scoped in Path so it only derives
the session root from server-side/control-plane metadata, or explicitly reject
non-control-plane base_dir values before calling canonicalize_wire,
containing_root, or with_session_root; keep the clone-only behavior for
untrusted/absent inputs.
…g macro The per-verb register_* fns repeated the same closure boilerplate — and the `resolver.session_scoped(req.base_dir.as_deref())` line in particular — eight times. Replace them with one `register_scoped!` macro (plus a no-config variant) so the base_dir folding is written ONCE and each verb is a single declarative line in register_all. Wire surface, schemas, and the register_all/catalog drift guard are unchanged.
|
Closing in favor of #376, which implements the workspace picker/control-plane solution independently. |
1 participant
Problem
A console-selected working directory arrives at the shell worker as the
per-call
base_dir, but the coder jail only permitted paths inside thestatically configured
host_roots. Any selected directory outside them wasrejected with C215:
So the working-directory picker could only ever scope a chat to a directory
that was already baked into the jail config — selecting a real project path
failed.
Fix
PathResolver::session_scopedfolds the selectedbase_dirinto theeffective allowed roots for that call (via
with_session_root), so everydownstream check treats it as a first-class root:
resolve/resolve_incontaining_root)is_rootandsession_rootIt is a no-op when
base_diris absent, already inside a configured root(only an
Arcbump), or cannot be canonicalised (the handler's ownresolve_inthen produces the precise error). Wired into everybase_dir-bearing coder dispatch closure (read / search / update / create /delete / list / tree / move).
Why this is safe to widen on
base_diris stamped by the harness control plane (workspace injection),never by the model, so only operator-chosen directories grow the jail.
resolve_instill scopes access to the session directory.is now a real root that
containing_rootcan relativise against.Test plan
cargo build— clean, 0 warningscargo test— green; addedsession_scoped_adds_selected_dir_outside_jailcovering: outside-jail dir rejected until added, reachable once added,
**/.envstill blocked under the added root, and no-op for an in-jail /absent
base_dir.Follow-ups (out of scope)
shell/config.yaml.host_roots.Summary by CodeRabbit
New Features
Bug Fixes