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

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;
 
 /**
@@ -17,8 +19,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 +39,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 +81,48 @@ 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);
+
+        // 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()) {
+            return false;
+        }
+
+        try {
+            $existed = Cache::get($key) !== null;
+            Cache::forget($key);
+
+            return $existed;
+        } finally {
+            $lock->release();
+        }
     }
 
     private function key(string $rawCode): string

tests/Unit/app/Domain/Oidc/Services/OidcMobileCodeTest.php

@@ -0,0 +1,62 @@
+<?php
+
+namespace Tests\Unit\app\Domain\Oidc\Services;
+
+use Leantime\Domain\Oidc\Services\OidcMobileCode;
+
+/**
+ * Unit tests for the mobile SSO one-time-code store.
+ *
+ * Pins the single-use contract: peekCode() is non-destructive, and consumeCode()
+ * burns the code exactly once (returns true for the caller that burns it, false
+ * for a second/unknown code). Runs against the array cache store from
+ * \Unit\TestCase, which supports the atomic lock consumeCode() takes.
+ */
+class OidcMobileCodeTest extends \Unit\TestCase
+{
+    private OidcMobileCode $codes;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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'));
+    }
+}