fix: block symlink escapes in path policy#2034
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a helper that finds and canonicalizes the nearest existing parent of a path, and uses it in SecurityPolicy::is_path_allowed (when workspace_only) to reject resolved prefixes that escape the configured workspace. Adds a Unix-only regression test and relaxes several symlink-related test assertions to accept earlier generic denial messages. ChangesSymlink Escape Prevention in Path Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🧹 Nitpick comments (1)
src/openhuman/security/policy.rs (1)
735-742: ⚡ Quick winLog resolved-prefix rejections.
This new deny path returns
falsesilently, so a symlink-escape block is hard to distinguish from traversal or forbidden-path failures during debugging. A structureddebug!/trace!here with the candidate and resolved prefix would make these decisions much easier to audit.As per coding guidelines, "Use log / tracing at
debugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."🤖 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/openhuman/security/policy.rs` around lines 735 - 742, Add a debug/trace log when a resolved prefix is rejected in the workspace-only check: inside the block where you compute candidate = self.workspace_dir.join(&expanded) and obtain resolved via resolve_existing_path_prefix(&candidate), if !self.is_resolved_path_allowed(&resolved) emit a debug! or trace! log that includes the candidate, the resolved path (resolved), and a short reason like "resolved prefix not allowed" before returning false; ensure you use the existing crate tracing/log macros consistent with surrounding code and avoid changing control flow or return value.
🤖 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 `@src/openhuman/security/policy.rs`:
- Around line 386-395: The resolve_existing_path_prefix function currently uses
current.exists() which follows symlinks and treats dangling symlinks as
non-existent; change the loop to use current.symlink_metadata() to detect if the
path itself exists (including as a symlink), and if it is a symlink use
current.read_link() to determine whether the symlink points inside the allowed
prefix (or canonicalize the symlink target when possible) before deciding to
return current.canonicalize().ok(); if symlink_metadata() indicates a dangling
symlink, treat that path as an existing prefix (return its canonicalized parent
or the symlink path itself as appropriate) instead of climbing to parent(), and
update callers like is_path_allowed to rely on the revised
resolve_existing_path_prefix behavior; also add a regression test that creates a
dangling symlink (e.g., escape_link -> /tmp/outside.txt) and verifies
is_path_allowed("escape_link") no longer allows an escape.
---
Nitpick comments:
In `@src/openhuman/security/policy.rs`:
- Around line 735-742: Add a debug/trace log when a resolved prefix is rejected
in the workspace-only check: inside the block where you compute candidate =
self.workspace_dir.join(&expanded) and obtain resolved via
resolve_existing_path_prefix(&candidate), if
!self.is_resolved_path_allowed(&resolved) emit a debug! or trace! log that
includes the candidate, the resolved path (resolved), and a short reason like
"resolved prefix not allowed" before returning false; ensure you use the
existing crate tracing/log macros consistent with surrounding code and avoid
changing control flow or return value.
🪄 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: 8bb9c54b-cd2d-47fc-894e-905af6e60fe9
📒 Files selected for processing (2)
src/openhuman/security/policy.rssrc/openhuman/security/policy_tests.rs
| fn resolve_existing_path_prefix(path: &Path) -> Option<PathBuf> { | ||
| let mut current = path; | ||
| loop { | ||
| if current.exists() { | ||
| return current.canonicalize().ok(); | ||
| } | ||
|
|
||
| current = current.parent()?; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust's standard library, does std::path::Path::exists()follow symlinks and returnfalse for dangling symlinks? What APIs should be used to detect an existing symlink itself (symlink_metadata) and read its target (read_link)?
💡 Result:
Yes, the std::path::Path::exists() method in Rust follows symbolic links [1]. If a path is a dangling symlink (a symlink pointing to a non-existent target), exists() will return false [1][2]. To detect a symlink itself without following it, or to read its target, you should use the following APIs from the std::fs module: 1. To detect if a path is a symlink: Use std::fs::symlink_metadata(path) to retrieve the file's metadata without following the link [2][3][4]. You can then call .file_type().is_symlink() on the returned Metadata object [3][4]. 2. To read the target of a symlink: Use std::fs::read_link(path), which returns a PathBuf containing the target path [2][3]. Note that this function will return an error if the path is not a symbolic link or does not exist [2]. For checking existence in a way that is less error-prone than Path::exists(), consider using std::fs::exists(path), which returns a Result<bool> and explicitly handles cases where existence cannot be determined (e.g., due to permission errors) [5]. Like Path::exists(), std::fs::exists() also follows symlinks and returns false for broken ones [5].
Citations:
- 1: https://doc.rust-lang.org/std/path/struct.Path.html
- 2: https://stackoverflow.com/questions/49283092/how-to-check-if-a-symlink-not-the-file-it-points-to-exists-in-rust
- 3: https://john-cd.com/rust_howto/categories/filesystem/symbolic_links.html
- 4: https://stackoverflow.com/questions/65752474/check-if-a-file-is-a-symlink-in-rust-2018-on-windows
- 5: https://doc.rust-lang.org/std/fs/fn.exists.html
Handle dangling symlinks as existing prefixes.
Line 389 uses current.exists(), which follows symlinks. A workspace symlink like escape_link -> /tmp/outside.txt is treated as "missing" when the target doesn't exist, causing the loop to climb to the workspace root and return true from is_path_allowed("escape_link"). This creates a write/create escape outside the workspace for dangling symlink targets. Use symlink_metadata() to detect the symlink itself and read_link() to resolve the target before falling back to parent(). Also add a regression test for dangling symlink targets.
Possible fix sketch
fn resolve_existing_path_prefix(path: &Path) -> Option<PathBuf> {
let mut current = path;
loop {
- if current.exists() {
- return current.canonicalize().ok();
+ match std::fs::symlink_metadata(current) {
+ Ok(meta) if meta.file_type().is_symlink() => {
+ let target = std::fs::read_link(current).ok()?;
+ let target = if target.is_absolute() {
+ target
+ } else {
+ current.parent()?.join(target)
+ };
+ return resolve_existing_path_prefix(&target);
+ }
+ Ok(_) => return current.canonicalize().ok(),
+ Err(err) if err.kind() == std::io::ErrorKind::NotFound => {}
+ Err(_) => return None,
}
current = current.parent()?;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn resolve_existing_path_prefix(path: &Path) -> Option<PathBuf> { | |
| let mut current = path; | |
| loop { | |
| if current.exists() { | |
| return current.canonicalize().ok(); | |
| } | |
| current = current.parent()?; | |
| } | |
| } | |
| fn resolve_existing_path_prefix(path: &Path) -> Option<PathBuf> { | |
| let mut current = path; | |
| loop { | |
| match std::fs::symlink_metadata(current) { | |
| Ok(meta) if meta.file_type().is_symlink() => { | |
| let target = std::fs::read_link(current).ok()?; | |
| let target = if target.is_absolute() { | |
| target | |
| } else { | |
| current.parent()?.join(target) | |
| }; | |
| return resolve_existing_path_prefix(&target); | |
| } | |
| Ok(_) => return current.canonicalize().ok(), | |
| Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} | |
| Err(_) => return None, | |
| } | |
| current = current.parent()?; | |
| } | |
| } |
🤖 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/openhuman/security/policy.rs` around lines 386 - 395, The
resolve_existing_path_prefix function currently uses current.exists() which
follows symlinks and treats dangling symlinks as non-existent; change the loop
to use current.symlink_metadata() to detect if the path itself exists (including
as a symlink), and if it is a symlink use current.read_link() to determine
whether the symlink points inside the allowed prefix (or canonicalize the
symlink target when possible) before deciding to return
current.canonicalize().ok(); if symlink_metadata() indicates a dangling symlink,
treat that path as an existing prefix (return its canonicalized parent or the
symlink path itself as appropriate) instead of climbing to parent(), and update
callers like is_path_allowed to rely on the revised resolve_existing_path_prefix
behavior; also add a regression test that creates a dangling symlink (e.g.,
escape_link -> /tmp/outside.txt) and verifies is_path_allowed("escape_link") no
longer allows an escape.
…ssages The new is_path_allowed() symlink check in policy.rs intercepts symlink escapes before the canonicalize-based checks in file_read/file_write. The resulting error message is "Path not allowed by security policy" rather than "Resolved path escapes workspace" / "Refusing to write through symlink". Update the three failing test assertions to accept either message so they correctly validate the block regardless of which guard fires. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Walkthrough
This PR adds symlink-escape protection to is_path_allowed() by resolving the nearest existing path prefix before checking workspace containment. The approach is sound and addresses issue #1927. CodeRabbit already flagged the dangling-symlink gap and the missing debug logging — I won't repeat those. I found one additional security concern in the fail-open logic and a minor test gap.
Change Summary
| File | Change type | Description |
|---|---|---|
src/openhuman/security/policy.rs |
Security fix | Added resolve_existing_path_prefix() helper + integrated into is_path_allowed() |
src/openhuman/security/policy_tests.rs |
Test | New is_path_allowed_blocks_workspace_symlink_escape test |
src/openhuman/tools/impl/filesystem/file_read.rs |
Test update | Loosened assertion to accept both error messages |
src/openhuman/tools/impl/filesystem/file_write.rs |
Test update | Loosened assertions to accept both error messages |
Per-file analysis
policy.rs — resolve_existing_path_prefix + integration
The helper walks up the path tree until it finds an existing component, then canonicalizes it. Clean, focused logic. The integration into is_path_allowed() correctly gates on workspace_only mode and non-empty paths.
However, the integration uses if let Some(resolved) which silently falls through to return true when None is returned. This is a fail-open pattern in security-critical code. See inline comment.
policy_tests.rs — new regression test
Good test structure following the existing pattern (#[cfg(unix)], temp dirs, cleanup). Covers symlink-to-outside and child-under-symlinked-dir. The test cleanup uses the same pattern as resolved_path_blocks_symlink_escape above it.
file_read.rs / file_write.rs — assertion loosening
Appropriate — the security invariant (operation blocked) is still verified. The comments explaining why the assertions changed are a nice touch.
| let candidate = self.workspace_dir.join(&expanded); | ||
| if let Some(resolved) = resolve_existing_path_prefix(&candidate) { | ||
| if !self.is_resolved_path_allowed(&resolved) { | ||
| return false; |
There was a problem hiding this comment.
[major] Fail-open on None — if resolve_existing_path_prefix returns None (canonicalization fails), this if let silently falls through to return true, allowing the path. Security code should fail closed.
On Unix this is practically unreachable (root always exists and canonicalizes), but a fail-open pattern in a security-sensitive path validation is a footgun. If canonicalize() ever fails (permissions, race condition, future platform), the entire symlink check is silently bypassed.
Suggestion — deny by default:
if self.workspace_only && !path.is_empty() {
let candidate = self.workspace_dir.join(&expanded);
match resolve_existing_path_prefix(&candidate) {
Some(resolved) => {
if !self.is_resolved_path_allowed(&resolved) {
return false;
}
}
None => return false, // fail closed
}
}| let _ = std::fs::remove_dir_all(&root); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
[minor] Missing positive test case — this test only verifies that escape paths are blocked, but doesn't verify that a normal workspace-relative path still passes after the change. Adding a quick assert!(policy.is_path_allowed("normal-file.txt")) would guard against over-restrictive regressions (e.g., if the None branch from resolve_existing_path_prefix were changed to deny).
|
Hey @LawyerLyu — thank you sincerely for taking the time to dig into this vulnerability and put together a fix. The We're closing this PR in favour of #2111, which takes the same core idea further: it renames Your PR was genuinely valuable: it helped sharpen exactly what a correct fix needed to handle, and the reviewer feedback it attracted directly informed the final solution. Really appreciate the contribution. 🙏 |
3 participants
Summary
SecurityPolicy::is_path_allowedagainst workspace symlink escapes.Problem
Issue #1927 reports that
is_path_allowed()validates the path string but does not resolve symlinks. A path such asescape_linkcan look workspace-relative while resolving outside the workspace, leaving callers dependent on remembering a separateis_resolved_path_allowed()check.Solution
workspace_dirand reject them if the resolved existing prefix is outside the workspace.Submission Checklist
## Related— N/A: no matrix feature ID applies.docs/RELEASE-MANUAL-SMOKE.md) — N/A: no release manual smoke surface changed.Closes #NNNin the## RelatedsectionImpact
Workspace-only file policy now rejects symlink paths that resolve outside the configured workspace even when callers only use
is_path_allowed(). Normal relative paths and non-existing create targets inside the workspace remain allowed.No migration or dependency impact.
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/security-policy-symlink-paths4a43f9dValidation Run
cargo test -p openhuman is_path_allowed_blocks_workspace_symlink_escape --libcargo fmt --manifest-path Cargo.toml --all --checkgit push -u fork fix/security-policy-symlink-paths(format, lint, compile, Rust check, command token lint)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
is_resolved_path_allowed()remains available for callers that already canonicalize; this PR adds defense in depth tois_path_allowed().Duplicate / Superseded PR Handling
Summary by CodeRabbit
Security
Tests