diff --git a/crates/agent-guard-core/src/decision.rs b/crates/agent-guard-core/src/decision.rs index b3cee44..cdb77a7 100644 --- a/crates/agent-guard-core/src/decision.rs +++ b/crates/agent-guard-core/src/decision.rs @@ -2,7 +2,12 @@ use serde::{Deserialize, Serialize}; // ── GuardDecision ───────────────────────────────────────────────────────────── -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +// `GuardDecision` drives enforcement, so it is `Serialize`-only by design: it +// must never be reconstructed from external input. Deriving `Deserialize` would +// let untrusted JSON (`{"decision":"allow"}`) synthesize an `Allow` without +// passing through the policy engine. Audit/receipt consumers read the +// serialized form; they never deserialize it back to drive a decision. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] #[serde(tag = "decision", rename_all = "snake_case")] pub enum GuardDecision { Allow, @@ -137,7 +142,11 @@ impl RuntimeDecision { // ── DecisionReason ──────────────────────────────────────────────────────────── +// `#[non_exhaustive]` forces cross-crate callers through `DecisionReason::new` +// (and its builders), which always supply a non-empty `message`, instead of a +// struct literal that could set `message: ""` and bypass that guarantee. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[non_exhaustive] pub struct DecisionReason { pub code: DecisionCode, pub message: String, diff --git a/crates/agent-guard-sdk/src/anomaly.rs b/crates/agent-guard-sdk/src/anomaly.rs index 82fbda1..89d707e 100644 --- a/crates/agent-guard-sdk/src/anomaly.rs +++ b/crates/agent-guard-sdk/src/anomaly.rs @@ -59,6 +59,10 @@ impl AnomalyDetector { Ok(guard) => guard, Err(_) => { // If the mutex is poisoned, we fail-closed for security. + tracing::error!( + actor = actor, + "AnomalyDetector state mutex poisoned in check; failing closed to Locked" + ); return AnomalyStatus::Locked; } }; @@ -123,7 +127,16 @@ impl AnomalyDetector { let now = Instant::now(); let mut states = match self.states.lock() { Ok(guard) => guard, - Err(_) => return, // Silent return on poison for non-critical update + // Fail-closed: recover the poisoned guard so the denial is still + // recorded and the Deny Fuse can still trip. Dropping it here would + // let an actor exhaust the threshold without ever locking. + Err(poisoned) => { + tracing::error!( + actor = actor, + "AnomalyDetector state mutex poisoned in report_denial; recovering to record denial" + ); + poisoned.into_inner() + } }; compact_states(&mut states, config, now); let state = states.entry(actor.to_string()).or_default(); diff --git a/crates/agent-guard-sdk/src/content_filter.rs b/crates/agent-guard-sdk/src/content_filter.rs index c8038ef..5b44378 100644 --- a/crates/agent-guard-sdk/src/content_filter.rs +++ b/crates/agent-guard-sdk/src/content_filter.rs @@ -64,7 +64,20 @@ fn scannable_text(tool: &Tool, payload: &str) -> Option { Tool::HttpRequest => "body", _ => return None, }; - let value: serde_json::Value = serde_json::from_str(payload).ok()?; + let value: serde_json::Value = match serde_json::from_str(payload) { + Ok(value) => value, + Err(e) => { + // A malformed payload means the scan/mask path is skipped. Warn so + // "no findings" is distinguishable from "scan skipped due to a + // payload the guard could not parse". + tracing::warn!( + tool = ?tool, + error = %e, + "content scan skipped: tool payload is not valid JSON" + ); + return None; + } + }; value .get(field) .and_then(serde_json::Value::as_str) diff --git a/crates/agent-guard-sdk/src/enforce.rs b/crates/agent-guard-sdk/src/enforce.rs index 3b4af47..dd82837 100644 --- a/crates/agent-guard-sdk/src/enforce.rs +++ b/crates/agent-guard-sdk/src/enforce.rs @@ -13,7 +13,7 @@ use agent_guard_sandbox::{Sandbox, SandboxContext, SandboxError, SandboxOutput}; use serde::Serialize; use uuid::Uuid; -use crate::approval::{ApprovalConfig, ApprovalRecord, ApprovalStatus}; +use crate::approval::{ApprovalConfig, ApprovalError, ApprovalRecord, ApprovalStatus}; use crate::executors::{ execute_http_request, execute_write_file, extract_bash_command_for_execution, }; @@ -526,12 +526,24 @@ impl Guard { } ApprovalStatus::Expired | ApprovalStatus::Pending => { if Instant::now() >= deadline { - // Best-effort terminal mark; ignore if already decided. - let _ = config.ledger.decide( + // Terminal mark. `AlreadyDecided` is expected when the + // request was resolved concurrently; any other error is + // a real ledger write failure that would leave the + // ledger `Pending` while we return a denial, so surface + // it rather than discarding it silently. + if let Err(e) = config.ledger.decide( &request_id, ApprovalStatus::Expired, Some("timeout".to_string()), - ); + ) { + if !matches!(e, ApprovalError::AlreadyDecided { .. }) { + tracing::warn!( + request_id = %request_id, + error = %e, + "failed to mark approval request Expired in ledger; state/audit may be inconsistent" + ); + } + } return Ok(RuntimeOutcome::Denied { request_id, reason: DecisionReason::new( @@ -668,9 +680,14 @@ impl Guard { // `stderr` is captured in the HandoffResult type for future surface // expansion (e.g. SIEM details), but is intentionally not part of the - // core ExecutionEvent schema today; flagging presence keeps this - // signal from being silently dropped. - let _ = stderr_present; + // core ExecutionEvent schema today. Emit a debug signal when it is + // present so the omission is observable rather than silently dropped. + if stderr_present { + tracing::debug!( + request_id = request_id, + "handoff result carried stderr; not included in core ExecutionEvent schema" + ); + } } } diff --git a/crates/agent-guard-sdk/src/executors.rs b/crates/agent-guard-sdk/src/executors.rs index ebfffc8..ce13657 100644 --- a/crates/agent-guard-sdk/src/executors.rs +++ b/crates/agent-guard-sdk/src/executors.rs @@ -172,6 +172,7 @@ pub(crate) fn is_always_blocked_ip(ip: &IpAddr) -> bool { || v4.is_broadcast() || v4.is_multicast() || v4.is_private() + || is_v4_shared_or_benchmark(v4) } IpAddr::V6(v6) => { // IPv4-mapped (`::ffff:a.b.c.d`), IPv4-compatible (`::a.b.c.d`), @@ -189,6 +190,22 @@ pub(crate) fn is_always_blocked_ip(ip: &IpAddr) -> bool { } } +/// IPv4 ranges that `std`'s helpers do not cover but that must never be +/// reachable from the outbound HTTP executor: +/// * `100.64.0.0/10` — RFC 6598 shared/CGNAT space, routed to internal +/// services and proxies in some cloud/ISP environments. +/// * `198.18.0.0/15` — RFC 2544 benchmarking range. +/// * `0.0.0.0/8` — RFC 1122 "this network"; `is_unspecified()` only catches +/// the single `0.0.0.0` address, not the whole block. +/// +/// Closes 2026-06-01 MEDIUM (SSRF deny-list gap). +fn is_v4_shared_or_benchmark(v4: &Ipv4Addr) -> bool { + let o = v4.octets(); + (o[0] == 100 && (o[1] & 0xc0) == 64) // 100.64.0.0/10 + || (o[0] == 198 && (o[1] & 0xfe) == 18) // 198.18.0.0/15 + || o[0] == 0 // 0.0.0.0/8 +} + pub(crate) fn is_ipv6_link_local(ip: &Ipv6Addr) -> bool { (ip.segments()[0] & 0xffc0) == 0xfe80 } @@ -224,6 +241,17 @@ pub(crate) fn ipv6_extract_embedded_ipv4(v6: &Ipv6Addr) -> Option { (segments[7] & 0xff) as u8, )); } + // 6to4 (`2002::/16`, RFC 3056): the embedded IPv4 is the two segments after + // the `2002` prefix, so `2002:AABB:CCDD::/48` carries `AA.BB.CC.DD`. + // Closes 2026-06-01 MEDIUM (embedded-private 6to4 SSRF gap). + if segments[0] == 0x2002 { + return Some(Ipv4Addr::new( + (segments[1] >> 8) as u8, + (segments[1] & 0xff) as u8, + (segments[2] >> 8) as u8, + (segments[2] & 0xff) as u8, + )); + } None } @@ -473,6 +501,56 @@ mod tests { assert!(!is_always_blocked_ip(&ip("64:ff9b::8.8.8.8"))); } + // ── 2026-06-01 MEDIUM: SSRF deny-list gaps (CGNAT / benchmark / + // this-network / 6to4) ───────────────────────────────────────────────── + + #[test] + fn blocks_rfc6598_cgnat_shared_space() { + // 100.64.0.0/10 spans the 100.64.x.x .. 100.127.x.x second octet. + assert!(is_always_blocked_ip(&ip("100.64.0.1"))); + assert!(is_always_blocked_ip(&ip("100.100.0.1"))); + assert!(is_always_blocked_ip(&ip("100.127.255.254"))); + } + + #[test] + fn allows_just_outside_cgnat_range() { + // 100.63.x.x and 100.128.x.x are public — must remain reachable. + assert!(!is_always_blocked_ip(&ip("100.63.255.255"))); + assert!(!is_always_blocked_ip(&ip("100.128.0.1"))); + } + + #[test] + fn blocks_rfc2544_benchmark_range() { + // 198.18.0.0/15 covers 198.18.x.x and 198.19.x.x. + assert!(is_always_blocked_ip(&ip("198.18.0.1"))); + assert!(is_always_blocked_ip(&ip("198.19.255.254"))); + // 198.17.x.x and 198.20.x.x are outside the block. + assert!(!is_always_blocked_ip(&ip("198.17.0.1"))); + assert!(!is_always_blocked_ip(&ip("198.20.0.1"))); + } + + #[test] + fn blocks_this_network_zero_slash_eight() { + // The whole 0.0.0.0/8 block, not just the unspecified 0.0.0.0. + assert!(is_always_blocked_ip(&ip("0.1.2.3"))); + assert!(is_always_blocked_ip(&ip("0.255.255.255"))); + } + + #[test] + fn blocks_6to4_embedded_private_ipv4() { + // 2002:AABB:CCDD::/48 routes to embedded AA.BB.CC.DD on 6to4 stacks. + // 2002:0a00:0001:: → 10.0.0.1 (RFC1918); 2002:a9fe:a9fe:: → metadata. + assert!(is_always_blocked_ip(&ip("2002:0a00:0001::"))); + assert!(is_always_blocked_ip(&ip("2002:7f00:0001::"))); // 127.0.0.1 + assert!(is_always_blocked_ip(&ip("2002:a9fe:a9fe::"))); // 169.254.169.254 + } + + #[test] + fn allows_6to4_to_public_ipv4() { + // 2002:0808:0808:: → 8.8.8.8 (public) must stay reachable. + assert!(!is_always_blocked_ip(&ip("2002:0808:0808::"))); + } + // ── 2026-05-25-2 HIGH: payload_declares_mutation_http silent-failure ─── // // Until this fix the function returned `false` on JSON parse failure diff --git a/crates/agent-guard-sdk/tests/integration.rs b/crates/agent-guard-sdk/tests/integration.rs index cda5847..94fbacd 100644 --- a/crates/agent-guard-sdk/tests/integration.rs +++ b/crates/agent-guard-sdk/tests/integration.rs @@ -726,12 +726,11 @@ fn audit_event_jsonl_is_valid_json() { use agent_guard_core::AuditEvent; let tool = Tool::ReadFile; let decision = GuardDecision::Deny { - reason: agent_guard_sdk::DecisionReason { - code: agent_guard_sdk::DecisionCode::DeniedByRule, - message: "test".to_string(), - details: None, - matched_rule: Some("tools.read_file.deny[0]".to_string()), - }, + reason: agent_guard_sdk::DecisionReason::new( + agent_guard_sdk::DecisionCode::DeniedByRule, + "test", + ) + .matched_rule("tools.read_file.deny[0]"), }; let event = AuditEvent::from_decision( "req-3".to_string(),