From a71a7ce84f1ac3cec00b9eb0eab8d561be59d593 Mon Sep 17 00:00:00 2001 From: Felix Martin Date: Mon, 25 May 2026 15:48:40 -0400 Subject: [PATCH] Close transition replay loopholes --- src/session.rs | 101 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 81 insertions(+), 20 deletions(-) diff --git a/src/session.rs b/src/session.rs index 1688eb5..99009a7 100644 --- a/src/session.rs +++ b/src/session.rs @@ -48,8 +48,7 @@ struct PolicyAppliedPayload { struct TransitionStartedPayload { schema_version: u16, commitment_id: String, - #[serde(default, skip_serializing_if = "Option::is_none")] - transition_policy_id: Option, + transition_policy_id: String, reason: String, return_target: String, expected_duration_secs: u64, @@ -62,7 +61,7 @@ struct TransitionStartedPayload { #[derive(Clone, Debug, PartialEq, Eq)] struct PendingTransitionPolicy { commitment_id: String, - policy_id: Option, + policy_id: String, runtime_state: RuntimeState, enforcement_level: EnforcementLevel, allowed_context: AllowedContext, @@ -238,7 +237,7 @@ impl SessionController { let transition_payload = TransitionStartedPayload { schema_version: POLICY_SCHEMA_VERSION, commitment_id: next_commitment.id.clone(), - transition_policy_id: Some(transition_policy.id.clone()), + transition_policy_id: transition_policy.id.clone(), reason: reason.into().trim().to_string(), return_target: return_target.into().trim().to_string(), expected_duration_secs: expected_duration.as_secs(), @@ -408,15 +407,9 @@ fn hydrate_session( .with_context(|| { format!("replay policy application at sequence {}", record.sequence) })?, - CommitmentState::Paused - if runtime_state == RuntimeState::Transition - && payload.runtime_state == RuntimeState::Transition => - { - RuntimeState::Transition - } _ => { return Err(anyhow!( - "policy requires active or transition commitment at sequence {}", + "policy requires active commitment at sequence {}", record.sequence )); } @@ -503,7 +496,7 @@ fn hydrate_session( } let mut commitment = active.clone(); commitment.state = composite.commitment_state; - commitment.transition_policy_id = payload.transition_policy_id.clone(); + commitment.transition_policy_id = Some(payload.transition_policy_id.clone()); pending_transition_policy = Some(PendingTransitionPolicy { commitment_id: commitment.id.clone(), policy_id: payload.transition_policy_id, @@ -583,13 +576,11 @@ fn apply_pending_transition_policy( record.sequence )); } - if let Some(expected_policy_id) = expected.policy_id.as_ref() { - if payload.policy_id != *expected_policy_id { - return Err(anyhow!( - "transition policy id mismatch at sequence {}", - record.sequence - )); - } + if payload.policy_id != expected.policy_id { + return Err(anyhow!( + "transition policy id mismatch at sequence {}", + record.sequence + )); } let policy = PolicySnapshot { @@ -662,6 +653,9 @@ fn ensure_record_commitment(record: &EventRecord, commitment_id: &str) -> Result } fn validate_transition_started_payload(payload: &TransitionStartedPayload) -> Result<()> { + if payload.transition_policy_id.trim().is_empty() { + return Err(anyhow!("transition policy id is required")); + } if payload.reason.trim().is_empty() { return Err(anyhow!("transition reason is required")); } @@ -748,7 +742,7 @@ mod tests { serde_json::to_value(TransitionStartedPayload { schema_version: POLICY_SCHEMA_VERSION, commitment_id: "commitment-test".to_string(), - transition_policy_id: Some("policy-transition-test".to_string()), + transition_policy_id: "policy-transition-test".to_string(), reason: "Need to inspect failures".to_string(), return_target: "Return to session controller".to_string(), expected_duration_secs: 300, @@ -1317,6 +1311,73 @@ mod tests { fs::remove_file(path).unwrap(); } + #[test] + fn rejects_extra_transition_policy_after_valid_transition_pair_on_replay() { + let path = temp_log_path("transition-extra-policy"); + let commitment_id = "commitment-test"; + append_active_session_with_transition_and_policy_payload( + &path, + transition_started_payload(), + transition_policy_payload(commitment_id), + ); + { + let mut payload = transition_policy_payload(commitment_id); + payload["policy_id"] = json!("policy-transition-extra"); + let mut log = EventLog::open(&path).unwrap(); + log.append( + EventType::PolicyApplied, + RuntimeState::Transition, + Some(commitment_id.to_string()), + payload, + ) + .unwrap(); + } + + let err = SessionController::new(&path) + .expect_err("extra transition policy outside the transition pair must be rejected"); + + assert!(error_chain_contains( + &err, + "policy requires active commitment" + )); + fs::remove_file(path).unwrap(); + } + + #[test] + fn rejects_transition_started_missing_policy_id_on_replay() { + let path = temp_log_path("transition-missing-policy-id"); + let mut payload = transition_started_payload(); + payload + .as_object_mut() + .unwrap() + .remove("transition_policy_id"); + append_active_session_with_transition_and_policy_payload( + &path, + payload, + transition_policy_payload("commitment-test"), + ); + + let err = SessionController::new(&path) + .expect_err("transition policy id is required in transition payload"); + + assert!(error_chain_contains(&err, "transition_policy_id")); + fs::remove_file(path).unwrap(); + } + + #[test] + fn rejects_transition_started_empty_policy_id_on_replay() { + let path = temp_log_path("transition-empty-policy-id"); + let mut payload = transition_started_payload(); + payload["transition_policy_id"] = json!(""); + append_active_session_with_transition_payload(&path, payload); + + let err = + SessionController::new(&path).expect_err("empty transition policy id must be rejected"); + + assert!(error_chain_contains(&err, "transition policy id")); + fs::remove_file(path).unwrap(); + } + #[test] fn transition_started_payload_has_stable_schema() { let path = temp_log_path("transition-payload");