Skip to content

Commit 321191c

Browse files
committed
Accept filenames containing .. and let file deletion work outside the workspace once authorised
1 parent 3053f0e commit 321191c

4 files changed

Lines changed: 363 additions & 42 deletions

File tree

CHANGELOG.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,13 @@ All notable changes to Sofos are documented in this file.
3636
### Security
3737

3838
- **Session ids passed to `--resume` are validated.** Ids containing path separators (`/`, `\`), or that are exactly `.` or `..`, are rejected with a clear error instead of being interpolated into a filesystem path. The interactive picker never produces such ids; this protects callers that pass an external string.
39+
- **Atomic file writes now stage through a temp file with an unpredictable name.** The earlier fixed `<file>.sofos.tmp` suffix let any process that could write to the same directory plant a file (potentially a symlink to elsewhere) at that exact path between the moment sofos started a write and the moment it created the temp file, redirecting the write to an attacker-chosen target. The staged name now includes 64 bits of randomness and is created exclusively, so a pre-existing file at that path produces a hard error instead of being clobbered through.
40+
- **Workspace path validation no longer rejects filenames that just *contain* `..` as a substring.** Names like `my..file.txt` or `cache..old/note.md` were refused by an over-eager substring check, even though they have no `..` traversal component. The check now walks real path components, so it still blocks `..` traversal but accepts legitimate names with embedded double dots.
41+
- **The resolve fallback now classifies workspace membership lexically.** When sofos couldn't find any existing ancestor to canonicalise against (a degenerate path-shape case during write resolution), an earlier version trusted the caller-supplied path string to decide whether the target was inside the workspace, which mis-classified a workspace-relative `../../etc/passwd` as inside. The fallback now collapses `.` / `..` components first and compares the lexical result against the workspace prefix.
42+
43+
### Changed
44+
45+
- **`delete_file` and `delete_directory` now accept external paths once the user grants Write access**, matching how `write_file` and `edit_file` already behaved. Earlier they rejected every path outside the workspace, which was confusing for users who had explicitly authorised the broader directory for writes.
3946

4047
### Removed
4148

src/tools/executor.rs

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1064,6 +1064,17 @@ impl ToolExecutor {
10641064
SofosError::ToolExecution("Missing 'path' parameter".to_string())
10651065
})?;
10661066

1067+
// Resolve before prompting so we can surface "file not
1068+
// found" without first asking for confirmation, and so
1069+
// the Write-access check on external paths happens
1070+
// BEFORE the user types y/n (consistent with
1071+
// `write_file` / `edit_file`).
1072+
let resolved = self.resolve_existing(path)?;
1073+
1074+
if !resolved.is_inside_workspace {
1075+
self.check_write_access(path, &resolved.canonical_str, &resolved.canonical)?;
1076+
}
1077+
10671078
let confirmed = confirm_destructive(&format!("Delete file '{}'?", path))?;
10681079

10691080
if !confirmed {
@@ -1073,14 +1084,25 @@ impl ToolExecutor {
10731084
)));
10741085
}
10751086

1076-
self.fs_tool.delete_file(path)?;
1087+
if resolved.is_inside_workspace {
1088+
self.fs_tool.delete_file(path)?;
1089+
} else {
1090+
self.fs_tool
1091+
.delete_file_with_outside_access(&resolved.canonical_str)?;
1092+
}
10771093
Ok(format!("Successfully deleted file '{}'", path))
10781094
}
10791095
ToolName::DeleteDirectory => {
10801096
let path = input["path"].as_str().ok_or_else(|| {
10811097
SofosError::ToolExecution("Missing 'path' parameter".to_string())
10821098
})?;
10831099

1100+
let resolved = self.resolve_existing(path)?;
1101+
1102+
if !resolved.is_inside_workspace {
1103+
self.check_write_access(path, &resolved.canonical_str, &resolved.canonical)?;
1104+
}
1105+
10841106
let confirmed = confirm_destructive(&format!(
10851107
"Delete directory '{}' and all its contents?",
10861108
path
@@ -1093,7 +1115,12 @@ impl ToolExecutor {
10931115
)));
10941116
}
10951117

1096-
self.fs_tool.delete_directory(path)?;
1118+
if resolved.is_inside_workspace {
1119+
self.fs_tool.delete_directory(path)?;
1120+
} else {
1121+
self.fs_tool
1122+
.delete_directory_with_outside_access(&resolved.canonical_str)?;
1123+
}
10971124
Ok(format!("Successfully deleted directory '{}'", path))
10981125
}
10991126
ToolName::MoveFile => {

0 commit comments

Comments
 (0)