CyberCoveLLC/Triple-C
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 20 Wiki Activity

Marketplace: check blob sizes from the object header before loading (PR review #9)

Browse Source 
GitTree::read_file now takes a cap and reads the object's size from its
header first, so a blob over MAX_MANIFEST_BYTES / MAX_ITEM_BYTES is refused
without being inflated, and the blob is taken rather than cloned.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
shadowdao
2026-09-27 13:01:48 -07:00
co-authored by Claude Opus 5.5
parent f2bb092586
commit 14852ead65
3 changed files with 159 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src-tauri/src/marketplace/catalog.rs
+28 -41
View File
@@ -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<Option<String>, 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<Option<String>, 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<String, String> {
}
fn read_plugin_catalog(tree: &dyn TreeView) -> Result<Option<Vec<serde_json::Value>>, 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<Vec<
_ => 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<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)
));
}
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
app/src-tauri/src/marketplace/git.rs
+5 -2
View File
@@ -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"
app/src-tauri/src/marketplace/tree.rs
+126 -8
View File
@@ -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<Option<Vec<DirEntry>>, String>;
/// Contents of the regular file at `path`. `Ok(None)` if absent or not a file.
fn read_file(&self, path: &str) -> Result<Option<Vec<u8>>, 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<Option<Vec<u8>>, ReadError>;
/// Stable content id of the entry at `path`; `None` if absent.
fn entry_id(&self, path: &str) -> Result<Option<String>, 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<ReadError> 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<String> 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<Option<Vec<u8>>, String> {
fn read_file(&self, path: &str, max_bytes: u64) -> Result<Option<Vec<u8>>, 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<Option<String>, String> {
@@ -268,8 +325,14 @@ impl TreeView for MemTree {
Ok(Some(out.into_values().collect()))
}
fn read_file(&self, path: &str) -> Result<Option<Vec<u8>>, String> {
fn read_file(&self, path: &str, max_bytes: u64) -> Result<Option<Vec<u8>>, 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}");
}
}