fix(design): refuse staging symlinks escaping into challenge secrets - #107
fix(design): refuse staging symlinks escaping into challenge secrets#107echobt wants to merge 2 commits into
Conversation
Miner-controlled out/pages (and other staging paths) could be replaced with symlinks into the design-challenge mount NS. Collect/read with symlink_metadata + O_NOFOLLOW, harden review run_command denylist, and document as R15.
📝 WalkthroughWalkthroughThe change hardens sandbox and screenshot staging reads against symlinks and non-regular files. It adds Linux ChangesSandbox security hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Miner
participant Sandbox
participant StagingReader
participant Orchestrator
Miner->>Sandbox: produce staged pages and environment files
Sandbox->>StagingReader: read staged outputs
StagingReader-->>Sandbox: validated contents or UnsafeOutput
Sandbox-->>Orchestrator: sandbox result
Orchestrator-->>Miner: terminal miner failure for unsafe output
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/design-sandbox/src/lib.rs (1)
707-735: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not use simulation markers as proof that
agent.pyexecuted.This test accepts
sim-install-ok,sim-run-ok, andsim-stubas success. Those values do not prove thatagent.pyexecuted.If this is only a simulation smoke test, rename it. Add an end-to-end challenge verification that forces
DESIGN_FORCE_SIM=falseand, when an OpenRouter key is configured, verifies intake, failure probes, leaf emission, raw weight submission, sealing, andsealed: true.As per coding guidelines, do not treat
sim-install-okorsim-run-okas proof thatagent.pyexecuted, and challenge verification must simulate end-to-end submission.🤖 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 `@crates/design-sandbox/src/lib.rs` around lines 707 - 735, Rename sim_runs_baseline_agent_py to clearly identify it as a simulation smoke test and stop using sim-install, sim-run, or sim-stub markers as evidence that agent.py executed. Add a separate end-to-end challenge verification that forces DESIGN_FORCE_SIM=false and, when an OpenRouter key is configured, validates intake, failure probes, leaf emission, raw weight submission, sealing, and a sealed: true result through simulated end-to-end submission.Source: Coding guidelines
🤖 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 `@crates/challenge-agentic/src/tools.rs`:
- Around line 475-478: Replace the lexical filtering around
run_command_forbidden_path with isolation that prevents commands from accessing
secret-bearing mounts, preferably removing run_command from those containers or
executing it in an isolated container or mount namespace. Preserve denial of
/run/base secret paths, and add coverage for glob-, variable-, and
interpreter-composed paths, including /proc/*/environ access, rather than
relying on shell-text inspection.
---
Outside diff comments:
In `@crates/design-sandbox/src/lib.rs`:
- Around line 707-735: Rename sim_runs_baseline_agent_py to clearly identify it
as a simulation smoke test and stop using sim-install, sim-run, or sim-stub
markers as evidence that agent.py executed. Add a separate end-to-end challenge
verification that forces DESIGN_FORCE_SIM=false and, when an OpenRouter key is
configured, validates intake, failure probes, leaf emission, raw weight
submission, sealing, and a sealed: true result through simulated end-to-end
submission.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bde1562-54e5-4dc4-8742-d2bbc06722bf
📒 Files selected for processing (7)
crates/challenge-agentic/src/tools.rscrates/design-challenge/src/orchestrator.rscrates/design-challenge/src/screenshot.rscrates/design-sandbox/src/lib.rsdocs/DESIGN_CHALLENGE.mddocs/OPERATOR_SECURITY.mddocs/THREAT_MODEL.md
| // File-mounted secrets + parent environ must stay unread (defense-in-depth; | ||
| // primary staging collectors also refuse symlinks — see design-sandbox). | ||
| let c = cmd.to_ascii_lowercase().replace('\\', "/"); | ||
| if c.contains("/proc") || c.contains("review-secrets") || c.contains("openrouter_api_key") { | ||
| if run_command_forbidden_path(&c) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Replace the lexical denylist with a boundary that prevents secret reads.
run_command_forbidden_path checks raw shell text before bash -lc expands it. cat /r??/base/* bypasses both the /run/base and challenge_sk needles. cat /pr??/1/environ similarly bypasses the /proc needle.
This permits secret disclosure from the review container. Remove run_command from secret-bearing containers, or execute it in an isolated container or mount namespace that does not contain secret mounts. Do not rely on command-text filtering. Add tests for glob, variable, and interpreter-composed paths.
As per coding guidelines, run_command must keep /run/base secret paths denied.
Also applies to: 542-556, 803-815
🤖 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 `@crates/challenge-agentic/src/tools.rs` around lines 475 - 478, Replace the
lexical filtering around run_command_forbidden_path with isolation that prevents
commands from accessing secret-bearing mounts, preferably removing run_command
from those containers or executing it in an isolated container or mount
namespace. Preserve denial of /run/base secret paths, and add coverage for
glob-, variable-, and interpreter-composed paths, including /proc/*/environ
access, rather than relying on shell-text inspection.
Source: Coding guidelines
Share read_staged_bytes with screenshot staging, keep agentic denylist delta to a single /run/base needle, and preserve the R15 collect fix.
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)
crates/challenge-agentic/src/tools.rs (1)
306-306: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winApply no-follow and regular-file checks to agentic workdir reads.
resolve_relcanonicalizes first, so laterfs::metadata,Path::is_dir, andfs::read_to_stringre-check targets by-path. Symlinked workdir entries can escapeworkdiror expose/proc/1/environand/run/base/*. Directory symlinks can create an unboundedgrep_walktraversal, and readers currently accept non-regular files.Use descriptor-based, no-follow filesystem access for
read_file,grep,tool_stat, metrics, pages, report reads,read_py, andload_primary_sources, and reject symlinks/non-regular files after the binding is fixed.Also applies to: 319-324, 347, 395-398, 400-404, 424, 517, 681-682
🤖 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 `@crates/challenge-agentic/src/tools.rs` at line 306, Replace path-based reads and metadata checks in read_file, grep/grep_walk, tool_stat, metrics, pages, report reads, read_py, and load_primary_sources with descriptor-based no-follow access rooted in the agentic workdir. After resolving each binding, reject symlinks and require regular files for file readers; allow only validated directories for traversal and prevent symlinked directories from expanding grep_walk. Apply the same validation to all referenced metadata, read_to_string, and is_dir call sites.
🤖 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 `@crates/challenge-agentic/src/tools.rs`:
- Line 306: Replace path-based reads and metadata checks in read_file,
grep/grep_walk, tool_stat, metrics, pages, report reads, read_py, and
load_primary_sources with descriptor-based no-follow access rooted in the
agentic workdir. After resolving each binding, reject symlinks and require
regular files for file readers; allow only validated directories for traversal
and prevent symlinked directories from expanding grep_walk. Apply the same
validation to all referenced metadata, read_to_string, and is_dir call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d92224d-8c68-4311-99f8-da5c390790e7
📒 Files selected for processing (4)
crates/challenge-agentic/src/tools.rscrates/design-challenge/src/orchestrator.rscrates/design-challenge/src/screenshot.rscrates/design-sandbox/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/design-challenge/src/orchestrator.rs
- crates/design-challenge/src/screenshot.rs
- crates/design-sandbox/src/lib.rs
1 participant
Summary
out/pages(and related staging paths) are not followed into the design-challenge mount NS (/run/base/*,/proc/1/environ).run_commanddenylist also covers/run/baseand secret path needles.Test plan
cargo fmt/clippy -D warningsondesign-sandbox,design-challenge,challenge-agenticcollect_out/read_staged_text/.miner_env.jsonreject symlinks; screenshotread_staging_bytesrefuses symlinks; agenticrun_commandforbids/run/base/...needlescargo test -p design-sandbox -p challenge-agentic -p design-challenge --lib --testsSummary by CodeRabbit
Security Enhancements
Documentation