fix(security): enforce action budget checks in file_read (#270)
This commit is contained in:
parent
2c0664ba1e
commit
60f3282ad4
1 changed files with 77 additions and 3 deletions
|
|
@ -4,6 +4,8 @@ use async_trait::async_trait;
|
||||||
use serde_json::json;
|
use serde_json::json;
|
||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
|
|
||||||
|
const MAX_FILE_SIZE_BYTES: u64 = 10 * 1024 * 1024;
|
||||||
|
|
||||||
/// Read file contents with path sandboxing
|
/// Read file contents with path sandboxing
|
||||||
pub struct FileReadTool {
|
pub struct FileReadTool {
|
||||||
security: Arc<SecurityPolicy>,
|
security: Arc<SecurityPolicy>,
|
||||||
|
|
@ -44,6 +46,14 @@ impl Tool for FileReadTool {
|
||||||
.and_then(|v| v.as_str())
|
.and_then(|v| v.as_str())
|
||||||
.ok_or_else(|| anyhow::anyhow!("Missing 'path' parameter"))?;
|
.ok_or_else(|| anyhow::anyhow!("Missing 'path' parameter"))?;
|
||||||
|
|
||||||
|
if self.security.is_rate_limited() {
|
||||||
|
return Ok(ToolResult {
|
||||||
|
success: false,
|
||||||
|
output: String::new(),
|
||||||
|
error: Some("Rate limit exceeded: too many actions in the last hour".into()),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
// Security check: validate path is within workspace
|
// Security check: validate path is within workspace
|
||||||
if !self.security.is_path_allowed(path) {
|
if !self.security.is_path_allowed(path) {
|
||||||
return Ok(ToolResult {
|
return Ok(ToolResult {
|
||||||
|
|
@ -79,15 +89,14 @@ impl Tool for FileReadTool {
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check file size AFTER canonicalization to prevent TOCTOU symlink bypass
|
// Check file size AFTER canonicalization to prevent TOCTOU symlink bypass
|
||||||
const MAX_FILE_SIZE: u64 = 10 * 1024 * 1024;
|
|
||||||
match tokio::fs::metadata(&resolved_path).await {
|
match tokio::fs::metadata(&resolved_path).await {
|
||||||
Ok(meta) => {
|
Ok(meta) => {
|
||||||
if meta.len() > MAX_FILE_SIZE {
|
if meta.len() > MAX_FILE_SIZE_BYTES {
|
||||||
return Ok(ToolResult {
|
return Ok(ToolResult {
|
||||||
success: false,
|
success: false,
|
||||||
output: String::new(),
|
output: String::new(),
|
||||||
error: Some(format!(
|
error: Some(format!(
|
||||||
"File too large: {} bytes (limit: {MAX_FILE_SIZE} bytes)",
|
"File too large: {} bytes (limit: {MAX_FILE_SIZE_BYTES} bytes)",
|
||||||
meta.len()
|
meta.len()
|
||||||
)),
|
)),
|
||||||
});
|
});
|
||||||
|
|
@ -102,6 +111,14 @@ impl Tool for FileReadTool {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if !self.security.record_action() {
|
||||||
|
return Ok(ToolResult {
|
||||||
|
success: false,
|
||||||
|
output: String::new(),
|
||||||
|
error: Some("Rate limit exceeded: action budget exhausted".into()),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
match tokio::fs::read_to_string(&resolved_path).await {
|
match tokio::fs::read_to_string(&resolved_path).await {
|
||||||
Ok(contents) => Ok(ToolResult {
|
Ok(contents) => Ok(ToolResult {
|
||||||
success: true,
|
success: true,
|
||||||
|
|
@ -130,6 +147,19 @@ mod tests {
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn test_security_with(
|
||||||
|
workspace: std::path::PathBuf,
|
||||||
|
autonomy: AutonomyLevel,
|
||||||
|
max_actions_per_hour: u32,
|
||||||
|
) -> Arc<SecurityPolicy> {
|
||||||
|
Arc::new(SecurityPolicy {
|
||||||
|
autonomy,
|
||||||
|
workspace_dir: workspace,
|
||||||
|
max_actions_per_hour,
|
||||||
|
..SecurityPolicy::default()
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn file_read_name() {
|
fn file_read_name() {
|
||||||
let tool = FileReadTool::new(test_security(std::env::temp_dir()));
|
let tool = FileReadTool::new(test_security(std::env::temp_dir()));
|
||||||
|
|
@ -204,6 +234,50 @@ mod tests {
|
||||||
assert!(result.error.as_ref().unwrap().contains("not allowed"));
|
assert!(result.error.as_ref().unwrap().contains("not allowed"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn file_read_blocks_when_rate_limited() {
|
||||||
|
let dir = std::env::temp_dir().join("zeroclaw_test_file_read_rate_limited");
|
||||||
|
let _ = tokio::fs::remove_dir_all(&dir).await;
|
||||||
|
tokio::fs::create_dir_all(&dir).await.unwrap();
|
||||||
|
tokio::fs::write(dir.join("test.txt"), "hello world")
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
let tool = FileReadTool::new(test_security_with(
|
||||||
|
dir.clone(),
|
||||||
|
AutonomyLevel::Supervised,
|
||||||
|
0,
|
||||||
|
));
|
||||||
|
let result = tool.execute(json!({"path": "test.txt"})).await.unwrap();
|
||||||
|
|
||||||
|
assert!(!result.success);
|
||||||
|
assert!(result
|
||||||
|
.error
|
||||||
|
.as_deref()
|
||||||
|
.unwrap_or("")
|
||||||
|
.contains("Rate limit exceeded"));
|
||||||
|
|
||||||
|
let _ = tokio::fs::remove_dir_all(&dir).await;
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn file_read_allows_readonly_mode() {
|
||||||
|
let dir = std::env::temp_dir().join("zeroclaw_test_file_read_readonly");
|
||||||
|
let _ = tokio::fs::remove_dir_all(&dir).await;
|
||||||
|
tokio::fs::create_dir_all(&dir).await.unwrap();
|
||||||
|
tokio::fs::write(dir.join("test.txt"), "readonly ok")
|
||||||
|
.await
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
let tool = FileReadTool::new(test_security_with(dir.clone(), AutonomyLevel::ReadOnly, 20));
|
||||||
|
let result = tool.execute(json!({"path": "test.txt"})).await.unwrap();
|
||||||
|
|
||||||
|
assert!(result.success);
|
||||||
|
assert_eq!(result.output, "readonly ok");
|
||||||
|
|
||||||
|
let _ = tokio::fs::remove_dir_all(&dir).await;
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn file_read_missing_path_param() {
|
async fn file_read_missing_path_param() {
|
||||||
let tool = FileReadTool::new(test_security(std::env::temp_dir()));
|
let tool = FileReadTool::new(test_security(std::env::temp_dir()));
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue