From 2d93db8bbbefbe3650bf1393877dba33a629f475 Mon Sep 17 00:00:00 2001 From: samuel2926i39-art Date: Sat, 29 Aug 2026 12:21:49 -0700 Subject: [PATCH] fix(stellar-contracts): unify expiry boundary semantics across authorization paths Different code paths compared `now` against `expires_at` with different operators for what should be the same "is this expired" question: - check_access (AccessGrant) and get_active_consents (Consent): now >= expires_at - compact_storage's consent cleanup sweep: now > expires_at - is_emergency_authorized (EmergencyOverride): now <= expires_at (i.e. expired only once now > expires_at) At the exact expiry instant these paths disagreed: a consent record could be "expired" for get_active_consents but not yet stale enough for the cleanup sweep to remove; an emergency override could still authorize access one full second after check_access-style logic would deny an equivalent access grant. Adds one canonical `is_expired(now, expires_at) -> bool` helper (`now >= expires_at`, the fail-safe/conservative direction) and routes every authorization-relevant expiry check through it: AccessGrant (check_access, compact_storage sweep), Consent (get_active_consents, compact_storage sweep), EmergencyOverride (is_emergency_authorized), and decryption-delegation tokens (compact_storage sweep). No public ABI, storage layout, or error discriminants changed - only comparison logic. Closes #1159 --- stellar-contracts/src/lib.rs | 33 ++++++++++--- stellar-contracts/src/test_access_control.rs | 52 ++++++++++++++++++++ 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/stellar-contracts/src/lib.rs b/stellar-contracts/src/lib.rs index ddf6d506..e9d63611 100644 --- a/stellar-contracts/src/lib.rs +++ b/stellar-contracts/src/lib.rs @@ -2801,7 +2801,7 @@ impl PetChainContract { { if grant.is_active && grant.grantee == caller { 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; } } @@ -2884,7 +2884,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; } @@ -9326,7 +9326,7 @@ impl PetChainContract { if let Some(access) = env.storage().instance().get::( &SystemKey::EmergencyOverride((pet_id, caller.clone())), ) { - if env.ledger().timestamp() <= access.expires_at { + if !is_expired(env.ledger().timestamp(), access.expires_at) { return true; } } @@ -10818,7 +10818,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); } @@ -10892,7 +10892,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)); } @@ -11039,7 +11039,7 @@ impl PetChainContract { for delegate in delegates.iter() { let key = DataKey::DecryptionToken((pet_id, delegate.clone())); if let Some(expires_at) = env.storage().instance().get::(&key) { - if now >= expires_at { + if is_expired(now, expires_at) { env.storage().instance().remove(&key); removed += 1; @@ -12844,6 +12844,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 60ec4130..d1bab68f 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() {