Harden OIDC token store lock and expiry clamp

FileTokenStore released its cross-process lock by bare path, so a hold
that outran lockStaleMillis (and was therefore stolen and recreated by a
peer) deleted the peer's live lock on release, admitting a third acquirer
alongside it and breaking the mutual exclusion the lock guards. The
release now verifies ownership: acquireLock stamps a unique nonce
(pid@host + timestamp + UUID) into the lock and returns it, and
releaseLock deletes the lock only when it still carries that nonce. A
stamp that cannot be written degrades to a lock-free refresh rather than
holding an unverifiable lock. The frozen cross-language contract in
design/oidc-token-persistence.md now documents the owner stamp and the
ownership-verified release so other clients mirror it.

OidcDeviceAuth.adopt() capped a loaded expiry but never floored it, so a
tampered expires_at_millis near Long.MIN_VALUE made the validity check
(now < expiresAtMillis - skew) underflow to a large positive and serve a
garbage-expiry token as valid forever. adopt() now clamps the expiry to
[0, now + maxLife], keeping the check underflow-safe while an
already-expired entry still falls through to a refresh.

Add tests: a peer-stolen lock survives our release; a tampered far-past
expiry is refreshed rather than served; the parseLast() truncated-
document reject; and a real FileTokenStore.save rename failure throws
OidcAuthException and leaves no .tmp file. Both regression guards were
proven to fail with their fix reverted.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: glasstiger

core/src/main/java/io/questdb/client/cutlass/auth/FileTokenStore.java

@@ -52,6 +52,7 @@
 import java.nio.file.attribute.PosixFilePermission;
 import java.nio.file.attribute.PosixFilePermissions;
 import java.util.Set;
+import java.util.UUID;
 
 /**
  * The default {@link TokenStore}: one plaintext JSON file per identity under a directory, with the
@@ -199,26 +200,25 @@ public void clear(TokenStoreKey key) {
     @Override
     public boolean inLock(TokenStoreKey key, CriticalSection action) {
         Path lock = null;
-        boolean held;
+        // the unique owner nonce stamped into the lock when we acquired it, or null if we did not (or could
+        // not) acquire one and are running lock-free; releaseLock deletes the lock only when it still carries
+        // this nonce, so we never delete a lock a peer has since stolen
+        String nonce = null;
         try {
             ensureDirectory();
             lock = lockFile(key);
-            held = acquireLock(lock);
+            nonce = acquireLock(lock);
         } catch (IOException e) {
             // could not prepare the lock directory or file; run without the lock. Layer-1 atomic
             // replacement still keeps every reader consistent - only a rotating-refresh-token race across
             // processes is left unguarded for this one refresh.
-            held = false;
+            nonce = null;
         }
         try {
             return action.run();
         } finally {
-            if (held) {
-                try {
-                    Files.deleteIfExists(lock);
-                } catch (IOException ignore) {
-                    // best-effort release; a leftover lock goes stale and the next acquirer steals it
-                }
+            if (nonce != null) {
+                releaseLock(lock, nonce);
             }
         }
     }
@@ -281,6 +281,15 @@ private static void createLockFile(Path lock) throws IOException {
         }
     }
 
+    private static String newLockNonce() {
+        // a per-acquisition owner stamp: the pid@host and the acquire time are human-readable debugging aids,
+        // and the random UUID guarantees two acquisitions never share a stamp even within one pid and one
+        // millisecond, so releaseLock's ownership check is exact rather than probabilistic
+        return ManagementFactory.getRuntimeMXBean().getName() // typically pid@host
+                + ' ' + System.currentTimeMillis()
+                + ' ' + UUID.randomUUID();
+    }
+
     private static boolean nullableEquals(String keyValue, StringSink fileValue) {
         boolean fileHasValue = fileValue.length() > 0;
         if (keyValue == null) {
@@ -402,6 +411,27 @@ private static void putStringMember(StringSink sink, String name, CharSequence v
         putString(sink, value);
     }
 
+    private static void releaseLock(Path lock, String nonce) {
+        // release our own lock only: re-read it and delete it solely when it still carries our nonce. A hold
+        // that outran lockStaleMillis may have been judged stale and stolen (deleted and recreated) by a peer;
+        // deleting by bare path would then remove the peer's live lock and admit a third acquirer alongside it,
+        // defeating the mutual exclusion this lock exists to provide. A microscopic window remains if a steal
+        // lands between the read and the delete, but that is bounded to one syscall gap rather than the whole
+        // hold, so a misconfigured staleness window degrades to at most the documented double-refresh rather
+        // than corrupting a peer's lock state.
+        try {
+            byte[] content = Files.readAllBytes(lock);
+            if (nonce.equals(new String(content, StandardCharsets.UTF_8))) {
+                Files.deleteIfExists(lock);
+            }
+            // otherwise a peer now owns this lock file; leave it for that owner (or staleness) to reclaim
+        } catch (NoSuchFileException e) {
+            // already gone (stolen and not yet recreated, or removed elsewhere); nothing to release
+        } catch (IOException ignore) {
+            // best-effort release; a leftover lock goes stale and the next acquirer steals it
+        }
+    }
+
     private static void restrictToOwner(Path directory) {
         // best-effort: the at-rest protection of the plaintext token files is exactly these owner-only
         // directory permissions, so tighten a pre-existing directory rather than trust whatever it had. On a
@@ -467,25 +497,40 @@ private static void writeAndFlush(Path file, byte[] content) throws IOException
         }
     }
 
-    private static void writeLockHolder(Path lock) {
-        // record the holder (pid@host) and a creation timestamp for debugging only; never fail acquisition
-        // over it. Staleness is judged by the file's mtime, not by parsing this content
+    private static boolean writeLockHolder(Path lock, String nonce) {
+        // stamp the lock with the owner nonce. Unlike the staleness mtime (which only needs to be recent),
+        // this content is what releaseLock checks before deleting, so it must be written reliably; report a
+        // failure so acquireLock drops an unverifiable lock rather than hold one it cannot safely release.
+        // Writing also refreshes the mtime, which is what isStale reads
         try {
-            String holder = ManagementFactory.getRuntimeMXBean().getName() // typically pid@host
-                    + ' ' + System.currentTimeMillis();
-            Files.write(lock, holder.getBytes(StandardCharsets.UTF_8), StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING);
-        } catch (Exception ignore) {
-            // best-effort metadata only
+            Files.write(lock, nonce.getBytes(StandardCharsets.UTF_8), StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING);
+            return true;
+        } catch (Exception e) {
+            return false;
         }
     }
 
-    private boolean acquireLock(Path lock) {
+    private String acquireLock(Path lock) {
+        // returns the unique owner nonce stamped into the lock on success, or null if it could not be acquired
+        // within the budget. releaseLock uses the nonce to verify ownership before deleting, so a hold that
+        // outran lockStaleMillis (and was stolen by a peer) never deletes the peer's lock on release
+        final String nonce = newLockNonce();
         final long deadline = System.currentTimeMillis() + lockAcquireBudgetMillis;
         while (true) {
             try {
                 createLockFile(lock);
-                writeLockHolder(lock);
-                return true;
+                if (writeLockHolder(lock, nonce)) {
+                    return nonce;
+                }
+                // the exclusive create won the lock but the owner nonce could not be stamped, so releaseLock
+                // could not later prove ownership and would risk deleting a peer's lock; drop the file we just
+                // created and degrade to a lock-free refresh rather than hold an unverifiable lock
+                try {
+                    Files.deleteIfExists(lock);
+                } catch (IOException ignore) {
+                    // another acquirer may have removed it; the next createLockFile settles the race
+                }
+                return null;
             } catch (FileAlreadyExistsException e) {
                 if (isStale(lock)) {
                     // a crashed holder left the lock behind; steal it
@@ -498,11 +543,11 @@ private boolean acquireLock(Path lock) {
                     // between several acquirers (or a misconfigured tiny lockStaleMillis) must not hot-spin
                 }
                 if (System.currentTimeMillis() >= deadline) {
-                    return false; // give up and run without the lock rather than stall a sign-in
+                    return null; // give up and run without the lock rather than stall a sign-in
                 }
                 Os.sleep(LOCK_POLL_SLICE_MILLIS);
             } catch (IOException e) {
-                return false; // unexpected IO; degrade to no lock
+                return null; // unexpected IO; degrade to no lock
             }
         }
     }

core/src/main/java/io/questdb/client/cutlass/auth/OidcDeviceAuth.java

@@ -1031,11 +1031,15 @@ private boolean adopt(PersistedToken token) {
         refreshToken = token.getRefreshToken();
         // the file is attacker-writable (and may have been written under a skewed clock), so bound how long
         // the loaded token is trusted exactly as storeTokens() bounds a token from the wire: never past
-        // MAX_EXPIRES_IN_SECONDS from now. Capping (not flooring) the expiry preserves an already-expired
-        // entry, so a stale access token still falls through to a refresh rather than being served forever.
+        // MAX_EXPIRES_IN_SECONDS from now. Clamp the expiry to [0, now + maxLife]: the ceiling stops a tampered
+        // far-future expiry from being trusted for decades, and the floor of 0 keeps a tampered far-past expiry
+        // in the past (1970, well before now) while keeping the validity check (now < expiresAtMillis - skew)
+        // underflow-safe - a near-Long.MIN_VALUE expiry would otherwise wrap that subtraction to a huge
+        // positive and serve a garbage-expiry token as valid forever. An already-expired entry still reads as
+        // expired and falls through to a refresh rather than being served.
         long maxTokenLifeMillis = MAX_EXPIRES_IN_SECONDS * 1000L;
         tokenTtlMillis = Math.max(0L, Math.min(token.getTokenTtlMillis(), maxTokenLifeMillis));
-        expiresAtMillis = Math.min(token.getExpiresAtMillis(), System.currentTimeMillis() + maxTokenLifeMillis);
+        expiresAtMillis = Math.max(0L, Math.min(token.getExpiresAtMillis(), System.currentTimeMillis() + maxTokenLifeMillis));
         // it is already on disk, so a later non-rotating refresh must not rewrite the file
         lastPersistedRefreshToken = refreshToken;
         return true;

core/src/test/java/io/questdb/client/test/cutlass/auth/FileTokenStoreTest.java

@@ -342,6 +342,37 @@ public void testInLockIsMutuallyExclusiveAcrossInstances() throws Exception {
         });
     }
 
+    @Test
+    public void testInLockReleaseDoesNotDeleteAStolenLock() throws Exception {
+        assertMemoryLeak(() -> {
+            Path dir = storeDir();
+            Files.createDirectories(dir);
+            // a tiny staleness window so our own in-progress hold is judged stale and a peer can steal it
+            FileTokenStore store = new FileTokenStore(dir, 1000, 50);
+            TokenStoreKey key = sampleKey();
+            Path lock = lockFile(dir, key);
+
+            // our critical section outlives the 50ms staleness window; while we are still inside it, a peer
+            // process judges our lock stale, steals it (deletes and recreates) and writes its own owner stamp.
+            // releaseLock must verify ownership and leave the peer's live lock intact, not delete it by bare
+            // path - otherwise a third acquirer could enter alongside the peer, defeating mutual exclusion.
+            store.inLock(key, () -> {
+                Os.sleep(120);
+                try {
+                    Files.deleteIfExists(lock);
+                    Files.write(lock, "peer-owner-stamp".getBytes(StandardCharsets.UTF_8));
+                } catch (Exception e) {
+                    throw new RuntimeException(e);
+                }
+                return true;
+            });
+
+            Assert.assertTrue("releaseLock must not delete a lock a peer has stolen", Files.exists(lock));
+            Assert.assertEquals("the peer's lock content must survive our release",
+                    "peer-owner-stamp", new String(Files.readAllBytes(lock), StandardCharsets.UTF_8));
+        });
+    }
+
     @Test
     public void testInLockReleasesLockWhenActionThrows() throws Exception {
         assertMemoryLeak(() -> {
@@ -542,6 +573,37 @@ public void testPermissionsOwnerOnly() throws Exception {
         });
     }
 
+    @Test
+    public void testSaveFailureLeavesNoTempFileAndThrows() throws Exception {
+        assertMemoryLeak(() -> {
+            Path dir = storeDir();
+            Files.createDirectories(dir);
+            FileTokenStore store = new FileTokenStore(dir);
+            TokenStoreKey key = sampleKey();
+            // make the atomic rename fail: the target path already exists as a NON-EMPTY directory, which a
+            // file-over-directory replace cannot overwrite, driving save() down its IOException path
+            Path target = tokenFile(dir, key);
+            Files.createDirectories(target);
+            Files.createFile(target.resolve("blocker"));
+
+            try {
+                store.save(key, sampleToken("ACCESS-1", "REFRESH-1"));
+                Assert.fail("save must throw when it cannot replace the target");
+            } catch (OidcAuthException expected) {
+                // the write-temp / flush / atomic-rename protocol must surface a wrapped failure, never a raw
+                // IOException, and never a half-written credential
+            }
+
+            // the temp file is the durability point of the protocol; a failed save must clean it up rather than
+            // leave a *.tmp credential fragment behind
+            boolean hasTmp;
+            try (java.nio.file.DirectoryStream<Path> entries = Files.newDirectoryStream(dir, "*.tmp")) {
+                hasTmp = entries.iterator().hasNext();
+            }
+            Assert.assertFalse("a failed save must not leave a *.tmp file behind", hasTmp);
+        });
+    }
+
     @Test
     public void testSaveThenLoadRoundTrip() throws Exception {
         assertMemoryLeak(() -> {
@@ -604,6 +666,22 @@ public void testSpecialCharactersAndNullsRoundTrip() throws Exception {
         });
     }
 
+    @Test
+    public void testTruncatedJsonReturnsNull() throws Exception {
+        assertMemoryLeak(() -> {
+            Path dir = storeDir();
+            Files.createDirectories(dir);
+            FileTokenStore store = new FileTokenStore(dir);
+            TokenStoreKey key = sampleKey();
+            // a crash mid-write on a filesystem without atomic rename, or a torn read, can leave a valid JSON
+            // prefix cut off before the closing brace. parseLast() must reject the truncated document rather
+            // than serve a half-parsed credential
+            Files.write(tokenFile(dir, key),
+                    "{\"v\":1,\"client_id\":\"questdb\"".getBytes(StandardCharsets.UTF_8));
+            Assert.assertNull("a truncated-but-prefix-valid file must be ignored", store.load(key));
+        });
+    }
+
     private static TokenStoreKey sampleKey() {
         return new TokenStoreKey("questdb", "https://idp.example.com:443/token",
                 "https://idp.example.com:443/device", "openid", null, false);

core/src/test/java/io/questdb/client/test/cutlass/auth/OidcDeviceAuthPersistenceTest.java

@@ -480,6 +480,31 @@ public void testTamperedFarFutureExpiryIsBoundedNotTrustedForever() throws Excep
         });
     }
 
+    @Test(timeout = 30_000)
+    public void testTamperedFarPastExpiryIsNotServedAsValid() throws Exception {
+        assertMemoryLeak(() -> {
+            AtomicInteger device = new AtomicInteger();
+            AtomicInteger token = new AtomicInteger();
+            MockOidcServer.Handler handler = countingHandler(device, token, "ACCESS-FRESH", "REFRESH-FRESH", "ACCESS-2", "REFRESH-2");
+            try (MockOidcServer server = new MockOidcServer(handler)) {
+                FakeTokenStore fake = new FakeTokenStore();
+                // a tampered entry claims an absurd, far-PAST expiry near Long.MIN_VALUE. adopt() clamps the
+                // expiry to [0, now + maxLife]; flooring at 0 is what keeps the validity check
+                // (now < expiresAtMillis - skew) underflow-safe - without the floor a near-Long.MIN_VALUE
+                // expiry wraps that subtraction to a huge positive and would serve the garbage-expiry token as
+                // valid forever. It must instead read as expired and fall back to a silent refresh.
+                fake.loadReturns = new PersistedToken("ACCESS-1", null, "REFRESH-1", Long.MIN_VALUE, Long.MIN_VALUE);
+                try (OidcDeviceAuth auth = baseBuilder(server).tokenStore(fake).build()) {
+                    String result = auth.signIn();
+                    Assert.assertEquals("a far-past expiry must not be served; the refresh token supplies a fresh one", "ACCESS-2", result);
+                    Assert.assertNotEquals("a garbage-expiry token must never be served as valid", "ACCESS-1", result);
+                }
+                Assert.assertEquals("a valid refresh token needs no device flow", 0, device.get());
+                Assert.assertTrue("the expired persisted token must trigger a token-endpoint refresh", token.get() >= 1);
+            }
+        });
+    }
+
     @Test(timeout = 30_000)
     public void testTamperedFileWithCrlfTokenFallsBackToDeviceFlow() throws Exception {
         assertMemoryLeak(() -> {

design/oidc-token-persistence.md

@@ -369,16 +369,24 @@ re-prompt. To eliminate it, serialise the *read-modify-write* of a refresh per i
   `Files.createFile`, Python `os.open(..., O_CREAT|O_EXCL)` / `open(p,"x")`) is a plain
   filesystem primitive that interoperates trivially. The contract mandates the lock-file
   scheme; OS advisory locks are out.
-- **Lock file:** `<hex>.lock` beside the token file, containing the holder's
-  `pid@host` and a creation timestamp for debugging. Acquire by exclusive-create; on
-  contention, spin with short backoff up to a small acquire budget (~3s); if it still
-  cannot be acquired, **proceed without it** (degrade to Layer 1) rather than fail a
-  sign-in. A lock older than a staleness timeout (10 minutes) is treated as abandoned
-  and stolen, so a crashed holder cannot wedge others. The window must dominate the
-  worst-case time a live holder can hold the lock: the refresh under the lock runs
-  send + await + parse, plus a body drain on a parse failure, each separately bounded by
-  the HTTP timeout (capped at 120s), so up to ~4×120s = ~480s — never the interactive
-  wait, which is not held under the lock. 10 minutes stays safely above that ~480s.
+- **Lock file:** `<hex>.lock` beside the token file, containing a unique per-acquisition
+  owner stamp — the holder's `pid@host`, a creation timestamp, and a random nonce.
+  Acquire by exclusive-create; on contention, spin with short backoff up to a small
+  acquire budget (~3s); if it still cannot be acquired, **proceed without it** (degrade to
+  Layer 1) rather than fail a sign-in. A lock older than a staleness timeout (10 minutes)
+  is treated as abandoned and stolen, so a crashed holder cannot wedge others. The window
+  must dominate the worst-case time a live holder can hold the lock: the refresh under the
+  lock runs send + await + parse, plus a body drain on a parse failure, each separately
+  bounded by the HTTP timeout (capped at 120s), so up to ~4×120s = ~480s — never the
+  interactive wait, which is not held under the lock. 10 minutes stays safely above that
+  ~480s.
+- **Release verifies ownership.** A holder releases by re-reading the lock and deleting it
+  **only when it still carries that holder's own owner stamp**, never by bare path. Should
+  a hold ever outrun the staleness window and be stolen and recreated by a peer, the
+  original holder must not delete the peer's live lock on release (which would admit a
+  third acquirer alongside the peer and break mutual exclusion). Each implementation
+  checks only its own stamp; it never has to parse another implementation's stamp, so the
+  random nonce keeps the check exact without coupling the language clients.
 - **Protocol (under the existing in-process `ReentrantLock`, only when a refresh is
   needed):**
   1. acquire `<hex>.lock`;
@@ -387,7 +395,7 @@ re-prompt. To eliminate it, serialise the *read-modify-write* of a refresh per i
      `validateTokenChars`) and **skip the network**;
   4. else POST the refresh with the current refresh token; `storeTokens()` writes the
      new token atomically *inside* the lock;
-  5. release (delete `<hex>.lock`).
+  5. release (delete `<hex>.lock` only if it still carries our own owner stamp).
 
   The interactive device flow does **not** hold the lock file (coordinating human prompts
   across processes is overkill and would hold a cross-process lock for up to 30 min); two