security: harden path validation against dangling symlink escapes#278
Merged
security: harden path validation against dangling symlink escapes#278
Conversation
Replace exists() gate in check_symlink_escape with symlink_metadata() (lstat). exists() follows symlinks and returns false for dangling ones, silently skipping validation. symlink_metadata() detects the symlink entry itself, allowing us to reject dangling symlinks whose targets cannot be verified as within workspace. Closes #277 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughReplaced existence-based symlink checks with Changes
Sequence Diagram(s)sequenceDiagram
participant Tool as Tool (Read/Write/Edit)
participant Security as Security::validate/revalidate/check
participant FS as Filesystem (OS)
participant Audit as Audit Logger
Tool->>Security: validate_path_in_workspace(path)
Security-->>Tool: SafePath (validated)
Tool->>Security: revalidate_path(path, workspace)
alt revalidation fails (dangling symlink / escape)
Security->>Audit: log violation (symlink_escape)
Security-->>Tool: Err (security violation)
else revalidation ok
Security-->>Tool: Ok
alt operation == write/edit
Tool->>Security: check_hardlink_write(path)
alt hardlink detected (nlink>1)
Security->>Audit: log hardlink_escape
Security-->>Tool: Err (blocked)
else allowed
Security-->>Tool: Ok
Tool->>FS: perform I/O (open/read/write)
FS-->>Tool: I/O result
end
else read/list/exists
Tool->>FS: perform I/O
FS-->>Tool: I/O result
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 (2)
src/security/path.rs (2)
606-607: Hardcoded path could cause test flakiness.The path
/tmp/attacker_controlled_dir_nonexistentcould theoretically exist on a test machine, causing the test to pass for the wrong reason (symlink resolves outside workspace rather than being dangling).Use a unique non-existent path
// Create a symlink that points outside workspace to a path that doesn't exist let symlink_path = canonical.join("future_escape"); - symlink("/tmp/attacker_controlled_dir_nonexistent", &symlink_path).unwrap(); + // Use a path that's extremely unlikely to exist + let nonexistent_outside = format!("/tmp/zepto_test_nonexistent_{}", std::process::id()); + symlink(&nonexistent_outside, &symlink_path).unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security/path.rs` around lines 606 - 607, The hardcoded path passed to symlink can exist on CI and make the test flaky; replace the literal "/tmp/attacker_controlled_dir_nonexistent" with a uniquely generated non-existent path (e.g. use a temp directory base plus a UUID or process PID) and assert it does not exist before calling symlink so the link is guaranteed dangling; update the call that constructs symlink_path (the canonical.join("future_escape") / symlink(...) usage) to use this unique path generator and ensure cleanup as needed.
235-238: Consider distinguishing "not found" from other filesystem errors.The current error handling treats all
symlink_metadatafailures as "path doesn't exist", which is appropriate for new file creation. However, other errors (e.g., permission denied, I/O errors) could mask security-relevant conditions.Optional: More granular error handling
Err(_) => { - // Path component doesn't exist yet — this is fine for new file creation - // (e.g., writing to workspace/subdir/newfile.txt where newfile.txt doesn't exist) + // Path component doesn't exist yet — this is fine for new file creation. + // Note: This also catches permission errors; for stricter security, + // consider checking e.kind() == std::io::ErrorKind::NotFound explicitly. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security/path.rs` around lines 235 - 238, The Err(_) arm handling symlink_metadata currently swallows all errors; change it to match Err(e) and inspect e.kind() (from std::io::Error) so that if e.kind() == NotFound you keep the current behavior, but for other kinds (e.g., PermissionDenied, Other/I/O errors) propagate or return an explicit error instead of ignoring it. Update the code path around the symlink_metadata call (the Err(_) branch) to return or map non-NotFound errors to an appropriate Result/Err so security-relevant FS errors aren't masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/security/path.rs`:
- Around line 210-231: The code currently ignores errors from
current.canonicalize(), allowing directories that fail canonicalization to
bypass checks; update the block in the path validation routine (the branch using
current.canonicalize(), canonical_workspace, log_audit_event, and returning
ZeptoError::SecurityViolation) so that if canonicalize() returns Err you log an
AuditCategory::PathSecurity/AuditSeverity::Critical "symlink_escape" (or
"canonicalize_failure") event including current.display() and the error, and
then return Err(ZeptoError::SecurityViolation(...))—i.e., fail closed on
canonicalize errors in the same manner as the dangling symlink handling.
---
Nitpick comments:
In `@src/security/path.rs`:
- Around line 606-607: The hardcoded path passed to symlink can exist on CI and
make the test flaky; replace the literal
"/tmp/attacker_controlled_dir_nonexistent" with a uniquely generated
non-existent path (e.g. use a temp directory base plus a UUID or process PID)
and assert it does not exist before calling symlink so the link is guaranteed
dangling; update the call that constructs symlink_path (the
canonical.join("future_escape") / symlink(...) usage) to use this unique path
generator and ensure cleanup as needed.
- Around line 235-238: The Err(_) arm handling symlink_metadata currently
swallows all errors; change it to match Err(e) and inspect e.kind() (from
std::io::Error) so that if e.kind() == NotFound you keep the current behavior,
but for other kinds (e.g., PermissionDenied, Other/I/O errors) propagate or
return an explicit error instead of ignoring it. Update the code path around the
symlink_metadata call (the Err(_) branch) to return or map non-NotFound errors
to an appropriate Result/Err so security-relevant FS errors aren't masked.
Comment on lines
+210
to
+231
| } else if meta.is_dir() { | ||
| // Regular directory — canonicalize to check for nested symlinks | ||
| if let Ok(canonical) = current.canonicalize() { | ||
| if !canonical.starts_with(canonical_workspace) { | ||
| log_audit_event( | ||
| AuditCategory::PathSecurity, | ||
| AuditSeverity::Critical, | ||
| "symlink_escape", | ||
| &format!( | ||
| "Symlink escape: '{}' resolves to '{}' outside workspace", | ||
| current.display(), | ||
| canonical.display() | ||
| ), | ||
| true, | ||
| ); | ||
| return Err(ZeptoError::SecurityViolation(format!( | ||
| "Symlink escape detected: '{}' resolves to '{}' which is outside workspace", | ||
| current.display(), | ||
| canonical.display() | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent failure on directory canonicalize may mask escape attempts.
When canonicalize() fails on a directory (line 212), the code silently continues without logging or rejecting. While non-existent paths are handled separately at lines 235-238, a directory that exists but fails to canonicalize (e.g., due to permission issues on a component, or a race condition where the directory becomes a symlink) would bypass validation.
Consider failing closed here, similar to the dangling symlink handling:
Proposed fix
} else if meta.is_dir() {
// Regular directory — canonicalize to check for nested symlinks
- if let Ok(canonical) = current.canonicalize() {
- if !canonical.starts_with(canonical_workspace) {
+ match current.canonicalize() {
+ Ok(canonical) => {
+ if !canonical.starts_with(canonical_workspace) {
+ log_audit_event(
+ AuditCategory::PathSecurity,
+ AuditSeverity::Critical,
+ "symlink_escape",
+ &format!(
+ "Symlink escape: '{}' resolves to '{}' outside workspace",
+ current.display(),
+ canonical.display()
+ ),
+ true,
+ );
+ return Err(ZeptoError::SecurityViolation(format!(
+ "Symlink escape detected: '{}' resolves to '{}' which is outside workspace",
+ current.display(),
+ canonical.display()
+ )));
+ }
+ }
+ Err(e) => {
log_audit_event(
AuditCategory::PathSecurity,
- AuditSeverity::Critical,
- "symlink_escape",
+ AuditSeverity::Warning,
+ "canonicalize_failed",
&format!(
- "Symlink escape: '{}' resolves to '{}' outside workspace",
+ "Cannot canonicalize directory '{}': {}",
current.display(),
- canonical.display()
+ e
),
- true,
+ false,
);
- return Err(ZeptoError::SecurityViolation(format!(
- "Symlink escape detected: '{}' resolves to '{}' which is outside workspace",
- current.display(),
- canonical.display()
- )));
+ // Fail closed: can't verify directory stays within workspace
+ return Err(ZeptoError::SecurityViolation(format!(
+ "Cannot verify directory '{}' is within workspace",
+ current.display()
+ )));
}
}
}📝 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.
Suggested change
| } else if meta.is_dir() { | |
| // Regular directory — canonicalize to check for nested symlinks | |
| if let Ok(canonical) = current.canonicalize() { | |
| if !canonical.starts_with(canonical_workspace) { | |
| log_audit_event( | |
| AuditCategory::PathSecurity, | |
| AuditSeverity::Critical, | |
| "symlink_escape", | |
| &format!( | |
| "Symlink escape: '{}' resolves to '{}' outside workspace", | |
| current.display(), | |
| canonical.display() | |
| ), | |
| true, | |
| ); | |
| return Err(ZeptoError::SecurityViolation(format!( | |
| "Symlink escape detected: '{}' resolves to '{}' which is outside workspace", | |
| current.display(), | |
| canonical.display() | |
| ))); | |
| } | |
| } | |
| } else if meta.is_dir() { | |
| // Regular directory — canonicalize to check for nested symlinks | |
| match current.canonicalize() { | |
| Ok(canonical) => { | |
| if !canonical.starts_with(canonical_workspace) { | |
| log_audit_event( | |
| AuditCategory::PathSecurity, | |
| AuditSeverity::Critical, | |
| "symlink_escape", | |
| &format!( | |
| "Symlink escape: '{}' resolves to '{}' outside workspace", | |
| current.display(), | |
| canonical.display() | |
| ), | |
| true, | |
| ); | |
| return Err(ZeptoError::SecurityViolation(format!( | |
| "Symlink escape detected: '{}' resolves to '{}' which is outside workspace", | |
| current.display(), | |
| canonical.display() | |
| ))); | |
| } | |
| } | |
| Err(e) => { | |
| log_audit_event( | |
| AuditCategory::PathSecurity, | |
| AuditSeverity::Warning, | |
| "canonicalize_failed", | |
| &format!( | |
| "Cannot canonicalize directory '{}': {}", | |
| current.display(), | |
| e | |
| ), | |
| false, | |
| ); | |
| // Fail closed: can't verify directory stays within workspace | |
| return Err(ZeptoError::SecurityViolation(format!( | |
| "Cannot verify directory '{}' is within workspace", | |
| current.display() | |
| ))); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/security/path.rs` around lines 210 - 231, The code currently ignores
errors from current.canonicalize(), allowing directories that fail
canonicalization to bypass checks; update the block in the path validation
routine (the branch using current.canonicalize(), canonical_workspace,
log_audit_event, and returning ZeptoError::SecurityViolation) so that if
canonicalize() returns Err you log an
AuditCategory::PathSecurity/AuditSeverity::Critical "symlink_escape" (or
"canonicalize_failure") event including current.display() and the error, and
then return Err(ZeptoError::SecurityViolation(...))—i.e., fail closed on
canonicalize errors in the same manner as the dangling symlink handling.
- Add revalidate_path() for re-checking path safety immediately before I/O - Add check_hardlink_write() to block writes to files with nlink > 1 - Wire revalidate_path into all filesystem tools (read, write, list, edit) - Wire check_hardlink_write into write paths (write_file, edit_file) - Wire revalidate_path into pdf_read, docx_read, transcribe tools - Export new functions from security module - Add 10 new tests (revalidation, hardlink blocking, symlink escape) Addresses R2 (TOCTOU) and R3 (hardlink alias) from #277. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/security/path.rs (1)
210-231:⚠️ Potential issue | 🟠 MajorFail closed when an existing directory cannot be canonicalized.
symlink_metadata()already provedcurrentexists. Ifcurrent.canonicalize()fails here, the code just skips the workspace-boundary check and keeps validating as if the directory were safe. Permission errors or rename races can still bypass the nested-symlink guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security/path.rs` around lines 210 - 231, In the meta.is_dir() branch, don't ignore failures from current.canonicalize(): if canonicalize() returns Err, treat it as a security violation (fail closed) — call log_audit_event with AuditCategory::PathSecurity/AuditSeverity::Critical and an appropriate "symlink_escape" or "canonicalize_failed" message including current.display() and the error, then return Err(ZeptoError::SecurityViolation(...)); keep the existing canonical.starts_with(canonical_workspace) check when canonicalize succeeds. Use the same symbols current.canonicalize(), canonical_workspace, log_audit_event, and ZeptoError::SecurityViolation to locate and implement the change.src/tools/pdf_read.rs (1)
52-54:⚠️ Potential issue | 🟠 MajorSame TOCTOU gap as the DOCX path helper.
revalidate_path()runs beforeresolve_path()returns, but the actualmetadata/PDF read happens later at Line 172 and Line 184. A path swap in between still bypasses the workspace check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/pdf_read.rs` around lines 52 - 54, The TOCTOU gap: call to revalidate_path(safe.as_path(), &self.workspace) happens before resolve_path returns but the actual PDF metadata/read occurs later, so re-validate immediately before any filesystem I/O. Update the code paths that call resolve_path(...) and then perform metadata or content reads (the PDF reader code that reads metadata and pages) to call revalidate_path(...) again using the resolved/canonical path right before opening/reading the file (or replace the earlier pre-check with a final check immediately prior to the I/O), ensuring you pass the exact path used for the later metadata/read operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/docx_read.rs`:
- Around line 53-55: The current revalidate_path(safe.as_path(),
&self.workspace) call in resolve_path is too early and can be bypassed by a
symlink swap later; move the revalidation into execute() immediately before the
final file open/read (the code paths that later perform I/O around the docx
extractor) or replace the separate check with a single validate-and-open step
that returns an already-open File/handle so the same inode is checked and used
for reading; update references in resolve_path, execute, and any final open/read
sites to use the new validated handle or the late revalidation call.
In `@src/tools/filesystem.rs`:
- Around line 179-191: The code currently calls
tokio::fs::create_dir_all(parent) before revalidating the resolved path, which
allows a TOCTOU symlink swap to escape the workspace; move the guard so you
revalidate the target and parent before mutating the FS: call
revalidate_path(full_path_ref, &workspace) (and revalidate the parent path if
parent.is_some()) prior to invoking tokio::fs::create_dir_all, then perform
check_hardlink_write(full_path_ref) and the directory creation as part of the
protected write sequence so all mutations happen only after workspace
validation.
In `@src/tools/transcribe.rs`:
- Around line 202-210: The TOCTOU bug: revalidate_path is run before I/O but
transcribe_file later re-reads paths (using resolved) and can be tricked by a
symlink swap; fix by either performing validation immediately before the final
open/read inside transcribe_file or change the flow to open the file first and
then inspect the opened file (e.g., use File::open + metadata on the File) to
ensure it is within the workspace; update calls to revalidate_path or replace
them with an open-then-inspect sequence in transcribe_file so the final read
uses a validated/opened file (refer to functions revalidate_path and
transcribe_file and the variable resolved).
---
Duplicate comments:
In `@src/security/path.rs`:
- Around line 210-231: In the meta.is_dir() branch, don't ignore failures from
current.canonicalize(): if canonicalize() returns Err, treat it as a security
violation (fail closed) — call log_audit_event with
AuditCategory::PathSecurity/AuditSeverity::Critical and an appropriate
"symlink_escape" or "canonicalize_failed" message including current.display()
and the error, then return Err(ZeptoError::SecurityViolation(...)); keep the
existing canonical.starts_with(canonical_workspace) check when canonicalize
succeeds. Use the same symbols current.canonicalize(), canonical_workspace,
log_audit_event, and ZeptoError::SecurityViolation to locate and implement the
change.
In `@src/tools/pdf_read.rs`:
- Around line 52-54: The TOCTOU gap: call to revalidate_path(safe.as_path(),
&self.workspace) happens before resolve_path returns but the actual PDF
metadata/read occurs later, so re-validate immediately before any filesystem
I/O. Update the code paths that call resolve_path(...) and then perform metadata
or content reads (the PDF reader code that reads metadata and pages) to call
revalidate_path(...) again using the resolved/canonical path right before
opening/reading the file (or replace the earlier pre-check with a final check
immediately prior to the I/O), ensuring you pass the exact path used for the
later metadata/read operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbd24946-7e35-4477-9a09-68b0fdbb656d
📒 Files selected for processing (6)
src/security/mod.rssrc/security/path.rssrc/tools/docx_read.rssrc/tools/filesystem.rssrc/tools/pdf_read.rssrc/tools/transcribe.rs
Comment on lines
+53
to
55
| // TOCTOU: re-validate immediately before I/O | ||
| revalidate_path(safe.as_path(), &self.workspace)?; | ||
| if !safe.as_path().exists() { |
There was a problem hiding this comment.
revalidate_path() is still too early to protect the actual read.
After Line 54, resolve_path() returns, and the tool does more path-based I/O later at Line 236 and Line 248. A symlink swap in that gap will bypass this check and let the extractor read a file outside the workspace. Move the revalidation to execute() immediately before the final open/read, or validate and open the same handle in one step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/docx_read.rs` around lines 53 - 55, The current
revalidate_path(safe.as_path(), &self.workspace) call in resolve_path is too
early and can be bypassed by a symlink swap later; move the revalidation into
execute() immediately before the final file open/read (the code paths that later
perform I/O around the docx extractor) or replace the separate check with a
single validate-and-open step that returns an already-open File/handle so the
same inode is checked and used for reading; update references in resolve_path,
execute, and any final open/read sites to use the new validated handle or the
late revalidation call.
Comment on lines
179
to
+191
| // Create parent directories if they don't exist | ||
| if let Some(parent) = Path::new(&full_path).parent() { | ||
| if let Some(parent) = full_path_ref.parent() { | ||
| if !parent.as_os_str().is_empty() { | ||
| tokio::fs::create_dir_all(parent).await.map_err(|e| { | ||
| ZeptoError::Tool(format!("Failed to create parent directories: {}", e)) | ||
| })?; | ||
| } | ||
| } | ||
|
|
||
| // TOCTOU: re-validate immediately before I/O | ||
| revalidate_path(full_path_ref, &workspace)?; | ||
| // Hardlink check: block writes to files with multiple hard links | ||
| check_hardlink_write(full_path_ref)?; |
There was a problem hiding this comment.
create_dir_all() is mutating the filesystem before the re-check.
Line 182 can already create directories through a parent symlink that was swapped after resolve_path() returned. If that race happens, we write outside the workspace before Line 189 rejects the path. Revalidate the parent/full path before create_dir_all() and treat directory creation as part of the guarded write sequence. Based on learnings: All filesystem tool operations must validate paths using workspace path validation to prevent symlink escape attacks.
Suggested ordering fix
let (full_path, workspace) = resolve_path(path, ctx)?;
let full_path_ref = Path::new(&full_path);
+ // TOCTOU: re-validate before any filesystem mutation
+ revalidate_path(full_path_ref, &workspace)?;
+
// Create parent directories if they don't exist
if let Some(parent) = full_path_ref.parent() {
if !parent.as_os_str().is_empty() {
+ revalidate_path(parent, &workspace)?;
tokio::fs::create_dir_all(parent).await.map_err(|e| {
ZeptoError::Tool(format!("Failed to create parent directories: {}", e))
})?;
}
}
- // TOCTOU: re-validate immediately before I/O
- revalidate_path(full_path_ref, &workspace)?;
// Hardlink check: block writes to files with multiple hard links
check_hardlink_write(full_path_ref)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/filesystem.rs` around lines 179 - 191, The code currently calls
tokio::fs::create_dir_all(parent) before revalidating the resolved path, which
allows a TOCTOU symlink swap to escape the workspace; move the guard so you
revalidate the target and parent before mutating the FS: call
revalidate_path(full_path_ref, &workspace) (and revalidate the parent path if
parent.is_some()) prior to invoking tokio::fs::create_dir_all, then perform
check_hardlink_write(full_path_ref) and the directory creation as part of the
protected write sequence so all mutations happen only after workspace
validation.
Comment on lines
+202
to
+210
| // TOCTOU: re-validate immediately before I/O | ||
| if let Some(ws) = &ctx.workspace { | ||
| if let Err(e) = revalidate_path(Path::new(&resolved), ws) { | ||
| return Ok(ToolOutput::error(format!( | ||
| "Path re-validation failed: {}", | ||
| e | ||
| ))); | ||
| } | ||
| } |
There was a problem hiding this comment.
The upload path can still be swapped after this check.
revalidate_path() runs here, but transcribe_file() later does separate path-based reads at Line 54 and Line 80 before sending the bytes to Groq. If resolved is replaced with a symlink after Line 204, this tool can upload content from outside the workspace. Re-run validation inside transcribe_file() immediately before the final open/read, or switch to an open-then-inspect flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/transcribe.rs` around lines 202 - 210, The TOCTOU bug:
revalidate_path is run before I/O but transcribe_file later re-reads paths
(using resolved) and can be tricked by a symlink swap; fix by either performing
validation immediately before the final open/read inside transcribe_file or
change the flow to open the file first and then inspect the opened file (e.g.,
use File::open + metadata on the File) to ensure it is within the workspace;
update calls to revalidate_path or replace them with an open-then-inspect
sequence in transcribe_file so the final read uses a validated/opened file
(refer to functions revalidate_path and transcribe_file and the variable
resolved).
Move revalidate_path() call before create_dir_all() so a symlink swap in an ancestor directory between resolve_path() and mkdir cannot create directories outside the workspace. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens path validation against three symlink/hardlink escape vectors identified in #277:
R1 — Dangling symlink bypass:
exists()follows symlinks and returnsfalsefor dangling ones, silently skippingcheck_symlink_escape. Fixed by usingsymlink_metadata()(lstat) which detects the symlink itself.R2 — TOCTOU (time-of-check-time-of-use): Gap between
validate_path_in_workspace()and actual I/O allows race-condition path swaps. Fixed by addingrevalidate_path()called immediately before every filesystem I/O in read_file, write_file, list_dir, edit_file, pdf_read, docx_read, and transcribe tools.R3 — Hardlink alias bypass: A hardlink inside workspace can alias an inode outside the trust boundary. Fixed by adding
check_hardlink_write()that blocks writes to files withnlink > 1, called before every write in write_file and edit_file.Changes
src/security/path.rs— Replaceexists()withsymlink_metadata()incheck_symlink_escape; addrevalidate_path()andcheck_hardlink_write()functions; 10 new testssrc/security/mod.rs— Exportrevalidate_pathandcheck_hardlink_writesrc/tools/filesystem.rs— Wirerevalidate_pathbefore all I/O,check_hardlink_writebefore writes; 3 new testssrc/tools/pdf_read.rs— Wirerevalidate_pathbefore file accesssrc/tools/docx_read.rs— Wirerevalidate_pathbefore file accesssrc/tools/transcribe.rs— Wirerevalidate_pathbefore file accessTest plan
cargo test --lib -- security::path— 28 tests pass (7 new: dangling symlink, revalidation, hardlink)cargo test --lib -- tools::filesystem— 31 tests pass (3 new: hardlink write blocking)cargo test --lib— 3046 tests passcargo clippy -- -D warnings— cleancargo fmt -- --check— cleanCloses #277
Summary by CodeRabbit
Bug Fixes
Tests