diff --git a/app/src-tauri/src/marketplace/catalog.rs b/app/src-tauri/src/marketplace/catalog.rs index 482d3e9..64127d5 100644 --- a/app/src-tauri/src/marketplace/catalog.rs +++ b/app/src-tauri/src/marketplace/catalog.rs @@ -10,7 +10,7 @@ use sha2::{Digest, Sha256}; -use crate::marketplace::tree::{hex, EntryKind, TreeView}; +use crate::marketplace::tree::{describe_size, hex, EntryKind, ReadError, TreeView}; use crate::models::marketplace::{is_valid_item_key, CatalogItem, ItemKind}; pub const MAX_ITEM_BYTES: u64 = 2 * 1024 * 1024; @@ -115,8 +115,9 @@ fn truncate_preview(text: &str) -> String { format!("{}\n…(truncated)", &text[..cut]) } -fn read_utf8(tree: &dyn TreeView, path: &str) -> Result, String> { - match tree.read_file(path)? { +/// A UTF-8 file of at most `max_bytes` (checked before it is loaded). +fn read_utf8(tree: &dyn TreeView, path: &str, max_bytes: u64) -> Result, String> { + match tree.read_file(path, max_bytes)? { None => Ok(None), Some(bytes) => String::from_utf8(bytes) .map(Some) @@ -187,16 +188,9 @@ fn plugin_source_path(source: &serde_json::Value) -> Result { } fn read_plugin_catalog(tree: &dyn TreeView) -> Result>, String> { - let Some(text) = read_utf8(tree, PLUGIN_CATALOG_PATH)? else { + let Some(text) = read_utf8(tree, PLUGIN_CATALOG_PATH, MAX_MANIFEST_BYTES)? 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 @@ -300,16 +294,19 @@ fn collect_dir( 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))?; + // Capped at what is left of the item's budget, so no file + // bigger than the whole item allows is ever loaded. + let data = match tree + .read_file(&format!("{}/{}", root, child_rel), MAX_ITEM_BYTES - *total) + { + Ok(Some(data)) => data, + Ok(None) => return Err(format!("{} vanished while reading", child_rel)), + Err(ReadError::TooLarge { .. }) => { + return Err(format!("is larger than {}", describe_size(MAX_ITEM_BYTES))) + } + Err(e) => return Err(e.into()), + }; *total += data.len() as u64; - if *total > MAX_ITEM_BYTES { - return Err(format!( - "is larger than {} MiB", - MAX_ITEM_BYTES / (1024 * 1024) - )); - } out.push(ItemFile { rel_path: child_rel, data, @@ -348,14 +345,8 @@ pub fn item_files(tree: &dyn TreeView, kind: ItemKind, key: &str) -> Result return Err(format!("{} is not a regular file", path)), } let data = tree - .read_file(&path)? + .read_file(&path, MAX_ITEM_BYTES)? .ok_or_else(|| format!("{} is missing", path))?; - if data.len() as u64 > MAX_ITEM_BYTES { - return Err(format!( - "is larger than {} MiB", - MAX_ITEM_BYTES / (1024 * 1024) - )); - } Ok(vec![ItemFile { rel_path: format!("{}.md", key), data, @@ -485,14 +476,8 @@ 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) - )); - } + let text = read_utf8(tree, &path, MAX_MANIFEST_BYTES)? + .ok_or_else(|| format!("{} is missing", path))?; serde_json::from_str(&text).map_err(|e| format!("{} is not valid JSON: {}", path, e)) } @@ -581,7 +566,7 @@ fn parse_single_files( continue; } match entry.kind { - EntryKind::File => match read_utf8(tree, &it.path) { + EntryKind::File => match read_utf8(tree, &it.path, MAX_ITEM_BYTES) { Ok(Some(text)) => describe_markdown(&mut it, &text, kind == ItemKind::Command), Ok(None) => it.invalid = Some(format!("{} is missing", it.path)), Err(e) => it.invalid = Some(e), @@ -623,11 +608,13 @@ fn parse_folders(tree: &dyn TreeView, kind: ItemKind, folder: &str, out: &mut Ve continue; } match kind { - ItemKind::Skill => match read_utf8(tree, &format!("{}/SKILL.md", it.path)) { - Ok(Some(text)) => describe_markdown(&mut it, &text, false), - Ok(None) => it.invalid = Some(format!("{} has no SKILL.md", it.path)), - Err(e) => it.invalid = Some(e), - }, + ItemKind::Skill => { + match read_utf8(tree, &format!("{}/SKILL.md", it.path), MAX_ITEM_BYTES) { + Ok(Some(text)) => describe_markdown(&mut it, &text, false), + Ok(None) => it.invalid = Some(format!("{} has no SKILL.md", it.path)), + Err(e) => it.invalid = Some(e), + } + } ItemKind::Hook => match read_hook_json(tree, &entry.name) { Ok(json) => { if let Some(name) = json diff --git a/app/src-tauri/src/marketplace/git.rs b/app/src-tauri/src/marketplace/git.rs index ee7d36a..4bc8f01 100644 --- a/app/src-tauri/src/marketplace/git.rs +++ b/app/src-tauri/src/marketplace/git.rs @@ -545,7 +545,10 @@ mod tests { assert!(has_commit(&repo, &first)); let tree = GitTree::open(&repo, &first).unwrap(); - assert_eq!(tree.read_file("agents/a.md").unwrap().unwrap(), b"one"); + assert_eq!( + tree.read_file("agents/a.md", 1024).unwrap().unwrap(), + b"one" + ); let hook = tree.list_dir("hooks/h").unwrap().unwrap(); assert!(hook[0].executable); assert!(tree.entry_id("agents/a.md").unwrap().is_some()); @@ -560,7 +563,7 @@ mod tests { assert_eq!( GitTree::open(&repo, &first) .unwrap() - .read_file("agents/a.md") + .read_file("agents/a.md", 1024) .unwrap() .unwrap(), b"one" diff --git a/app/src-tauri/src/marketplace/tree.rs b/app/src-tauri/src/marketplace/tree.rs index f39dd7f..37d3ed0 100644 --- a/app/src-tauri/src/marketplace/tree.rs +++ b/app/src-tauri/src/marketplace/tree.rs @@ -29,12 +29,56 @@ pub struct DirEntry { pub trait TreeView { /// Entries of the directory at `path` (`""` = root). `Ok(None)` if absent or not a dir. fn list_dir(&self, path: &str) -> Result>, String>; - /// Contents of the regular file at `path`. `Ok(None)` if absent or not a file. - fn read_file(&self, path: &str) -> Result>, String>; + /// Contents of the regular file at `path`, if it is at most `max_bytes`. + /// `Ok(None)` if absent or not a file. A larger file is + /// [`ReadError::TooLarge`], decided before its contents are loaded. + fn read_file(&self, path: &str, max_bytes: u64) -> Result>, ReadError>; /// Stable content id of the entry at `path`; `None` if absent. fn entry_id(&self, path: &str) -> Result, String>; } +/// Why [`TreeView::read_file`] returned no contents. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ReadError { + /// The file is larger than the caller's cap (known from the object + /// header, so nothing was inflated). + TooLarge { + path: String, + max_bytes: u64, + }, + Other(String), +} + +/// `"2 MiB"`, `"64 KiB"` or `"N bytes"`. +pub(crate) fn describe_size(bytes: u64) -> String { + const KIB: u64 = 1024; + const MIB: u64 = 1024 * 1024; + if bytes >= MIB && bytes % MIB == 0 { + format!("{} MiB", bytes / MIB) + } else if bytes >= KIB && bytes % KIB == 0 { + format!("{} KiB", bytes / KIB) + } else { + format!("{} bytes", bytes) + } +} + +impl From for String { + fn from(e: ReadError) -> String { + match e { + ReadError::TooLarge { path, max_bytes } => { + format!("{} is larger than {}", path, describe_size(max_bytes)) + } + ReadError::Other(msg) => msg, + } + } +} + +impl From for ReadError { + fn from(msg: String) -> Self { + ReadError::Other(msg) + } +} + /// Hex-encode `bytes`. Shared by [`MemTree`]'s content id (test-only) and /// `catalog::item_fingerprint`'s plugin-entry hash (production), so there is /// one hex formatter rather than two copies of the same `format!("{:02x}")`. @@ -122,18 +166,31 @@ impl TreeView for GitTree { Ok(Some(out)) } - fn read_file(&self, path: &str) -> Result>, String> { + fn read_file(&self, path: &str, max_bytes: u64) -> Result>, ReadError> { let Some((id, mode)) = self.lookup(path)? else { return Ok(None); }; if !mode.is_blob() { return Ok(None); } - let blob = self + // The header alone gives the size; a blob over the cap is never + // inflated (a compressible multi-GB file would otherwise be). + let size = self + .repo + .find_header(id) + .map_err(|e| format!("Could not read {}: {}", path, e))? + .size(); + if size > max_bytes { + return Err(ReadError::TooLarge { + path: path.to_string(), + max_bytes, + }); + } + let mut blob = self .repo .find_blob(id) .map_err(|e| format!("Could not read {}: {}", path, e))?; - Ok(Some(blob.data.clone())) + Ok(Some(blob.take_data())) } fn entry_id(&self, path: &str) -> Result, String> { @@ -268,8 +325,14 @@ impl TreeView for MemTree { Ok(Some(out.into_values().collect())) } - fn read_file(&self, path: &str) -> Result>, String> { + fn read_file(&self, path: &str, max_bytes: u64) -> Result>, ReadError> { match self.nodes.get(path) { + Some(MemNode::File { data, .. }) if data.len() as u64 > max_bytes => { + Err(ReadError::TooLarge { + path: path.to_string(), + max_bytes, + }) + } Some(MemNode::File { data, .. }) => Ok(Some(data.clone())), _ => Ok(None), } @@ -325,8 +388,8 @@ mod tests { assert!(hook[0].executable); assert_eq!(t.list_dir("agents/a.md").unwrap(), None); assert_eq!(t.list_dir("missing").unwrap(), None); - assert_eq!(t.read_file("agents/a.md").unwrap().unwrap(), b"x"); - assert_eq!(t.read_file("agents").unwrap(), None); + assert_eq!(t.read_file("agents/a.md", 10).unwrap().unwrap(), b"x"); + assert_eq!(t.read_file("agents", 10).unwrap(), None); } #[test] @@ -350,4 +413,59 @@ mod tests { ); assert_eq!(a.entry_id("nope").unwrap(), None); } + + /// Review #9: the size comes from the object header, so a blob over the + /// cap is refused without its body ever being inflated. The fixture's + /// loose object is cut short after its header: reading the body would + /// fail, while the header still names the full size. + #[test] + fn git_tree_refuses_an_oversized_blob_from_its_header() { + use crate::marketplace::git::test_support::{git, git_available, init_repo}; + if !git_available() { + return; + } + const CAP: u64 = 64 * 1024; + let dir = tempfile::tempdir().unwrap(); + let big = "x".repeat(CAP as usize + 1); + let commit = init_repo( + dir.path(), + &[ + ("agents/big.md", &big, false), + ("agents/small.md", "hi", false), + ], + ); + let blob = git(dir.path(), &["rev-parse", "HEAD:agents/big.md"]); + let loose = dir + .path() + .join(".git/objects") + .join(&blob[..2]) + .join(&blob[2..]); + let bytes = std::fs::read(&loose).unwrap(); + let mut perms = std::fs::metadata(&loose).unwrap().permissions(); + #[allow(clippy::permissions_set_readonly_false)] + perms.set_readonly(false); // git writes objects read-only + std::fs::set_permissions(&loose, perms).unwrap(); + std::fs::write(&loose, &bytes[..40.min(bytes.len())]).unwrap(); + + let tree = GitTree::open(&dir.path().join(".git"), &commit).unwrap(); + let err = String::from(tree.read_file("agents/big.md", CAP).unwrap_err()); + assert!(err.contains("larger than 64 KiB"), "{err}"); + // The body really is unreadable: under a cap it fits, the read fails + // for another reason — so the refusal above never inflated it. + let body = String::from(tree.read_file("agents/big.md", 2 * CAP).unwrap_err()); + assert!(!body.contains("larger than"), "{body}"); + assert_eq!( + tree.read_file("agents/small.md", CAP).unwrap().unwrap(), + b"hi" + ); + assert_eq!(tree.read_file("agents/missing.md", CAP).unwrap(), None); + } + + #[test] + fn mem_tree_applies_the_same_cap() { + let t = MemTree::new().file("a.md", "12345"); + assert_eq!(t.read_file("a.md", 5).unwrap().unwrap(), b"12345"); + let err = String::from(t.read_file("a.md", 4).unwrap_err()); + assert!(err.contains("a.md is larger than 4 bytes"), "{err}"); + } }