tkmind/tkmind_go
1
0
Fork 0
Code Issues Pull Requests Actions 5 Packages Projects Releases Wiki Activity

fix: contain and bound skill supporting file reads (#11342)

Browse Source 
Signed-off-by: Jasper Hugo <jasper@spiral.xyz>
This commit is contained in:
Jasper
2026-08-20 10:48:51 +00:00
committed by GitHub
parent 875e4bde02
commit df899654fe
11 changed files with 1031 additions and 89 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Cargo.lock
Generated
+1
View File
@@ -4985,6 +4985,7 @@ dependencies = [
"nanoid",
"nostr",
"nostr-sdk",
"ntapi",
"oauth2",
"once_cell",
"openssl",
crates/goose/Cargo.toml
+1
View File
@@ -236,6 +236,7 @@ gethostname = "1.1.0"
[target.'cfg(target_os = "windows")'.dependencies]
winapi = { workspace = true, features = ["accctrl", "aclapi", "fileapi", "handleapi", "minwinbase", "sddl", "securitybaseapi", "winbase", "winerror"] }
keyring = { workspace = true, features = ["windows-native"], optional = true }
ntapi = { version = "0.4.3", default-features = false }
# Platform-specific GPU acceleration for Whisper and local inference
[target.'cfg(target_os = "macos")'.dependencies]
crates/goose/src/agents/large_response_handler.rs
+2 -2
View File
@@ -4,7 +4,7 @@ use std::io::Write;
const DEFAULT_LARGE_TEXT_THRESHOLD: usize = 200_000;
fn large_text_threshold() -> usize {
pub(crate) fn max_tool_response_size() -> usize {
Config::global()
.get_param::<usize>("GOOSE_MAX_TOOL_RESPONSE_SIZE")
.unwrap_or(DEFAULT_LARGE_TEXT_THRESHOLD)
@@ -14,7 +14,7 @@ fn large_text_threshold() -> usize {
pub fn process_tool_response(
response: Result<CallToolResult, ErrorData>,
) -> Result<CallToolResult, ErrorData> {
let threshold = large_text_threshold();
let threshold = max_tool_response_size();
match response {
Ok(mut result) => {
let mut processed_contents = Vec::new();
crates/goose/src/agents/mod.rs
+1
View File
@@ -31,6 +31,7 @@ pub use execute_commands::{context_management_unsupported_message, COMPACT_TRIGG
pub use extension::{ExtensionConfig, ExtensionError};
pub use extension_manager::ExtensionManager;
pub use goose_agent::events::AgentEvent;
pub(crate) use large_response_handler::max_tool_response_size;
pub use prompt_manager::PromptManager;
pub use schedule_tool::ScheduleTool;
pub use subagent_handler::SUBAGENT_TOOL_REQUEST_TYPE;
crates/goose/src/agents/platform_extensions/summon.rs
+102 -23
View File
@@ -197,6 +197,11 @@ fn parse_agent_content(content: &str, path: &Path) -> Option<SourceEntry> {
format!("Agent{}", model_info)
});
let mut properties = std::collections::HashMap::new();
if let Some(model) = metadata.model {
properties.insert("model".to_string(), serde_json::Value::String(model));
}
Some(SourceEntry {
source_type: SourceType::Agent,
name: metadata.name,
@@ -206,7 +211,7 @@ fn parse_agent_content(content: &str, path: &Path) -> Option<SourceEntry> {
global: false,
writable: true,
supporting_files: Vec::new(),
properties: std::collections::HashMap::new(),
properties,
})
}
@@ -217,16 +222,17 @@ fn scan_recipes_from_dir(
sources: &mut Vec<SourceEntry>,
seen: &mut std::collections::HashSet<String>,
) {
let entries = match std::fs::read_dir(dir) {
let Ok(source_dir) = dir.canonicalize() else {
return;
};
let entries = match std::fs::read_dir(&source_dir) {
Ok(e) => e,
Err(_) => return,
};
for entry in entries.flatten() {
let path = entry.path();
if !path.is_file() {
continue;
}
let file_name = entry.file_name();
let path = source_dir.join(&file_name);
let ext = path.extension().and_then(|e| e.to_str()).unwrap_or("");
if !RECIPE_FILE_EXTENSIONS.contains(&ext) {
@@ -243,7 +249,19 @@ fn scan_recipes_from_dir(
continue;
}
match Recipe::from_file_path(&path) {
let content = match crate::skills::read_source_file_with_limit(
&source_dir,
Path::new(&file_name),
crate::agents::max_tool_response_size(),
) {
Ok(content) => content,
Err(error) => {
warn!("Failed to read recipe {}: {}", path.display(), error);
continue;
}
};
match Recipe::from_content(&content) {
Ok(recipe) => {
seen.insert(name.clone());
sources.push(SourceEntry {
@@ -277,23 +295,28 @@ fn scan_agents_from_dir(
sources: &mut Vec<SourceEntry>,
seen: &mut std::collections::HashSet<String>,
) {
let entries = match std::fs::read_dir(dir) {
let Ok(source_dir) = dir.canonicalize() else {
return;
};
let entries = match std::fs::read_dir(&source_dir) {
Ok(e) => e,
Err(_) => return,
};
for entry in entries.flatten() {
let path = entry.path();
if !path.is_file() {
continue;
}
let file_name = entry.file_name();
let path = source_dir.join(&file_name);
let ext = path.extension().and_then(|e| e.to_str()).unwrap_or("");
if ext != "md" {
continue;
}
let content = match std::fs::read_to_string(&path) {
let content = match crate::skills::read_source_file_with_limit(
&source_dir,
Path::new(&file_name),
crate::agents::max_tool_response_size(),
) {
Ok(c) => c,
Err(e) => {
warn!("Failed to read agent file {}: {}", path.display(), e);
@@ -1575,18 +1598,15 @@ impl SummonClient {
source: &SourceEntry,
params: &DelegateParams,
) -> Result<Recipe, String> {
let agent_content = if source.path.is_empty() {
if source.path.is_empty() {
return Err("Agent source has no path".to_string());
} else {
std::fs::read_to_string(&source.path)
.map_err(|e| format!("Failed to read agent file: {}", e))?
};
}
let (metadata, _): (AgentMetadata, String) = parse_frontmatter(&agent_content)
.map_err(|e| format!("Failed to parse agent frontmatter: {}", e))?
.ok_or("No frontmatter found in agent file")?;
let model = metadata.model;
let model = source
.properties
.get("model")
.and_then(serde_json::Value::as_str)
.map(str::to_string);
// max_turns is set later in build_task_config so it can incorporate params.max_turns
// with the correct priority ordering; setting it here would cause it to be overridden
@@ -2246,6 +2266,13 @@ You review code."#;
let source = parse_agent_content(agent, Path::new("")).unwrap();
assert_eq!(source.name, "reviewer");
assert!(source.description.contains("sonnet"));
assert_eq!(
source
.properties
.get("model")
.and_then(|value| value.as_str()),
Some("sonnet")
);
}
#[test]
@@ -2335,6 +2362,29 @@ You review code."#;
assert_eq!(sources[0].name, "reviewer");
}
#[cfg(unix)]
#[test]
fn agent_scan_rejects_symlinked_source_file() {
let temp_dir = TempDir::new().unwrap();
let outside = TempDir::new().unwrap();
fs::write(
outside.path().join("outside.md"),
"---\nname: outside\n---\nUntrusted agent.",
)
.unwrap();
std::os::unix::fs::symlink(
outside.path().join("outside.md"),
temp_dir.path().join("outside.md"),
)
.unwrap();
let mut sources = Vec::new();
let mut seen = HashSet::new();
scan_agents_from_dir(temp_dir.path(), &mut sources, &mut seen);
assert!(sources.is_empty());
}
#[test]
fn test_recipe_scan_skips_non_recipe_project_config_files() {
let temp_dir = TempDir::new().unwrap();
@@ -2369,6 +2419,35 @@ You review code."#;
assert_eq!(sources[0].description, "Real recipe");
}
#[cfg(unix)]
#[test]
fn recipe_scan_rejects_symlinked_source_file() {
let temp_dir = TempDir::new().unwrap();
let outside = TempDir::new().unwrap();
fs::write(
outside.path().join("outside.yaml"),
"title: Outside\ndescription: Outside recipe\ninstructions: Untrusted",
)
.unwrap();
std::os::unix::fs::symlink(
outside.path().join("outside.yaml"),
temp_dir.path().join("outside.yaml"),
)
.unwrap();
let mut sources = Vec::new();
let mut seen = HashSet::new();
scan_recipes_from_dir(
temp_dir.path(),
SourceType::Recipe,
false,
&mut sources,
&mut seen,
);
assert!(sources.is_empty());
}
#[tokio::test]
async fn test_discover_recipes_and_agents() {
let temp_dir = TempDir::new().unwrap();
crates/goose/src/agents/state_machine/ops_skills.rs
+36 -18
View File
@@ -146,9 +146,6 @@ fn load_supporting_file(
relative_path: &str,
) -> CallToolResult {
let skill_dir = PathBuf::from(&skill.path);
let canonical_skill_dir = skill_dir
.canonicalize()
.unwrap_or_else(|_| skill_dir.clone());
for file_path in &skill.supporting_files {
let file_path = Path::new(file_path);
let Ok(relative) = file_path.strip_prefix(&skill_dir) else {
@@ -157,22 +154,10 @@ fn load_supporting_file(
if relative.to_string_lossy().replace('\\', "/") != relative_path {
continue;
}
return match file_path.canonicalize() {
Ok(canonical) if canonical.starts_with(&canonical_skill_dir) => {
match std::fs::read_to_string(&canonical) {
Ok(content) => CallToolResult::success(vec![ContentBlock::text(format!(
"# Loaded: {skill_name}\n\n{content}\n\n---\nFile loaded into context."
))]),
Err(error) => CallToolResult::error(vec![ContentBlock::text(format!(
"Failed to read '{skill_name}': {error}"
))]),
}
}
Ok(_) => CallToolResult::error(vec![ContentBlock::text(format!(
"Refusing to load '{skill_name}': resolves outside the skill directory"
))]),
return match crate::skills::load_supporting_file(&skill_dir, relative, skill_name) {
Ok(content) => CallToolResult::success(vec![ContentBlock::text(content)]),
Err(error) => CallToolResult::error(vec![ContentBlock::text(format!(
"Failed to resolve '{skill_name}': {error}"
"Failed to read '{skill_name}': {error}"
))]),
};
}
@@ -360,3 +345,36 @@ impl Operation<Session, GooseEffect> for SkillOperation {
applied([response.into()])
}
}
#[cfg(test)]
mod tests {
use super::*;
use std::collections::HashMap;
#[test]
fn supporting_file_loader_reads_nested_regular_file() {
let root = tempfile::tempdir().unwrap();
let skill_dir = std::fs::canonicalize(root.path()).unwrap();
let nested = skill_dir.join("nested");
std::fs::create_dir(&nested).unwrap();
let file = nested.join("guide.md");
std::fs::write(&file, "Nested guidance.").unwrap();
let skill = SourceEntry {
source_type: SourceType::Skill,
name: "test-skill".to_string(),
description: String::new(),
content: String::new(),
path: skill_dir.to_string_lossy().into_owned(),
global: false,
writable: true,
supporting_files: vec![file.to_string_lossy().into_owned()],
properties: HashMap::new(),
};
let result = load_supporting_file(&skill, "test-skill/nested/guide.md", "nested/guide.md");
assert_eq!(result.is_error, Some(false));
let text = result.content[0].as_text().expect("expected text");
assert!(text.text.contains("Nested guidance."));
}
}
crates/goose/src/recipe/read_recipe_file_content.rs
+35 -13
View File
@@ -12,27 +12,32 @@ pub struct RecipeFile {
pub fn read_recipe_file<P: AsRef<Path>>(recipe_path: P) -> Result<RecipeFile> {
let raw_path = recipe_path.as_ref();
let path = convert_path_with_tilde_expansion(raw_path);
let content = fs::read_to_string(&path)
.map_err(|e| anyhow!("Failed to read recipe file {}: {}", path.display(), e))?;
let canonical = path.canonicalize().map_err(|e| {
let parent = path
.parent()
.filter(|parent| !parent.as_os_str().is_empty())
.unwrap_or_else(|| Path::new("."));
let parent_dir = parent.canonicalize().map_err(|e| {
anyhow!(
"Failed to resolve absolute path for {}: {}",
path.display(),
"Failed to resolve recipe directory {}: {}",
parent.display(),
e
)
})?;
let parent_dir = canonical
.parent()
.ok_or_else(|| anyhow!("Resolved path has no parent: {}", canonical.display()))?
.to_path_buf();
let file_name = path
.file_name()
.ok_or_else(|| anyhow!("Recipe path has no file name: {}", path.display()))?;
let content = crate::skills::read_source_file_with_limit(
&parent_dir,
Path::new(file_name),
crate::agents::max_tool_response_size(),
)
.map_err(|e| anyhow!("Failed to read recipe file {}: {}", path.display(), e))?;
let file_path = parent_dir.join(file_name);
Ok(RecipeFile {
content,
parent_dir,
file_path: canonical,
file_path,
})
}
@@ -99,4 +104,21 @@ mod tests {
.to_string()
.contains("Failed to read parameter file"));
}
#[cfg(unix)]
#[test]
fn read_recipe_file_rejects_symlink() {
let temp_dir = TempDir::new().unwrap();
let outside = TempDir::new().unwrap();
let outside_recipe = outside.path().join("outside.yaml");
std::fs::write(
&outside_recipe,
"title: Outside\ndescription: Outside\ninstructions: Untrusted",
)
.unwrap();
let linked_recipe = temp_dir.path().join("linked.yaml");
std::os::unix::fs::symlink(outside_recipe, &linked_recipe).unwrap();
assert!(read_recipe_file(linked_recipe).is_err());
}
}
crates/goose/src/skills/client.rs
+22 -24
View File
@@ -149,9 +149,6 @@ impl McpClientTrait for SkillsClient {
&& matches!(s.source_type, SourceType::Skill | SourceType::BuiltinSkill)
}) {
let skill_dir = PathBuf::from(&skill.path);
let canonical_skill_dir = skill_dir
.canonicalize()
.unwrap_or_else(|_| skill_dir.clone());
for file_path in &skill.supporting_files {
let file_path_buf = Path::new(file_path);
@@ -162,30 +159,14 @@ impl McpClientTrait for SkillsClient {
continue;
}
return Ok(match file_path_buf.canonicalize() {
Ok(canonical) if canonical.starts_with(&canonical_skill_dir) => {
match std::fs::read_to_string(&canonical) {
Ok(content) => {
CallToolResult::success(vec![ContentBlock::text(format!(
"# Loaded: {}\n\n{}\n\n---\nFile loaded into context.",
skill_name, content
))])
}
Err(e) => CallToolResult::error(vec![ContentBlock::text(format!(
"Failed to read '{}': {}",
skill_name, e
))]),
}
}
Ok(_) => CallToolResult::error(vec![ContentBlock::text(format!(
"Refusing to load '{}': resolves outside the skill directory",
skill_name
))]),
let result = match super::load_supporting_file(&skill_dir, rel, skill_name) {
Ok(content) => CallToolResult::success(vec![ContentBlock::text(content)]),
Err(e) => CallToolResult::error(vec![ContentBlock::text(format!(
"Failed to resolve '{}': {}",
"Failed to read '{}': {}",
skill_name, e
))]),
});
};
return Ok(result);
}
let available: Vec<String> = skill
@@ -289,6 +270,8 @@ mod tests {
"---\nname: my-skill\ndescription: A test skill\n---\nDo the thing.",
)
.unwrap();
fs::create_dir(skill_dir.join("nested")).unwrap();
fs::write(skill_dir.join("nested/guide.md"), "Nested guidance.").unwrap();
let session = std::sync::Arc::new(crate::session::Session {
working_dir: temp_dir.path().to_path_buf(),
@@ -324,6 +307,21 @@ mod tests {
};
assert!(text.contains("my-skill"));
assert!(text.contains("Do the thing"));
let args: JsonObject =
serde_json::from_value(serde_json::json!({"name": "my-skill/nested/guide.md"}))
.unwrap();
let result = client
.call_tool(&ctx, "load_skill", Some(args), CancellationToken::new())
.await
.unwrap();
assert!(!result.is_error.unwrap_or(false));
let text = match &result.content[0] {
rmcp::model::ContentBlock::Text(t) => &t.text,
_ => panic!("expected text"),
};
assert!(text.contains("Nested guidance."));
}
#[tokio::test]
crates/goose/src/skills/mod.rs
+14 -2
View File
@@ -5,8 +5,10 @@
mod arguments;
mod builtin;
pub mod client;
mod supporting_files;
pub use client::{SkillsClient, EXTENSION_NAME};
pub(crate) use supporting_files::{load_supporting_file, read_source_file_with_limit};
use crate::config::{paths::Paths, Config};
use crate::plugins::installed_plugin_skill_dirs;
@@ -442,6 +444,9 @@ fn scan_skills_from_dir(dir: &Path, global: bool, seen: &mut HashSet<String>) ->
let Some(skill_dir) = skill_file.parent() else {
continue;
};
let Ok(registered_skill_dir) = skill_dir.canonicalize() else {
continue;
};
let content = match std::fs::read_to_string(&skill_file) {
Ok(c) => c,
Err(e) => {
@@ -450,7 +455,7 @@ fn scan_skills_from_dir(dir: &Path, global: bool, seen: &mut HashSet<String>) ->
}
};
if let Some(mut source) = parse_skill_content(&content, skill_dir, global) {
if let Some(mut source) = parse_skill_content(&content, &registered_skill_dir, global) {
if !seen.contains(&source.name) {
let mut files = Vec::new();
let mut visited_support_dirs = HashSet::new();
@@ -460,7 +465,14 @@ fn scan_skills_from_dir(dir: &Path, global: bool, seen: &mut HashSet<String>) ->
&mut |path| !should_skip_dir(path) && !path.join("SKILL.md").is_file(),
&mut |path| {
if path.file_name().and_then(|n| n.to_str()) != Some("SKILL.md") {
files.push(path.to_string_lossy().into_owned());
if let Ok(relative) = path.strip_prefix(skill_dir) {
files.push(
registered_skill_dir
.join(relative)
.to_string_lossy()
.into_owned(),
);
}
}
},
);
crates/goose/src/skills/supporting_files.rs
+791
View File
@@ -0,0 +1,791 @@
use std::fs;
use std::io::{self, Read};
use std::path::{Component, Path};
const LOADED_FILE_PREFIX: &str = "# Loaded: ";
const LOADED_FILE_SEPARATOR: &str = "\n\n";
const LOADED_FILE_SUFFIX: &str = "\n\n---\nFile loaded into context.";
pub(crate) fn load_supporting_file(
skill_dir: &Path,
relative: &Path,
skill_name: &str,
) -> io::Result<String> {
load_supporting_file_with_limit(
skill_dir,
relative,
skill_name,
crate::agents::max_tool_response_size(),
)
}
fn load_supporting_file_with_limit(
skill_dir: &Path,
relative: &Path,
skill_name: &str,
max_characters: usize,
) -> io::Result<String> {
let wrapper_characters = LOADED_FILE_PREFIX.chars().count()
+ skill_name.chars().count()
+ LOADED_FILE_SEPARATOR.chars().count()
+ LOADED_FILE_SUFFIX.chars().count();
let content_limit = max_characters
.checked_sub(wrapper_characters)
.ok_or_else(|| {
io::Error::new(
io::ErrorKind::InvalidInput,
format!(
"maximum tool response size of {max_characters} characters is too small to load '{skill_name}'"
),
)
})?;
let content = read_supporting_file_with_limit(skill_dir, relative, content_limit)?;
Ok(format!(
"{LOADED_FILE_PREFIX}{skill_name}{LOADED_FILE_SEPARATOR}{content}{LOADED_FILE_SUFFIX}"
))
}
fn read_supporting_file_with_limit(
skill_dir: &Path,
relative: &Path,
max_characters: usize,
) -> io::Result<String> {
read_supporting_file_with_hook(skill_dir, relative, max_characters, |_| {})
}
pub(crate) fn read_source_file_with_limit(
source_dir: &Path,
relative: &Path,
max_characters: usize,
) -> io::Result<String> {
read_supporting_file_with_limit(source_dir, relative, max_characters)
}
fn max_utf8_bytes(max_characters: usize) -> io::Result<usize> {
max_characters.checked_mul(4).ok_or_else(|| {
io::Error::new(
io::ErrorKind::InvalidInput,
"configured supporting file size limit is too large",
)
})
}
fn read_utf8_with_limit(mut reader: impl io::Read, max_characters: usize) -> io::Result<String> {
let max_bytes = max_utf8_bytes(max_characters)?;
let read_size = max_bytes.checked_add(1).ok_or_else(|| {
io::Error::new(
io::ErrorKind::InvalidInput,
"configured supporting file size limit is too large",
)
})?;
let mut bytes = Vec::new();
reader
.by_ref()
.take(read_size as u64)
.read_to_end(&mut bytes)?;
if bytes.len() > max_bytes {
return Err(file_encoding_too_large(max_bytes));
}
let content = String::from_utf8(bytes)
.map_err(|error| io::Error::new(io::ErrorKind::InvalidData, error))?;
if content.chars().count() > max_characters {
return Err(file_too_large(max_characters));
}
Ok(content)
}
fn file_too_large(max_characters: usize) -> io::Error {
io::Error::new(
io::ErrorKind::InvalidData,
format!("supporting file exceeds the maximum size of {max_characters} characters"),
)
}
fn file_encoding_too_large(max_bytes: usize) -> io::Error {
io::Error::new(
io::ErrorKind::InvalidData,
format!("supporting file exceeds the maximum encoded size of {max_bytes} bytes"),
)
}
fn read_opened_file(file: fs::File, max_characters: usize) -> io::Result<String> {
let max_bytes = max_utf8_bytes(max_characters)?;
if file.metadata()?.len() > max_bytes as u64 {
return Err(file_encoding_too_large(max_bytes));
}
read_utf8_with_limit(file, max_characters)
}
fn validated_relative_components(path: &Path) -> io::Result<Vec<&std::ffi::OsStr>> {
let mut components = Vec::new();
for component in path.components() {
match component {
Component::Normal(component) => components.push(component),
Component::CurDir => {}
Component::ParentDir | Component::RootDir | Component::Prefix(_) => {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"supporting file path must stay within the skill directory",
));
}
}
}
if components.is_empty() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"supporting file path must name a file",
));
}
Ok(components)
}
#[cfg(any(target_os = "linux", target_os = "android"))]
fn directory_traversal_flags() -> libc::c_int {
libc::O_PATH | libc::O_DIRECTORY | libc::O_NOFOLLOW | libc::O_CLOEXEC
}
#[cfg(all(
unix,
any(
target_vendor = "apple",
target_os = "aix",
target_os = "freebsd",
target_os = "illumos",
target_os = "netbsd",
target_os = "solaris"
)
))]
fn directory_traversal_flags() -> libc::c_int {
libc::O_SEARCH | libc::O_DIRECTORY | libc::O_NOFOLLOW | libc::O_CLOEXEC
}
#[cfg(all(
unix,
not(any(
target_vendor = "apple",
target_os = "aix",
target_os = "android",
target_os = "freebsd",
target_os = "illumos",
target_os = "linux",
target_os = "netbsd",
target_os = "solaris"
))
))]
fn directory_traversal_flags() -> libc::c_int {
libc::O_RDONLY | libc::O_DIRECTORY | libc::O_NOFOLLOW | libc::O_CLOEXEC
}
#[cfg(unix)]
fn open_skill_root(
skill_dir: &Path,
after_opened_component: &mut impl FnMut(&Path),
) -> io::Result<fs::File> {
use std::os::unix::fs::OpenOptionsExt;
let mut options = fs::OpenOptions::new();
options.read(true).custom_flags(directory_traversal_flags());
let mut directory = options.open(Path::new("/"))?;
let mut opened_path = std::path::PathBuf::from("/");
let mut saw_root = false;
for component in skill_dir.components() {
match component {
Component::RootDir if !saw_root => saw_root = true,
Component::Normal(component) if saw_root => {
directory = open_at(&directory, component, directory_traversal_flags())?;
opened_path.push(component);
after_opened_component(&opened_path);
}
Component::CurDir if saw_root => {}
_ => {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"skill path must be an absolute normalized path",
));
}
}
}
if !saw_root || opened_path != skill_dir {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"skill path must be an absolute normalized path",
));
}
if !directory.metadata()?.is_dir() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"skill path is not a directory",
));
}
Ok(directory)
}
#[cfg(unix)]
fn read_supporting_file_with_hook(
skill_dir: &Path,
relative: &Path,
max_characters: usize,
mut after_opened_component: impl FnMut(&Path),
) -> io::Result<String> {
let components = validated_relative_components(relative)?;
let (file_name, ancestors) = components.split_last().unwrap();
let mut directory = open_skill_root(skill_dir, &mut after_opened_component)?;
let mut opened_path = std::path::PathBuf::new();
for ancestor in ancestors {
directory = open_at(&directory, ancestor, directory_traversal_flags())?;
opened_path.push(ancestor);
after_opened_component(&opened_path);
}
let file = open_at(
&directory,
file_name,
libc::O_RDONLY | libc::O_NONBLOCK | libc::O_NOFOLLOW | libc::O_CLOEXEC,
)?;
if !file.metadata()?.is_file() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"supporting file is not a regular file",
));
}
read_opened_file(file, max_characters)
}
#[cfg(unix)]
fn open_at(
directory: &fs::File,
name: &std::ffi::OsStr,
flags: libc::c_int,
) -> io::Result<fs::File> {
use std::ffi::CString;
use std::os::fd::{AsRawFd, FromRawFd};
use std::os::unix::ffi::OsStrExt;
let name = CString::new(name.as_bytes()).map_err(|_| {
io::Error::new(
io::ErrorKind::InvalidInput,
"supporting file path contains a NUL byte",
)
})?;
// SAFETY: openat does not retain the name pointer, and no creation flag requiring a mode is set.
let descriptor = unsafe { libc::openat(directory.as_raw_fd(), name.as_ptr(), flags) };
if descriptor < 0 {
return Err(io::Error::last_os_error());
}
// SAFETY: openat returned a new owned descriptor on success.
Ok(unsafe { fs::File::from_raw_fd(descriptor) })
}
#[cfg(windows)]
fn open_skill_root(
skill_dir: &Path,
after_opened_component: &mut impl FnMut(&Path),
) -> io::Result<fs::File> {
use std::os::windows::fs::OpenOptionsExt;
use winapi::um::winbase::{FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT};
use winapi::um::winnt::{
FILE_READ_ATTRIBUTES, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, FILE_TRAVERSE,
SYNCHRONIZE,
};
let root_anchor = skill_dir
.ancestors()
.last()
.filter(|path| path.has_root())
.ok_or_else(|| {
io::Error::new(
io::ErrorKind::InvalidInput,
"skill path must be an absolute normalized path",
)
})?;
let relative = skill_dir.strip_prefix(root_anchor).map_err(|_| {
io::Error::new(
io::ErrorKind::InvalidInput,
"skill path must be an absolute normalized path",
)
})?;
let components = if relative.as_os_str().is_empty() {
Vec::new()
} else {
validated_relative_components(relative)?
};
let mut options = fs::OpenOptions::new();
options
.access_mode(FILE_TRAVERSE | FILE_READ_ATTRIBUTES | SYNCHRONIZE)
.share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE)
.custom_flags(FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT);
let mut directory = options.open(root_anchor)?;
let root_metadata = directory.metadata()?;
if windows_metadata_is_reparse_point(&root_metadata) || !root_metadata.is_dir() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"skill path is not a directory",
));
}
let mut opened_path = root_anchor.to_path_buf();
for component in components {
directory = windows_open_at(&directory, component, true)?;
let metadata = directory.metadata()?;
if windows_metadata_is_reparse_point(&metadata) || !metadata.is_dir() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"skill path ancestor is not a regular directory",
));
}
opened_path.push(component);
after_opened_component(&opened_path);
}
if opened_path != skill_dir {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"skill path must be an absolute normalized path",
));
}
Ok(directory)
}
#[cfg(windows)]
fn read_supporting_file_with_hook(
skill_dir: &Path,
relative: &Path,
max_characters: usize,
mut after_opened_component: impl FnMut(&Path),
) -> io::Result<String> {
let components = validated_relative_components(relative)?;
let (file_name, ancestors) = components.split_last().unwrap();
let mut directory = open_skill_root(skill_dir, &mut after_opened_component)?;
let mut opened_path = std::path::PathBuf::new();
for ancestor in ancestors {
directory = windows_open_at(&directory, ancestor, true)?;
let metadata = directory.metadata()?;
if windows_metadata_is_reparse_point(&metadata) || !metadata.is_dir() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"supporting file path ancestor is not a regular directory",
));
}
opened_path.push(ancestor);
after_opened_component(&opened_path);
}
let file = windows_open_at(&directory, file_name, false)?;
let metadata = file.metadata()?;
if windows_metadata_is_reparse_point(&metadata) || !metadata.is_file() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"supporting file is not a regular file",
));
}
read_opened_file(file, max_characters)
}
#[cfg(windows)]
fn windows_open_at(
directory: &fs::File,
name: &std::ffi::OsStr,
directory_only: bool,
) -> io::Result<fs::File> {
use ntapi::ntioapi::{
NtCreateFile, FILE_DIRECTORY_FILE, FILE_OPEN, FILE_OPEN_REPARSE_POINT,
FILE_SYNCHRONOUS_IO_NONALERT, IO_STATUS_BLOCK,
};
use std::os::windows::ffi::OsStrExt;
use std::os::windows::io::{AsRawHandle, FromRawHandle};
use winapi::shared::ntdef::{
HANDLE, NT_SUCCESS, OBJECT_ATTRIBUTES, OBJ_CASE_INSENSITIVE, UNICODE_STRING,
};
use winapi::um::winnt::{
FILE_GENERIC_READ, FILE_READ_ATTRIBUTES, FILE_SHARE_DELETE, FILE_SHARE_READ,
FILE_SHARE_WRITE, FILE_TRAVERSE, SYNCHRONIZE,
};
let mut name: Vec<u16> = name.encode_wide().collect();
let name_bytes = name
.len()
.checked_mul(std::mem::size_of::<u16>())
.and_then(|length| u16::try_from(length).ok())
.ok_or_else(|| {
io::Error::new(
io::ErrorKind::InvalidInput,
"supporting file path component is too long",
)
})?;
let mut unicode_name = UNICODE_STRING {
Length: name_bytes,
MaximumLength: name_bytes,
Buffer: name.as_mut_ptr(),
};
let mut attributes = OBJECT_ATTRIBUTES {
Length: std::mem::size_of::<OBJECT_ATTRIBUTES>() as u32,
RootDirectory: directory.as_raw_handle() as HANDLE,
ObjectName: &mut unicode_name,
Attributes: OBJ_CASE_INSENSITIVE,
SecurityDescriptor: std::ptr::null_mut(),
SecurityQualityOfService: std::ptr::null_mut(),
};
let mut handle: HANDLE = std::ptr::null_mut();
// SAFETY: IO_STATUS_BLOCK is a plain C data structure initialized before the synchronous call.
let mut io_status: IO_STATUS_BLOCK = unsafe { std::mem::zeroed() };
let mut create_options = FILE_OPEN_REPARSE_POINT | FILE_SYNCHRONOUS_IO_NONALERT;
if directory_only {
create_options |= FILE_DIRECTORY_FILE;
}
let desired_access = if directory_only {
FILE_TRAVERSE | FILE_READ_ATTRIBUTES | SYNCHRONIZE
} else {
FILE_GENERIC_READ
};
// SAFETY: all pointers reference initialized values for the duration of the synchronous call.
let status = unsafe {
NtCreateFile(
&mut handle,
desired_access,
&mut attributes,
&mut io_status,
std::ptr::null_mut(),
0,
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
FILE_OPEN,
create_options,
std::ptr::null_mut(),
0,
)
};
if !NT_SUCCESS(status) {
return Err(windows_nt_status_error(status));
}
// SAFETY: NtCreateFile returned a new owned handle on success.
Ok(unsafe { fs::File::from_raw_handle(handle.cast()) })
}
#[cfg(windows)]
fn windows_metadata_is_reparse_point(metadata: &fs::Metadata) -> bool {
use std::os::windows::fs::MetadataExt;
use winapi::um::winnt::FILE_ATTRIBUTE_REPARSE_POINT;
metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0
}
#[cfg(windows)]
fn windows_nt_status_error(status: winapi::shared::ntdef::NTSTATUS) -> io::Error {
// SAFETY: RtlNtStatusToDosError accepts every NTSTATUS value.
let error = unsafe { ntapi::ntrtl::RtlNtStatusToDosError(status) };
io::Error::from_raw_os_error(error as i32)
}
#[cfg(not(any(unix, windows)))]
fn read_supporting_file_with_hook(
_skill_dir: &Path,
relative: &Path,
_max_characters: usize,
_after_opened_component: impl FnMut(&Path),
) -> io::Result<String> {
validated_relative_components(relative)?;
Err(io::Error::new(
io::ErrorKind::Unsupported,
"secure supporting file reads are not supported on this platform",
))
}
#[cfg(test)]
mod tests {
use super::*;
#[cfg(any(unix, windows))]
#[test]
fn reads_nested_regular_file() {
let root = tempfile::tempdir().unwrap();
let skill_dir = fs::canonicalize(root.path()).unwrap();
let nested = skill_dir.join("nested");
fs::create_dir(&nested).unwrap();
fs::write(nested.join("guide.md"), "nested guidance").unwrap();
let content = read_supporting_file_with_limit(
&skill_dir,
Path::new("nested/guide.md"),
crate::agents::max_tool_response_size(),
)
.unwrap();
assert_eq!(content, "nested guidance");
}
#[cfg(all(
unix,
any(
target_vendor = "apple",
target_os = "aix",
target_os = "android",
target_os = "freebsd",
target_os = "illumos",
target_os = "linux",
target_os = "netbsd",
target_os = "solaris"
)
))]
#[test]
fn reads_through_search_only_ancestor() {
use std::os::unix::fs::PermissionsExt;
let root = tempfile::tempdir().unwrap();
let skill_dir = root.path().join("skill");
fs::create_dir(&skill_dir).unwrap();
fs::write(skill_dir.join("guide.md"), "search-only guidance").unwrap();
let skill_dir = fs::canonicalize(skill_dir).unwrap();
let original_permissions = fs::metadata(root.path()).unwrap().permissions();
fs::set_permissions(root.path(), fs::Permissions::from_mode(0o111)).unwrap();
let result = read_supporting_file_with_limit(
&skill_dir,
Path::new("guide.md"),
crate::agents::max_tool_response_size(),
);
fs::set_permissions(root.path(), original_permissions).unwrap();
assert_eq!(result.unwrap(), "search-only guidance");
}
#[cfg(any(unix, windows))]
#[test]
fn reads_utf8_file_at_exact_character_limit() {
let root = tempfile::tempdir().unwrap();
let skill_dir = fs::canonicalize(root.path()).unwrap();
fs::write(skill_dir.join("guide.md"), "🙂🙂🙂🙂").unwrap();
let content =
read_supporting_file_with_limit(&skill_dir, Path::new("guide.md"), 4).unwrap();
assert_eq!(content, "🙂🙂🙂🙂");
}
#[cfg(any(unix, windows))]
#[test]
fn wrapped_file_respects_total_character_limit() {
let root = tempfile::tempdir().unwrap();
let skill_dir = fs::canonicalize(root.path()).unwrap();
fs::write(skill_dir.join("guide.md"), "🙂🙂🙂🙂").unwrap();
let skill_name = "test-skill/guide.md";
let wrapper_characters = LOADED_FILE_PREFIX.chars().count()
+ skill_name.chars().count()
+ LOADED_FILE_SEPARATOR.chars().count()
+ LOADED_FILE_SUFFIX.chars().count();
let max_characters = wrapper_characters + 4;
let content = load_supporting_file_with_limit(
&skill_dir,
Path::new("guide.md"),
skill_name,
max_characters,
)
.unwrap();
assert_eq!(content.chars().count(), max_characters);
assert!(content.contains("🙂🙂🙂🙂"));
}
#[cfg(any(unix, windows))]
#[test]
fn rejects_file_that_exceeds_wrapped_character_limit() {
let root = tempfile::tempdir().unwrap();
let skill_dir = fs::canonicalize(root.path()).unwrap();
fs::write(skill_dir.join("guide.md"), "ééééé").unwrap();
let skill_name = "test-skill/guide.md";
let wrapper_characters = LOADED_FILE_PREFIX.chars().count()
+ skill_name.chars().count()
+ LOADED_FILE_SEPARATOR.chars().count()
+ LOADED_FILE_SUFFIX.chars().count();
let error = load_supporting_file_with_limit(
&skill_dir,
Path::new("guide.md"),
skill_name,
wrapper_characters + 4,
)
.expect_err("wrapped supporting-file limit was not enforced");
assert_eq!(error.kind(), io::ErrorKind::InvalidData);
assert!(error
.to_string()
.contains("exceeds the maximum size of 4 characters"));
}
#[cfg(any(unix, windows))]
#[test]
fn rejects_file_one_character_over_size_limit() {
let root = tempfile::tempdir().unwrap();
let skill_dir = fs::canonicalize(root.path()).unwrap();
fs::write(skill_dir.join("guide.md"), "ééééé").unwrap();
let error = read_supporting_file_with_limit(&skill_dir, Path::new("guide.md"), 4)
.expect_err("oversized supporting file was accepted");
assert_eq!(error.kind(), io::ErrorKind::InvalidData);
assert!(error
.to_string()
.contains("exceeds the maximum size of 4 characters"));
}
#[test]
fn streaming_limit_reads_only_limit_plus_one() {
use std::cell::Cell;
use std::rc::Rc;
struct CountingReader(Rc<Cell<usize>>);
impl io::Read for CountingReader {
fn read(&mut self, buffer: &mut [u8]) -> io::Result<usize> {
buffer.fill(b'a');
self.0.set(self.0.get() + buffer.len());
Ok(buffer.len())
}
}
let bytes_read = Rc::new(Cell::new(0));
let error = read_utf8_with_limit(CountingReader(Rc::clone(&bytes_read)), 4)
.expect_err("streaming size limit was not enforced");
assert_eq!(error.kind(), io::ErrorKind::InvalidData);
assert!(error
.to_string()
.contains("exceeds the maximum encoded size of 16 bytes"));
assert_eq!(bytes_read.get(), 17);
}
#[cfg(unix)]
#[test]
fn rejects_symlinked_ancestor() {
let root = tempfile::tempdir().unwrap();
let outside = tempfile::tempdir().unwrap();
let skill_dir = fs::canonicalize(root.path()).unwrap();
fs::write(outside.path().join("secret.txt"), "outside secret").unwrap();
std::os::unix::fs::symlink(outside.path(), skill_dir.join("nested")).unwrap();
let result = read_supporting_file_with_limit(
&skill_dir,
Path::new("nested/secret.txt"),
crate::agents::max_tool_response_size(),
);
assert!(result.is_err());
}
#[cfg(unix)]
#[test]
fn stays_in_opened_ancestor_after_symlink_swap() {
let root = tempfile::tempdir().unwrap();
let outside = tempfile::tempdir().unwrap();
let skill_dir = fs::canonicalize(root.path()).unwrap();
let nested = skill_dir.join("nested");
let moved_nested = skill_dir.join("moved-nested");
fs::create_dir(&nested).unwrap();
fs::write(nested.join("payload"), "safe content").unwrap();
fs::write(outside.path().join("payload"), "outside secret").unwrap();
let content = read_supporting_file_with_hook(
&skill_dir,
Path::new("nested/payload"),
crate::agents::max_tool_response_size(),
|opened_path| {
if opened_path == Path::new("nested") {
fs::rename(&nested, &moved_nested).unwrap();
std::os::unix::fs::symlink(outside.path(), &nested).unwrap();
}
},
)
.unwrap();
assert_eq!(content, "safe content");
assert!(!content.contains("outside secret"));
}
#[cfg(unix)]
#[test]
fn rejects_skill_root_replaced_with_symlink_during_open() {
let parent = tempfile::tempdir().unwrap();
let outside = tempfile::tempdir().unwrap();
let parent = fs::canonicalize(parent.path()).unwrap();
let skill_dir = parent.join("skill");
let moved_skill_dir = parent.join("moved-skill");
fs::create_dir(&skill_dir).unwrap();
fs::write(skill_dir.join("payload"), "safe content").unwrap();
fs::write(outside.path().join("payload"), "outside secret").unwrap();
let result = read_supporting_file_with_hook(
&skill_dir,
Path::new("payload"),
crate::agents::max_tool_response_size(),
|opened_path| {
if opened_path == parent {
fs::rename(&skill_dir, &moved_skill_dir).unwrap();
std::os::unix::fs::symlink(outside.path(), &skill_dir).unwrap();
}
},
);
assert!(result.is_err());
}
#[cfg(windows)]
#[test]
fn windows_stays_in_opened_ancestor_after_directory_swap() {
let root = tempfile::tempdir().unwrap();
let skill_dir = fs::canonicalize(root.path()).unwrap();
let nested = skill_dir.join("nested");
let moved_nested = skill_dir.join("moved-nested");
fs::create_dir(&nested).unwrap();
fs::write(nested.join("payload"), "safe content").unwrap();
let content = read_supporting_file_with_hook(
&skill_dir,
Path::new("nested/payload"),
crate::agents::max_tool_response_size(),
|opened_path| {
if opened_path == Path::new("nested") {
fs::rename(&nested, &moved_nested).unwrap();
fs::create_dir(&nested).unwrap();
fs::write(nested.join("payload"), "outside secret").unwrap();
}
},
)
.unwrap();
assert_eq!(content, "safe content");
assert!(!content.contains("outside secret"));
}
#[cfg(windows)]
#[test]
fn windows_rejects_skill_root_replaced_with_symlink_during_open() {
let parent = tempfile::tempdir().unwrap();
let outside = tempfile::tempdir().unwrap();
let parent = fs::canonicalize(parent.path()).unwrap();
let skill_dir = parent.join("skill");
let moved_skill_dir = parent.join("moved-skill");
let replacement = parent.join("replacement");
fs::create_dir(&skill_dir).unwrap();
fs::write(skill_dir.join("payload"), "safe content").unwrap();
fs::write(outside.path().join("payload"), "outside secret").unwrap();
if std::os::windows::fs::symlink_dir(outside.path(), &replacement).is_err() {
return;
}
let result = read_supporting_file_with_hook(
&skill_dir,
Path::new("payload"),
crate::agents::max_tool_response_size(),
|opened_path| {
if opened_path == parent {
fs::rename(&skill_dir, &moved_skill_dir).unwrap();
fs::rename(&replacement, &skill_dir).unwrap();
}
},
);
assert!(result.is_err());
}
}
crates/goose/src/sources.rs
+26 -7
View File
@@ -74,6 +74,25 @@ fn project_file_path(slug: &str) -> PathBuf {
projects_dir().join(format!("{slug}.md"))
}
fn read_source_path(path: &Path) -> std::io::Result<String> {
let parent = path
.parent()
.filter(|parent| !parent.as_os_str().is_empty())
.unwrap_or_else(|| Path::new("."))
.canonicalize()?;
let file_name = path.file_name().ok_or_else(|| {
std::io::Error::new(
std::io::ErrorKind::InvalidInput,
"source path has no file name",
)
})?;
crate::skills::read_source_file_with_limit(
&parent,
Path::new(file_name),
crate::agents::max_tool_response_size(),
)
}
fn build_source_markdown(
name: &str,
description: &str,
@@ -140,7 +159,7 @@ fn validate_project_slug(slug: &str) -> Result<(), Error> {
/// Read the `metadata:` field out of an existing SKILL.md, returning an
/// empty map if the file is missing, malformed, or carries no metadata.
fn read_existing_skill_properties(skill_dir: &Path) -> HashMap<String, serde_json::Value> {
let raw = match fs::read_to_string(skill_dir.join("SKILL.md")) {
let raw = match read_source_path(&skill_dir.join("SKILL.md")) {
Ok(s) => s,
Err(_) => return HashMap::new(),
};
@@ -152,7 +171,7 @@ fn read_existing_skill_properties(skill_dir: &Path) -> HashMap<String, serde_jso
/// Read the properties bag out of an existing project file.
fn read_existing_project_properties(file: &Path) -> HashMap<String, serde_json::Value> {
let raw = match fs::read_to_string(file) {
let raw = match read_source_path(file) {
Ok(s) => s,
Err(_) => return HashMap::new(),
};
@@ -162,7 +181,7 @@ fn read_existing_project_properties(file: &Path) -> HashMap<String, serde_json::
/// Read the properties bag out of an existing agent file.
fn read_existing_agent_properties(file: &Path) -> HashMap<String, serde_json::Value> {
let raw = match fs::read_to_string(file) {
let raw = match read_source_path(file) {
Ok(s) => s,
Err(_) => return HashMap::new(),
};
@@ -177,7 +196,7 @@ fn project_entry_from_file(file: &Path) -> Option<SourceEntry> {
if slug.is_empty() {
return None;
}
let raw = fs::read_to_string(file).ok()?;
let raw = read_source_path(file).ok()?;
let (title, description, content, mut properties) = parse_project_frontmatter(&raw);
let display_name = if title.is_empty() {
slug.clone()
@@ -384,7 +403,7 @@ fn parse_agent_frontmatter(raw: &str) -> Result<(MarkdownSourceFrontmatter, Stri
}
fn agent_source_entry(path: &Path, global: bool, writable: bool) -> Result<SourceEntry, Error> {
let raw = fs::read_to_string(path)
let raw = read_source_path(path)
.map_err(|e| Error::internal_error().data(format!("Failed to read agent file: {e}")))?;
let (frontmatter, content) = parse_agent_frontmatter(&raw)?;
Ok({
@@ -988,7 +1007,7 @@ pub fn export_source_with_roots(
let dir = resolve_discoverable_skill_dir(path)?;
let md = dir.join("SKILL.md");
let raw = fs::read_to_string(&md).map_err(|e| {
let raw = read_source_path(&md).map_err(|e| {
Error::internal_error().data(format!("Failed to read SKILL.md: {e}"))
})?;
let (description, content) = parse_skill_frontmatter(&raw);
@@ -1031,7 +1050,7 @@ pub fn export_source_with_roots(
}
SourceType::Project => {
let file = resolve_project_path(path)?;
let raw = fs::read_to_string(&file).map_err(|e| {
let raw = read_source_path(&file).map_err(|e| {
Error::internal_error().data(format!("Failed to read project file: {e}"))
})?;
let (title, description, content, properties) = parse_project_frontmatter(&raw);