Skip to content

Commit 4de39be

Browse files
yogthosYogthos
andauthored
fix(F7): canonicalize paths in permission check_path to follow symlinks (#82)
Track F-HIGH #7 from ROADMAP.md. ## Problem `resolve_absolute` (`permission/checker.rs:370-376`) joined relative paths against working_dir but never followed symlinks. A user could create: ln -s /etc/passwd ./benign-name.txt …and any deny rule on `/etc/**` would be bypassed because the pattern matcher only saw `./benign-name.txt`. Same hole for external-directory rules in `Accept` mode. ## Fix `resolve_absolute` now calls `std::fs::canonicalize` on the joined path. canonicalize() resolves symlinks AND normalizes `.` / `..` lexically — the path the rule matches is now the real target, not the link name. Three-level fallback for nonexistent paths (writes to new files need to resolve too): 1. Try `canonicalize(full_path)` — works for existing paths + symlinks. 2. If that fails (NotFound on write-to-new), try `canonicalize(parent) + basename` — catches `/safe/parent/../../etc/passwd` style attacks where the parent exists. 3. If even the parent can't canonicalize (test fixture dirs that don't exist on disk), fall back to the lexical join. Matches pre-F7 behavior so existing permission tests on not-yet-existing paths still match. ## Tests Two new tests in `permission::checker::tests`: - `resolve_absolute_follows_symlinks`: creates a tempdir with `real-secret.txt` and a `benign-name.txt` symlink to it; asserts resolve returns the real target's canonical path, not the link name. Handles macOS `/tmp -> /private/tmp` by canonicalizing the expected path too. - `resolve_absolute_handles_nonexistent_via_parent_canonicalize`: parent dir exists but the leaf doesn't; asserts the result is `canonical(parent) / leaf` — proves the second-level fallback works without breaking parent resolution. 662 pass (was 660). All build profiles clean. Co-authored-by: Yogthos <yogthos@gmail.com>
1 parent 9700da8 commit 4de39be

1 file changed

Lines changed: 92 additions & 3 deletions

File tree

src/permission/checker.rs

Lines changed: 92 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -369,10 +369,41 @@ impl PermissionChecker {
369369

370370
fn resolve_absolute(path: &str, working_dir: &str) -> String {
371371
let p = Path::new(path);
372-
if p.is_absolute() {
373-
p.to_string_lossy().to_string()
372+
let joined = if p.is_absolute() {
373+
p.to_path_buf()
374374
} else {
375-
Path::new(working_dir).join(p).to_string_lossy().to_string()
375+
Path::new(working_dir).join(p)
376+
};
377+
// F7: canonicalize so symlinks resolve to their real target.
378+
// Without this, a symlink like `safe_link -> /etc/passwd` would
379+
// be checked against rules as `safe_link`, bypassing any
380+
// `/etc/**` deny / external-directory rule. opencode handles
381+
// this implicitly via TypeScript fs APIs that follow links;
382+
// Rust requires the explicit call.
383+
//
384+
// Fallback: if canonicalize fails (path doesn't exist yet —
385+
// e.g. a write to a new file), normalize `.` / `..` lexically
386+
// and return the joined path as-is. The non-existence is
387+
// intentional for write ops; canonicalize() would return
388+
// NotFound and we'd lose the path entirely.
389+
match std::fs::canonicalize(&joined) {
390+
Ok(canonical) => canonical.to_string_lossy().to_string(),
391+
Err(_) => {
392+
// The path doesn't exist (write to new file, parent
393+
// also missing, etc.). Try canonicalize on the parent
394+
// then re-append the basename — catches
395+
// `/safe/parent/../../etc/passwd` style attacks where
396+
// the parent exists but the leaf doesn't. If even the
397+
// parent doesn't canonicalize, fall back to the
398+
// LEXICAL join (matches pre-F7 behavior so rules on
399+
// not-yet-existing paths still match).
400+
if let (Some(parent), Some(name)) = (joined.parent(), joined.file_name())
401+
&& let Ok(canonical_parent) = std::fs::canonicalize(parent)
402+
{
403+
return canonical_parent.join(name).to_string_lossy().to_string();
404+
}
405+
joined.to_string_lossy().to_string()
406+
}
376407
}
377408
}
378409

@@ -389,6 +420,64 @@ mod tests {
389420
)
390421
}
391422

423+
/// F7: `resolve_absolute` must follow symlinks so a symlink
424+
/// pointing at a deny-listed path can't bypass the rule.
425+
#[test]
426+
fn resolve_absolute_follows_symlinks() {
427+
// Create a temp dir with a real file + a symlink to it.
428+
// Use a unique dir per test process to avoid collisions
429+
// across parallel test runs.
430+
let dir =
431+
std::env::temp_dir().join(format!("dirge-f7-symlink-test-{}", std::process::id(),));
432+
let _ = std::fs::remove_dir_all(&dir);
433+
std::fs::create_dir_all(&dir).unwrap();
434+
let target = dir.join("real-secret.txt");
435+
std::fs::write(&target, "hunter2").unwrap();
436+
let link = dir.join("benign-name.txt");
437+
438+
#[cfg(unix)]
439+
std::os::unix::fs::symlink(&target, &link).unwrap();
440+
#[cfg(windows)]
441+
std::os::windows::fs::symlink_file(&target, &link).unwrap();
442+
443+
let resolved = resolve_absolute(link.to_str().unwrap(), "/");
444+
// The resolved path must match the real target, not the
445+
// symlink name. Canonicalize the comparand too — on macOS
446+
// /tmp is itself a symlink to /private/tmp.
447+
let expected = std::fs::canonicalize(&target)
448+
.unwrap()
449+
.to_string_lossy()
450+
.into_owned();
451+
assert_eq!(resolved, expected, "symlink should resolve to its target",);
452+
453+
let _ = std::fs::remove_dir_all(&dir);
454+
}
455+
456+
/// F7: nonexistent paths (writes to new files) must still
457+
/// resolve sensibly. They can't canonicalize fully but we
458+
/// canonicalize the parent so `/real/parent/../../etc/passwd`
459+
/// becomes `/etc/passwd` instead of staying lexical.
460+
#[test]
461+
fn resolve_absolute_handles_nonexistent_via_parent_canonicalize() {
462+
let dir =
463+
std::env::temp_dir().join(format!("dirge-f7-newfile-test-{}", std::process::id(),));
464+
let _ = std::fs::remove_dir_all(&dir);
465+
std::fs::create_dir_all(&dir).unwrap();
466+
let new_file = dir.join("does-not-exist-yet.txt");
467+
468+
let resolved = resolve_absolute(new_file.to_str().unwrap(), "/");
469+
// The leaf doesn't canonicalize but the parent does.
470+
// Expected form: canonical(parent) / "does-not-exist-yet.txt"
471+
let expected_parent = std::fs::canonicalize(&dir).unwrap();
472+
let expected = expected_parent
473+
.join("does-not-exist-yet.txt")
474+
.to_string_lossy()
475+
.into_owned();
476+
assert_eq!(resolved, expected);
477+
478+
let _ = std::fs::remove_dir_all(&dir);
479+
}
480+
392481
// Regression: "allow always" → `cd *` saved to session allowlist must
393482
// satisfy the NEXT bash check for `cd /absolute/path`. Before the fix,
394483
// path-glob semantics on `*` (`[^/]*`) refused to match the absolute

0 commit comments

Comments
 (0)