(MOT-3872) feat(shell): deny-only policy — commands, fs jail, per-call env - #428
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
skill-check — worker0 verified, 39 skipped (no docs/).
Four for four. Nicely done. |
|
Warning Review limit reached
Next review available in: 51 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 (2)
📝 WalkthroughWalkthroughThe shell worker removes allowlist-based command gating, switches execution policy to denylist-only matching, updates removed-key migration handling, and changes shipped defaults and docs to reflect unjailed-by-default filesystem behavior. Tests, fixtures, changelog, and versioning are updated to match. ChangesDeny-only shell exec policy
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shell/README.md (1)
300-304: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore a recovery hint for
allowlistparse failures.Dropping the allowlist troubleshooting row leaves no README guidance for the new 0.8.0 fail-closed parse error, so operators have to infer the fix from elsewhere. Please add a short note that
allowlistmust be removed and the stored value rewritten viaconfiguration::set.🤖 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/README.md` around lines 300 - 304, Restore the missing troubleshooting entry in the README’s recovery hints so `allowlist` parse failures are documented alongside the other boot/reload errors. Update the existing recovery-hints list to mention that when the new fail-closed `allowlist` parse error appears, the stored value must be removed and rewritten through `configuration::set`; keep the wording consistent with the surrounding `fs.host_roots` and `config keys removed` notes.
🧹 Nitpick comments (1)
tech-specs/2026-06-agentic/shell-deny-only-policy.md (1)
53-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpec text doesn't match the actual implementation location/helper.
This says the removal is wired via
deleted("allowlist")added toREMOVED_TOP_LEVEL_KEYS, butshell/src/config.rsinstead adds a newREMOVED_EXEC_KEYStable populated via a newdeleted_in("allowlist", "0.8.0")helper (sincedeleted()hardcodes"0.7.0"). Sinceconfig.rsexplicitly points back to this spec file in its doc comment, keeping the two in sync avoids confusing future readers/maintainers who go looking forREMOVED_TOP_LEVEL_KEYS.✏️ Suggested wording fix
-- Add `deleted("allowlist")` to `REMOVED_TOP_LEVEL_KEYS` (`src/config.rs`), - following the 0.7.0 hard-migration convention: any stored value or YAML +- Add a new `REMOVED_EXEC_KEYS` table (`src/config.rs`) with + `deleted_in("allowlist", "0.8.0")`, following the 0.7.0 hard-migration + convention: any stored value or YAML seed still carrying the key — including the inert `allowlist: []` the old seed wrote — is rejected at parse with the `configuration::set (id: shell)` hint. No silent tolerance.🤖 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 `@tech-specs/2026-06-agentic/shell-deny-only-policy.md` around lines 53 - 61, The spec text is out of sync with the implementation in ShellConfig::config.rs: it describes wiring removal through REMOVED_TOP_LEVEL_KEYS with deleted("allowlist"), but the code actually uses REMOVED_EXEC_KEYS with deleted_in("allowlist", "0.8.0"). Update the wording in this spec to match the real helper/table names and versioned migration path so readers are directed to the correct location in shell/src/config.rs.
🤖 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.
Outside diff comments:
In `@shell/README.md`:
- Around line 300-304: Restore the missing troubleshooting entry in the README’s
recovery hints so `allowlist` parse failures are documented alongside the other
boot/reload errors. Update the existing recovery-hints list to mention that when
the new fail-closed `allowlist` parse error appears, the stored value must be
removed and rewritten through `configuration::set`; keep the wording consistent
with the surrounding `fs.host_roots` and `config keys removed` notes.
---
Nitpick comments:
In `@tech-specs/2026-06-agentic/shell-deny-only-policy.md`:
- Around line 53-61: The spec text is out of sync with the implementation in
ShellConfig::config.rs: it describes wiring removal through
REMOVED_TOP_LEVEL_KEYS with deleted("allowlist"), but the code actually uses
REMOVED_EXEC_KEYS with deleted_in("allowlist", "0.8.0"). Update the wording in
this spec to match the real helper/table names and versioned migration path so
readers are directed to the correct location in shell/src/config.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee20cf5a-01a0-4952-8ee5-ee4529f979cb
⛔ Files ignored due to path filters (1)
shell/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
shell/ARCHITECTURE.mdshell/CHANGELOG.mdshell/Cargo.tomlshell/README.mdshell/config.collect.yamlshell/config.yamlshell/iii.worker.yamlshell/src/config.rsshell/src/configuration.rsshell/src/exec/backend.rsshell/src/exec/host.rsshell/src/exec/policy.rsshell/src/functions/exec.rsshell/src/functions/types.rsshell/src/main.rsshell/src/telemetry.rsshell/tests/e2e/README.mdshell/tests/e2e/config-jailed.yamlshell/tests/e2e/config.yamlshell/tests/e2e/run-tests.shshell/tests/e2e/workers/harness/src/cases-edge.tsshell/tests/e2e/workers/harness/src/cases-exec-break.tsshell/tests/e2e/workers/harness/src/cases-exec-sandbox.tsshell/tests/e2e/workers/harness/src/cases-exec-stdin.tsshell/tests/e2e/workers/harness/src/cases-safety.tsshell/tests/function_handlers.rstech-specs/2026-06-agentic/shell-deny-only-policy.md
💤 Files with no reviewable changes (3)
- shell/tests/e2e/config.yaml
- shell/config.collect.yaml
- shell/tests/e2e/config-jailed.yaml
ytallo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/skills/SKILL.md`:
- Around line 55-59: Clarify the `shell::fs::*` boundary description so it
matches the actual path-validation behavior: the current statement that symlinks
are never followed is misleading. Update the `shell::fs::*` docs to say symlinks
may be resolved during containment checks for jail enforcement, while denylisted
paths are still always rejected and absolute/relative path rules remain
unchanged.
🪄 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: d9da5963-61b5-4625-ac43-85d7d4880a0e
📒 Files selected for processing (10)
.github/workflows/shell-e2e.ymlREADME.mdshell/ARCHITECTURE.mdshell/CHANGELOG.mdshell/README.mdshell/config.yamlshell/skills/SKILL.mdshell/src/config.rsshell/src/configuration.rsshell/tests/e2e/README.md
✅ Files skipped from review due to trivial changes (3)
- README.md
- shell/tests/e2e/README.md
- .github/workflows/shell-e2e.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- shell/src/configuration.rs
- shell/src/config.rs
| - `shell::fs::*` honors `fs.host_roots` as a jail WHEN it's set — empty (the | ||
| shipped default) means unjailed, confined only by `fs.denylist_paths` — and | ||
| always refuses denylisted paths regardless; paths must be absolute (unless | ||
| jailed, where a relative path resolves against the primary root) and | ||
| symlinks are never followed. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the referenced files and inspect the relevant sections.
git ls-files | rg '(^|/)(SKILL\.md|ARCHITECTURE\.md|README\.md)$'
for f in shell/skills/SKILL.md ARCHITECTURE.md README.md; do
if [ -f "$f" ]; then
echo "===== $f ====="
wc -l "$f"
fi
done
echo "===== SKILL.md excerpt ====="
sed -n '45,70p' shell/skills/SKILL.md
echo "===== ARCHITECTURE.md symlink/path-validation excerpts ====="
rg -n -C 3 'canonicaliz|symlink|jail|S215|C215' ARCHITECTURE.md
echo "===== README.md symlink/path-validation excerpts ====="
rg -n -C 3 'canonicaliz|symlink|jail|S215|C215' README.mdRepository: iii-hq/workers
Length of output: 3559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== shell/ARCHITECTURE.md ====="
wc -l shell/ARCHITECTURE.md
rg -n -C 3 'canonicaliz|symlink|jail|S215|C215|resolve' shell/ARCHITECTURE.md
echo "===== shell/README.md ====="
wc -l shell/README.md
rg -n -C 3 'canonicaliz|symlink|jail|S215|C215|resolve' shell/README.mdRepository: iii-hq/workers
Length of output: 22920
Clarify symlink handling in shell::fs::* boundaries
symlinks are never followed conflicts with the path-validation docs: containment checks canonicalize the existing path portion and reject symlinks that escape the jail. Update this line to say symlinks in the path are resolved for jail checks, not treated as opaque.
🧰 Tools
🪛 SkillSpector (2.3.7)
[error] 6: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
🤖 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/skills/SKILL.md` around lines 55 - 59, Clarify the `shell::fs::*`
boundary description so it matches the actual path-validation behavior: the
current statement that symlinks are never followed is misleading. Update the
`shell::fs::*` docs to say symlinks may be resolved during containment checks
for jail enforcement, while denylisted paths are still always rejected and
absolute/relative path rules remain unchanged.
4be0fbf to
4caf7da
Compare
…wlist and planted-binary guard
…escription, and comments
Flip fs.allow_unjailed to true and clear host_roots in the shipped seed_default()/config.yaml so shell::fs::* and shell::exec's per-call cwd match shell::exec's own deny-only security model instead of being confinement-based by default. coder::* is unaffected — it falls back to its own default roots (cwd + /tmp) whenever host_roots is empty. Default::default() (used when an operator config omits the fs section entirely) still fails closed; only this explicit seed opts in.
… doc gaps - README: Upgrading to 0.8.0 section (allowlist removal with a runnable configuration::set example, plus the fs-jail default change and how to keep the old jailed-to-/tmp behavior), matching troubleshooting entries, tilde-expansion caveat, coder::*/shell::fs::* jail divergence, C-code table, config-status agent-callable caveat, npm install line in Quick start. - config.yaml: fix the comment conflating the fs jail with the sandbox as a joint security boundary for exec — only the sandbox is one. - ARCHITECTURE.md/SKILL.md/configuration.rs: fix stale "jailed to /tmp" and "shell::fs::* is jailed" claims now that the shipped default is unjailed. - CHANGELOG: add the fs-jail-default change as a second 0.8.0 breaking entry with a Migration section, matching the 0.6.0/0.7.0 structure. - Fix e2e test-count drift across three docs (143+1 / ~27 / 144 vs the actual 170) by pointing at the harness's own report instead of a number that goes stale every time a case is added.
Drop the env.allow gate on shell::exec/exec_bg's per-call `env` override — an agent may now set any key except the ones in DANGEROUS_ENV_KEYS, matching the same deny-only philosophy already applied to command exec and the fs jail. env.allow keeps its other job unchanged: which vars get forwarded from the worker's own environment when env.inherit is false.
…a/descriptions Update ExecRequest/ExecBgRequest doc comments (the LLM-facing schema) and the shell::exec/exec_bg .description() strings to describe the per-call env override as deny-only, matching the src/exec/policy.rs change.
README/ARCHITECTURE/config.yaml/CHANGELOG updated for the env.allow change: Configure section, per-call env section, Functions table, S210 error row, config table, shipped seed comments, and a new Upgrading to 0.8.0 entry (pure widening, no action required). Also fixed two leftover stale references in policy.rs's own doc comments the first pass missed.
4caf7da to
a99d45a
Compare
1 participant
The shell worker is now permissive-first across every axis it gates: no
command allowlist, the fs jail is opt-in rather than defaulted on, and the
per-call
envoverride onshell::exec/exec_bgis deny-only. Allow/askpolicy for commands lives entirely in the approval-gate; the sandbox
backend is the real security boundary for untrusted exec.
Deny-only command policy
allowlistconfig key removed and hard-rejected at parse (0.8.0removed-key migration with a
configuration::sethint), matching the0.7.0 precedent.
allowlist bypass, and blocked the legitimate case of executing your own
build output inside the jail.
(plain-string Handler rejections, policy-before-backend-dispatch) are
preserved via the denylist path.
Unjailed default
config.yaml/seed_default()now setfs.allow_unjailed: truewithempty
fs.host_roots—shell::fs::*andshell::exec's per-callcwdoperate against the real filesystem by default, confined only by
fs.denylist_paths, matchingshell::execitself (deny-only, neverconfinement-based).
coder::*is unaffected: it falls back to its own default roots (engineworkspace cwd +
/tmp) wheneverhost_rootsis empty, regardless ofallow_unjailed.Default::default()(used when an operator config omits thefssectionentirely) is unchanged — still fails closed unless explicitly opted in.
Only the shipped seed opts in explicitly.
fs.host_rootsalready stored areunaffected — the stored value always wins over the seed. Only a fresh
zero-config install, or a stored value that gets nulled, picks up the new
unjailed default.
Deny-only per-call env override
envoverride onshell::exec/exec_bgno longer requiresa key to be in
env.allow— only the hardcoded dangerous-key denylist(PATH, IFS, HOME, LD_/DYLD_, interpreter startup keys, ...) applies,
unconditionally.
env.allowkeeps its other, unrelated job: which vars get forwarded fromthe worker's own environment when
env.inheritis false.needs a rewrite.
Docs
README/ARCHITECTURE/CHANGELOG/SKILL.md updated throughout for all three
changes, including a new "Upgrading to 0.8.0" section with a runnable
configuration::setmigration example, a C-code table forcoder::*thatdidn't exist before, and a documented tilde-expansion caveat on path
handling.
Breaking: stored shell configurations carrying
allowlist(even[]) failclosed at boot until rewritten. A fresh install now boots with the fs jail
off by default — see
shell/README.md#upgrading-to-080if you want the oldjailed-to-
/tmpbehavior. The per-call env change is purely additive. Seeshell/CHANGELOG.md0.8.0 for all three.Spec: tech-specs/2026-06-agentic/shell-deny-only-policy.md
Test plan
cargo test(unit + integration, all targets) — passes cleanlycargo clippy --all-targets -- -D warnings,cargo fmt --check— cleanshell/tests/e2e/run-tests.sh(deny-only cases) — 170/170 passed against a live engineFixes MOT-3872