stellar-contracts: unify access-grant expiry boundary semantics

stellar-contracts/src/lib.rs

@@ -174,7 +174,7 @@
 // `pub use` keeps every type nameable exactly as before (`Dispute`,
 // `DisputeKey`, ...) from both this file and external callers.
 mod disputes;
 pub use disputes::*;
 
 #[cfg(test)]
 mod test_attachment_limit;
@@ -182,13 +182,13 @@
 mod test_behavior_records;
 #[cfg(test)]
 mod test_breeding;
 #[cfg(test)]
 mod test_breeding_genetics;
 #[cfg(test)]
 mod test_discriminant_stability;
 #[cfg(test)]
 mod test_dispute_voting;
 #[cfg(test)]
 // NOTE: test_disputes.rs and test_book_slot.rs were wired but reference
 // contract features (dispute arbitration + slot booking) that were removed
 // and no longer exist in this crate, so they fail to compile. They are
@@ -212,7 +212,7 @@
 mod test_nutrition_plan;
 #[cfg(test)]
 mod test_pet_birthday_validation;
 #[cfg(test)]
 mod test_search_medical_records;
 #[cfg(test)]
 mod test_access_grant_pagination;
@@ -299,7 +299,7 @@
 const MAX_INGREDIENTS: u32 = 50;
 
 /// Maximum prerequisite IDs in `TrainingMilestone::prerequisites`.
 const MAX_PREREQUISITES: u32 = 20;
 
 /// Maximum entries in the chain-of-custody Vec stored per pet.
 /// ~100 transfers × ~80 bytes/entry = ~8 KiB, well within the 64 KiB limit.
@@ -486,7 +486,7 @@
     TooManySearchTokens = 5,
 }
 
 #[contracterror]
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 #[repr(u32)]
 pub enum ContractError {
@@ -2792,7 +2792,7 @@
         subscriber.require_auth();
 
         if event_types.is_empty() || pet_ids.is_empty() || ttl == 0 {
             panic_with_error!(&env, ContractError::InvalidInput);
         }
 
         let now = env.ledger().timestamp();
@@ -2824,7 +2824,7 @@
         }
 
         if active_count >= Self::max_subscriptions_per_address(env.clone()) {
             panic_with_error!(&env, ContractError::TooManyItems);
         }
 
         let current_id: u64 = env
@@ -3018,7 +3018,7 @@
             // the revocation without needing a separate sweep. (#1162)
             if grant.is_active && grant.grantee == caller && grant.granter == pet.owner {
                 if let Some(expires_at) = grant.expires_at {
-                    if env.ledger().timestamp() >= expires_at {
+                    if is_expired(env.ledger().timestamp(), expires_at) {
                         return AccessLevel::None;
                     }
                 }
@@ -3101,7 +3101,7 @@
                     if consent.is_active {
                         // Filter out expired consents
                         if let Some(expires_at) = consent.expires_at {
-                            if now >= expires_at {
+                            if is_expired(now, expires_at) {
                                 expired_count = expired_count.saturating_add(1);
                                 continue;
                             }
@@ -3904,7 +3904,7 @@
     /// and read rarely) so critical persistent records are not silently
     /// archived/expired by the ledger.
     fn bump_persistent_ttl<K>(env: &Env, key: &K)
     where
         K: IntoVal<Env, Val>,
     {
         env.storage()
@@ -4994,7 +4994,7 @@
         Self::validate_pet_name(&env, &name);
         Self::validate_breed(&env, &species, &breed);
         let canonical_microchip = microchip_id
             .as_ref()
             .map(|value| Self::canonicalize_microchip_id(&env, value));
         if let Some(ref identifier) = canonical_microchip {
             if env.storage().instance().has(&DataKey::MicrochipIndex(identifier.clone())) {
@@ -5244,7 +5244,7 @@
                         if existing_id != id {
                             panic_with_error!(&env, ContractError::InvalidInput);
                         }
                     }
                 }
                 if let Some(ref previous) = pet.microchip_id {
                     env.storage().instance().remove(&DataKey::MicrochipIndex(previous.clone()));
@@ -5778,7 +5778,7 @@
         })
     }
 
     fn parse_birthday_timestamp(birthday: &String) -> Result<u64, ContractError> {
         let len = birthday.len() as usize;
         if len == 0 || len > 20 {
             return Err(ContractError::InvalidInput);
@@ -5821,7 +5821,7 @@
         Ok(days_since_epoch * 86_400)
     }
 
     fn parse_fixed_digits(bytes: &[u8]) -> Result<u32, ContractError> {
         let mut value = 0u32;
         for b in bytes {
             if !b.is_ascii_digit() {
@@ -5849,7 +5849,7 @@
         }
     }
 
     fn days_from_civil(year: i32, month: i32, day: i32) -> Result<u64, ContractError> {
         let adjusted_year = year - if month <= 2 { 1 } else { 0 };
         let era = if adjusted_year >= 0 {
             adjusted_year / 400
@@ -6320,7 +6320,7 @@
                 }
             }
             idx += 1;
         }
 
         let last_examined = idx.saturating_sub(1);
         let next_cursor = if last_examined < count { last_examined } else { 0 };
@@ -6935,7 +6935,7 @@
     ///
     /// `field` names the offending field so callers can surface a clear error.
     /// Returns `ContractError::InvalidInput` when the limit is exceeded.
     fn validate_len(field: &str, value: &String, max: u32) -> Result<(), ContractError> {
         let _ = field;
         if value.len() > max {
             return Err(ContractError::InvalidInput);
@@ -8276,7 +8276,7 @@
             issue_time: now,
             expiry,
             revoked: false,
             revoked_at: None,
             revocation_reason: None,
         };
         env.storage()
@@ -8332,7 +8332,7 @@
 
         // Lifecycle check: reject revoked or expired certificates.
         if let Some(lifecycle) = env
             .storage()
             .instance()
             .get::<MedicalKey, CertificateLifecycle>(&MedicalKey::CertificateLifecycle((
                 pet_id, vaccination_id,
@@ -9869,7 +9869,7 @@
             .all(|b| matches!(b, b'a'..=b'z' | b'2'..=b'7'))
     }
 
     fn validate_ipfs_hash(_env: &Env, hash: &String) -> Result<(), ContractError> {
         let len = hash.len() as usize;
         if len > 128 {
             return Err(ContractError::InvalidIpfsHash);
@@ -10364,15 +10364,10 @@
         if caller == owner {
             return true;
         }
-        if let Some(access) = env
-            .storage()
-            .instance()
-            .get::<SystemKey, EmergencyOverride>(&SystemKey::EmergencyOverride((
-                pet_id,
-                caller.clone(),
-            )))
-        {
-            if env.ledger().timestamp() <= access.expires_at {
+        if let Some(access) = env.storage().instance().get::<SystemKey, EmergencyOverride>(
+            &SystemKey::EmergencyOverride((pet_id, caller.clone())),
+        ) {
+            if !is_expired(env.ledger().timestamp(), access.expires_at) {
                 return true;
             }
         }
@@ -11899,7 +11894,7 @@
                         .instance()
                         .get::<ConsentKey, Consent>(&ConsentKey::Consent(cid))
                     {
-                        let expired = consent.expires_at.map(|exp| now > exp).unwrap_or(false);
+                        let expired = consent.expires_at.map(|exp| is_expired(now, exp)).unwrap_or(false);
                         if !consent.is_active || expired {
                             stale_indices.push_back(i);
                         }
@@ -11973,7 +11968,7 @@
                     let key = DataKey::AccessGrant((pet_id, grantee.clone()));
                     if let Some(grant) = env.storage().instance().get::<DataKey, AccessGrant>(&key)
                     {
-                        let expired = grant.expires_at.map(|exp| now >= exp).unwrap_or(false);
+                        let expired = grant.expires_at.map(|exp| is_expired(now, exp)).unwrap_or(false);
                         if !grant.is_active || expired {
                             stale.push_back((i, grantee));
                         }
@@ -12120,13 +12115,8 @@
 
         for delegate in delegates.iter() {
             let key = DataKey::DecryptionToken((pet_id, delegate.clone()));
-            if let Some(token) = env
-                .storage()
-                .instance()
-                .get::<DataKey, DecryptionDelegation>(&key)
-            {
-                let stale = now >= token.expires_at || token.key_version != current_version;
-                if stale {
+            if let Some(expires_at) = env.storage().instance().get::<DataKey, u64>(&key) {
+                if is_expired(now, expires_at) {
                     env.storage().instance().remove(&key);
                     removed += 1;
 
@@ -13292,7 +13282,7 @@
             panic_with_error!(&env, ContractError::InputStringTooLong);
         }
         // Bound medications Vec to prevent unbounded inline serialisation. (#1153)
         if medications.len() > MAX_VEC_MEDS {
             panic_with_error!(&env, ContractError::TooManyItems);
         }
 
@@ -14300,6 +14290,27 @@
         .unwrap_or_else(|| panic_with_error!(env, ContractError::CounterOverflow))
 }
 
+// --- EXPIRY BOUNDARY HELPER (Issue #1159) ---
+//
+// Different authorization paths in this contract (access grants, consent
+// records, emergency-override authorization, decryption-delegation tokens)
+// each independently compared `now` against an `expires_at` timestamp, and
+// had drifted onto two different conventions: some treated the exact
+// expiry instant as already-expired (`now >= expires_at`), others treated
+// it as still-valid (`now <= expires_at`, i.e. only expired once
+// `now > expires_at`). At the exact boundary second, a grant/consent/
+// override could be "expired" under one code path and "still active"
+// under another for the same timestamp.
+//
+// This defines the single canonical rule used everywhere in this contract:
+// a resource with expiry `expires_at` is expired starting at, and
+// including, `expires_at` itself. This is the fail-safe direction (it
+// denies access one instant earlier than the lenient alternative would),
+// and matches what most existing call sites already did.
+pub(crate) fn is_expired(now: u64, expires_at: u64) -> bool {
+    now >= expires_at
+}
+
 // --- ENCRYPTION HELPERS ---
 fn encrypt_sensitive_data(env: &Env, data: &Bytes, key: &Bytes) -> (Bytes, Bytes) {
     let nonce = derive_encryption_nonce(env);

stellar-contracts/src/test_access_control.rs

@@ -270,6 +270,58 @@ fn test_access_expiry() {
     assert_eq!(access_level, AccessLevel::None);
 }
 
+// Issue #1159: exact-boundary case. At `now == expires_at` (the expiry
+// instant itself), check_access and the compact_storage cleanup sweep must
+// agree that the grant is expired — both now route through the single
+// shared `is_expired` helper (`now >= expires_at`).
+#[test]
+fn test_access_grant_expires_at_exact_boundary_instant() {
+    let env = Env::default();
+    env.mock_all_auths();
+    env.budget().reset_unlimited();
+    let contract_id = env.register_contract(None, PetChainContract);
+    let client = PetChainContractClient::new(&env, &contract_id);
+
+    let owner = Address::generate(&env);
+    let grantee = Address::generate(&env);
+
+    let pet_id = client.register_pet(
+        &owner,
+        &String::from_str(&env, "Rex"),
+        &String::from_str(&env, "2019-01-01"),
+        &Gender::Male,
+        &Species::Dog,
+        &String::from_str(&env, "Boxer"),
+        &String::from_str(&env, "Brindle"),
+        &28u32,
+        &None,
+        &PrivacyLevel::Private,
+    );
+
+    let now = 1000;
+    env.ledger().with_mut(|l| l.timestamp = now);
+
+    let expires_at = now + 100;
+    client.grant_access(&pet_id, &grantee, &AccessLevel::Full, &Some(expires_at));
+
+    // One instant before expiry: still active.
+    env.ledger().with_mut(|l| l.timestamp = expires_at - 1);
+    assert_eq!(client.check_access(&pet_id, &grantee), AccessLevel::Full);
+
+    // Exactly at the expiry instant: must already be expired (not one
+    // second later) — this is the boundary #1159 is about.
+    env.ledger().with_mut(|l| l.timestamp = expires_at);
+    assert_eq!(client.check_access(&pet_id, &grantee), AccessLevel::None);
+
+    // compact_storage's cleanup sweep must agree at the same instant: the
+    // grant is stale and gets removed, not kept around as "not yet expired".
+    let removed = client.compact_storage(&pet_id, &owner);
+    assert!(
+        removed > 0,
+        "compact_storage must treat a grant as stale at the exact expiry instant, matching check_access"
+    );
+}
+
 #[test]
 #[ignore = "extend_access_grant not yet implemented"]
 fn test_extend_access_grant_updates_expiry() {