Marketplace: validate tree entry names and cap depth/manifest size

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 <noreply@anthropic.com>
2026-09-27 08:55:32 -07:00
parent 2e62728b06
commit b09f811ac1
2 changed files with 181 additions and 5 deletions
@@ -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/<key>/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<String, String> {
@@ -162,6 +190,13 @@ fn read_plugin_catalog(tree: &dyn TreeView) -> Result<Option<Vec<serde_json::Val
    let Some(text) = read_utf8(tree, PLUGIN_CATALOG_PATH)? else {
        return Ok(None);
    };
+    if text.len() as u64 > 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<String, S
 }

 /// Recursively collect a folder's files, enforcing the item rules.
+///
+/// `depth` is how many directory levels below the item root this call is
+/// (the root call is `0`); `entries_seen` counts directories *and* files
+/// together against [`MAX_ITEM_FILES`] — a tree with few files but many
+/// directories is just as much a resource-exhaustion shape as many files,
+/// and only counting files would let it through uncapped.
 fn collect_dir(
    tree: &dyn TreeView,
    root: &str,
    rel: &str,
+    depth: usize,
+    entries_seen: &mut usize,
    out: &mut Vec<ItemFile>,
    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<Vec<
            }
            let mut out = Vec::new();
            let mut total = 0u64;
-            collect_dir(tree, &path, "", &mut out, &mut total)?;
+            let mut entries_seen = 0usize;
+            collect_dir(tree, &path, "", 0, &mut entries_seen, &mut out, &mut total)?;
            let required = match kind {
                ItemKind::Skill => Some("SKILL.md"),
                ItemKind::Hook => Some("hook.json"),
@@ -423,6 +486,13 @@ fn validate_hooks(hooks: &serde_json::Value) -> Result<Vec<String>, String> {
 fn read_hook_json(tree: &dyn TreeView, key: &str) -> Result<serde_json::Value, String> {
    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()
@@ -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;