From b09f811ac1045189850e51fd3bde7e3bbdba0964 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 27 Sep 2026 08:55:32 -0700 Subject: [PATCH] Marketplace: validate tree entry names and cap depth/manifest size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix round 1 from PR review of the tree/catalog parsing: - collect_dir now rejects an entry whose name is ".", "..", empty, or contains "/", "\" or NUL before it becomes part of an item's rel_path — a crafted git tree could otherwise walk a file outside the item's own folder once that path is joined against the item root downstream. - collect_dir caps recursion at 32 directory levels and counts directories (not just files) toward MAX_ITEM_FILES, so a tree that is wide or deep rather than merely file-heavy is still bounded. - hook.json and plugins/.claude-plugin/marketplace.json are now rejected unparsed above 1 MiB, rather than handed to serde_json regardless of size. A pre-read size query (checking a blob's size before reading it) is deferred per controller ruling — this round reads the blob and checks its length before parsing, which is enough for the JSON-parsing DoS shape being closed here. Co-Authored-By: Claude Opus 5.5 --- app/src-tauri/src/marketplace/catalog.rs | 163 ++++++++++++++++++++++- app/src-tauri/src/marketplace/tree.rs | 23 ++++ 2 files changed, 181 insertions(+), 5 deletions(-) diff --git a/app/src-tauri/src/marketplace/catalog.rs b/app/src-tauri/src/marketplace/catalog.rs index 1624582..482d3e9 100644 --- a/app/src-tauri/src/marketplace/catalog.rs +++ b/app/src-tauri/src/marketplace/catalog.rs @@ -17,6 +17,17 @@ pub const MAX_ITEM_BYTES: u64 = 2 * 1024 * 1024; pub const MAX_ITEM_FILES: usize = 200; /// Preview text is truncated to this many bytes (on a char boundary). const MAX_PREVIEW_BYTES: usize = 64 * 1024; +/// How many directory levels `collect_dir` will descend into an item before +/// giving up on it as invalid. A crafted tree can nest directories far deeper +/// than any real item would (or, over a network `TreeView`, be effectively +/// unbounded), so this is a hard stop rather than a performance nicety. +const MAX_ITEM_DEPTH: usize = 32; +/// `hooks//hook.json` and `plugins/.claude-plugin/marketplace.json` are +/// parsed as JSON before anything else about the item is known, so they are +/// capped and rejected *unparsed* well below `MAX_ITEM_BYTES` — a bound on +/// the whole item is not a bound on what one `serde_json::from_str` call is +/// asked to chew through. +const MAX_MANIFEST_BYTES: u64 = 1024 * 1024; const PLUGIN_CATALOG_PATH: &str = "plugins/.claude-plugin/marketplace.json"; @@ -123,6 +134,23 @@ fn invalid_name_reason(key: &str) -> String { ) } +/// Reject a git tree entry name that could escape the item root once it is +/// joined into a `/`-separated relative path: `.` and `..` (traversal), an +/// empty name (nothing to join), and any name containing `/`, `\` or a NUL +/// byte (a path separator on this or another OS, or a string terminator in +/// C-based tooling downstream). `TreeView` implementations are trusted to +/// return real entries, but a git tree is repo-controlled content, not +/// something this app authored, so a hostile blob naming a tree entry `..` +/// must not turn into a file written outside the item's own folder. +fn valid_entry_name(name: &str) -> bool { + !name.is_empty() + && name != "." + && name != ".." + && !name.contains('/') + && !name.contains('\\') + && !name.contains('\0') +} + /// Normalise a plugin `source` into a path under `plugins/`, refusing /// anything that is not a plain relative path staying inside `plugins/`. fn plugin_source_path(source: &serde_json::Value) -> Result { @@ -162,6 +190,13 @@ fn read_plugin_catalog(tree: &dyn TreeView) -> Result MAX_MANIFEST_BYTES { + return Err(format!( + "{} is larger than {} MiB", + PLUGIN_CATALOG_PATH, + MAX_MANIFEST_BYTES / (1024 * 1024) + )); + } let json: serde_json::Value = serde_json::from_str(&text) .map_err(|e| format!("{} is not valid JSON: {}", PLUGIN_CATALOG_PATH, e))?; let plugins = json @@ -199,13 +234,27 @@ fn item_path(tree: &dyn TreeView, kind: ItemKind, key: &str) -> Result, total: &mut u64, ) -> Result<(), String> { + if depth > MAX_ITEM_DEPTH { + return Err(format!( + "is nested more than {} directories deep", + MAX_ITEM_DEPTH + )); + } let path = if rel.is_empty() { root.to_string() } else { @@ -215,6 +264,12 @@ fn collect_dir( .list_dir(&path)? .ok_or_else(|| format!("{} is not a folder", path))?; for entry in entries { + if !valid_entry_name(&entry.name) { + return Err(format!( + "contains an entry with an invalid name ({:?})", + entry.name + )); + } let child_rel = if rel.is_empty() { entry.name.clone() } else { @@ -233,15 +288,22 @@ fn collect_dir( child_rel )); } - EntryKind::Dir => collect_dir(tree, root, &child_rel, out, total)?, + EntryKind::Dir => { + *entries_seen += 1; + if *entries_seen > MAX_ITEM_FILES { + return Err(format!("has more than {} files", MAX_ITEM_FILES)); + } + collect_dir(tree, root, &child_rel, depth + 1, entries_seen, out, total)? + } EntryKind::File => { + *entries_seen += 1; + if *entries_seen > MAX_ITEM_FILES { + return Err(format!("has more than {} files", MAX_ITEM_FILES)); + } let data = tree .read_file(&format!("{}/{}", root, child_rel))? .ok_or_else(|| format!("{} vanished while reading", child_rel))?; *total += data.len() as u64; - if out.len() + 1 > MAX_ITEM_FILES { - return Err(format!("has more than {} files", MAX_ITEM_FILES)); - } if *total > MAX_ITEM_BYTES { return Err(format!( "is larger than {} MiB", @@ -311,7 +373,8 @@ pub fn item_files(tree: &dyn TreeView, kind: ItemKind, key: &str) -> Result Some("SKILL.md"), ItemKind::Hook => Some("hook.json"), @@ -423,6 +486,13 @@ fn validate_hooks(hooks: &serde_json::Value) -> Result, String> { fn read_hook_json(tree: &dyn TreeView, key: &str) -> Result { let path = format!("hooks/{}/hook.json", key); let text = read_utf8(tree, &path)?.ok_or_else(|| format!("{} is missing", path))?; + if text.len() as u64 > MAX_MANIFEST_BYTES { + return Err(format!( + "{} is larger than {} MiB", + path, + MAX_MANIFEST_BYTES / (1024 * 1024) + )); + } serde_json::from_str(&text).map_err(|e| format!("{} is not valid JSON: {}", path, e)) } @@ -822,6 +892,89 @@ mod tests { assert!(parse_catalog(&t)[0].invalid.is_some()); } + #[test] + fn rejects_tree_entries_whose_name_could_escape_the_item() { + for bad_name in ["..", ".", "", "a\0b"] { + let t = MemTree::new() + .file("skills/s/SKILL.md", "x") + .raw_named_file("skills/s", bad_name, "evil"); + let err = item_files(&t, ItemKind::Skill, "s").unwrap_err(); + assert!( + err.contains("invalid name") || err.contains("invalid entry"), + "{:?}: {}", + bad_name, + err + ); + let items = parse_catalog(&t); + let skill = items.iter().find(|i| i.key == "s").unwrap(); + assert!( + skill.invalid.is_some(), + "{:?} should mark the item invalid", + bad_name + ); + } + } + + #[test] + fn directories_count_toward_the_item_file_limit() { + // Well under MAX_ITEM_FILES by file count alone (151 files), but 150 + // directories on top of that pushes total entries past the limit — + // a shape the old "only count files" logic let through. + let mut t = MemTree::new().file("skills/wide/SKILL.md", "x"); + for i in 0..150 { + t = t.file(&format!("skills/wide/d{}/f.txt", i), "x"); + } + let err = item_files(&t, ItemKind::Skill, "wide").unwrap_err(); + assert!(err.contains("more than 200 files"), "{}", err); + } + + #[test] + fn item_folders_cannot_nest_past_the_depth_cap() { + let mut nested = "skills/deep".to_string(); + for i in 0..40 { + nested = format!("{}/d{}", nested, i); + } + let t = MemTree::new() + .file("skills/deep/SKILL.md", "x") + .file(&format!("{}/leaf.txt", nested), "x"); + let err = item_files(&t, ItemKind::Skill, "deep").unwrap_err(); + assert!(err.contains("nested more than 32"), "{}", err); + assert!(parse_catalog(&t)[0].invalid.is_some()); + } + + #[test] + fn hook_json_over_1_mib_is_rejected_without_being_parsed() { + let huge = "x".repeat(MAX_MANIFEST_BYTES as usize + 1); + let t = MemTree::new().file("hooks/big/hook.json", &huge); + let items = parse_catalog(&t); + let hook = items.iter().find(|i| i.key == "big").unwrap(); + assert!( + hook.invalid + .as_deref() + .unwrap() + .contains("larger than 1 MiB"), + "{:?}", + hook.invalid + ); + } + + #[test] + fn plugin_catalog_over_1_mib_is_rejected_without_being_parsed() { + let huge = "x".repeat(MAX_MANIFEST_BYTES as usize + 1); + let t = MemTree::new().file("plugins/.claude-plugin/marketplace.json", &huge); + let items = parse_catalog(&t); + assert_eq!(items.len(), 1); + assert!( + items[0] + .invalid + .as_deref() + .unwrap() + .contains("larger than 1 MiB"), + "{:?}", + items[0].invalid + ); + } + #[test] fn hooks_must_name_known_events_and_commands() { let t = MemTree::new() diff --git a/app/src-tauri/src/marketplace/tree.rs b/app/src-tauri/src/marketplace/tree.rs index d56dc02..e600b25 100644 --- a/app/src-tauri/src/marketplace/tree.rs +++ b/app/src-tauri/src/marketplace/tree.rs @@ -92,6 +92,29 @@ impl MemTree { self } + /// Place a file so that, inside `dir`, it is listed under the literal + /// entry name `name` — including a name `file`/`exec_file`/`symlink` + /// could never be asked to produce because it doesn't correspond to any + /// real filesystem path a caller here would construct: `.`, `..`, empty, + /// or containing `/`, `\` or a NUL byte. Exists only so a test can drive + /// `catalog::collect_dir`'s hostile-entry-name rejection without relying + /// on incidental behaviour of path-string splitting. + pub fn raw_named_file(mut self, dir: &str, name: &str, contents: &str) -> Self { + let path = if dir.is_empty() { + name.to_string() + } else { + format!("{}/{}", dir, name) + }; + self.nodes.insert( + path, + MemNode::File { + data: contents.as_bytes().to_vec(), + executable: false, + }, + ); + self + } + fn is_dir(&self, path: &str) -> bool { if path.is_empty() { return true;