Skip to content

Commit 8b39ed1

Browse files
axpnetclaude
andcommitted
fix(providers,cli,mcp): never descend into a symlink-to-directory
GAP-A02 is the standing rule: a symlink is unlinked, never followed. `rmdir_recursive`, the used-bytes scan, the rm/purge plan, the MCP delete preview and MCP storage_quota honoured it. Fifteen other recursive walks did not. They gated the descent on `is_dir` alone, and `list` resolves a symlink-to-directory to `is_dir = true` because callers need that to render the entry, so every one of them walked straight into the link. On a tree holding `sub/loop -> ..`, `lsjson -R` emitted 123 entries for 3 real ones, stopping only at the MAX_SCAN_DEPTH cap of 100, and `tree -d 6` reported 6 directories and 3 files where 2 and 1 exist. The `visited` sets do not catch the cycle: each pass yields a longer, distinct path string. `build_tree` even carried a comment claiming "Symlink loop detection" over one of them. Put the invariant on the type that carries the data, as `RemoteEntry::is_walkable_dir()`, and gate every descent on it. The entry keeps `is_dir = true`: only the traversal is refused. Swapping the guard naively would drop a symlink-to-directory into each walker's `else` branch, where `get -r` would try to download it and `dedupe` would hash it. `rmdirs` skips it entirely, out of both the descent stack and the empty-directory candidate list, because SSH_FXP_RMDIR does not follow links and removing the link because its target looks empty is not this tool's call. Two walks needed more than the guard. `tree`'s text renderer decided to descend by sniffing a trailing `/` off the rendered name, which a symlink-to-directory also carries; it now carries `walkable` explicitly on its queue item. The local_tree agent tool typed entries with `metadata()`, which follows links; it now uses `file_type()`, which does not. Providers other than SFTP always report `is_symlink = false`, so `is_walkable_dir()` collapses to `is_dir` and nothing changes for S3, WebDAV or FTP. Live on a plain OpenSSH remote: the cycle tree drops from 123 entries at depth 81 to 3 at depth 1, and `tree -d 6` from 6 dirs / 3 files to the true 2 / 1. On the symlink matrix `link-to-dir` is still listed with `IsDir: true` while `link-to-dir/inner.txt` is gone, and `ls -l`, `rm --dry-run -r` and `size` are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3eebe9e commit 8b39ed1

3 files changed

Lines changed: 115 additions & 23 deletions

File tree

src-tauri/src/ai_core/remote_tools.rs

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1543,7 +1543,9 @@ async fn tree(ctx: &dyn ToolCtx, args: &Value) -> Result<Value, ToolError> {
15431543
"depth": depth + 1,
15441544
}));
15451545
}
1546-
if entry.is_dir && depth + 1 < max_depth + 1 {
1546+
// Reported with `is_dir: true`, but a symlink-to-dir is never
1547+
// walked (GAP-A02): a link to `..` would never terminate.
1548+
if entry.is_walkable_dir() && depth + 1 < max_depth + 1 {
15471549
queue.push_back((entry.path, depth + 1));
15481550
}
15491551
}
@@ -2197,7 +2199,9 @@ async fn cleanup(ctx: &dyn ToolCtx, args: &Value) -> Result<Value, ToolError> {
21972199
Ok(entries) => {
21982200
for entry in entries {
21992201
if entry.is_dir {
2200-
dirs.push((entry.path.clone(), depth + 1));
2202+
if entry.is_walkable_dir() {
2203+
dirs.push((entry.path.clone(), depth + 1));
2204+
}
22012205
} else if entry.name.ends_with(".aerotmp") || entry.path.ends_with(".aerotmp") {
22022206
orphans.push((entry.path.clone(), entry.size));
22032207
}
@@ -2418,7 +2422,9 @@ async fn sync_doctor(ctx: &dyn ToolCtx, args: &Value) -> Result<Value, ToolError
24182422
if let Ok(entries) = backend.list(&dir).await {
24192423
for e in entries {
24202424
if e.is_dir {
2421-
queue.push((e.path.clone(), depth + 1));
2425+
if e.is_walkable_dir() {
2426+
queue.push((e.path.clone(), depth + 1));
2427+
}
24222428
} else {
24232429
let relative = e
24242430
.path
@@ -2531,7 +2537,9 @@ async fn dedupe(ctx: &dyn ToolCtx, args: &Value) -> Result<Value, ToolError> {
25312537
Ok(entries) => {
25322538
for e in entries {
25332539
if e.is_dir {
2534-
dirs.push((e.path, depth + 1));
2540+
if e.is_walkable_dir() {
2541+
dirs.push((e.path, depth + 1));
2542+
}
25352543
} else {
25362544
files.push((e.path, e.size, e.modified));
25372545
}
@@ -2763,7 +2771,9 @@ async fn reconcile(ctx: &dyn ToolCtx, args: &Value) -> Result<Value, ToolError>
27632771
};
27642772
for e in entries {
27652773
if e.is_dir {
2766-
queue.push((e.path.clone(), depth + 1));
2774+
if e.is_walkable_dir() {
2775+
queue.push((e.path.clone(), depth + 1));
2776+
}
27672777
} else {
27682778
let rel = e
27692779
.path

src-tauri/src/bin/aeroftp_cli.rs

Lines changed: 66 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7452,7 +7452,12 @@ async fn scan_remote_tree_with_progress(
74527452
format!("{}/{}", rel_prefix, entry.name)
74537453
};
74547454
if entry.is_dir {
7455-
queue.push((entry.path.clone(), entry_rel, current_depth + 1));
7455+
// A symlink-to-directory is listed but never walked
7456+
// (GAP-A02): syncing through one would duplicate the
7457+
// target's tree, and a link to `..` never terminates.
7458+
if entry.is_walkable_dir() {
7459+
queue.push((entry.path.clone(), entry_rel, current_depth + 1));
7460+
}
74567461
continue;
74577462
}
74587463
if opts.skip_filenames.iter().any(|name| name == &entry.name) {
@@ -13787,7 +13792,9 @@ async fn size_remote_dir(
1378713792
entry.path.clone()
1378813793
};
1378913794
if entry.is_dir {
13790-
stack.push(child);
13795+
if entry.is_walkable_dir() {
13796+
stack.push(child);
13797+
}
1379113798
} else {
1379213799
total_bytes += entry.size;
1379313800
files += 1;
@@ -28102,8 +28109,12 @@ async fn cmd_get_recursive(
2810228109
Ok(entries) => {
2810328110
for e in entries {
2810428111
if e.is_dir {
28105-
queue.push((e.path.clone(), depth + 1));
28106-
dirs.push(e.path);
28112+
// Never walk (nor recreate locally) a symlink-to-dir:
28113+
// it would copy the target's tree twice, or cycle.
28114+
if e.is_walkable_dir() {
28115+
queue.push((e.path.clone(), depth + 1));
28116+
dirs.push(e.path);
28117+
}
2810728118
} else {
2810828119
let relative = e
2810928120
.path
@@ -35095,7 +35106,7 @@ async fn cmd_find(
3509535106
}
3509635107
break;
3509735108
}
35098-
if e.is_dir {
35109+
if e.is_walkable_dir() {
3509935110
queue.push((e.path.clone(), depth + 1));
3510035111
}
3510135112
if matcher.is_match(&e.name) {
@@ -35742,7 +35753,11 @@ async fn cmd_rmdirs(url: &str, path: &str, force: bool, cli: &Cli, format: Outpu
3574235753
match provider.list(&dir).await {
3574335754
Ok(entries) => {
3574435755
for entry in entries {
35745-
if !entry.is_dir {
35756+
// A symlink-to-directory is neither descended into nor
35757+
// offered as an empty-directory candidate (GAP-A02):
35758+
// SSH_FXP_RMDIR does not follow links, and removing the
35759+
// link because its target looks empty is not our call.
35760+
if !entry.is_walkable_dir() {
3574635761
continue;
3574735762
}
3574835763
let rel = entry.path.strip_prefix(&root_norm).unwrap_or(&entry.path);
@@ -36090,7 +36105,9 @@ async fn cmd_lsjson(
3609036105
};
3609136106
// Directories are still descended even when filtered out of
3609236107
// the output, so `--files-only -R` still finds nested files.
36093-
let descend = recursive && e.is_dir && depth < max_depth;
36108+
// A symlink-to-directory is reported but never descended into
36109+
// (GAP-A02): it is still emitted with `IsDir: true`.
36110+
let descend = recursive && e.is_walkable_dir() && depth < max_depth;
3609436111
let emit = if files_only {
3609536112
!e.is_dir
3609636113
} else if dirs_only {
@@ -39484,7 +39501,9 @@ async fn cmd_cleanup(
3948439501
for entry in entries {
3948539502
scanned_entries += 1;
3948639503
if entry.is_dir {
39487-
dirs.push((entry.path.clone(), depth + 1));
39504+
if entry.is_walkable_dir() {
39505+
dirs.push((entry.path.clone(), depth + 1));
39506+
}
3948839507
} else if entry.name.ends_with(".aerotmp") || entry.path.ends_with(".aerotmp") {
3948939508
orphans.push((entry.path.clone(), entry.size, entry.modified.clone()));
3949039509
}
@@ -39730,7 +39749,9 @@ async fn cmd_dedupe(
3973039749
Ok(entries) => {
3973139750
for entry in entries {
3973239751
if entry.is_dir {
39733-
dirs.push((entry.path.clone(), depth + 1));
39752+
if entry.is_walkable_dir() {
39753+
dirs.push((entry.path.clone(), depth + 1));
39754+
}
3973439755
} else {
3973539756
files.push((entry.path.clone(), entry.size, entry.modified.clone()));
3973639757
}
@@ -41117,7 +41138,9 @@ async fn cmd_sync(
4111741138
format!("{}/{}", rel_prefix, e.name)
4111841139
};
4111941140
if e.is_dir {
41120-
queue.push((e.path.clone(), entry_rel, depth + 1));
41141+
if e.is_walkable_dir() {
41142+
queue.push((e.path.clone(), entry_rel, depth + 1));
41143+
}
4112141144
} else {
4112241145
let relative = entry_rel;
4112341146
if !relative.is_empty() && relative != BISYNC_SNAPSHOT_FILE {
@@ -42468,6 +42491,10 @@ async fn cmd_tree(url: &str, path: &str, max_depth: usize, cli: &Cli, format: Ou
4246842491
name: String,
4246942492
depth: usize,
4247042493
prefix: String,
42494+
/// Whether the walk may list this entry's children. Carried explicitly
42495+
/// rather than sniffed back out of the rendered `name`, which cannot
42496+
/// tell a real directory from a symlink-to-directory (GAP-A02).
42497+
walkable: bool,
4247142498
}
4247242499

4247342500
let mut file_count: usize = 0;
@@ -42485,7 +42512,10 @@ async fn cmd_tree(url: &str, path: &str, max_depth: usize, cli: &Cli, format: Ou
4248542512
if depth >= max_depth || *entry_count >= MAX_SCAN_ENTRIES {
4248642513
return Vec::new();
4248742514
}
42488-
// Symlink loop detection: skip already-visited paths
42515+
// Re-entry guard: skip a path we already expanded. This does NOT catch
42516+
// a symlink loop, because every pass through `sub/loop -> ..` yields a
42517+
// longer, distinct path string. Loops are prevented by refusing to
42518+
// descend into symlinks at all (see `is_walkable_dir` below).
4248942519
if !visited.insert(path.to_string()) {
4249042520
return Vec::new();
4249142521
}
@@ -42499,7 +42529,7 @@ async fn cmd_tree(url: &str, path: &str, max_depth: usize, cli: &Cli, format: Ou
4249942529
break;
4250042530
}
4250142531
*entry_count += 1;
42502-
let children = if e.is_dir {
42532+
let children = if e.is_walkable_dir() {
4250342533
Box::pin(build_tree(
4250442534
provider,
4250542535
&e.path,
@@ -42649,6 +42679,7 @@ async fn cmd_tree(url: &str, path: &str, max_depth: usize, cli: &Cli, format: Ou
4264942679
),
4265042680
depth: 1,
4265142681
prefix: child_prefix.to_string(),
42682+
walkable: e.is_walkable_dir(),
4265242683
});
4265342684
if e.is_dir {
4265442685
dir_count += 1;
@@ -42670,8 +42701,10 @@ async fn cmd_tree(url: &str, path: &str, max_depth: usize, cli: &Cli, format: Ou
4267042701
println!("{}", item.name);
4267142702

4267242703
if item.depth < max_depth {
42673-
// Check if this is a directory by checking the trailing /
42674-
if item.name.ends_with('/') && tree_visited.insert(item.path.clone()) {
42704+
// A symlink-to-directory renders with a trailing `/` like any
42705+
// other directory, so the descent is gated on `walkable`, not
42706+
// on the rendered name.
42707+
if item.walkable && tree_visited.insert(item.path.clone()) {
4267542708
if let Ok(children) = provider.list(&item.path).await {
4267642709
let mut sorted: Vec<_> = children.into_iter().collect();
4267742710
sorted.sort_by(|a, b| match (a.is_dir, b.is_dir) {
@@ -42699,6 +42732,7 @@ async fn cmd_tree(url: &str, path: &str, max_depth: usize, cli: &Cli, format: Ou
4269942732
),
4270042733
depth: item.depth + 1,
4270142734
prefix: format!("{}{}", item.prefix, child_prefix),
42735+
walkable: e.is_walkable_dir(),
4270242736
});
4270342737
if e.is_dir {
4270442738
dir_count += 1;
@@ -45780,7 +45814,10 @@ async fn cmd_crypt_ls(
4578045814
};
4578145815
if entry.is_dir {
4578245816
rows.push((rel.clone(), true, 0));
45783-
stack.push((entry.path.clone(), rel, depth + 1));
45817+
// Listed, never walked (GAP-A02).
45818+
if entry.is_walkable_dir() {
45819+
stack.push((entry.path.clone(), rel, depth + 1));
45820+
}
4578445821
} else {
4578545822
rows.push((rel, false, entry.size));
4578645823
}
@@ -46350,7 +46387,11 @@ async fn cmd_crypt_get(
4635046387
entries_seen += 1;
4635146388
let ltarget = ldir.join(crypt_sanitize_component(&plain));
4635246389
if entry.is_dir {
46353-
stack.push((entry.path.clone(), ltarget, depth + 1));
46390+
// Never follow a symlink-to-dir into the local tree: the
46391+
// target's plaintext would be written twice (GAP-A02).
46392+
if entry.is_walkable_dir() {
46393+
stack.push((entry.path.clone(), ltarget, depth + 1));
46394+
}
4635446395
continue;
4635546396
}
4635646397
let ciphertext = match provider.download_to_bytes(&entry.path).await {
@@ -47576,7 +47617,9 @@ async fn cmd_sync_doctor(
4757647617
if let Ok(entries) = provider.list(&dir).await {
4757747618
for e in entries {
4757847619
if e.is_dir {
47579-
queue.push((e.path.clone(), depth + 1));
47620+
if e.is_walkable_dir() {
47621+
queue.push((e.path.clone(), depth + 1));
47622+
}
4758047623
} else {
4758147624
let relative = e
4758247625
.path
@@ -52895,6 +52938,11 @@ async fn execute_cli_tool(
5289552938
}
5289652939
let meta = entry.metadata().ok();
5289752940
let is_dir = meta.as_ref().map(|m| m.is_dir()).unwrap_or(false);
52941+
// `file_type()` does not follow the link, unlike
52942+
// `metadata()` above: a symlink-to-directory is still
52943+
// rendered as a directory, but never descended into,
52944+
// or a link to `..` walks its own parent forever.
52945+
let is_symlink = entry.file_type().map(|t| t.is_symlink()).unwrap_or(false);
5289852946
if !is_dir {
5289952947
if let Some(ref glob) = glob_filter {
5290052948
let pattern = glob.trim_start_matches('*').to_lowercase();
@@ -52908,7 +52956,7 @@ async fn execute_cli_tool(
5290852956
"is_dir": is_dir,
5290952957
"size": meta.as_ref().map(|m| m.len()).unwrap_or(0),
5291052958
});
52911-
if is_dir {
52959+
if is_dir && !is_symlink {
5291252960
let children = build_tree(
5291352961
&entry.path(),
5291452962
depth + 1,

src-tauri/src/providers/types.rs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1573,6 +1573,21 @@ impl RemoteEntry {
15731573
}
15741574
}
15751575

1576+
/// Whether a recursive walk may descend into this entry.
1577+
///
1578+
/// A symlink is unlinked, never followed (`sftp.rs` GAP-A02). `list`
1579+
/// resolves a symlink-to-directory and reports `is_dir = true`, which is
1580+
/// what callers need to render it, but descending into one opens a
1581+
/// cur/parent cycle: a link to `..` makes the walk revisit its own parent
1582+
/// under an ever longer path, so a `visited` set keyed on the path never
1583+
/// catches it.
1584+
///
1585+
/// Every recursive walk over `StorageProvider::list` must gate its descent
1586+
/// on this, never on `is_dir` alone.
1587+
pub fn is_walkable_dir(&self) -> bool {
1588+
self.is_dir && !self.is_symlink
1589+
}
1590+
15761591
/// Get file extension (used in tests and MIME type detection)
15771592
#[allow(dead_code)]
15781593
pub fn extension(&self) -> Option<&str> {
@@ -2193,4 +2208,23 @@ mod s3_config_assume_role_tests {
21932208
assert!(cfg.role_mfa_serial.is_none());
21942209
assert!(cfg.role_mfa_token_code.is_none());
21952210
}
2211+
2212+
#[test]
2213+
fn test_is_walkable_dir_refuses_symlinks() {
2214+
// A plain directory is walkable.
2215+
let mut dir = RemoteEntry::directory("d".into(), "/d".into());
2216+
assert!(dir.is_walkable_dir());
2217+
2218+
// A symlink-to-directory is reported as a directory (callers render
2219+
// it) but must never be descended into: a link to `..` cycles.
2220+
dir.is_symlink = true;
2221+
assert!(dir.is_dir);
2222+
assert!(!dir.is_walkable_dir());
2223+
2224+
// Files are never walkable, symlink or not.
2225+
let mut file = RemoteEntry::file("f".into(), "/f".into(), 10);
2226+
assert!(!file.is_walkable_dir());
2227+
file.is_symlink = true;
2228+
assert!(!file.is_walkable_dir());
2229+
}
21962230
}

0 commit comments

Comments
 (0)