mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-09-02 09:39:37 +00:00
fix: security hardening across all layers (#35)
* fix: comprehensive security hardening across all layers Critical: - Replace --dangerously-skip-permissions with explicit tool allowlist via settings.json (Claude Code bridge) - Constant-time token comparison (subtle crate) in web auth and orchestrator auth to prevent timing attacks High: - Revoke tokens and clean up handles on container creation failure - Drop SETUID/SETGID capabilities from containers (keep only CHOWN) - Disable redirect following in HTTP tool and WASM wrapper (SSRF) - Reject URL userinfo (@) in WASM allowlist parser (host confusion) - Fix binary body bypassing leak detection (from_utf8 -> from_utf8_lossy) - Protect identity files from LLM overwrites (prompt injection defense) - Prevent tool shadowing: built-in tools cannot be replaced dynamically - User-scoped job APIs: list/detail/cancel/restart/prompt/events/files - CORS restricted to localhost origins, WebSocket origin validation - Sandbox shell fail-closed: no silent fallback to unsandboxed execution - Scrub secrets from log broadcaster before SSE broadcast - XSS sanitization on rendered markdown in web UI - WASM epoch ticker thread so timeout deadlines actually fire Medium: - Cap state transition history at 200 entries - SSE/WebSocket connection limit (100 max) - Request body size limit (1MB) - Response body size limit enforcement in WASM HTTP - UTF-8 safe string truncation (routine engine, shell tool) - Fix PolicyAction::Sanitize to actually run the sanitizer - TOCTOU fix in scheduler and context manager (hold write lock) - Project file serving moved behind auth - Path traversal guard on project_id - Session file permissions set to 0600 on unix - AtomicUsize for routine running_count (panic-safe) - Completion detection hardened against false positives and tool injection - Tool output no longer drives job completion (only LLM response) Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address security review findings across all layers - Fix path traversal sandbox bypass via lexical normalization (file.rs) - Fix SSRF via DNS rebinding with pre-request hostname resolution (http.rs) - Add token budget enforcement on LLM calls (reasoning.rs, state.rs) - Fix cross-user chat history leak with ownership verification (store.rs, server.rs) - Add sliding-window rate limiter on gateway chat endpoint (server.rs) - Harden extension install: HTTPS-only, 50MB cap, WASM magic validation (manager.rs) - Add destructive command blocklist that overrides shell auto-approval (shell.rs) - Add 5MB response body size cap to HTTP tool (http.rs) Co-Authored-By: Claude Opus 4.6 <[email protected]> * refactor: deduplicate shared helpers and remove dead code Extract floor_char_boundary and llm_signals_completion into src/util.rs, unifying diverging phrase lists from agent/worker.rs and worker/runtime.rs. Remove dead RespondResult::usage(), duplicate PROTECTED_IDENTITY_FILES constant, double LeakDetector scanning in WebLogLayer, and invalid 0.0.0.0 origin from WebSocket allow list. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address PR review findings and CI test failures - Fix record_failed_approve: .truncate(true) wiped the attempts file before reading, so failed pairing attempts never accumulated and rate limiting never triggered. - Guard wizard WASM test: skip gracefully when channel build artifacts are absent (CI doesn't compile wasm32-wasip2 targets). - Fix DNS rebinding check: use port 0 instead of hardcoded 443, since the port is irrelevant for hostname resolution. - Remove hardcoded CORS port 3001: the dynamic addr.port() entries already cover the actual server port. - Require WebSocket Origin header: reject connections that omit it entirely, since browsers always send Origin for WS upgrades and a missing header indicates a non-browser client bypassing the check. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address second round of PR review findings - store.rs: reintroduce file locking around read-modify-write in record_failed_approve (concurrent callers could clobber each other). - sse.rs: replace load+check+fetch_add with atomic fetch_update in both subscribe_raw() and subscribe() to prevent overshooting max_connections. - ws.rs: decrement WS tracker before early return when subscribe_raw() returns None (connection limit reached), fixing a counter leak. - server.rs: parse WS Origin host exactly instead of prefix matching, preventing bypass via crafted origins like http://localhost.evil.com. - workspace_integration.rs: skip tests gracefully when Postgres is unreachable instead of panicking (fixes 10 CI failures). Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: add Origin header to WS integration tests The Origin header requirement added in a3b0190 broke the WS gateway integration tests. Test clients now send Origin: http://127.0.0.1:{port} to match the server's localhost validation. Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
This commit is contained in:
Illia Polosukhin
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
e0a43c81f9
commit
33ef0a6ea5
+106
-12
@@ -4,18 +4,26 @@
|
||||
//! output back to the orchestrator via HTTP. Supports follow-up prompts via
|
||||
//! `--resume`.
|
||||
//!
|
||||
//! Security model: the Docker container is the primary security boundary
|
||||
//! (cap-drop ALL, non-root user, memory limits, network isolation).
|
||||
//! As defense-in-depth, a project-level `.claude/settings.json` is written
|
||||
//! before spawning with an explicit tool allowlist. Only listed tools are
|
||||
//! auto-approved; unknown/future tools would require interactive approval,
|
||||
//! which times out harmlessly in the non-interactive container.
|
||||
//!
|
||||
//! ```text
|
||||
//! ┌─────────────────────────────────────────────┐
|
||||
//! │ Docker Container │
|
||||
//! │ │
|
||||
//! │ ironclaw claude-bridge --job-id <uuid> │
|
||||
//! ┌──────────────────────────────────────────────┐
|
||||
//! │ Docker Container │
|
||||
//! │ │
|
||||
//! │ ironclaw claude-bridge --job-id <uuid> │
|
||||
//! │ └─ writes /workspace/.claude/settings.json │
|
||||
//! │ └─ claude -p "task" --output-format │
|
||||
//! │ stream-json --dangerously-skip-perms │
|
||||
//! │ └─ reads stdout line-by-line │
|
||||
//! │ └─ POSTs events to orchestrator │
|
||||
//! │ └─ polls for follow-up prompts │
|
||||
//! │ └─ on follow-up: claude --resume │
|
||||
//! └─────────────────────────────────────────────┘
|
||||
//! │ stream-json │
|
||||
//! │ └─ reads stdout line-by-line │
|
||||
//! │ └─ POSTs events to orchestrator │
|
||||
//! │ └─ polls for follow-up prompts │
|
||||
//! │ └─ on follow-up: claude --resume │
|
||||
//! └──────────────────────────────────────────────┘
|
||||
//! ```
|
||||
|
||||
use std::sync::Arc;
|
||||
@@ -36,6 +44,8 @@ pub struct ClaudeBridgeConfig {
|
||||
pub max_turns: u32,
|
||||
pub model: String,
|
||||
pub timeout: Duration,
|
||||
/// Tool patterns to auto-approve via project-level settings.json.
|
||||
pub allowed_tools: Vec<String>,
|
||||
}
|
||||
|
||||
/// A Claude Code streaming event (NDJSON line from `--output-format stream-json`).
|
||||
@@ -119,8 +129,37 @@ impl ClaudeBridgeRuntime {
|
||||
Ok(Self { config, client })
|
||||
}
|
||||
|
||||
/// Write project-level `.claude/settings.json` with the tool allowlist.
|
||||
///
|
||||
/// This replaces `--dangerously-skip-permissions` with an explicit set of
|
||||
/// auto-approved tools. The Docker container is still the primary security
|
||||
/// boundary; this is defense-in-depth.
|
||||
fn write_permission_settings(&self) -> Result<(), WorkerError> {
|
||||
let settings_json = build_permission_settings(&self.config.allowed_tools);
|
||||
let settings_dir = std::path::Path::new("/workspace/.claude");
|
||||
std::fs::create_dir_all(settings_dir).map_err(|e| WorkerError::ExecutionFailed {
|
||||
reason: format!("failed to create /workspace/.claude/: {e}"),
|
||||
})?;
|
||||
std::fs::write(settings_dir.join("settings.json"), &settings_json).map_err(|e| {
|
||||
WorkerError::ExecutionFailed {
|
||||
reason: format!("failed to write settings.json: {e}"),
|
||||
}
|
||||
})?;
|
||||
tracing::info!(
|
||||
job_id = %self.config.job_id,
|
||||
tools = ?self.config.allowed_tools,
|
||||
"Wrote Claude Code permission settings"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Run the bridge: fetch job, spawn claude, stream events, handle follow-ups.
|
||||
pub async fn run(&self) -> Result<(), WorkerError> {
|
||||
// Write project-level settings with explicit tool allowlist.
|
||||
// This replaces --dangerously-skip-permissions with defense-in-depth:
|
||||
// only the listed tools are auto-approved, unknown tools fail safely.
|
||||
self.write_permission_settings()?;
|
||||
|
||||
// Fetch the job description from the orchestrator
|
||||
let job = self.client.get_job().await?;
|
||||
|
||||
@@ -226,7 +265,6 @@ impl ClaudeBridgeRuntime {
|
||||
.arg(prompt)
|
||||
.arg("--output-format")
|
||||
.arg("stream-json")
|
||||
.arg("--dangerously-skip-permissions")
|
||||
.arg("--max-turns")
|
||||
.arg(self.config.max_turns.to_string())
|
||||
.arg("--model")
|
||||
@@ -380,6 +418,19 @@ impl ClaudeBridgeRuntime {
|
||||
}
|
||||
}
|
||||
|
||||
/// Build the JSON content for `.claude/settings.json` with the given tool allowlist.
|
||||
///
|
||||
/// Produces a Claude Code project settings file that auto-approves the listed
|
||||
/// tools while leaving any unknown/future tools unapproved (defense-in-depth).
|
||||
fn build_permission_settings(allowed_tools: &[String]) -> String {
|
||||
let settings = serde_json::json!({
|
||||
"permissions": {
|
||||
"allow": allowed_tools,
|
||||
}
|
||||
});
|
||||
serde_json::to_string_pretty(&settings).expect("static JSON structure is always valid")
|
||||
}
|
||||
|
||||
/// Convert a Claude stream event into one or more event payloads for the orchestrator.
|
||||
fn stream_event_to_payloads(event: &ClaudeStreamEvent) -> Vec<JobEventPayload> {
|
||||
let mut payloads = Vec::new();
|
||||
@@ -465,7 +516,16 @@ fn stream_event_to_payloads(event: &ClaudeStreamEvent) -> Vec<JobEventPayload> {
|
||||
}
|
||||
|
||||
fn truncate(s: &str, max_len: usize) -> &str {
|
||||
if s.len() <= max_len { s } else { &s[..max_len] }
|
||||
if s.len() <= max_len {
|
||||
s
|
||||
} else {
|
||||
// Walk back from max_len to find a valid UTF-8 char boundary.
|
||||
let mut end = max_len;
|
||||
while end > 0 && !s.is_char_boundary(end) {
|
||||
end -= 1;
|
||||
}
|
||||
&s[..end]
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -641,4 +701,38 @@ mod tests {
|
||||
assert_eq!(truncate("hello world", 5), "hello");
|
||||
assert_eq!(truncate("", 5), "");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_build_permission_settings_default_tools() {
|
||||
let tools: Vec<String> = ["Bash(*)", "Read", "Edit(*)", "Glob", "Grep"]
|
||||
.into_iter()
|
||||
.map(String::from)
|
||||
.collect();
|
||||
let json_str = build_permission_settings(&tools);
|
||||
let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap();
|
||||
let allow = parsed["permissions"]["allow"].as_array().unwrap();
|
||||
assert_eq!(allow.len(), 5);
|
||||
assert_eq!(allow[0], "Bash(*)");
|
||||
assert_eq!(allow[1], "Read");
|
||||
assert_eq!(allow[2], "Edit(*)");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_build_permission_settings_empty_tools() {
|
||||
let json_str = build_permission_settings(&[]);
|
||||
let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap();
|
||||
let allow = parsed["permissions"]["allow"].as_array().unwrap();
|
||||
assert!(allow.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_build_permission_settings_is_valid_json() {
|
||||
let tools = vec!["Bash(npm run *)".to_string(), "Read".to_string()];
|
||||
let json_str = build_permission_settings(&tools);
|
||||
// Must be valid JSON
|
||||
let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap();
|
||||
// Must have the expected structure
|
||||
assert!(parsed["permissions"].is_object());
|
||||
assert!(parsed["permissions"]["allow"].is_array());
|
||||
}
|
||||
}
|
||||
|
||||
+38
-8
@@ -238,7 +238,7 @@ Work independently to complete this job. Report when done."#,
|
||||
reason: format!("respond_with_tools failed: {}", e),
|
||||
})?;
|
||||
|
||||
match respond_result {
|
||||
match respond_result.result {
|
||||
RespondResult::Text(response) => {
|
||||
self.post_event(
|
||||
"message",
|
||||
@@ -249,11 +249,7 @@ Work independently to complete this job. Report when done."#,
|
||||
)
|
||||
.await;
|
||||
|
||||
let response_lower = response.to_lowercase();
|
||||
if response_lower.contains("complete")
|
||||
|| response_lower.contains("finished")
|
||||
|| response_lower.contains("done")
|
||||
{
|
||||
if crate::util::llm_signals_completion(&response) {
|
||||
if last_output.is_empty() {
|
||||
last_output = response.clone();
|
||||
}
|
||||
@@ -431,7 +427,11 @@ Work independently to complete this job. Report when done."#,
|
||||
wrapped,
|
||||
));
|
||||
|
||||
output.contains("TASK_COMPLETE") || output.contains("JOB_DONE")
|
||||
// Tool output should never signal job completion. Only the LLM's
|
||||
// natural language response should decide when a job is done. A
|
||||
// tool could return text containing "TASK_COMPLETE" in its output
|
||||
// (e.g. from file contents) and trigger a false positive.
|
||||
false
|
||||
}
|
||||
Err(e) => {
|
||||
tracing::warn!("Tool {} failed: {}", selection.tool_name, e);
|
||||
@@ -486,6 +486,36 @@ fn truncate(s: &str, max: usize) -> String {
|
||||
if s.len() <= max {
|
||||
s.to_string()
|
||||
} else {
|
||||
format!("{}...", &s[..max])
|
||||
let end = crate::util::floor_char_boundary(s, max);
|
||||
format!("{}...", &s[..end])
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use crate::worker::runtime::truncate;
|
||||
|
||||
#[test]
|
||||
fn test_truncate_within_limit() {
|
||||
assert_eq!(truncate("hello", 10), "hello");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_truncate_at_limit() {
|
||||
assert_eq!(truncate("hello", 5), "hello");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_truncate_beyond_limit() {
|
||||
let result = truncate("hello world", 5);
|
||||
assert_eq!(result, "hello...");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_truncate_multibyte_safe() {
|
||||
// "é" is 2 bytes in UTF-8; slicing at byte 1 would panic without safety
|
||||
let result = truncate("é is fancy", 1);
|
||||
// Should truncate to 0 chars (can't fit "é" in 1 byte)
|
||||
assert_eq!(result, "...");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user