fix(oidc): lock-guarded consumeCode — close the double-mint race (Copilot follow-up to #3662) by gloriafolaron · Pull Request #3663 · Leantime/leantime

gloriafolaron · 2026-07-18T21:32:06Z

Addresses the Copilot review comment on the merged #3662 (OidcMobileCode.php):

Cache::pull() is not guaranteed atomic across cache drivers; it can be implemented as get+forget, so two concurrent callers may still both observe a value and mint.

Correct — Laravel's Cache::pull() is get() then forget(), not a single atomic op on any driver, so the "atomic consume" in #3662 didn't fully close the peek→consume double-mint race.

Fix

OidcMobileCode::consumeCode() now takes a non-blocking cache lock keyed on the code and performs the get()+forget() inside it:

Only the lock holder runs the read-and-delete and can return true → at most one exchange mints from a single-use code.
A concurrent consumer that can't take the lock gets false immediately (no blocking) and its exchange is rejected.
Fails closed if the store doesn't implement LockProvider: it logs an error and returns false rather than degrading to a racy non-atomic consume (per Copilot's follow-up — a non-lock store is a misconfiguration, not something to silently degrade for). All default Leantime stores (file/array/redis/memcached/database) support locks, so this only trips on an exotic misconfiguration.

Tests

New OidcMobileCodeTest (5 tests / 7 assertions) against the array store (which supports the lock): peekCode() is non-destructive, consumeCode() burns exactly once (true then false), a consumed code no longer peeks, unknown code → false/null. MobileTest still green (it mocks the store). Pint clean.

Follow-up to #3662 / #3637.

🤖 Generated with Claude Code

… race Copilot (on #3662) correctly flagged that Cache::pull is get()+forget() — not atomic on any driver — so the 'atomic consume' didn't fully close the peek->consume double-mint race: two concurrent exchanges could both read a valid code before either deleted it, and both mint. OidcMobileCode::consumeCode() now takes a non-blocking cache lock keyed on the code and does the get()+forget() inside it, so only the lock holder can return true; a concurrent consumer gets false and must not mint. Falls back to the plain pull only if the store lacks lock support (all default Leantime stores have it). New OidcMobileCodeTest (5 tests): peekCode non-destructive, consumeCode burns once (true then false), consumed code no longer peeks, unknown code -> false/null. MobileTest still green (it mocks the store). Pint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR

Copilot

Pull request overview

This PR hardens the OIDC mobile SSO one-time-code consumption path by making OidcMobileCode::consumeCode() lock-guarded, closing the concurrency window where two parallel exchanges could both mint from the same single-use code. It also adds focused unit coverage for the code store’s single-use contract.

Changes:

Guard consumeCode() with a non-blocking cache lock and perform get() + forget() under the lock.
Add a new unit test suite validating peekCode() non-destructiveness and one-time consumption behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
app/Domain/Oidc/Services/OidcMobileCode.php	Adds lock-guarded consume logic to prevent concurrent double-minting from a single one-time code.
tests/Unit/app/Domain/Oidc/Services/OidcMobileCodeTest.php	Adds unit tests that pin the single-use semantics (peek vs consume, unknown/consumed behaviors).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        } catch (\Throwable) {
+            // Store doesn't support atomic locks — fall back to a plain
+            // read-and-delete. All default Leantime cache stores DO support
+            // locks; this is belt-and-suspenders for an exotic configuration.
+            return Cache::pull($key) !== null;
+        }


…t on #3663) Copilot flagged that the lock-creation fallback dropped back to a non-atomic Cache::pull — reintroducing the double-mint race — and that catching \Throwable was too broad. Replace it with an explicit LockProvider capability check: if the store can't lock, log + return false (fail CLOSED) instead of a best-effort racy consume. No broad catch. All default Leantime stores support locks, so this only trips on a misconfigured exotic store. OidcMobileCodeTest (5) + MobileTest (9) still green; Pint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

+        // Fail CLOSED: without atomic locks we cannot guarantee single-use, so
+        // refuse rather than fall back to a racy non-atomic consume (which would
+        // reintroduce the double-mint window this method exists to prevent). All
+        // default Leantime stores implement LockProvider; a store here that
+        // doesn't is a misconfiguration worth surfacing loudly, not degrading to.
+        if (! Cache::getStore() instanceof LockProvider) {
+            Log::error('OidcMobileCode: cache store does not support atomic locks; refusing mobile SSO code consume. Configure a lock-capable cache store (file/redis/memcached/database).');
+
+            return false;


marcelfolaron · 2026-07-19T12:14:00Z

Status: triaged · Priority: P2 · Next action: human review · Owner: @broskees / @marcelfolaron

Picked up in the daily sweep — new since the last pass. Correct follow-up to the Copilot note on the merged #3662: Cache::pull() is get()+forget() and not atomic on any driver, so the lock-guarded consumeCode() is the right fix to actually close the double-mint race. The fail-closed behavior when the store isn't a LockProvider is the safe default. Advisory only — reviewers already requested; merge gate stays with the team.

…ods for mobile SSO (#3664) * feat(status): public /status discovery endpoint — advertise auth methods for mobile SSO The mobile app calls GET /status (unauthenticated) at connect time to discover which login methods an instance offers, so it can show the right affordances — notably the OIDC 'Sign in with SSO' flow, which stays DORMANT in the app until a backend advertises it here. This is the discovery half of the mobile OIDC bridge (#3637); without it the SSO button never appears even when the bridge is deployed. - app/Domain/Status/Controllers/Index.php — GET /status returns the SAFE public tier: authMethods (password / ldap / oidc, from config), oidcLoginUrl when OIDC is enabled, instanceName, core version, minAppVersion, and an empty ssoProviders that AdvancedAuth can populate via the 'publicStatus' filter. Deliberately OMITS plugin inventory / versions / dbVersion — unauthenticated that is a recon gift (CVE matching); those stay behind auth (the authenticated mobileStatus). - AuthCheck: allow-list 'status' / 'status.index' as public. - Tests: IndexTest (password-only, oidc-advertised + login URL, no-inventory-leak) + AuthCheck public-route test. Pint clean. Answers Q5 in docs/backend-mobile-auth-bridge-plan.md. Follow-up to #3637/#3662/#3663. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR * fix(status): address #3664 Copilot review — strip sensitive keys post-filter + docblocks - Defense in depth: after the publicStatus filter runs, strip a denylist of known-sensitive keys (plugins / plugin versions / dbVersion) so a misbehaving filter (or a future edit) can't leak the recon-risk inventory into this unauthenticated response, even if a plugin gets the public-safe contract wrong. - Add init()/get() method docblocks to match the controller doc style. IndexTest green (3 tests / 10 assertions); Pint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

gloriafolaron requested a review from a team as a code owner July 18, 2026 21:32

gloriafolaron requested review from broskees, Copilot and marcelfolaron and removed request for a team July 18, 2026 21:32

Copilot started reviewing on behalf of gloriafolaron July 18, 2026 21:32 View session

Copilot AI reviewed Jul 18, 2026

View reviewed changes

gloriafolaron requested a review from Copilot July 18, 2026 22:07

Copilot started reviewing on behalf of gloriafolaron July 18, 2026 22:08 View session

Copilot AI reviewed Jul 18, 2026

View reviewed changes

gloriafolaron mentioned this pull request Jul 19, 2026

feat(status): public /status discovery endpoint — advertise auth methods for mobile SSO #3664

Merged

marcelfolaron added the Bug label Jul 19, 2026

marcelfolaron approved these changes Jul 19, 2026

View reviewed changes

marcelfolaron merged commit f22832d into master Jul 19, 2026
12 of 13 checks passed

marcelfolaron deleted the fix/oidc-consume-lock branch July 19, 2026 18:29

gloriafolaron mentioned this pull request Jul 19, 2026

docs(oidc): reword consumeCode docblock to lead with the lock, not Cache::pull #3666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(oidc): lock-guarded consumeCode — close the double-mint race (Copilot follow-up to #3662)#3663

fix(oidc): lock-guarded consumeCode — close the double-mint race (Copilot follow-up to #3662)#3663
marcelfolaron merged 2 commits into
masterfrom
fix/oidc-consume-lock

gloriafolaron commented Jul 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

marcelfolaron commented Jul 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

gloriafolaron commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

marcelfolaron commented Jul 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gloriafolaron commented Jul 18, 2026 •

edited

Loading