From ce0bc7c5678182a5ce2f17658ee1b52b64d6d2c2 Mon Sep 17 00:00:00 2001 From: Jasper Date: Mon, 24 Aug 2026 16:51:01 +0000 Subject: [PATCH] fix(checks): reject symlinked check sources (#11489) --- crates/goose/src/checks/mod.rs | 146 +++++++++++++++++++++++++++++++-- crates/goose/src/sources.rs | 31 +++++++ 2 files changed, 168 insertions(+), 9 deletions(-) diff --git a/crates/goose/src/checks/mod.rs b/crates/goose/src/checks/mod.rs index bb3fc9541..3d8e96513 100644 --- a/crates/goose/src/checks/mod.rs +++ b/crates/goose/src/checks/mod.rs @@ -74,7 +74,13 @@ pub struct Check { impl Check { /// Read and parse a check file from disk. pub fn from_path(path: &Path) -> Result { - let content = fs::read_to_string(path) + let parent = path + .parent() + .ok_or_else(|| anyhow!("check {}: invalid path", path.display()))?; + let file_name = path + .file_name() + .ok_or_else(|| anyhow!("check {}: invalid filename", path.display()))?; + let content = crate::skills::read_source_file(parent, Path::new(file_name)) .with_context(|| format!("read check file: {}", path.display()))?; Self::parse(&content, path) } @@ -345,19 +351,28 @@ pub fn discover_with_globals( }; for dir in global_dirs { - for check in read_checks_dir(dir, "", LoadMode::Lenient)? { + let Ok(canonical_dir) = dir.canonicalize() else { + continue; + }; + for mut check in read_checks_dir(&canonical_dir, &canonical_dir, "", LoadMode::Lenient)? { + if let Some(file_name) = check.path.file_name().map(ToOwned::to_owned) { + check.path = dir.join(file_name); + } record(check, 0); } } - let root_dir = repo_root.join(".agents").join("checks"); - for check in read_checks_dir(&root_dir, "", LoadMode::Strict)? { + let root_dir = canonical_repo_root.join(".agents").join("checks"); + for check in read_checks_dir(&canonical_repo_root, &root_dir, "", LoadMode::Strict)? { record(check, scope_priority("")); } for scope in &scope_dirs { - let dir = repo_root.join(scope).join(".agents").join("checks"); - for check in read_checks_dir(&dir, scope, LoadMode::Strict)? { + let dir = canonical_repo_root + .join(scope) + .join(".agents") + .join("checks"); + for check in read_checks_dir(&canonical_repo_root, &dir, scope, LoadMode::Strict)? { let p = scope_priority(scope); record(check, p); } @@ -393,17 +408,30 @@ enum LoadMode { Lenient, } -fn read_checks_dir(dir: &Path, scope_dir: &str, mode: LoadMode) -> Result> { - if !dir.is_dir() { +fn read_checks_dir( + scan_root: &Path, + dir: &Path, + scope_dir: &str, + mode: LoadMode, +) -> Result> { + if !checks_dir_is_unlinked(scan_root, dir)? { return Ok(Vec::new()); } + match fs::symlink_metadata(dir) { + Ok(metadata) if metadata.is_dir() => {} + Ok(_) => return Ok(Vec::new()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), + Err(error) => { + return Err(error).with_context(|| format!("inspect checks dir {}", dir.display())) + } + } let mut out = Vec::new(); let entries = fs::read_dir(dir).with_context(|| format!("read checks dir {}", dir.display()))?; for entry in entries { let entry = entry?; let path = entry.path(); - if !path.is_file() { + if !entry.file_type()?.is_file() { continue; } if path.extension().and_then(|s| s.to_str()) != Some("md") { @@ -434,6 +462,30 @@ fn read_checks_dir(dir: &Path, scope_dir: &str, mode: LoadMode) -> Result Result { + let relative = dir + .strip_prefix(scan_root) + .with_context(|| format!("checks dir {} is outside scan root", dir.display()))?; + let mut current = scan_root.to_path_buf(); + for component in relative.components() { + let std::path::Component::Normal(component) = component else { + return Ok(false); + }; + current.push(component); + match fs::symlink_metadata(¤t) { + Ok(metadata) if metadata.file_type().is_symlink() => return Ok(false), + Ok(metadata) if metadata.is_dir() => {} + Ok(_) => return Ok(false), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(error) => { + return Err(error) + .with_context(|| format!("inspect checks dir component {}", current.display())) + } + } + } + Ok(true) +} + /// Priority of a scope for shadowing checks/overrides. Higher = closer. fn scope_priority(scope_dir: &str) -> usize { if scope_dir.is_empty() { @@ -722,6 +774,29 @@ tools: [Bash, Read, Grep] assert_eq!(result.checks[0].description.as_deref(), Some("repo")); } + #[cfg(unix)] + #[test] + fn allows_symlinked_global_check_directories() { + use std::os::unix::fs::symlink; + + let dir = tempdir().unwrap(); + let root = dir.path().join("repo"); + fs::create_dir_all(&root).unwrap(); + let global = dir.path().join("global"); + write( + &global.join("perf.md"), + "---\nname: perf\ndescription: global\n---\nglobal body", + ); + let global_link = dir.path().join("global-link"); + symlink(&global, &global_link).unwrap(); + + let result = discover_with_globals(&root, &[], std::slice::from_ref(&global_link)).unwrap(); + + assert_eq!(result.checks.len(), 1); + assert_eq!(result.checks[0].body, "global body"); + assert_eq!(result.checks[0].path, global_link.join("perf.md")); + } + #[test] fn user_check_named_repo_rules_is_not_overwritten_by_root_review_md() { let dir = tempdir().unwrap(); @@ -754,6 +829,59 @@ tools: [Bash, Read, Grep] assert_eq!(result.checks.len(), 1); } + #[cfg(unix)] + #[test] + fn skips_symlinked_check_directories() { + use std::os::unix::fs::symlink; + + let dir = tempdir().unwrap(); + let root = dir.path().join("repo"); + let external_checks = dir.path().join("external-checks"); + write( + &external_checks.join("secret.md"), + "---\nname: secret\n---\nexternal body", + ); + fs::create_dir_all(root.join(".agents")).unwrap(); + symlink(&external_checks, root.join(".agents/checks")).unwrap(); + + let result = discover_with_globals(&root, &[], &[]).unwrap(); + + assert!(result.checks.is_empty()); + } + + #[cfg(unix)] + #[test] + fn skips_checks_behind_symlinked_agent_directory() { + use std::os::unix::fs::symlink; + + let dir = tempdir().unwrap(); + let root = dir.path().join("repo"); + let external_agents = dir.path().join("external-agents"); + write( + &external_agents.join("checks/secret.md"), + "---\nname: secret\n---\nexternal body", + ); + fs::create_dir_all(&root).unwrap(); + symlink(&external_agents, root.join(".agents")).unwrap(); + + let result = discover_with_globals(&root, &[], &[]).unwrap(); + + assert!(result.checks.is_empty()); + } + + #[test] + fn strict_project_check_read_failures_are_not_skipped() { + let dir = tempdir().unwrap(); + let root = dir.path().join("repo"); + let check = root.join(".agents/checks/oversized.md"); + let oversized = "x".repeat(crate::scheduler::MAX_SCHEDULE_RECIPE_BYTES as usize + 1); + write(&check, &oversized); + + let error = discover_with_globals(&root, &[], &[]).unwrap_err(); + + assert!(error.to_string().contains("read check file")); + } + #[cfg(unix)] #[test] fn skips_review_md_symlinks_outside_repo() { diff --git a/crates/goose/src/sources.rs b/crates/goose/src/sources.rs index 038396a10..1b74f247a 100644 --- a/crates/goose/src/sources.rs +++ b/crates/goose/src/sources.rs @@ -1835,6 +1835,37 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn list_agent_sources_rejects_symlinked_review_check_outside_project() { + use std::os::unix::fs::symlink; + + let tmp = TempDir::new().unwrap(); + let project = tmp.path().join("project"); + let checks_dir = project.join(".agents").join("checks"); + std::fs::create_dir_all(&checks_dir).unwrap(); + + let private_check = tmp.path().join("private").join("secret-check.md"); + std::fs::create_dir_all(private_check.parent().unwrap()).unwrap(); + std::fs::write( + &private_check, + "---\nname: secret-check\ndescription: hidden\n---\nTOP_SECRET_CHECK", + ) + .unwrap(); + symlink(&private_check, checks_dir.join("secret-check.md")).unwrap(); + + let listed = list_sources( + Some(SourceType::Agent), + Some(project.to_str().unwrap()), + false, + ) + .unwrap(); + + assert!(!listed.iter().any(|source| { + source.name == "secret-check" && source.content.contains("TOP_SECRET_CHECK") + })); + } + #[test] fn update_rejects_path_traversal() { let tmp = TempDir::new().unwrap();