fix: expire oid4vci recovery state on a test clock and gate unlisted citation roots - #1101
Merged
Merged
Conversation
Every deadline the offer store holds is a whole Unix second, so a recovery test that expired state by configuring a one-second window and sleeping past it was asking the wall clock to land where the test needed it: state created just before a second boundary held almost none of the window it was given, and a loaded machine spent the rest, so a request the test expected to be served came back refused. Give the service a clock to read instead. `DeliveryService` holds the function it reads the current second from, `with_clock` replaces it, and the recovery tests that need a deadline to have passed move their own reading forward by whole seconds rather than sleeping. Those tests now run on the lifetimes a deployment may configure, and a wallet proof they send is dated by the same reading the service judges it against. The same rewrite covers the two other parts of that test, for the access token and the nonce windows, and the sibling saturation test that also expired an offer ledger by sleeping. Security review note: this touches expiry enforcement, so the seam is built to be unreachable from a deployment. `DeliveryService::load` reads the system clock, `with_clock` is hidden from the documented surface and reachable only from a caller that already constructs a service from halves, and no configuration key selects a clock. The store is unchanged: it still takes the reading as an argument and still expires an entry when `expires_at <= now`. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…directory A citation into a top-level directory REPOSITORY_ROOTS omits parses as no citation at all, so its anchor is checked against nothing. That gap used to surface only through a docs-suite test gated behind the path-filtered Docs checks job, so a PR that added a tracked directory without touching docs paths could merge clean and leave the gap for the next PR to hit. Add checkRepositoryRoots, which compares git's tracked top-level directories against REPOSITORY_ROOTS and runs from the CLI entry point on every invocation, including the path-filter-free Evidence anchors job that already gates every pull request. Rewrite the existing "names every top-level directory" test to call the new function against the real checkout instead of duplicating its git-diffing logic, and add tests proving it fails closed and names the missing directory. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two CI reliability fixes from the merge run of #1071, #1074, and #1087.
test(oid4vci): expire recovery state on a clock the test moves
Closes #1066.
expiry_cleanup_preserves_live_state_and_releases_expired_stateconfigured a one-second offer window and slept past it. Store deadlines are whole Unix seconds, so state created just before a boundary held almost none of its window, and a loaded runner spent the rest: the supposedly live redemption came back 400. It failed again on 2026-09-15 on #1074, which touches nothing in this crate.DeliveryServicenow holds the function it reads the current second from, and the recovery tests move their own reading forward instead of sleeping. They run on lifetimes a deployment may actually configure (the old 1s and 5s values are outsideStoreConfig::validate()'s 60..=900 range), and a wallet proof is dated by the same reading the service judges it against. The preserved assertions are unchanged: an expired redemption stays indistinguishable from an unknown code, two live offers still fit only if the expired one was pruned, and the live redemption still returns 200.The same rewrite covers the access-token and nonce parts of that test and the sibling
token_saturation_does_not_spend_the_offer_that_could_not_be_exchanged, which carried a 5.1s sleep with the same hazard. The recovery suite drops from about 11s of sleeps to 0.18s.Security review note. This touches expiry enforcement. The seam is built to be unreachable from a deployment:
DeliveryService::loadinstalls the system clock,with_clockis hidden from the documented surface and reachable only throughwith_halves, which already takes the authorizer and issuer and is strictly more powerful. No configuration key selects a clock.store.rsis untouched and still expires an entry whenexpires_at <= now.fix(docs): fail the evidence-anchor checker on an unlisted top-level directory
A citation into a top-level directory
REPOSITORY_ROOTSomits parses as no citation at all, so its anchor is checked against nothing. That invariant was only asserted by a test in the docs suite, which runs in the path-filteredDocs checksjob.#1071 added the tracked top-level directory
scripts/while touching onlyscripts/install-local.sh, so the path filter skippedDocs checks, it merged green, and main then failed those checks for the next PR that ran them. The roots list was corrected in #1087; this closes the gap that let it through.checkRepositoryRootscompares git's tracked top-level directories againstREPOSITORY_ROOTSand runs from the CLI entry point, so it runs in theEvidence anchorsjob, which has no path filter and already gates the branch. No extra CI cost. The existing test now calls that function instead of duplicating its logic, and two tests prove it fails closed and names the missing directory.Verification
cargo fmt --check,cargo clippy -p registry-evidence-oid4vci --all-targets -- -D warnings: clean.cargo test --locked -p registry-evidence-oid4vci: 157 + 1 + 4 + 5 + 15 passed, 0 failed (1 ignored is the pinned stock-issuer container gate).products/evidence/scripts/check-source-neutrality.sh: passed.docs/site:npm test601/601,npm run checkclean, andnode docs/site/scripts/check-evidence-anchors.mjspasses on the real tree.