Close transition replay loopholes

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: String,
-    transition_policy_id: Option<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<String>,
+    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 {
-        if payload.policy_id != *expected_policy_id {
+        return Err(anyhow!(
-            return Err(anyhow!(
+            "transition policy id mismatch at sequence {}",
-                "transition policy id mismatch at sequence {}",
+            record.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");