From c817727fe76b3f143db8ed5c59a986df56f09d3e Mon Sep 17 00:00:00 2001 From: Gloria <55953099+gloriafolaron@users.noreply.github.com> Date: Sat, 18 Jul 2026 17:31:44 -0400 Subject: [PATCH 1/2] =?UTF-8?q?fix(oidc):=20lock-guarded=20consumeCode=20?= =?UTF-8?q?=E2=80=94=20genuinely=20close=20the=20double-mint=20race?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR --- app/Domain/Oidc/Services/OidcMobileCode.php | 47 +++++++++++--- .../Oidc/Services/OidcMobileCodeTest.php | 62 +++++++++++++++++++ 2 files changed, 101 insertions(+), 8 deletions(-) create mode 100644 tests/Unit/app/Domain/Oidc/Services/OidcMobileCodeTest.php diff --git a/app/Domain/Oidc/Services/OidcMobileCode.php b/app/Domain/Oidc/Services/OidcMobileCode.php index 8808df3b99..d175eb8abf 100644 --- a/app/Domain/Oidc/Services/OidcMobileCode.php +++ b/app/Domain/Oidc/Services/OidcMobileCode.php @@ -17,8 +17,9 @@ * * Cache, not the DB: these live for at most 60s and are consumed once, so a TTL * cache entry is the natural fit (and self-expiring — no sweep needed). The code - * is hashed into the cache key so the raw code is never stored. Cache::pull - * reads-and-deletes, so a replayed code can't be exchanged twice. + * is hashed into the cache key so the raw code is never stored. consumeCode() + * guards its read-and-delete with a cache lock, so a replayed or concurrently + * exchanged code can't be exchanged twice. * * PKCE: each code is bound to the app-supplied `code_challenge`. The exchange * requires the matching verifier, so a code intercepted from the app-scheme @@ -36,6 +37,8 @@ class OidcMobileCode private const TTL_SECONDS = 60; + private const LOCK_SECONDS = 5; + /** * Mint a code bound to a user id + the PKCE code_challenge. Returns the RAW * code (only its hash is used as the cache key, so the raw value is never @@ -76,16 +79,44 @@ public function peekCode(string $rawCode): ?array } /** - * Atomically burn the code and report whether THIS caller consumed it. + * Burn the code once and report whether THIS caller consumed it. * - * Cache::pull is a single read-and-delete, so of two concurrent exchanges - * that both peekCode()'d the same code, exactly one gets true here — the - * other gets false and must not mint (closes the peek→consume double-mint - * race). Also returns false for an already-consumed or unknown code. + * Cache::pull is get()+forget() — NOT a single atomic op on any driver — so a + * bare pull could let two concurrent exchanges both read a valid code before + * either deletes it, and both mint. We guard the read-and-delete with a + * non-blocking cache lock keyed on the code: only the lock holder performs the + * get()+forget() and can return true, so at most one exchange mints from a + * single-use code. A caller that can't take the lock (a concurrent consume is + * in flight) gets false and must not mint; so does an already-consumed or + * unknown code. */ public function consumeCode(string $rawCode): bool { - return Cache::pull($this->key($rawCode)) !== null; + $key = $this->key($rawCode); + + try { + $lock = Cache::lock($key.'.lock', self::LOCK_SECONDS); + } 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; + } + + // Non-blocking: the loser of a concurrent consume gets false immediately + // instead of waiting, and its exchange is rejected. + if (! $lock->get()) { + return false; + } + + try { + $existed = Cache::get($key) !== null; + Cache::forget($key); + + return $existed; + } finally { + $lock->release(); + } } private function key(string $rawCode): string diff --git a/tests/Unit/app/Domain/Oidc/Services/OidcMobileCodeTest.php b/tests/Unit/app/Domain/Oidc/Services/OidcMobileCodeTest.php new file mode 100644 index 0000000000..58c4bcc5ed --- /dev/null +++ b/tests/Unit/app/Domain/Oidc/Services/OidcMobileCodeTest.php @@ -0,0 +1,62 @@ +codes = new OidcMobileCode; + } + + public function test_peek_is_non_destructive_and_returns_the_payload(): void + { + $code = $this->codes->createCode(42, 'challenge-abc'); + + $first = $this->codes->peekCode($code); + $second = $this->codes->peekCode($code); + + $this->assertSame(['userId' => 42, 'challenge' => 'challenge-abc'], $first); + $this->assertSame($first, $second, 'peekCode() must not consume the code'); + } + + public function test_consume_returns_true_once_then_false(): void + { + $code = $this->codes->createCode(7, 'ch'); + + $this->assertTrue($this->codes->consumeCode($code), 'first consume burns the code'); + $this->assertFalse($this->codes->consumeCode($code), 'a single-use code cannot be consumed twice'); + } + + public function test_consumed_code_no_longer_peeks(): void + { + $code = $this->codes->createCode(5, 'ch'); + $this->codes->consumeCode($code); + + $this->assertNull($this->codes->peekCode($code), 'a burned code is gone'); + } + + public function test_consume_unknown_code_returns_false(): void + { + $this->assertFalse($this->codes->consumeCode('never-minted')); + } + + public function test_peek_unknown_code_returns_null(): void + { + $this->assertNull($this->codes->peekCode('never-minted')); + } +} From 61d1dc7348cf71c7f468362c1028f4d26f8546fa Mon Sep 17 00:00:00 2001 From: Gloria <55953099+gloriafolaron@users.noreply.github.com> Date: Sat, 18 Jul 2026 18:07:51 -0400 Subject: [PATCH 2/2] fix(oidc): fail closed when cache store lacks locks (addresses Copilot on #3663) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01NtfhjPwUEEHJb4Jf7714eR --- app/Domain/Oidc/Services/OidcMobileCode.php | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/app/Domain/Oidc/Services/OidcMobileCode.php b/app/Domain/Oidc/Services/OidcMobileCode.php index d175eb8abf..071089b8d6 100644 --- a/app/Domain/Oidc/Services/OidcMobileCode.php +++ b/app/Domain/Oidc/Services/OidcMobileCode.php @@ -2,7 +2,9 @@ namespace Leantime\Domain\Oidc\Services; +use Illuminate\Contracts\Cache\LockProvider; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Str; /** @@ -94,15 +96,19 @@ public function consumeCode(string $rawCode): bool { $key = $this->key($rawCode); - try { - $lock = Cache::lock($key.'.lock', self::LOCK_SECONDS); - } 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; + // 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; } + $lock = Cache::lock($key.'.lock', self::LOCK_SECONDS); + // Non-blocking: the loser of a concurrent consume gets false immediately // instead of waiting, and its exchange is rejected. if (! $lock->get()) {