Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions crates/ironclaw_host_api/src/approval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ impl InvocationFingerprint {
input: &'a serde_json::Value,
}

let canonical_input = canonical_json(input);
let canonical_input = canonical_json(input)?;
let payload = Payload {
version: 1,
kind: "dispatch",
Expand All @@ -67,21 +67,38 @@ impl InvocationFingerprint {
}
}

fn canonical_json(value: &serde_json::Value) -> serde_json::Value {
const MAX_CANONICAL_JSON_DEPTH: usize = 64;

fn canonical_json(value: &serde_json::Value) -> Result<serde_json::Value, HostApiError> {
canonical_json_at_depth(value, 0)
}

fn canonical_json_at_depth(
value: &serde_json::Value,
depth: usize,
) -> Result<serde_json::Value, HostApiError> {
if depth > MAX_CANONICAL_JSON_DEPTH {
return Err(HostApiError::invariant(
"canonical_json: max depth exceeded",
));
}

match value {
serde_json::Value::Array(items) => {
serde_json::Value::Array(items.iter().map(canonical_json).collect())
}
serde_json::Value::Array(items) => items
.iter()
.map(|item| canonical_json_at_depth(item, depth + 1))
.collect::<Result<Vec<_>, _>>()
.map(serde_json::Value::Array),
serde_json::Value::Object(map) => {
let mut entries = map.iter().collect::<Vec<_>>();
entries.sort_by_key(|(key, _)| *key);
let mut canonical = serde_json::Map::new();
for (key, value) in entries {
canonical.insert(key.clone(), canonical_json(value));
canonical.insert(key.clone(), canonical_json_at_depth(value, depth + 1)?);
}
serde_json::Value::Object(canonical)
Ok(serde_json::Value::Object(canonical))
}
_ => value.clone(),
_ => Ok(value.clone()),
}
}

Expand Down
28 changes: 28 additions & 0 deletions crates/ironclaw_host_api/tests/host_api_contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,34 @@ fn invocation_fingerprint_is_stable_and_input_hashed() {
assert!(!first.as_str().contains("secret payload"));
}

#[test]
fn invocation_fingerprint_rejects_deeply_nested_input() {
let ctx = sample_context();
let capability = CapabilityId::new("echo.say").unwrap();
let estimate = ResourceEstimate::default();
let mut input = serde_json::Value::String("leaf".to_string());

for _ in 0..10_000 {
let mut object = serde_json::Map::new();
object.insert("a".to_string(), input);
input = serde_json::Value::Object(object);
}

// serde_json::Value drops nested objects recursively; leak this intentionally
// so the test exercises fingerprint rejection rather than Value teardown.
let input = Box::leak(Box::new(input));

let err =
InvocationFingerprint::for_dispatch(&ctx.resource_scope, &capability, &estimate, input)
.unwrap_err();

assert!(matches!(
err,
HostApiError::InvariantViolation { reason }
if reason == "canonical_json: max depth exceeded"
));
}

#[test]
fn invocation_fingerprint_changes_when_authorized_invocation_changes() {
let ctx = sample_context();
Expand Down
20 changes: 20 additions & 0 deletions crates/ironclaw_resources/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,11 @@ pub enum ResourceError {
LimitExceeded(Box<ResourceDenial>),
#[error("resource reservation {id} already exists")]
ReservationAlreadyExists { id: ResourceReservationId },
#[error("invalid resource estimate for {dimension}: {reason}")]
InvalidEstimate {
dimension: ResourceDimension,
reason: &'static str,
},
#[error("resource reservation {id} does not match requested scope or estimate")]
ReservationMismatch { id: ResourceReservationId },
#[error("unknown resource reservation {id}")]
Expand Down Expand Up @@ -427,6 +432,8 @@ impl ResourceGovernor for InMemoryResourceGovernor {
estimate: ResourceEstimate,
reservation_id: ResourceReservationId,
) -> Result<ResourceReservation, ResourceError> {
validate_estimate(&estimate)?;

let mut state = self.lock_state();
if state.reservations.contains_key(&reservation_id) {
return Err(ResourceError::ReservationAlreadyExists { id: reservation_id });
Expand Down Expand Up @@ -566,6 +573,19 @@ impl ResourceGovernor for InMemoryResourceGovernor {
}
}

fn validate_estimate(estimate: &ResourceEstimate) -> Result<(), ResourceError> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

While this function correctly validates ResourceEstimate to prevent negative reservations, the governor's reconcile method (which processes ResourceUsage) currently lacks a similar check. A negative USD value in ResourceUsage could allow a runtime to 'refund' spent balances by decreasing the account's usage tally, which constitutes a similar fail-closed gap. Consider extending this validation logic to cover ResourceUsage as well. Additionally, ensure that if this operation fails, any accumulated metrics are propagated out along with the error rather than discarded.

References
  1. When an operation can fail, ensure that any accumulated metrics (like resource usage) are propagated out along with the error, not discarded.

if let Some(usd) = estimate.usd
&& usd < Decimal::ZERO
{
return Err(ResourceError::InvalidEstimate {
dimension: ResourceDimension::Usd,
reason: "must be non-negative",
});
}

Ok(())
}

/// Returns the first denied dimension in canonical resource order.
///
/// This intentionally reports one denial rather than aggregating all failed
Expand Down
27 changes: 27 additions & 0 deletions crates/ironclaw_resources/tests/resource_governor_contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,33 @@ fn reserve_with_id_uses_requested_identifier_and_rejects_duplicates() {
));
}

#[test]
fn reserve_with_id_rejects_negative_usd_estimates() {
let governor = InMemoryResourceGovernor::new();
let scope = sample_scope("tenant1", "user1", Some("project1"));
let account = ResourceAccount::tenant(scope.tenant_id.clone());

let err = governor
.reserve_with_id(
scope,
ResourceEstimate {
usd: Some(dec!(-100.00)),
..ResourceEstimate::default()
},
ResourceReservationId::new(),
)
.unwrap_err();

assert!(matches!(
err,
ResourceError::InvalidEstimate {
dimension: ResourceDimension::Usd,
reason: "must be non-negative"
}
));
assert_eq!(governor.reserved_for(&account).usd, dec!(0));
}

#[test]
fn usd_tally_saturates_instead_of_panicking_on_decimal_overflow() {
let governor = InMemoryResourceGovernor::new();
Expand Down
Loading