Repository navigation
fix(saas): harden the user shell sandbox (#7154 review follow-ups) - #7195
Conversation
The shell tool now checks the boot guard before executing commands, so invocations are rejected while the runtime is still starting up. This prevents user agents from running shell commands before the environment is fully initialized. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…ensitive host network, finite CPU, refusal not allowed Co-authored-by: Medulla <medulla@tinyhumans.ai>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughSandbox policy resolution now validates canonical paths and rejects host-network variants. Runtime validation rejects non-finite CPU limits. SaaS policy resolution failures now return a disallowed result. ChangesSandbox enforcement
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers:
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: hardening the SaaS user shell sandbox. It matches the sandbox validation, networking, resource-limit, and failure-handling updates. |
| Docstring Coverage | |
Docstring coverage is 86.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 5 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the sandbox gate,
“HOST” and spaces meet their fate.
The paths resolve, the limits shine,
No endless CPU slips the line.
A safer burrow, neat and fine!
Comment @coderabbitai help to get the list of available commands.
Tiny Sweeper reviewThis change hardens the SaaS user-shell sandbox: it canonicalizes the sandbox directory to block symlink escapes, refuses host network names in any casing or with whitespace, rejects non-finite CPU limits at boot, and reports a sandbox-refused shell command as not allowed. Unit tests cover each behavior; two earlier findings remain open: the sandbox-refused shell result path is still not driven by any test, and end-to-end jobs are pending. State: Reviewing pending checks Review snapshot
Completeness: Complete What changedThe pull request modifies the sandbox policy construction and boot guard checks. In the sandbox policy builder, the action directory is now canonicalized and must resolve exactly to the agent's expected `sandbox/` directory under the agents root, and the resolved path is used as the workspace root, so a symlink pointing elsewhere on the host is refused. Host-network detection moved to a shared helper that trims and compares case-insensitively, since Docker matches the network name case-insensitively, and both the boot guard and the policy builder now use it. The boot guard also rejects non-finite CPU limits (previously only NaN was caught, letting infinities through). In the shell tool, when the SaaS sandbox refuses a command before anything runs, the result flag changed from true to false so it is reported as not allowed. Dependency changes pin hmac to 0.13 against sha2 0.11 via a renamed alias, and the gateway imports were updated accordingly. Features
Tests
Findings
Resolved this pass
Pending checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS) Before merge
How this fits togetherflowchart LR
n0["run_with_security_in_context"]:::impacted
n1["run_sandboxed"]:::impacted
n2["execute_in_context"]:::impacted
n3["runtime_path_for_command"]:::impacted
n4["Value"]:::impacted
n5["external_effect_with_args"]:::impacted
n0 -->|calls| n1
n0 -->|calls| n3
n1 -->|calls| n3
n2 -->|calls| n0
n2 -->|uses| n4
n2 -->|calls| n5
n5 -->|uses| n4
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3287489c1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0096 · 139,330 in / 9,286 out · 17,668 cached (13%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0056 · 68,340 in / 3,826 out · 10,480 cached (15%) · gpt-5.6-luna
security: $0.0036 · 45,040 in / 1,936 out · 5,524 cached (12%) · gpt-5.6-luna
tests: $0.0001 · 6,501 in / 554 out · 1,536 cached (24%) · glm-5.3-flash
description: $0.0001 · 6,469 in / 194 out · 64 cached (1%) · glm-5.3-flash
e2e: $0.0001 · 7,980 in / 768 out · 64 cached (1%) · glm-5.3-flash
tinysweeper
Bot
added
the
priority: p2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The gateway signature now uses hmac 0.13 with sha2 0.11, the versions tinychannels-bus already pulls in, so the dependency graph gains no new packages. The import switches to the aliased sha2_011 crate and adds KeyInit, which hmac 0.13 requires for key construction. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Merging with |
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0012 · 92,953 in / 8,386 out · 9,164 cached (10%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0004 · 30,819 in / 2,905 out · 4,058 cached (13%) · gpt-5.6-luna
security: $0.0004 · 33,216 in / 2,105 out · 3,570 cached (11%) · gpt-5.6-luna
tests: $0.0001 · 7,197 in / 537 out · 1,536 cached (21%) · glm-5.3-flash
description: $0.0001 · 7,142 in / 284 out · 0 cached (0%) · glm-5.3-flash
e2e: $0.0001 · 8,799 in / 888 out · 0 cached (0%) · glm-5.3-flash
| } | ||
| Err(why) => { | ||
| tracing::warn!(reason = %why, "[shell] SaaS sandbox refused the command"); | ||
| // Nothing ran: report it as not allowed. |
There was a problem hiding this comment.
Sandbox-refused shell result is still not driven end to end
The refused-SaaS-sandbox branch changed from returning true to false (false, ToolResult::error(...)), yet no test in this pull request exercises that error path. The comment asserts "Nothing ran: report it as not allowed", but the only thing pinning that claim would be a test, and no diff here contains one — a revert of the false back to true would pass the suite. A test that forces the sandbox construction/invocation to fail and asserts the shell tool reports allowed == false (and an error result) would earn its keep. Severity stays at medium as previously given; the code did not change the substance of the gap, only the flag value.
[RULE] untested-error-path ·
| if config.network.trim() == "host" { | ||
| // Resolve symlinks: the directory mounted read-write must really be this | ||
| // agent's sandbox, not a link from it to somewhere else on the host. | ||
| let resolved = action_dir |
There was a problem hiding this comment.
Canonicalized workspace_root never exercised by an end-to-end test
The policy now mounts workspace_root: resolved — the canonicalized path — into the container, and refuses sandboxes whose path resolves outside the agent directory. The unit tests cover policy construction, but no end-to-end test launches the sandbox and exercises a command against the mounted workspace, which is what would catch a regression here (e.g. canonicalization diverging from what Docker actually mounts, or the expected-path comparison failing on a legitimate layout). A test would have to start the system with a real agent sandbox directory, run a shell command through the sandboxed tool, and assert the command sees and can write the agent's sandbox.
[RULE] e2e-uncovered ·
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0189 · 259,138 in / 25,313 out · 28,884 cached (11%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0098 · 111,379 in / 11,262 out · 12,366 cached (11%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0088 · 109,215 in / 8,961 out · 9,094 cached (8%) · gpt-5.6-luna
tests: $0.0002 · 16,426 in / 2,119 out · 4,160 cached (25%) · glm-5.3-flash
description: $0.0001 · 7,163 in / 662 out · 1,408 cached (20%) · glm-5.3-flash
e2e: $0.0001 · 8,820 in / 667 out · 1,728 cached (20%) · glm-5.3-flash
| } | ||
|
|
||
| #[test] | ||
| fn an_uppercase_host_network_or_an_infinite_cpu_limit_is_refused() { |
There was a problem hiding this comment.
Drive the sandbox refusal through the shell path
This test calls the private boot-guard checker directly and only proves that two Violation::Sandbox values are produced. It does not exercise the shell/tool execution path that consumes the boot result, so a wiring error could still allow the refused shell operation while this test passes. Add an end-to-end test that configures host_shell, triggers the refusal, and asserts the resulting shell operation is blocked.
[RULE] missing-end-to-end-test ·
| )); | ||
| } | ||
| if is_host_network(&config.network) { | ||
| return Err("the sandbox cannot use the host network".to_string()); |
There was a problem hiding this comment.
Drive sandbox refusal through an end-to-end test
The refusal is still covered only by the direct sandbox_policy_with unit test. That does not verify that the shell execution path propagates this error to its caller and prevents execution, which was the behavior this guard was added to enforce. Add an end-to-end test that submits a sandbox command with a refused network and asserts the command result is refused rather than executed.
[RULE] missing-end-to-end-coverage ·
| // Nothing ran: report it as not allowed. | ||
| ( | ||
| true, | ||
| false, |
There was a problem hiding this comment.
Add an end-to-end test for canonicalized workspace_root
The canonicalized workspace_root execution path is still not exercised by an end-to-end test, so regressions in the directory normalization and sandbox-policy handoff can go unnoticed. Add coverage that supplies a path requiring canonicalization and verifies the command is executed or rejected using the canonicalized workspace root. This concern predates the one-line result flag change and is retained from the earlier review.
[RULE] missing-end-to-end-test ·
| Ok(SandboxPolicy { | ||
| backend: SandboxBackendKind::Docker, | ||
| workspace_root: action_dir.to_path_buf(), | ||
| workspace_root: resolved, |
There was a problem hiding this comment.
Exercise canonicalized workspace roots end to end
The new canonicalization is asserted only by the policy unit test. No end-to-end test demonstrates that the resulting workspace_root is the path actually mounted or used by a command, so a downstream consumer that still uses the original path could silently bypass this protection. Add an end-to-end test with a canonicalized sandbox path and verify the command operates in the resolved sandbox.
[RULE] missing-end-to-end-coverage ·
| hmac = "0.12" | ||
| # The SaaS gateway signature. 0.13 on sha2 0.11, the versions tinychannels-bus | ||
| # already brings, so it adds no package to the graph. | ||
| hmac = "0.13" |
There was a problem hiding this comment.
Update the lockfile with the new dependency edges
This manifest changes the direct HMAC version and adds a new direct sha2 alias, but the pull request contains no corresponding Cargo.lock change. Even though the lockfile already contains hmac 0.13.0 and sha2 0.11.0 for other packages, Cargo still needs to record these dependencies for openhuman-core; otherwise commands run with --locked can fail with a lockfile-needs-update error. Regenerate the workspace lockfile and commit the resulting package dependency updates.
[RULE] lockfile-sync ·
| network: network.into(), | ||
| ..SaasSandboxConfig::default() | ||
| }; | ||
| assert!( |
There was a problem hiding this comment.
Drive the sandbox refusal through shell execution
This only proves that sandbox_policy_with returns an error; it does not verify that the user-facing shell path propagates the refusal instead of ignoring, translating, or bypassing it. Add an end-to-end test that invokes the shell operation with a refused sandbox configuration and asserts the returned result is refused.
[RULE] missing-end-to-end-test ·
| sandbox_policy_with(&root, &SaasSandboxConfig::default(), &action, &state).unwrap(); | ||
| assert_eq!(policy.backend, SandboxBackendKind::Docker); | ||
| assert_eq!(policy.workspace_root, action); | ||
| assert_eq!(policy.workspace_root, action.canonicalize().unwrap()); |
There was a problem hiding this comment.
Exercise the canonicalized workspace root end to end
The test checks the value returned by the policy helper, but no end-to-end execution verifies that the canonicalized path is the path actually handed to the container runner and mounted as the workspace. Add coverage through the shell/container execution path that asserts the resolved workspace root is used.
[RULE] missing-end-to-end-test ·
| f.config.sandbox.cpu_limit = f64::INFINITY; | ||
| let mut i = inputs(&f, &[]); | ||
| i.sandbox_available = true; | ||
| let found = violations(&i); |
There was a problem hiding this comment.
Drive the sandbox-refused shell result end to end
This only exercises boot_guard::check through a unit-test helper and never runs the shell operation that consumes the sandbox refusal. The refusal result could therefore be lost or bypassed in the real shell path. Add an end-to-end test that invokes the shell flow and asserts the refused result reaches the caller.
[RULE] missing-end-to-end-test ·
| f.config.sandbox.cpu_limit = f64::INFINITY; | ||
| let mut i = inputs(&f, &[]); | ||
| i.sandbox_available = true; | ||
| let found = violations(&i); |
There was a problem hiding this comment.
Exercise canonicalized workspace_root end to end
The added test checks only the number of boot violations and does not exercise the runtime path using the canonicalized workspace_root. The canonicalization behavior therefore remains unverified at the integration boundary where an incorrect root could affect sandbox placement or authorization. Add an end-to-end test that supplies the relevant root and verifies the runtime uses the canonicalized path.
[RULE] missing-end-to-end-test ·
| @@ -376,8 +376,9 @@ impl ShellTool { | |||
| } | |||
| Err(why) => { | |||
| tracing::warn!(reason = %why, "[shell] SaaS sandbox refused the command"); | |||
There was a problem hiding this comment.
Sandbox-refused shell result is still not driven by any test
The Err arm now flips the boolean from true to false, a behavioural change that decides how callers classify the outcome, and no test in this pull request exercises it: no shell-level test drives the SaaS-sandbox error path, and no end-to-end test asserts a refused sandbox surfaces as not-allowed. Nothing in the diff would fail if this flag were flipped back. Add a test that forces the sandbox call to fail and asserts the resulting ToolResult/status.
[RULE] untested-behaviour ·
Summary
These are the round-1 review fixes for #7154 (SaaS host tool groups and the per-user sandbox). #7154 was merged before they were applied.
user_agents::tools::sandbox_policycanonicalizes the action dir and requires it to resolve to exactly<root>/agents/<id>/sandbox. The resolved path is what gets mounted read-write. Before, asandboxentry that was a symlink passed the textual check and could point the container's writable mount anywhere on the host. A missing sandbox directory is refused too.is_host_network, which trims and compares case-insensitively, soHOSTorHostcan't slip past.cpu_limit = inf.allowed = false, because nothing ran.Points I declined, from the same review:
SaasToolGroups contain only file and shell tools, which belong to no gated domain family, so the allowlist can't re-enable a disabled domain.BootInputscall sites: every construction site already passessandbox_available.host_files: those tools are confined by the user's forced policy: autonomy on,workspace_only,action_dir= the user's sandbox, and in SaaS no projects or/tmp/openhumangrants.Test plan
cargo test -p openhuman --lib -- user_agents::tools core::runtime::boot_guard tools::impl::system::shell: 37 passed. New tests:a_symlinked_sandbox_is_refuseda_missing_sandbox_is_refusedthe_host_network_is_refused_in_any_casean_uppercase_host_network_or_an_infinite_cpu_limit_is_refusedcargo test -p openhuman-cli --test saas_mode_e2e: 9 passed.cargo clippy -p openhuman --all-targets: no new warnings. The one warning, inharness_assembly_tests.rs, is already on main.Summary by CodeRabbit