Skip to content

stellar-contracts: unify access-grant expiry boundary semantics - #1281

Merged
llinsss merged 2 commits into
DogStark:mainfrom
samuel2926i39-art:fix/1159-access-expiry-boundary
Aug 31, 2026
Merged

stellar-contracts: unify access-grant expiry boundary semantics#1281
llinsss merged 2 commits into
DogStark:mainfrom
samuel2926i39-art:fix/1159-access-expiry-boundary

Conversation

@samuel2926i39-art

Copy link
Copy Markdown
Contributor

Summary

Different authorization-relevant expiry checks in stellar-contracts/src/lib.rs compared now against expires_at using different operators for what should be one consistent question ("is this expired"):

Path Entity Old comparison
check_access AccessGrant now >= expires_at
get_active_consents Consent now >= expires_at
compact_storage's consent cleanup sweep Consent now > expires_at
compact_storage's access-grant cleanup sweep AccessGrant now >= expires_at
is_emergency_authorized EmergencyOverride now <= expires_at authorized (i.e. expired only when now > expires_at)
compact_storage's decryption-delegation sweep delegation token now >= expires_at

At the exact expiry instant (now == expires_at) these disagreed: a consent record could already read as "expired" via get_active_consents while the cleanup sweep didn't yet consider it stale enough to remove; an emergency responder's override could still authorize access for one full extra second past the point an equivalent access grant would already be denied by check_access.

What changed

  • Added one canonical helper in stellar-contracts/src/lib.rs (near the existing safe_increment counter helper): pub(crate) fn is_expired(now: u64, expires_at: u64) -> bool { now >= expires_at } — the fail-safe/conservative direction (denies one instant earlier rather than later), and the convention the majority of existing call sites already used.
  • Routed every authorization-relevant expiry check through it: check_access, get_active_consents, both compact_storage cleanup-sweep branches (consent and access-grant), is_emergency_authorized, and the decryption-delegation cleanup sweep.
  • Net behavior change is in exactly two places: the consent cleanup sweep (>>=, now agrees with get_active_consents) and is_emergency_authorized (<= → strict <, now agrees with check_access's convention for the equivalent access-grant case). Every other call site already used >= and is unchanged in behavior, just now expressed via the shared helper instead of an inline comparison, so a future edit can't silently drift it back out of sync.
  • No public ABI, storage layout, or ContractError discriminants changed — this is comparison-logic only.

⚠️ Verification limitation — please read before reviewing

I could not compile, run, or test any of this. stellar-contracts (the crate this file belongs to) currently fails to build on a clean upstream/main checkout with 346 pre-existing compile errors — duplicate const definitions, a #[contracterror] macro panicking, ContractError not resolving inside some impl blocks, tests calling contract methods that don't exist (e.g. extend_access_grant, referenced by an already-#[ignore]d test in test_access_control.rs) — none of which are related to expiry logic or anything this PR touches. This is a much larger, unrelated problem with the base branch itself.

Given that, everything here was done by careful direct code reading rather than compiler/test feedback:

  • I traced every place AccessGrant/Consent/EmergencyOverride/delegation-token expires_at fields are compared against now across the whole file (grepped and manually read each call site) to build the table above.
  • I ran cargo build --lib after making the change and confirmed none of the 346 errors are newly introduced by or located at any line this PR touches (checked by exact line number and by searching the full error log for is_expired) — but this is not the same as a passing build, since the crate doesn't produce one either way.
  • I added one new test (test_access_grant_expires_at_exact_boundary_instant in test_access_control.rs, modeled directly on the existing, already-passing-in-spirit test_access_expiry test's exact conventions) covering the boundary for check_access + compact_storage agreement, but I cannot confirm it actually compiles or passes given the crate-wide breakage.

I'd strongly recommend a maintainer either fix the underlying compile breakage first, or manually build/test this specific diff against a working local checkout before merging — I don't want to claim green CI that doesn't exist. Happy to also take on the broader compile-breakage fix as its own effort if that's useful, though it looked substantial enough (346 errors) to warrant separate scoping.

Threat-model note

The behavior change makes both fixed paths more conservative, not less: consent records become eligible for cleanup one instant earlier than before (no data-integrity risk — cleanup just removes already-inactive/expired records), and emergency-override authorization now denies access at the same instant an equivalent access grant already would, closing a one-second window where two conceptually-identical "is this still authorized" checks could disagree. Neither change touches who can grant or revoke access — only exactly when an already-set expiry takes effect.

Closes #1159

…ization 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 DogStark#1159
@drips-wave

drips-wave Bot commented Aug 29, 2026

Copy link
Copy Markdown

@samuel2926i39-art Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@llinsss
llinsss merged commit d0632e8 into DogStark:main Aug 31, 2026
0 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[stellar-contracts] Make access-grant expiry boundary semantics consistent

2 participants