fix: security hardening across all layers

[ilblackdragon]

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)

Removing review

[Copilot]

Pull request overview

This is a comprehensive security hardening PR that addresses multiple security vulnerabilities and implements defense-in-depth measures across the entire application stack. The PR focuses on preventing privilege escalation, SSRF attacks, path traversal, prompt injection, timing attacks, and resource exhaustion.

Changes:

• Replaces unsafe permissions model in Claude Code bridge with explicit tool allowlist via settings.json
• Implements constant-time token comparison to prevent timing attacks in authentication
• Adds comprehensive input validation and sanitization (URL userinfo rejection, private IP detection, path traversal fixes)
• Enforces resource limits (connection limits, response body size caps, transition history caps, request body size limits)
• Hardens completion detection to prevent tool output injection and false positives
• Implements user-scoped job APIs with authorization checks throughout the web gateway
• Adds XSS sanitization for rendered markdown in the web UI
• Removes dangerous container capabilities (SETUID/SETGID) and disables HTTP redirects

Reviewed changes

Copilot reviewed 40 out of 41 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/worker/claude_bridge.rs	Replaces --dangerously-skip-permissions with explicit tool allowlist via settings.json
src/worker/runtime.rs	Hardens completion detection with phrase-level matching and UTF-8 safe truncation
src/tools/wasm/wrapper.rs	Adds DNS rebinding protection, response size limits, and redirect blocking for WASM HTTP
src/tools/wasm/runtime.rs	Implements WASM epoch ticker thread for timeout enforcement
src/tools/wasm/allowlist.rs	Rejects URLs with userinfo to prevent allowlist bypass
src/tools/registry.rs	Prevents tool shadowing by protecting built-in tool names
src/tools/builtin/shell.rs	Adds explicit approval requirement for destructive commands and fail-closed sandbox execution
src/tools/builtin/memory.rs	Protects identity files from LLM overwrites (prompt injection defense)
src/tools/builtin/http.rs	Adds DNS rebinding protection, response size limits, and redirect blocking
src/tools/builtin/file.rs	Implements lexical path normalization to fix TOCTOU path traversal vulnerability
src/tools/builder/core.rs	Updates to use RespondOutput for token tracking
src/sandbox/container.rs	Drops SETUID/SETGID capabilities, keeps only CHOWN
src/safety/mod.rs	Fixes PolicyAction::Sanitize to actually run the sanitizer
src/safety/leak_detector.rs	Uses from_utf8_lossy to prevent binary body bypass of leak detection
src/orchestrator/job_manager.rs	Revokes tokens and cleans up handles on container creation failure
src/orchestrator/auth.rs	Implements constant-time token comparison
src/main.rs	Passes allowed_tools configuration to Claude bridge
src/llm/session.rs	Sets restrictive permissions (0600) on session files
src/llm/reasoning.rs	Adds TokenUsage tracking and RespondOutput wrapper
src/llm/mod.rs	Exports new RespondOutput and TokenUsage types
src/history/store.rs	Adds user-scoped query methods for sandbox jobs and conversations
src/extensions/manager.rs	Enforces HTTPS for extension downloads, adds WASM magic number validation
src/context/state.rs	Caps transition history at 200 entries, adds token budget tracking
src/context/manager.rs	Fixes TOCTOU race in job creation with write lock
src/config.rs	Adds allowed_tools configuration for Claude Code
src/channels/web/ws.rs	Adds connection limit enforcement for WebSocket subscriptions
src/channels/web/static/app.js	Implements HTML sanitization for rendered markdown
src/channels/web/sse.rs	Adds connection limit enforcement (max 100 concurrent)
src/channels/web/server.rs	Implements rate limiting, user-scoped APIs, CORS restrictions, origin validation, body size limits
src/channels/web/mod.rs	Initializes rate limiter in gateway state
src/channels/web/log_layer.rs	Scrubs secrets from log messages before SSE broadcast
src/channels/web/auth.rs	Implements constant-time token comparison in middleware
src/channels/wasm/wrapper.rs	Adds response body size enforcement for channel WASM HTTP
src/agent/worker.rs	Updates completion detection to use phrase-level matching
src/agent/scheduler.rs	Fixes TOCTOU race with write lock held during job scheduling
src/agent/routine_engine.rs	Uses AtomicUsize for panic-safe running_count and UTF-8 safe truncation
src/agent/agent_loop.rs	Adds token usage logging and explicit approval for destructive shell commands
Cargo.toml	Adds subtle crate dependency for constant-time comparisons

Comments suppressed due to low confidence (1)

src/channels/web/server.rs:1068

• The job list handler applies redundant filtering. Line 1045 calls list_sandbox_jobs_for_user which already filters by user_id in the SQL query, but then line 1052 applies an additional filter for the same condition. This is redundant and suggests uncertainty about whether the database query is doing the right thing. Either trust the database query and remove the application-level filter, or add a defensive assertion/warning if the filter catches anything (which would indicate a bug in the database query).

    // Scope jobs to the authenticated user.
    let mut jobs: Vec<JobInfo> = sandbox_jobs
        .iter()
        .filter(|j| j.user_id == state.user_id)
        .map(|j| {
            let ui_state = match j.status.as_str() {
                "creating" => "pending",
                "running" => "in_progress",
                s => s,
            };
            JobInfo {
                id: j.id,
                title: j.task.clone(),
                state: ui_state.to_string(),
                user_id: j.user_id.clone(),
                created_at: j.created_at.to_rfc3339(),
                started_at: j.started_at.map(|dt| dt.to_rfc3339()),
            }
        })
        .collect();

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The regex-based HTML sanitization is insufficient for preventing XSS. Regular expressions cannot reliably parse HTML, and attackers can craft payloads that bypass these patterns (e.g., using encoded characters, case variations, nested elements, or malformed HTML that browsers still interpret). Consider using a proper HTML sanitization library like DOMPurify instead of regex-based string replacement, or implement a stricter allowlist-based approach that only permits specific safe HTML elements and attributes.

Suggested change

	html = html.replace(/<script\b[^<]*(?:(?!<\/script>)<[^<]*)*<\/script>/gi, '');
	html = html.replace(/<iframe\b[^>]*>[\s\S]*?<\/iframe>/gi, '');
	html = html.replace(/<object\b[^>]*>[\s\S]*?<\/object>/gi, '');
	html = html.replace(/<embed\b[^>]*\/?>/gi, '');
	html = html.replace(/<form\b[^>]*>[\s\S]*?<\/form>/gi, '');
	html = html.replace(/<style\b[^>]*>[\s\S]*?<\/style>/gi, '');
	html = html.replace(/<link\b[^>]*\/?>/gi, '');
	html = html.replace(/<base\b[^>]*\/?>/gi, '');
	html = html.replace(/<meta\b[^>]*\/?>/gi, '');
	// Remove event handler attributes (onclick, onerror, onload, etc.)
	html = html.replace(/\s+on\w+\s*=\s*"[^"]*"/gi, '');
	html = html.replace(/\s+on\w+\s*=\s*'[^']*'/gi, '');
	html = html.replace(/\s+on\w+\s*=\s*[^\s>]+/gi, '');
	// Remove javascript: and data: URLs in href/src attributes
	html = html.replace(/(href|src|action)\s*=\s*["']?\s*javascript\s*:/gi, '$1="');
	html = html.replace(/(href|src|action)\s*=\s*["']?\s*data\s*:/gi, '$1="');
	return html;
	// Prefer a robust DOM-based sanitizer when available.
	if (typeof DOMPurify !== 'undefined') {
	return DOMPurify.sanitize(html, {
	// Allow a safe subset of HTML commonly produced by Markdown.
	ALLOWED_TAGS: [
	'a', 'abbr', 'b', 'blockquote', 'br', 'code', 'del', 'em', 'h1', 'h2',
	'h3', 'h4', 'h5', 'h6', 'hr', 'i', 'img', 'kbd', 'li', 'ol', 'p',
	'pre', 's', 'span', 'strong', 'sub', 'sup', 'ul'
	],
	ALLOWED_ATTR: [
	'href', 'title', 'alt', 'src', 'class'
	],
	// Disallow inline event handlers and dangerous URL schemes.
	FORBID_ATTR: ['onerror', 'onload', 'onclick', 'onmouseover', 'style'],
	ALLOWED_URI_REGEXP: /^(?:(?:https?|mailto|ftp|tel):|[^a-z]|[a-z+.\-]+(?:[^a-z+.\-:]|$))/i
	});
	}
	// Fallback: if DOMPurify is not available, escape the HTML entirely.
	// This is safer than attempting regex-based HTML sanitization.
	return escapeHtml(html);

[ilblackdragon]

Valid concern. The regex-based sanitization is a defense-in-depth layer on top of the server-side safety pipeline. Switching to DOMPurify would be better but is a frontend-only change outside the scope of this Rust security hardening PR. Tracking as a follow-up.

[Copilot]

The WASM epoch ticker thread runs in an infinite loop with no shutdown mechanism. When the runtime is dropped, this background thread will continue running indefinitely, consuming resources and potentially keeping the process alive. The thread should either be marked as a daemon thread or have a shutdown channel that allows graceful termination when the runtime is dropped.

[ilblackdragon]

Acknowledged. The WasmRuntime is created once at startup and lives for the process lifetime, so the orphaned thread is harmless in practice (no repeated create/drop cycles). Adding an Arc<AtomicBool> shutdown flag is a good improvement but low priority given the single-instance usage pattern.

[Copilot]

The rate limiter has a race condition in the window reset logic. Between lines 86-92, multiple threads could see that the window has expired (line 87), then all proceed to reset it (lines 89-91). This could result in over-allocation: if 10 threads all see the window expired, they all reset remaining to max_requests - 1, but only one of them should consume a request. The window reset should use a compare-and-swap operation to ensure only one thread resets the window, or the reset and decrement should be atomic.

[ilblackdragon]

Fixed in 6897aaf. The .truncate(true) was wiping the attempts file before reading, so failures never accumulated. Removed the truncate-then-read pattern; the function now reads existing data first, appends the new failure, prunes expired entries, then writes back.

[ilblackdragon]

Note: The rate limiter race condition mentioned here is about the web server's RateLimiter::check() window reset (not the pairing store bug). The window reset race is real but low impact: the gateway is localhost-only, so worst case is a brief double-window burst from concurrent local requests. The decrement path already uses compare_exchange_weak correctly. Could tighten the window reset with a CAS loop in a follow-up.

[Copilot]

The DNS rebinding protection hardcodes port 443 for all hostname resolutions (line 610). This means URLs with non-standard ports (e.g., http://example.com:8080) will be resolved incorrectly. The code should extract and use the actual port from the URL, or use a default based on the scheme (80 for HTTP, 443 for HTTPS). While DNS resolution doesn't strictly require a port for the hostname lookup, using the wrong port could cause resolution to fail or behave unexpectedly depending on the resolver implementation.

[ilblackdragon]

Fixed in 6897aaf. Changed the hardcoded :443 to :0. The port is irrelevant for hostname resolution (ToSocketAddrs needs a host:port format but the port doesn't affect which IPs the hostname resolves to).

[Copilot]

The CORS configuration has hardcoded port 3001 alongside the dynamic server port (lines 269-270). This suggests that port 3001 is meant for development, but it's unclear why both the actual server port and port 3001 are allowed. If the server is running on a different port, allowing 3001 creates an unnecessary CORS hole. Consider removing the hardcoded port 3001 entries, or document why they need to remain and under what conditions they're used.

Suggested change

	"http://localhost:3001".parse().expect("valid origin"),
	"http://127.0.0.1:3001".parse().expect("valid origin"),

[ilblackdragon]

Fixed in 6897aaf. Removed the hardcoded localhost:3001 and 127.0.0.1:3001 entries. The dynamic addr.port() origins on the lines above already cover the actual server port.

[Copilot]

The identity file protection has two different constant definitions with different values. At line 23-27, PROTECTED_IDENTITY_FILES uses workspace path constants (e.g., paths::IDENTITY), while at line 252-253 it uses hardcoded strings (e.g., "IDENTITY.md"). These should be unified to use the same source of truth to avoid inconsistencies where one check might miss a file that the other would catch.

[ilblackdragon]

Fixed in aba9dad. The duplicate PROTECTED_IDENTITY_FILES constant with hardcoded strings has been removed; the inline block now uses the module-level constant that references paths:: constants.

[Copilot]

Token usage is being tracked and logged but never actually enforced. The add_tokens method exists on JobContext and returns an error when the budget is exceeded, but it's never called in the code. The agent loop logs token usage but doesn't call ctx.add_tokens(output.usage.total()) or similar to enforce the token budget. This means the token budget feature added in this PR is incomplete and won't actually limit runaway LLM usage.

Suggested change

	// Enforce token budget using JobContext
	if let Err(err) = ctx.add_tokens(
	output.usage.input_tokens + output.usage.output_tokens,
	) {
	tracing::warn!("Token budget exceeded for thread {thread_id}: {err}");
	return Err(err.into());
	}

[ilblackdragon]

Valid observation. The add_tokens() method exists on JobContext but is not wired into the agent loop yet. However, max_tokens defaults to 0 (budget enforcement disabled), so this is a feature gap rather than a bug. Wiring it in is a separate feature PR.

[Copilot]

The WebSocket origin validation is incomplete. It only checks if the Origin header is present and validates it, but if the Origin header is missing entirely, the connection is allowed without validation (line 545 uses if let Some). Browsers should always send an Origin header for WebSocket connections, but a malicious client could omit it. The validation should reject connections that lack an Origin header to prevent bypass.

Suggested change

	if let Some(origin) = headers.get("origin").and_then(|v| v.to_str().ok()) {
	let is_local = origin.starts_with("http://localhost")
	|| origin.starts_with("http://127.0.0.1")
	|| origin.starts_with("http://[::1]")
	|| origin.starts_with("http://0.0.0.0");
	if !is_local {
	return Err((
	StatusCode::FORBIDDEN,
	"WebSocket origin not allowed".to_string(),
	));
	}
	let origin = headers
	.get("origin")
	.and_then(|v| v.to_str().ok())
	.ok_or((
	StatusCode::FORBIDDEN,
	"WebSocket origin header missing or invalid".to_string(),
	))?;
	let is_local = origin.starts_with("http://localhost")
	|| origin.starts_with("http://127.0.0.1")
	|| origin.starts_with("http://[::1]")
	|| origin.starts_with("http://0.0.0.0");
	if !is_local {
	return Err((
	StatusCode::FORBIDDEN,
	"WebSocket origin not allowed".to_string(),
	));

[ilblackdragon]

Fixed in 6897aaf. Changed the if let Some(origin) pattern to require the Origin header outright. Browsers always send it for WebSocket upgrades, so a missing Origin now returns 403.

[Copilot]

Pull request overview

Copilot reviewed 44 out of 45 changed files in this pull request and generated 14 comments.

Comments suppressed due to low confidence (1)

src/channels/wasm/wrapper.rs:292

• This wrapper still uses reqwest::Client::new() (default redirect policy). That undermines SSRF hardening because an allowlisted URL can redirect to a disallowed target. Build the client with redirect(Policy::none()) and treat 3xx responses as denied (matching the other HTTP wrappers).

        let result = tokio::runtime::Handle::current().block_on(async {
            let client = reqwest::Client::new();

            let mut request = match method.to_uppercase().as_str() {
                "GET" => client.get(&url),

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

record_failed_approve() now does an unlocked read-modify-write of the approve-attempts file. Concurrent failures can clobber each other (lost updates), weakening/bypassing the rate limit. Consider reintroducing file locking around the entire read+write sequence (or an atomic update strategy).

[ilblackdragon]

Fixed in a3b0190. Reintroduced file locking (lock_exclusive/unlock) around the read-modify-write cycle, with .truncate(false) so the existing data is preserved.

[ilblackdragon]

Fixed in a3b0190. Reintroduced lock_exclusive()/unlock() around the read-modify-write cycle. The function now locks the file before reading, modifies in memory, writes back, and unlocks.

[Copilot]

parse_url() rejects userinfo via authority.contains('@'), but the function later repeats essentially the same userinfo rejection on host/host_and_port. Keeping two checks increases drift risk; consider removing the redundant userinfo check and centralizing this logic in one place.

[ilblackdragon]

Valid nit. The two checks guard different code paths (early reject vs. post-parse validation) but both ultimately block userinfo. Consolidating to a single check would reduce drift risk. Deferred for now as it doesn't affect correctness, but happy to clean it up in a follow-up.

[Copilot]

RateLimiter::check has a race in the window reset path: multiple concurrent callers can all observe an expired window and each reset the counters, allowing more than max_requests actions per window. Also self.max_requests - 1 will underflow if max_requests is ever 0. Consider using a compare-and-swap/fetch_update on window_start so only one caller resets the window and handle max_requests == 0 explicitly.

[ilblackdragon]

Acknowledged. The race is real but low-impact: the gateway binds to localhost only, so concurrent callers are limited to the local machine. Fixing it properly requires either a mutex or an atomic window epoch, both of which add complexity for a localhost-only rate limiter. Noting for a future pass if the gateway is ever exposed publicly.

[Copilot]

WebSocket Origin validation uses starts_with("http://localhost") / starts_with("http://127.0.0.1"), which can be bypassed with origins like http://localhost.evil.com. Parse the Origin as a URL and compare the host (and optionally port) for exact matches instead of prefix matching.

[ilblackdragon]

Fixed in a3b0190. Origin host is now extracted and compared exactly (not prefix-matched), so http://localhost.evil.com is correctly rejected.

[ilblackdragon]

Fixed in a3b0190. Replaced starts_with("http://localhost") with exact host extraction that strips the scheme, splits on : (port) and / (path), then matches the host component exactly against localhost, 127.0.0.1, and [::1]. Origins like http://localhost.evil.com no longer pass.

[Copilot]

If state.sse.subscribe_raw() returns None (connection limit reached), this function returns early after incrementing ws_tracker, but never decrements it. This will leak the tracked connection count and may permanently block new connections. Decrement before returning, or only increment after successfully subscribing.

[ilblackdragon]

Fixed in a3b0190. The WS tracker is now decremented before the early return when subscribe_raw() returns None.

[ilblackdragon]

Fixed in a3b0190. Added tracker.decrement() before the early return when subscribe_raw() returns None. The counter now stays consistent.

[Copilot]

register_sync() inserts into tools and then tries to mark the name as built-in using builtin_names.try_write(). If that lock is contended, the protected name may not be recorded even though the tool was registered, allowing a later dynamic register() to shadow it. Consider making the built-in marking non-fallible (awaiting a write lock, or initializing a protected-name set up front).

[ilblackdragon]

Valid. try_write() failing silently means a tool could be registered without its name being protected as built-in. In practice, register_sync is only called during single-threaded startup before any concurrent access, so contention is unlikely. Still, switching to write() (blocking) or propagating the error would be more correct. Noting for a follow-up.

[Copilot]

subscribe_raw() uses load() < max followed by fetch_add(1), which can oversubscribe under concurrency. Use an atomic fetch_update/compare_exchange loop to increment only when the current count is below max_connections (otherwise return None).

Suggested change

	let current = self.connection_count.load(Ordering::Relaxed);
	if current >= self.max_connections {
	return None;
	}
	let counter = Arc::clone(&self.connection_count);
	counter.fetch_add(1, Ordering::Relaxed);
	// Atomically increment the connection count only if we are still below the limit.
	let update_result = self.connection_count.fetch_update(
	Ordering::Relaxed,
	Ordering::Relaxed,
	|current| {
	if current < self.max_connections {
	Some(current + 1)
	} else {
	None
	}
	},
	);
	if update_result.is_err() {
	// Another thread reached the limit first.
	return None;
	}
	let counter = Arc::clone(&self.connection_count);

[ilblackdragon]

Fixed in a3b0190. Both subscribe_raw() and subscribe() now use fetch_update with a CAS loop to atomically increment only when below max_connections.

[ilblackdragon]

Fixed in a3b0190 (same change as the subscribe() fix above). Both methods now use fetch_update for atomic increment-if-below-max.

[Copilot]

Max response enforcement happens after response.bytes() buffers the full body, so it doesn't actually prevent memory exhaustion when Content-Length is missing/incorrect. Consider streaming the body with a hard byte limit. Also, leak detection is skipped for non-UTF8 bodies (from_utf8); using from_utf8_lossy would avoid a binary-prefix bypass.

[ilblackdragon]

Same issue as the built-in HTTP tool (comment on http.rs:245). Both need a streaming body reader with a byte cap. Will address together in the streaming response follow-up.

[Copilot]

The max response size check occurs after response.bytes() has already buffered the full response body in memory, so a large body without Content-Length can still OOM the host. Use a streaming read with a hard byte cap rather than reading the full body first.

[ilblackdragon]

Same pattern as the other two response-buffering comments (http.rs:245, tools/wasm/wrapper.rs:275). All three need the same streaming body reader fix. Tracking together.

[Copilot]

The test name test_sanitize_action_forces_sanitization_when_injection_check_disabled doesn’t match what it asserts (it verifies no sanitization and no modification). Rename the test (or adjust it to actually exercise a Sanitize policy violation) so failures are interpretable.

Suggested change

	fn test_sanitize_action_forces_sanitization_when_injection_check_disabled() {
	fn test_no_sanitization_when_injection_check_disabled_for_normal_text() {

[Copilot]

Pull request overview

Copilot reviewed 45 out of 46 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[tribendu]

Documentation Fixes Applied ✅

Issue #35: AGENTS.md + CLAUDE.md Documentation

1. Fixed Incorrect Rust Version References

• Changed: Rust 1.85+/edition 2024 → Rust 1.75+/edition 2021
• Files updated:
  • (2 occurrences - Quick Reference section and Coding Standards)
  • (1 occurrence - Quick Reference header)
• Reason: Rust edition 2024 and version 1.85 do not exist (current stable is 1.83, edition 2021 is the latest)

2. Restored Tool Authentication Documentation

• Added new section: under Security Model
• Location: after the Secrets section
• Content includes:
  • Authentication Flow (5-step process):
    1. Declaration in
    2. Setup via OAuth flow or CLI wizard
    3. Storage with AES-256-GCM encryption
    4. Runtime injection with per-job auth tokens
    5. Revocation via CLI or deletion
  • Security Guarantees:
    • Isolation (no encryption key or database access)
    • Least Privilege (only declared credentials)
    • No Persistence (memory-only during execution)
    • Audit Trail (all access logged)
  • MCP Tools authentication mechanism
  • Built-in vs WASM vs MCP comparison table

Commit Details

• Branch:
• Commit:
• Files changed: 2 files, 55 insertions(+), 3 deletions(-)
• Changes: 2 fixes applied