lywsvip/ironclaw
1
0
Fork 0
mirror of https://github.com/nearai/ironclaw.git synced 2026-09-03 08:06:01 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(approvals): persist approval state before issuing lease

Browse Source 
Addresses audit finding F2.

Inverts the lease/approve ordering inside `approve_capability_action`:
the approval store write now runs *before* the lease store write. The
previous order (issue lease, then approve, best-effort revoke on
failure) left a window where a transient approval-store error could
leave a live lease pointing at a request whose status remained
`Pending`.

The approval record is now treated as the authority of record. Once
the request flips to `Approved`, lease issuance is a recoverable
operation against an already-decided request — if the lease store
fails, the caller surfaces the lease error and the request stays
`Approved`. The previous best-effort `let _ = self.leases.revoke(...)`
swallow is gone with the same edit.

Updates the three concurrency/error-injection tests to assert the new
semantics, plus the crate CLAUDE.md guardrail. No external test
fixtures break — the public resolver API is unchanged.
This commit is contained in:
ilblackdragon@gmail.com
2026-05-16 09:14:48 -07:00
parent 80f8b1fcaa
commit 3e8b2303e1
3 changed files with 45 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
crates/ironclaw_approvals/CLAUDE.md
View File

@@ -2,6 +2,6 @@
- Own approval resolution workflow: pending approval record to scoped lease or denial.
- Do not prompt users, dispatch capabilities, manage processes, reserve resources, or import runtime/dispatcher/capability workflow crates.
- Approve fail-closed: issue durable lease first, then mark approved; revoke issued lease best-effort if status write fails.
- Approve fail-closed: persist `approve` (the authority record) first, then issue the lease. If the lease store fails after approval is persisted, the request stays `Approved` and the caller surfaces the lease error — no rollback to `Pending`. The approval record is the durable decision; lease re-issuance against an already-decided request is recoverable.
- Denials issue no lease.
- Audit emission is metadata-only and best-effort.
- Audit emission is metadata-only and best-effort. Failures are logged at `debug!` and never alter resolution outcomes.

39
crates/ironclaw_approvals/src/lib.rs
View File

@@ -86,7 +86,8 @@ where
}
let capability = capability_for_action(record.request.action.as_ref(), expected_action)
.ok_or(ApprovalResolutionError::UnsupportedAction)?;
.ok_or(ApprovalResolutionError::UnsupportedAction)?
.clone();
let invocation_fingerprint = record
.request
@@ -94,10 +95,27 @@ where
.clone()
.ok_or(ApprovalResolutionError::MissingInvocationFingerprint)?;
let resolved_by = approval.issued_by.clone();
// F2: persist the approval state *before* issuing the lease. The
// approval record is the authority of record — once it flips to
// `Approved`, any subsequent lease re-issue is a recoverable
// operation against an already-decided request. The previous
// order (issue lease, then approve, best-effort revoke on
// failure) left a window where a transient store error could
// produce a live lease whose approval status was still
// `Pending`. See audit finding F2.
let approved_record = match self.approvals.approve(scope, request_id).await {
Ok(record) => record,
Err(RunStateError::ApprovalNotPending { status, .. }) => {
return Err(ApprovalResolutionError::NotPending { status });
}
Err(error) => return Err(error.into()),
};
let grant = CapabilityGrant {
id: CapabilityGrantId::new(),
capability: capability.clone(),
grantee: record.request.requested_by.clone(),
capability,
grantee: approved_record.request.requested_by.clone(),
issued_by: approval.issued_by,
constraints: GrantConstraints {
allowed_effects: approval.allowed_effects,
@@ -109,21 +127,12 @@ where
max_invocations: approval.max_invocations,
},
};
let mut lease = CapabilityLease::new(record.scope.clone(), grant);
let mut lease = CapabilityLease::new(approved_record.scope.clone(), grant);
lease.invocation_fingerprint = Some(invocation_fingerprint);
let lease = self.leases.issue(lease).await?;
if let Err(error) = self.approvals.approve(scope, request_id).await {
let _ = self.leases.revoke(&lease.scope, lease.grant.id).await;
return match error {
RunStateError::ApprovalNotPending { status, .. } => {
Err(ApprovalResolutionError::NotPending { status })
}
error => Err(error.into()),
};
}
self.emit_audit_best_effort(ironclaw_host_api::AuditEnvelope::approval_resolved(
&record.scope,
&record.request,
&approved_record.scope,
&approved_record.request,
resolved_by,
ApprovalDecisionKind::Approved,
))

29
crates/ironclaw_approvals/tests/approval_resolution_contract.rs
View File

@@ -134,7 +134,13 @@ async fn approving_pending_dispatch_request_preserves_reviewed_grant_constraints
}
#[tokio::test]
async fn approving_pending_request_keeps_pending_when_lease_issue_fails() {
async fn approving_pending_request_marks_request_approved_even_when_lease_issue_fails() {
// F2 semantics: the approval record is the authority of record. Once
// it flips to `Approved`, lease issuance is a recoverable operation
// against an already-decided request. If lease issuance fails, the
// request must stay `Approved` (not roll back to `Pending`) so that
// the system can surface the error to the caller and re-attempt
// lease issuance later.
let approvals = InMemoryApprovalRequestStore::new();
let leases = FailingIssueLeaseStore;
let resolver = ApprovalResolver::new(&approvals, &leases);
@@ -176,12 +182,15 @@ async fn approving_pending_request_keeps_pending_when_lease_issue_fails() {
.unwrap()
.unwrap()
.status,
ApprovalStatus::Pending
ApprovalStatus::Approved
);
}
#[tokio::test]
async fn approving_pending_request_revokes_issued_lease_when_approval_update_fails() {
async fn approving_pending_request_issues_no_lease_when_approval_update_fails() {
// F2 semantics: with persist-approval-first ordering, a store error
// on the approval write must short-circuit before any lease is
// issued. There is no orphaned lease to revoke.
let invocation_id = InvocationId::new();
let scope = sample_scope(invocation_id, "tenant1", "user1");
let approval = approval_request(invocation_id, CapabilityId::new("echo.say").unwrap());
@@ -215,13 +224,15 @@ async fn approving_pending_request_revokes_issued_lease_when_approval_update_fai
.unwrap_err();
assert!(matches!(err, ApprovalResolutionError::RunState(_)));
let issued = leases.leases_for_scope(&scope).await;
assert_eq!(issued.len(), 1);
assert_eq!(issued[0].status, CapabilityLeaseStatus::Revoked);
assert_eq!(leases.leases_for_scope(&scope).await, Vec::new());
}
#[tokio::test]
async fn approving_pending_request_revokes_issued_lease_when_status_was_resolved_concurrently() {
async fn approving_pending_request_issues_no_lease_when_status_was_resolved_concurrently() {
// F2 semantics: when a concurrent resolver has already flipped the
// status off `Pending`, the lease store must remain empty — the
// approval write fails first under persist-approval-first ordering,
// so no lease is ever created.
let invocation_id = InvocationId::new();
let scope = sample_scope(invocation_id, "tenant1", "user1");
let approval = approval_request(invocation_id, CapabilityId::new("echo.say").unwrap());
@@ -261,9 +272,7 @@ async fn approving_pending_request_revokes_issued_lease_when_status_was_resolved
status: ApprovalStatus::Denied
}
));
let issued = leases.leases_for_scope(&scope).await;
assert_eq!(issued.len(), 1);
assert_eq!(issued[0].status, CapabilityLeaseStatus::Revoked);
assert_eq!(leases.leases_for_scope(&scope).await, Vec::new());
}
#[tokio::test]