diff --git a/crates/ironclaw_host_api/src/ids.rs b/crates/ironclaw_host_api/src/ids.rs index 6beae8d919e..c654321ce51 100644 --- a/crates/ironclaw_host_api/src/ids.rs +++ b/crates/ironclaw_host_api/src/ids.rs @@ -99,6 +99,20 @@ fn validate_name_segment(kind: &'static str, value: &str) -> Result<(), HostApiE macro_rules! string_id { ($name:ident, $kind:literal, $validator:ident) => { + string_id!(@build $name, $kind, $validator); + string_id!(@deserialize_strict $name); + }; + ($name:ident, $kind:literal, $validator:ident, accepts_system_sentinel) => { + string_id!(@build $name, $kind, $validator); + string_id!(@sentinel_constructor $name); + // Wire deserialize stays STRICT even for sentinel-aware id kinds: a + // bare `TenantId`/`UserId` decoded from untrusted JSON must reject the + // sentinel. Sentinel admission is scoped to `ResourceScope`'s own + // field-level `deserialize_with` (a TRUSTED-PERSISTENCE shape), never + // to the id type itself. See `ResourceScope` docs in resource.rs. + string_id!(@deserialize_strict $name); + }; + (@build $name:ident, $kind:literal, $validator:ident) => { #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct $name(String); @@ -109,10 +123,11 @@ macro_rules! string_id { Ok(Self(value)) } - /// Construct without validation. Reserved for sentinel values - /// that intentionally contain bytes the validator rejects (e.g. - /// [`crate::SYSTEM_RESERVED_ID`]), so no caller-supplied - /// identifier can collide with them. + /// Construct without validation. For non-sentinel trusted string + /// rehydration (e.g. DB row values handed over from a typed + /// upstream). To mint the system sentinel + /// ([`crate::SYSTEM_RESERVED_ID`]), use `system_sentinel()` instead + /// — it is the single `git grep`-able authority-elevating path. pub fn from_trusted(value: String) -> Self { Self(value) } @@ -140,7 +155,8 @@ macro_rules! string_id { serializer.serialize_str(&self.0) } } - + }; + (@deserialize_strict $name:ident) => { impl<'de> Deserialize<'de> for $name { fn deserialize(deserializer: D) -> Result where @@ -151,6 +167,26 @@ macro_rules! string_id { } } }; + (@sentinel_constructor $name:ident) => { + impl $name { + /// Construct the [`crate::SYSTEM_RESERVED_ID`] sentinel value of + /// this id type. + /// + /// SECURITY-CRITICAL: this is the blessed path for minting a + /// sentinel-valued id. It exists so every authority-elevating + /// constructor is `git grep`-able as `system_sentinel`, rather + /// than hidden behind the more general `from_trusted` escape + /// hatch. New code that needs the sentinel must call this + /// method — never `from_trusted(SYSTEM_RESERVED_ID.to_string())`. + /// + /// `ResourceScope::is_system` returns `true` only when BOTH the + /// tenant and user ids of a scope come from this constructor; + /// no authority decision may key on a single field. + pub fn system_sentinel() -> Self { + Self::from_trusted(crate::SYSTEM_RESERVED_ID.to_string()) + } + } + }; } macro_rules! uuid_id { @@ -191,8 +227,20 @@ macro_rules! uuid_id { }; } -string_id!(TenantId, "tenant", validate_scope_id); -string_id!(UserId, "user", validate_scope_id); +// SECURITY-CRITICAL: `accepts_system_sentinel` opts these id types into +// admitting [`crate::SYSTEM_RESERVED_ID`] during JSON deserialization and +// exposes the `system_sentinel()` constructor — the only blessed path that +// can mint a sentinel-valued id. Every other id kind stays strict and +// rejects the sentinel on the wire. Adding this flag to any new id type +// requires security review: any HTTP/RPC endpoint that deserializes such an +// id from an untrusted request body becomes an authority-elevation vector. +string_id!( + TenantId, + "tenant", + validate_scope_id, + accepts_system_sentinel +); +string_id!(UserId, "user", validate_scope_id, accepts_system_sentinel); string_id!(AgentId, "agent", validate_scope_id); string_id!(ProjectId, "project", validate_scope_id); string_id!(MissionId, "mission", validate_scope_id); diff --git a/crates/ironclaw_host_api/src/resource.rs b/crates/ironclaw_host_api/src/resource.rs index 2aa14082b52..1269b62d119 100644 --- a/crates/ironclaw_host_api/src/resource.rs +++ b/crates/ironclaw_host_api/src/resource.rs @@ -111,8 +111,8 @@ impl ResourceScope { /// validation rejects, so no user-supplied identifier can collide. pub fn system() -> Self { Self { - tenant_id: TenantId::from_trusted(SYSTEM_RESERVED_ID.to_string()), - user_id: UserId::from_trusted(SYSTEM_RESERVED_ID.to_string()), + tenant_id: TenantId::system_sentinel(), + user_id: UserId::system_sentinel(), agent_id: None, project_id: None, mission_id: None, diff --git a/crates/ironclaw_host_api/tests/host_api_contract.rs b/crates/ironclaw_host_api/tests/host_api_contract.rs index de850380409..4cc7a1881a3 100644 --- a/crates/ironclaw_host_api/tests/host_api_contract.rs +++ b/crates/ironclaw_host_api/tests/host_api_contract.rs @@ -206,6 +206,145 @@ fn scope_ids_reject_path_segments_and_controls() { } } +#[test] +fn system_resource_scope_round_trips_through_json() { + let scope = ResourceScope::system(); + assert!(scope.is_system()); + + let encoded = serde_json::to_string(&scope).expect("serialize system scope"); + let decoded: ResourceScope = serde_json::from_str(&encoded).expect("deserialize system scope"); + + assert!(decoded.is_system()); + assert_eq!(decoded.tenant_id.as_str(), SYSTEM_RESERVED_ID); + assert_eq!(decoded.user_id.as_str(), SYSTEM_RESERVED_ID); + + // Sentinel stays unreachable through ordinary construction; only + // `system_sentinel()` may mint it, so user input can never collide. + assert!(TenantId::new(SYSTEM_RESERVED_ID).is_err()); + assert!(UserId::new(SYSTEM_RESERVED_ID).is_err()); +} + +#[test] +fn system_sentinel_constructor_produces_the_reserved_value() { + // `system_sentinel()` is the blessed cross-crate path for minting a + // sentinel-valued id. Pin its identity and pin that `ResourceScope::system` + // (which routes through this constructor) classifies as system — this is + // the contract the four legitimate sentinel call sites (host_api, + // loop_support, turns, threads) depend on. + assert_eq!(TenantId::system_sentinel().as_str(), SYSTEM_RESERVED_ID); + assert_eq!(UserId::system_sentinel().as_str(), SYSTEM_RESERVED_ID); + assert!(ResourceScope::system().is_system()); +} + +#[test] +fn bare_id_kinds_reject_system_sentinel_on_the_wire() { + // SECURITY GUARD (PR review finding #1): NO bare id type — not even the + // sentinel-aware TenantId/UserId — admits the sentinel during its own + // `Deserialize`. Sentinel admission lives exclusively on `ResourceScope`'s + // field-level `deserialize_with` (a TRUSTED-PERSISTENCE shape). A bare + // TenantId/UserId decoded from an untrusted body must reject the sentinel, + // otherwise any endpoint deserializing one becomes an authority-elevation + // vector. Each kind asserts independently so a macro regression is caught. + let raw = json!(SYSTEM_RESERVED_ID); + + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw.clone()).is_err()); + assert!(serde_json::from_value::(raw).is_err()); +} + +#[test] +fn tenant_and_user_ids_reject_sentinel_near_misses_on_the_wire() { + // The sentinel is exactly `\x1fSYSTEM\x1f`. Any near-miss containing + // ANY control byte (including the right one in the wrong place, or + // alongside extra content) must be rejected by the validator, otherwise + // an attacker who controlled the wire could try to smuggle an + // almost-sentinel through the sentinel-aware deserialize path. + let control_byte_near_misses = [ + "\x1eSYSTEM\x1f", // wrong leading control byte + "\x1fSYSTEM\x1e", // wrong trailing control byte + "\x1fSYSTEM", // missing trailing byte + "SYSTEM\x1f", // missing leading byte + "\x1fSYSTEM\x1fx", // trailing extra + "x\x1fSYSTEM\x1f", // leading extra + "\x1fsystem\x1f", // wrong case + "\x1f SYSTEM \x1f", // padded + ]; + for value in control_byte_near_misses { + assert!( + serde_json::from_value::(json!(value)).is_err(), + "TenantId must reject control-byte near-miss {value:?}" + ); + assert!( + serde_json::from_value::(json!(value)).is_err(), + "UserId must reject control-byte near-miss {value:?}" + ); + } +} + +#[test] +fn sentinel_look_alikes_without_control_bytes_do_not_classify_as_system() { + // Strings like "SYSTEM" or "system" deserialize successfully (they are + // valid scope-ids), but the sentinel-equality check in + // `ResourceScope::is_system` must NOT mistake them for the real sentinel. + // This is the wire-level pin: no string a caller can legally supply + // through TenantId/UserId may be classified as system. + for look_alike in ["SYSTEM", "system", "SYSTEMx"] { + let tenant: TenantId = serde_json::from_value(json!(look_alike)) + .unwrap_or_else(|e| panic!("TenantId should accept {look_alike:?}: {e}")); + let user: UserId = serde_json::from_value(json!(look_alike)) + .unwrap_or_else(|e| panic!("UserId should accept {look_alike:?}: {e}")); + assert_ne!(tenant.as_str(), SYSTEM_RESERVED_ID); + assert_ne!(user.as_str(), SYSTEM_RESERVED_ID); + + let scope = ResourceScope { + tenant_id: tenant, + user_id: user, + agent_id: None, + project_id: None, + mission_id: None, + thread_id: None, + invocation_id: InvocationId::new(), + }; + assert!( + !scope.is_system(), + "{look_alike:?} must never classify as the system sentinel" + ); + } +} + +#[test] +fn partial_sentinel_resource_scope_is_not_treated_as_system() { + // `is_system` requires BOTH tenant_id and user_id to be the sentinel. + // If only one field carries the sentinel and the other is a normal id, + // the scope must round-trip through JSON without being misclassified. + // This pins the contract: no authority decision may key on a single + // field of the scope. + let mut tenant_only = ResourceScope::system(); + tenant_only.user_id = UserId::new("alice").unwrap(); + let encoded = serde_json::to_string(&tenant_only).expect("serialize tenant-only"); + let decoded: ResourceScope = serde_json::from_str(&encoded).expect("deserialize tenant-only"); + assert!(!decoded.is_system()); + assert_eq!(decoded.tenant_id.as_str(), SYSTEM_RESERVED_ID); + assert_eq!(decoded.user_id.as_str(), "alice"); + + let mut user_only = ResourceScope::system(); + user_only.tenant_id = TenantId::new("acme").unwrap(); + let encoded = serde_json::to_string(&user_only).expect("serialize user-only"); + let decoded: ResourceScope = serde_json::from_str(&encoded).expect("deserialize user-only"); + assert!(!decoded.is_system()); + assert_eq!(decoded.tenant_id.as_str(), "acme"); + assert_eq!(decoded.user_id.as_str(), SYSTEM_RESERVED_ID); +} + #[test] fn local_default_resource_scope_uses_default_agent_and_bootstrap_project() { let invocation_id = InvocationId::new(); diff --git a/crates/ironclaw_loop_support/src/budget_accountant.rs b/crates/ironclaw_loop_support/src/budget_accountant.rs index 7e576b25eae..9a94664af2e 100644 --- a/crates/ironclaw_loop_support/src/budget_accountant.rs +++ b/crates/ironclaw_loop_support/src/budget_accountant.rs @@ -17,8 +17,7 @@ use async_trait::async_trait; use chrono::Utc; use dashmap::{DashMap, DashSet}; use ironclaw_host_api::{ - InvocationId, ResourceEstimate, ResourceReservationId, ResourceScope, ResourceUsage, - SYSTEM_RESERVED_ID, UserId, + InvocationId, ResourceEstimate, ResourceReservationId, ResourceScope, ResourceUsage, UserId, }; use ironclaw_resources::{ BudgetApprovalGate, BudgetEvent, BudgetEventSink, BudgetGateId, BudgetGateStatus, @@ -397,7 +396,7 @@ impl GovernorBackedAccountant { .actor .as_ref() .map(|actor| actor.user_id.clone()) - .unwrap_or_else(|| UserId::from_trusted(SYSTEM_RESERVED_ID.to_string())); + .unwrap_or_else(UserId::system_sentinel); ResourceScope { tenant_id: context.scope.tenant_id.clone(), user_id, @@ -680,6 +679,10 @@ mod tests { use rust_decimal_macros::dec; fn run_context() -> LoopRunContext { + run_context_without_actor().with_actor(TurnActor::new(UserId::new("acct-user").unwrap())) + } + + fn run_context_without_actor() -> LoopRunContext { let scope = TurnScope::new( TenantId::new("tenant-acct").unwrap(), None, @@ -739,7 +742,6 @@ mod tests { }, }; LoopRunContext::new(scope, TurnId::new(), TurnRunId::new(), profile) - .with_actor(TurnActor::new(UserId::new("acct-user").unwrap())) } fn sample_request() -> LoopModelRequest { @@ -783,6 +785,38 @@ mod tests { let _ = snapshot; } + #[test] + fn resource_scope_falls_back_to_system_sentinel_user_without_actor() { + // PR review finding #5: exercise the no-actor branch of + // `resource_scope`. With no actor, `user_id` resolves to the system + // sentinel; the real tenant keeps the scope from classifying as system. + let accountant = GovernorBackedAccountant::new( + Arc::new(InMemoryResourceGovernor::new()), + Arc::new(ZeroCostTable), + ); + let context = run_context_without_actor(); + assert!(context.actor.is_none()); + + let scope = accountant.resource_scope(&context); + assert_eq!( + scope.user_id.as_str(), + ironclaw_host_api::SYSTEM_RESERVED_ID + ); + assert_eq!(scope.tenant_id.as_str(), "tenant-acct"); + assert!(!scope.is_system()); + } + + #[test] + fn resource_scope_uses_actor_user_when_present() { + let accountant = GovernorBackedAccountant::new( + Arc::new(InMemoryResourceGovernor::new()), + Arc::new(ZeroCostTable), + ); + let scope = accountant.resource_scope(&run_context()); + assert_eq!(scope.user_id.as_str(), "acct-user"); + assert!(!scope.is_system()); + } + #[tokio::test] async fn pre_model_call_returns_budget_exceeded_when_limit_zero_but_negative() { // A negative-USD estimate would be invalid input, but our default diff --git a/crates/ironclaw_threads/src/contract.rs b/crates/ironclaw_threads/src/contract.rs index f9943866127..11bcc8bb964 100644 --- a/crates/ironclaw_threads/src/contract.rs +++ b/crates/ironclaw_threads/src/contract.rs @@ -29,11 +29,10 @@ impl ThreadScope { pub fn to_resource_scope(&self) -> ironclaw_host_api::ResourceScope { ironclaw_host_api::ResourceScope { tenant_id: self.tenant_id.clone(), - user_id: self.owner_user_id.clone().unwrap_or_else(|| { - ironclaw_host_api::UserId::from_trusted( - ironclaw_host_api::SYSTEM_RESERVED_ID.to_string(), - ) - }), + user_id: self + .owner_user_id + .clone() + .unwrap_or_else(ironclaw_host_api::UserId::system_sentinel), agent_id: Some(self.agent_id.clone()), project_id: self.project_id.clone(), mission_id: self.mission_id.clone(), @@ -501,6 +500,14 @@ pub struct UpdateThreadGoalRequest { #[cfg(test)] mod tests { use super::*; + + fn scope(owner: Option) -> ThreadScope { + ThreadScope { + tenant_id: TenantId::new("acme").unwrap(), + agent_id: AgentId::new("agent-1").unwrap(), + project_id: None, + owner_user_id: owner, + mission_id: None, use ironclaw_common::AttachmentKind; fn sample_ref() -> AttachmentRef { @@ -516,6 +523,24 @@ mod tests { } #[test] + fn to_resource_scope_falls_back_to_system_sentinel_user_without_owner() { + // PR review finding #4: drive the real caller. An ownerless thread + // scope must resolve `user_id` to the system sentinel, while the real + // tenant keeps the scope from classifying as fully system. + let resource = scope(None).to_resource_scope(); + assert_eq!( + resource.user_id.as_str(), + ironclaw_host_api::SYSTEM_RESERVED_ID + ); + assert_eq!(resource.tenant_id.as_str(), "acme"); + assert!(!resource.is_system()); + } + + #[test] + fn to_resource_scope_uses_explicit_owner_when_present() { + let resource = scope(Some(UserId::new("alice").unwrap())).to_resource_scope(); + assert_eq!(resource.user_id.as_str(), "alice"); + assert!(!resource.is_system()); fn text_constructor_carries_no_attachments() { let content = MessageContent::text("hello"); assert_eq!(content.as_text(), "hello"); diff --git a/crates/ironclaw_turns/src/scope.rs b/crates/ironclaw_turns/src/scope.rs index b5b21b8e958..aeda96ba7e9 100644 --- a/crates/ironclaw_turns/src/scope.rs +++ b/crates/ironclaw_turns/src/scope.rs @@ -113,9 +113,10 @@ impl TurnScope { pub fn to_resource_scope(&self) -> ironclaw_host_api::ResourceScope { let mut scope = ironclaw_host_api::ResourceScope::system(); scope.tenant_id = self.tenant_id.clone(); - scope.user_id = self.explicit_owner_user_id().cloned().unwrap_or_else(|| { - UserId::from_trusted(ironclaw_host_api::SYSTEM_RESERVED_ID.to_string()) - }); + scope.user_id = self + .explicit_owner_user_id() + .cloned() + .unwrap_or_else(UserId::system_sentinel); scope.agent_id = self.agent_id.clone(); scope.project_id = self.project_id.clone(); scope.mission_id = None; @@ -227,4 +228,39 @@ mod tests { }) ); } + + #[test] + fn to_resource_scope_falls_back_to_system_sentinel_user_without_owner() { + // PR review finding #3: drive the real caller. A turn scope anchored at + // tenant level with no explicit owner must resolve `user_id` to the + // system sentinel — and because the tenant is a real id, the resulting + // scope must NOT classify as fully system (is_system needs both fields). + let scope = TurnScope::new( + TenantId::new("acme").unwrap(), + None, + None, + ThreadId::new("thread-1").unwrap(), + ); + let resource = scope.to_resource_scope(); + assert_eq!( + resource.user_id.as_str(), + ironclaw_host_api::SYSTEM_RESERVED_ID + ); + assert_eq!(resource.tenant_id.as_str(), "acme"); + assert!(!resource.is_system()); + } + + #[test] + fn to_resource_scope_uses_explicit_owner_when_present() { + let scope = TurnScope::new_with_owner( + TenantId::new("acme").unwrap(), + None, + None, + ThreadId::new("thread-1").unwrap(), + Some(UserId::new("alice").unwrap()), + ); + let resource = scope.to_resource_scope(); + assert_eq!(resource.user_id.as_str(), "alice"); + assert!(!resource.is_system()); + } }