diff --git a/stellar-contracts/src/lib.rs b/stellar-contracts/src/lib.rs index 9d69a46..f120b6f 100644 --- a/stellar-contracts/src/lib.rs +++ b/stellar-contracts/src/lib.rs @@ -3018,7 +3018,7 @@ impl PetChainContract { // 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 @@ impl PetChainContract { 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; } @@ -10364,15 +10364,10 @@ impl PetChainContract { if caller == owner { return true; } - if let Some(access) = env - .storage() - .instance() - .get::(&SystemKey::EmergencyOverride(( - pet_id, - caller.clone(), - ))) - { - if env.ledger().timestamp() <= access.expires_at { + if let Some(access) = env.storage().instance().get::( + &SystemKey::EmergencyOverride((pet_id, caller.clone())), + ) { + if !is_expired(env.ledger().timestamp(), access.expires_at) { return true; } } @@ -11899,7 +11894,7 @@ impl PetChainContract { .instance() .get::(&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 @@ impl PetChainContract { let key = DataKey::AccessGrant((pet_id, grantee.clone())); if let Some(grant) = env.storage().instance().get::(&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 @@ impl PetChainContract { for delegate in delegates.iter() { let key = DataKey::DecryptionToken((pet_id, delegate.clone())); - if let Some(token) = env - .storage() - .instance() - .get::(&key) - { - let stale = now >= token.expires_at || token.key_version != current_version; - if stale { + if let Some(expires_at) = env.storage().instance().get::(&key) { + if is_expired(now, expires_at) { env.storage().instance().remove(&key); removed += 1; @@ -14300,6 +14290,27 @@ pub(crate) fn safe_increment(env: &Env, count: u64) -> u64 { .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); diff --git a/stellar-contracts/src/test_access_control.rs b/stellar-contracts/src/test_access_control.rs index 60ec413..d1bab68 100644 --- a/stellar-contracts/src/test_access_control.rs +++ b/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() {