Repository navigation
feat(sandbox): add default deny settings - #10940
Conversation
📝 WalkthroughWalkthroughGlobal sandbox deny settings are added to configuration and schema, merged with CLI restrictions for ChangesSandbox settings
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Settings
participant RunExec
participant SandboxConfig
Settings->>RunExec: Provide global deny settings
RunExec->>SandboxConfig: Combine global and CLI deny flags
SandboxConfig-->>RunExec: Apply sandbox restrictions
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Greptile SummaryThis PR introduces global sandbox deny defaults via
Confidence Score: 5/5The change is a clean additive feature with all deny flags defaulting to false, so existing behavior is completely unchanged unless explicitly opted-in. The refactor correctly preserves the semantics of the original OR logic, the new No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "test(sandbox): remove unsupported e2e te..." | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/run.rs (1)
716-748: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated deny-flag composition logic.
This settings+CLI deny composition block is duplicated verbatim in
src/cli/exec.rs(lines 237-255). Consider a shared constructor onSandboxConfig(e.g.SandboxConfig::from_settings(sandbox_settings, cli_deny_all, cli_deny_read, ..., allow_read, ...)) insrc/sandbox/mod.rsto avoid the two call sites drifting if the composition rule changes.♻️ Proposed refactor
+impl SandboxConfig { + pub fn from_settings_and_cli( + settings: &crate::config::settings::SandboxSettings, + cli_deny_all: bool, + cli_deny_read: bool, + cli_deny_write: bool, + cli_deny_net: bool, + cli_deny_env: bool, + allow_read: Vec<PathBuf>, + allow_write: Vec<PathBuf>, + allow_net: Vec<String>, + allow_env: Vec<String>, + ) -> Self { + Self { + deny_read: settings.deny_all || settings.deny_read || cli_deny_all || cli_deny_read, + deny_write: settings.deny_all || settings.deny_write || cli_deny_all || cli_deny_write, + deny_net: settings.deny_all || settings.deny_net || cli_deny_all || cli_deny_net, + deny_env: settings.deny_all || settings.deny_env || cli_deny_all || cli_deny_env, + allow_read, + allow_write, + allow_net, + allow_env, + } + } +}Then call it from both
run.rsandexec.rs.🤖 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 `@src/cli/run.rs` around lines 716 - 748, Extract the duplicated sandbox settings and CLI flag composition into a shared constructor, such as SandboxConfig::from_settings, in the SandboxConfig implementation within sandbox/mod.rs. Have it accept the sandbox settings, deny flags, and allow lists, apply the existing OR-composition rules, and replace the inline construction in both the run command and exec command paths.
🤖 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.
Nitpick comments:
In `@src/cli/run.rs`:
- Around line 716-748: Extract the duplicated sandbox settings and CLI flag
composition into a shared constructor, such as SandboxConfig::from_settings, in
the SandboxConfig implementation within sandbox/mod.rs. Have it accept the
sandbox settings, deny flags, and allow lists, apply the existing OR-composition
rules, and replace the inline construction in both the run command and exec
command paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 7724fb32-d226-419d-b95b-aa1fa15a4701
📒 Files selected for processing (6)
docs/sandboxing.mde2e/cli/test_sandbox_settingsschema/mise.jsonsettings.tomlsrc/cli/exec.rssrc/cli/run.rs
|
Addressed the AI review feedback in 236b375:
Validation passed:
This comment was generated by an AI coding assistant. |
Summary
[settings.sandbox]deny defaults matching the sandbox CLI deny flagsSandboxConfigpath for bothmise execandmise runThis addresses the design requested in discussion #10669 without introducing task defaults, implicit templates, or composable templates.
User impact
Users can set a sandbox policy once, including at a monorepo root:
Individual tasks can continue to add
allow_read,allow_write,allow_net, andallow_envexceptions.E2E coverage
This PR intentionally does not include an E2E test. Filesystem sandbox enforcement on Linux requires Landlock, but the GitHub Actions E2E runner reports Landlock as
NotEnabled. An E2E exercising[settings.sandbox] deny_all = truetherefore fails while applying the kernel restriction rather than testing the settings behavior. The settings/CLI composition is covered by focused unit tests instead.Validation
mise run formatmise run render:schemacargo test test_from_settings_and_cli --all-featurescargo test test_settings_toml_is_sorted --all-featuresSummary by CodeRabbit
New Features
deny_all,deny_env,deny_net,deny_read,deny_write).Documentation