fix(security): prevent path validation bypass via command flags#516
Conversation
Updated SecurityPolicy argument validation to extract potential paths from flags like --key=value and -Xvalue. This ensures that security checks (traversal, workspace bounds, forbidden paths) are applied consistently to all path-like inputs, even when embedded in flags. Also hardened string slicing for short flag extraction to be UTF-8 safe. Added regression tests and updated the Sentinnel journal.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughSummary by CodeRabbitBug Fixes
WalkthroughPath validation in command argument handling is hardened to extract and check embedded paths within flag assignments ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-04-11 to 2026-04-11 |
Deploying corvus with
|
| Latest commit: |
bf5881f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://75093e69.corvus-42x.pages.dev |
| Branch Preview URL: | https://fix-security-path-flag-bypas.corvus-42x.pages.dev |
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/agent-runtime/src/security/policy.rs`:
- Around line 435-462: The path-guard is bypassed when flag values are quoted
because normalize_arg_for_path_checks runs before splitting --key=value, leaving
effective_arg like "\"/etc/passwd\"" which prevents Path::new from detecting
absolute/parent paths; fix by dequoting/normalizing the extracted effective_arg
immediately after the split (call the existing normalize_arg_for_path_checks or
a small dequote utility on effective_arg) before calling is_likely_path,
Path::new checks, matches_any_forbidden_path, or is_path_allowed, and then reuse
is_path_allowed for the final allow/deny decision; also add a regression test
covering forms like --file="/etc/passwd" and -C"../secret" to ensure quoted
values are rejected.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4d27f49-2af1-404b-bee7-e50e36853ce6
📒 Files selected for processing (2)
.agents/journal/sentinnel-journal.mdclients/agent-runtime/src/security/policy.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (6)
clients/agent-runtime/src/{security,gateway,tools}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Treat
src/security/,src/gateway/,src/tools/as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Files:
clients/agent-runtime/src/security/policy.rs
clients/agent-runtime/src/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
clients/agent-runtime/src/**/*.rs: Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements
Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency
Files:
clients/agent-runtime/src/security/policy.rs
clients/agent-runtime/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Run
cargo fmt --all -- --check,cargo clippy --all-targets -- -D warnings, andcargo testfor code validation, or document which checks were skipped and why
Files:
clients/agent-runtime/src/security/policy.rs
clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs
📄 CodeRabbit inference engine (clients/agent-runtime/AGENTS.md)
Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Files:
clients/agent-runtime/src/security/policy.rs
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.
Files:
clients/agent-runtime/src/security/policy.rs
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/agent-runtime/src/security/policy.rs
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
.agents/journal/sentinnel-journal.mdclients/agent-runtime/src/security/policy.rs
📚 Learning: 2026-02-21T09:07:52.298Z
Learnt from: yacosta738
Repo: dallay/corvus PR: 62
File: .agents/journal/sentinnel-journal.md:1-1
Timestamp: 2026-02-21T09:07:52.298Z
Learning: Branding guideline: The intentional brand name for the security-first agent in the dallay/corvus repository is 'Sentinnel' (with double n). Do not treat it as a typo of 'Sentinel'. Ensure all agent-related docs and journals under .agents/journal consistently use 'Sentinnel' with double n.
Applied to files:
.agents/journal/sentinnel-journal.md
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
clients/agent-runtime/src/security/policy.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/agent-runtime/src/security/policy.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow
Applied to files:
clients/agent-runtime/src/security/policy.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Applied to files:
clients/agent-runtime/src/security/policy.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path
Applied to files:
clients/agent-runtime/src/security/policy.rs
🔇 Additional comments (1)
.agents/journal/sentinnel-journal.md (1)
20-23: Nice audit trail.This captures the bypass shape and mitigation clearly enough to help future regressions.
| // Extract potential path from flags (e.g. --file=/path or -C/path) | ||
| let effective_arg = if arg.starts_with("--") { | ||
| arg.split_once('=').map(|(_, v)| v).unwrap_or(arg) | ||
| } else if arg.starts_with('-') && arg.len() > 2 { | ||
| arg.char_indices() | ||
| .nth(2) | ||
| .map(|(idx, _)| &arg[idx..]) | ||
| .unwrap_or("") | ||
| } else { | ||
| arg | ||
| }; | ||
|
|
||
| if !is_likely_path(effective_arg) { | ||
| continue; | ||
| } | ||
|
|
||
| if Path::new(arg) | ||
| if Path::new(effective_arg) | ||
| .components() | ||
| .any(|c| matches!(c, std::path::Component::ParentDir)) | ||
| || (self.workspace_only && (arg.starts_with('/') || arg.starts_with('~'))) | ||
| || (self.workspace_only | ||
| && (effective_arg.starts_with('/') || effective_arg.starts_with('~'))) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Check against forbidden paths (e.g. /etc, ~/.ssh) | ||
| if matches_any_forbidden_path(arg, &self.forbidden_paths) { | ||
| if matches_any_forbidden_path(effective_arg, &self.forbidden_paths) { | ||
| return false; |
There was a problem hiding this comment.
Quoted flag values still evade the new path guard.
normalize_arg_for_path_checks runs before --key=value is split, so grep --file="/etc/passwd" reaches this block with effective_arg == "\"/etc/passwd\"". Path::new then treats it as a relative string, so the absolute/forbidden-path checks never fire and the command is allowed. Normalize/dequote the extracted value first, then reuse is_path_allowed for the final decision; please also add a regression for the quoted form.
Suggested fix
- let effective_arg = if arg.starts_with("--") {
- arg.split_once('=').map(|(_, v)| v).unwrap_or(arg)
- } else if arg.starts_with('-') && arg.len() > 2 {
- arg.char_indices()
- .nth(2)
- .map(|(idx, _)| &arg[idx..])
- .unwrap_or("")
- } else {
- arg
- };
-
- if !is_likely_path(effective_arg) {
+ let raw_effective_arg = if arg.starts_with("--") {
+ arg.split_once('=').map(|(_, v)| v).unwrap_or(arg)
+ } else if arg.starts_with('-') && arg.len() > 2 {
+ arg.char_indices()
+ .nth(2)
+ .map(|(idx, _)| &arg[idx..])
+ .unwrap_or("")
+ } else {
+ arg
+ };
+
+ let effective_arg = match normalize_arg_for_path_checks(
+ raw_effective_arg.trim_start_matches('='),
+ ) {
+ Some(arg) => arg,
+ None => return false,
+ };
+
+ if !is_likely_path(&effective_arg) {
continue;
}
-
- if Path::new(effective_arg)
- .components()
- .any(|c| matches!(c, std::path::Component::ParentDir))
- || (self.workspace_only
- && (effective_arg.starts_with('/') || effective_arg.starts_with('~')))
- {
- return false;
- }
-
- // Check against forbidden paths (e.g. /etc, ~/.ssh)
- if matches_any_forbidden_path(effective_arg, &self.forbidden_paths) {
+
+ if !self.is_path_allowed(&effective_arg) {
return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/agent-runtime/src/security/policy.rs` around lines 435 - 462, The
path-guard is bypassed when flag values are quoted because
normalize_arg_for_path_checks runs before splitting --key=value, leaving
effective_arg like "\"/etc/passwd\"" which prevents Path::new from detecting
absolute/parent paths; fix by dequoting/normalizing the extracted effective_arg
immediately after the split (call the existing normalize_arg_for_path_checks or
a small dequote utility on effective_arg) before calling is_likely_path,
Path::new checks, matches_any_forbidden_path, or is_path_allowed, and then reuse
is_path_allowed for the final allow/deny decision; also add a regression test
covering forms like --file="/etc/passwd" and -C"../secret" to ensure quoted
values are rejected.
yacosta738
deleted the
fix/security-path-flag-bypass-14289455833197750325
branch
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
1 participant
This PR hardens the
SecurityPolicyin the Rust agent runtime to prevent bypasses where absolute paths or traversal sequences are passed as values within command-line flags.Previously, the argument validation loop only scrutinized standalone arguments, allowing payloads like
grep --file=/etc/passwdorgit -C/etc statusto bypass workspace and forbidden path checks.Changes:
is_segment_validto extract potential path values from long flags (--key=value) and short flags (-Xvalue).char_indicesto prevent panics on multi-byte UTF-8 characters.is_command_allowed_blocks_path_in_flagscovering various bypass scenarios..agents/journal/sentinnel-journal.mdwith the discovery and mitigation.Security Impact:
Performance Impact:
PR created automatically by Jules for task 14289455833197750325 started by @yacosta738