Reject OIDC endpoint fragment, harden token lock

Endpoint.parse now rejects an endpoint URL carrying a fragment (#...).
pathOnly() stripped the fragment before the issuer-path pin while
postForm sent endpoint.path verbatim on the wire, so a "#/../other/token"
payload from a tampered /settings could steer the credential POST to a
path the pin never validated on a server that normalizes '..'. Rejecting
the fragment makes the validated path and the sent path identical.

The token-store lock's stamp write and its stamp-failure cleanup now
verify ownership the way release and steal already do. writeLockHolder
refuses to overwrite a lock a peer stamped while this holder was
pre-empted in the create->stamp gap (a long pause or cross-machine clock
skew wider than the empty-lock grace), and the cleanup drops only a
still-empty lock, never a peer's live one by bare path. The residual
degrades to a re-prompt on a rotating refresh token, never a torn or
forged credential; the design doc and the overstated grace comment are
corrected to say so. The on-disk lock protocol is unchanged.

Add end-to-end tests proving the httpTokenProvider token reaches the
Authorization: Bearer header on the wire and rotates per request, that a
failed flush re-sends the same baked token without re-pulling, and that
adopt() re-saves a kept refresh token the file omitted. The endpoint
fragment and re-save guards are verified both ways.

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

@@ -611,6 +611,17 @@ private static boolean writeLockHolder(Path lock, String nonce) {
         // failure so acquireLock drops an unverifiable lock rather than hold one it cannot safely release.
         // Writing also refreshes the mtime, which is what the staleness age check reads
         try {
+            // acquireLock created this lock empty; if it already carries a stamp, a peer judged it stale and
+            // stole+restamped it in the create->stamp gap (a long GC/suspend pause between the two syscalls, or
+            // a cross-machine clock skew wider than the empty-lock grace). A plain WRITE|TRUNCATE_EXISTING has no
+            // exclusivity and would overwrite the peer's stamp, leaving two processes each believing they hold
+            // the lock. Refuse instead - honouring releaseLock's own-stamp ownership rule - so acquireLock
+            // degrades to a lock-free refresh (the documented best-effort residual) rather than clobber a live
+            // peer's stamp. A readLockHolder that throws (our file was moved away during the peer's steal) is
+            // caught below and likewise fails the stamp.
+            if (readLockHolder(lock) != null) {
+                return false;
+            }
             Files.write(lock, nonce.getBytes(StandardCharsets.UTF_8), StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING);
             return true;
         } catch (Exception e) {
@@ -632,11 +643,16 @@ private String acquireLock(Path lock) {
                 }
                 // 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
+                // created and degrade to a lock-free refresh rather than hold an unverifiable lock. Remove it
+                // only while it is still the empty file we created: writeLockHolder also returns false when a
+                // peer stole and restamped this path in the create->stamp gap, and deleting that peer's non-empty
+                // live lock by bare path would admit a third holder (mirrors releaseLock's own-stamp rule).
                 try {
-                    Files.deleteIfExists(lock);
+                    if (Files.size(lock) == 0) {
+                        Files.deleteIfExists(lock);
+                    }
                 } catch (IOException ignore) {
-                    // another acquirer may have removed it; the next createLockFile settles the race
+                    // gone (a peer moved it during its steal) or unreadable; another acquirer settles the race
                 }
                 return null;
             } catch (FileAlreadyExistsException e) {
@@ -721,9 +737,14 @@ private void stealIfStale(Path lock) {
             // an empty/unreadable lock is never a validly-held lock (a holder stamps right after creating): it
             // is a peer mid-create/stamp (recovers on its own in microseconds) or one a crash orphaned in that
             // gap. Steal it on the short empty-lock grace rather than the full staleness window, so a crash
-            // orphan stops wedging peers for the whole window; the grace dwarfs the create->stamp gap, so a
-            // peer mid-stamp is never pre-empted. The capture-verify below still confirms the lock is unchanged
-            // before completing the steal.
+            // orphan stops wedging peers for the whole window. The grace normally dwarfs the create->stamp gap,
+            // but a pause wider than the grace (a long GC/safepoint or a suspend landing between the two
+            // syscalls) or a cross-machine clock skew (isOlderThan compares the local clock to the file mtime)
+            // can still make a freshly-created empty lock look stale and pre-empt a peer mid-stamp. That never
+            // forges or tears a credential - Layer-1 atomic rename holds, and the pre-empted peer's
+            // writeLockHolder refuses to overwrite this stamp - it degrades to a concurrent refresh (a re-prompt
+            // on a rotating-refresh-token IdP), the same best-effort residual inLock already accepts. The
+            // capture-verify below still confirms the lock is unchanged before completing the steal.
             return;
         }
         final Path captured = lock.resolveSibling(lock.getFileName().toString() + '.' + UUID.randomUUID() + ".tmp");

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

@@ -1928,6 +1928,14 @@ static Endpoint parse(String url) {
                 }
                 i += Character.charCount(cp);
             }
+            // reject a fragment (#...): it has no meaning in an endpoint url (RFC 3986 fragments are client-side
+            // only, never sent to a server), and folding it into the path opens a pin-bypass - pathOnly() strips
+            // it before the issuer-path check while postForm sends endpoint.path verbatim on the wire, so a
+            // lenient server that normalizes a '..' hidden past the '#' (POST /realms/acme#/../other/token) could
+            // resolve the request-target to a path the issuer-path pin never validated. Fail closed instead.
+            if (url.indexOf('#') >= 0) {
+                throw new OidcAuthException().put("invalid url, a fragment (#) is not supported [url=").put(url).put(']');
+            }
             int schemeEnd = url.indexOf("://");
             if (schemeEnd < 0) {
                 throw new OidcAuthException().put("invalid url, expected a scheme [url=").put(url).put(']');

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

@@ -341,6 +341,41 @@ public void testRefreshUnderLockKeepsLiveRefreshTokenWhenPeerEntryOmitsIt() thro
         });
     }
 
+    @Test(timeout = 30_000)
+    public void testRefreshUnderLockResavesKeptRefreshTokenWhenPeerEntryOmitsIt() throws Exception {
+        // the other half of the NPE fix: when the coordinated re-read adopts a peer entry that omits the refresh
+        // token, adopt() keeps the live refresh token AND records that the file carried none
+        // (lastPersistedRefreshToken=null). The refresh that follows must therefore RE-SAVE the kept token, so a
+        // restart still finds it on disk. If adopt() instead marked the kept token as already-persisted, the save
+        // would be skipped and the refresh token would silently vanish from disk, forcing a needless re-prompt.
+        assertMemoryLeak(() -> {
+            MockOidcServer.Handler handler = (method, path, body) -> {
+                if (path.startsWith(DEVICE_PATH)) {
+                    return MockOidcServer.json(200, deviceAuthJson());
+                }
+                // non-rotating refresh: the same REFRESH-1 comes back, so ONLY the null-vs-REFRESH-1 lastPersisted
+                // bookkeeping (not a token change) decides whether persistIfRotated re-saves
+                return MockOidcServer.json(200, tokenJson("REFRESHED-ACCESS", null, "REFRESH-1", 3600));
+            };
+            try (MockOidcServer server = new MockOidcServer(handler)) {
+                FakeTokenStore fake = new FakeTokenStore();
+                long now = System.currentTimeMillis();
+                // our own entry, adopted on load: an expired served token carrying REFRESH-1
+                fake.stored = new PersistedToken("OLD-ACCESS", null, "REFRESH-1", now - 60_000, 300_000);
+                // a peer overwrites the file with an expired served token and NO refresh_token while we hold the
+                // lock; the re-read adopts it, keeps REFRESH-1, and records that the file carried no refresh token
+                fake.peerInstallsOnLock = new PersistedToken("PEER-ACCESS", null, null, now - 60_000, 300_000);
+                try (OidcDeviceAuth auth = baseBuilder(server).tokenStore(fake).build()) {
+                    Assert.assertEquals("REFRESHED-ACCESS", auth.getToken());
+                }
+                Assert.assertTrue("the kept refresh token must be re-saved (the file carried none), not skipped as already-persisted",
+                        fake.saves.get() >= 1);
+                Assert.assertNotNull("the re-saved entry must exist", fake.stored);
+                Assert.assertEquals("the re-saved entry must carry the kept refresh token", "REFRESH-1", fake.stored.getRefreshToken());
+            }
+        });
+    }
+
     @Test(timeout = 30_000)
     public void testRestartRefreshesExpiredTokenSkippingDeviceFlow() throws Exception {
         assertMemoryLeak(() -> {

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

@@ -1052,6 +1052,12 @@ public void testEndpointParseRejectsMalformedUrls() {
         assertBuildFails("https://idp/devic\r\ne", "https://idp/t", "illegal character");
         assertBuildFails("https://idp/d", "https://idp/toke\r\nX-Injected:1", "illegal character");
         assertBuildFails("https://idp/d", "https://idp/t?a=b\nc", "illegal character");
+        // a fragment (#...) is rejected: pathOnly() strips it before the issuer-path pin while postForm sends
+        // endpoint.path verbatim on the wire, so folding a "#/../other/token" past the '#' would let a lenient
+        // server that normalizes '..' resolve the request-target to a path the pin never validated. Fail closed
+        assertBuildFails("https://idp/realms/acme#/../other/device", "https://idp/realms/acme/token", "fragment");
+        assertBuildFails("https://idp/d", "https://idp/realms/acme#/../other/token", "fragment");
+        assertBuildFails("https://idp/d#", "https://idp/t", "fragment");
     }
 
     @Test(timeout = 30_000)

core/src/test/java/io/questdb/client/test/cutlass/line/LineHttpSenderTokenProviderTest.java

@@ -27,9 +27,11 @@
 import io.questdb.client.HttpTokenProvider;
 import io.questdb.client.Sender;
 import io.questdb.client.cutlass.line.LineSenderException;
+import io.questdb.client.test.cutlass.auth.MockOidcServer;
 import org.junit.Assert;
 import org.junit.Test;
 
+import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 
@@ -42,10 +44,12 @@
  * {@code .httpTokenProvider(auth::getToken)} - be wired before the interactive sign-in
  * has completed.
  * <p>
- * An explicit {@code protocol_version} keeps {@link Sender.LineSenderBuilder#build()} from probing
- * the server, and auto-flush is disabled, so rows can be buffered against a port nobody listens on
- * without ever opening a connection. Each test runs under {@code assertMemoryLeak} so the sender's
- * native buffers are proven freed on close.
+ * The deferral tests pin an explicit {@code protocol_version} to keep {@link Sender.LineSenderBuilder#build()}
+ * from probing the server, and disable auto-flush, so rows buffer against a port nobody listens on without
+ * opening a connection. The end-to-end tests instead flush against a {@link MockOidcServer} and assert the
+ * pulled token actually reaches the {@code Authorization: Bearer} header on the wire, is re-queried per
+ * request as a rotating provider refreshes, and is re-sent verbatim (not re-pulled) on a retry. Each test
+ * runs under {@code assertMemoryLeak} so the sender's native buffers are proven freed on close.
  */
 public class LineHttpSenderTokenProviderTest {
 
@@ -98,6 +102,38 @@ public void testControlOrNonAsciiProviderTokenIsRejected() throws Exception {
         });
     }
 
+    @Test(timeout = 30_000)
+    public void testFailedFlushReSendsSameTokenWithoutRePull() throws Exception {
+        assertMemoryLeak(() -> {
+            // a failed flush preserves the buffered request - token included - and re-sends it verbatim on retry
+            // rather than re-pulling the provider (the documented contract on httpTokenProvider()). Here the first
+            // send gets a retryable 500 and the retry must carry the SAME baked token, with the provider queried
+            // only once - so a rotating provider does not change the credential mid-retry of one buffered batch.
+            AtomicInteger requests = new AtomicInteger();
+            try (MockOidcServer server = new MockOidcServer((method, path, body) ->
+                    requests.incrementAndGet() == 1
+                            ? MockOidcServer.chunkedJson(500, "boom") // first send: retryable server error
+                            : MockOidcServer.json(204, ""))) {        // retry: success
+                AtomicInteger calls = new AtomicInteger();
+                HttpTokenProvider provider = () -> "TOKEN-" + calls.incrementAndGet();
+                try (Sender sender = Sender.builder(Sender.Transport.HTTP)
+                        .address("127.0.0.1:" + server.port())
+                        .protocolVersion(Sender.PROTOCOL_VERSION_V1)
+                        .disableAutoFlush()
+                        .httpTokenProvider(provider)
+                        .build()) {
+                    sender.table("t").longColumn("v", 1L).atNow();
+                    sender.flush(); // first send 500 -> retry -> 204
+                }
+                Assert.assertEquals("the provider must be pulled once, not re-pulled on retry", 1, calls.get());
+                List<String> auth = server.requestAuthHeaders();
+                Assert.assertEquals("the failed send plus its retry must be two requests", 2, auth.size());
+                Assert.assertEquals("the first send carries the pulled token", "Bearer TOKEN-1", auth.get(0));
+                Assert.assertEquals("the retry must re-send the same baked token", "Bearer TOKEN-1", auth.get(1));
+            }
+        });
+    }
+
     @Test
     public void testNullOrEmptyProviderTokenIsRejected() throws Exception {
         assertMemoryLeak(() -> {
@@ -136,6 +172,34 @@ public void testProviderTokenNotPulledAtBuildAndPulledOnFirstRow() throws Except
         });
     }
 
+    @Test(timeout = 30_000)
+    public void testTokenReachesAuthorizationHeaderAndRotatesPerFlush() throws Exception {
+        assertMemoryLeak(() -> {
+            // end-to-end against a real socket: the pulled token must reach the "Authorization: Bearer" header
+            // on the wire (not merely be pulled), and a rotating provider must be re-queried per request so a
+            // long-lived sender follows token refreshes rather than sending a token captured once.
+            try (MockOidcServer server = new MockOidcServer((method, path, body) -> MockOidcServer.json(204, ""))) {
+                AtomicInteger calls = new AtomicInteger();
+                HttpTokenProvider provider = () -> "TOKEN-" + calls.incrementAndGet();
+                try (Sender sender = Sender.builder(Sender.Transport.HTTP)
+                        .address("127.0.0.1:" + server.port())
+                        .protocolVersion(Sender.PROTOCOL_VERSION_V1)
+                        .disableAutoFlush()
+                        .httpTokenProvider(provider)
+                        .build()) {
+                    sender.table("t").longColumn("v", 1L).atNow();
+                    sender.flush();
+                    sender.table("t").longColumn("v", 2L).atNow();
+                    sender.flush();
+                }
+                List<String> auth = server.requestAuthHeaders();
+                Assert.assertEquals("two flushes must send two requests", 2, auth.size());
+                Assert.assertEquals("the first request must carry the first pulled token", "Bearer TOKEN-1", auth.get(0));
+                Assert.assertEquals("the second flush must re-query the provider and carry the rotated token", "Bearer TOKEN-2", auth.get(1));
+            }
+        });
+    }
+
     private static void assertProviderTokenRejected(HttpTokenProvider provider, String expectedMessage) {
         try (Sender sender = Sender.builder(Sender.Transport.HTTP)
                 .address("127.0.0.1:1")

design/oidc-token-persistence.md

@@ -395,11 +395,19 @@ re-prompt. To eliminate it, serialise the *read-modify-write* of a refresh per i
   between the two leaves an empty `<hex>.lock` whose mtime is fresh — which the staleness check
   would protect for the whole window, wedging peers into lock-free refreshes. Treat a lock that
   carries no readable owner stamp as stealable once it is older than a short grace (a few
-  seconds) instead of the full window. The grace MUST comfortably exceed the create→stamp gap
-  (microseconds) so a peer momentarily between its create and its stamp is never pre-empted —
-  pre-empting it would steal a lock its rightful owner is about to hold and force that owner to
-  degrade. The capture-then-verify steal below still aborts if the lock acquires a stamp in the
-  gap. The Python client MUST mirror this empty-lock grace.
+  seconds) instead of the full window. The grace normally dwarfs the create→stamp gap
+  (microseconds), but cannot be guaranteed to: a pause wider than the grace (a long GC/safepoint
+  or a process suspend landing between the two syscalls) or a cross-machine clock skew (the age
+  check compares the local clock against the file's mtime) can make a freshly-created empty lock
+  look stale and let a peer pre-empt a holder mid-stamp. This never forges or tears a credential —
+  Layer 1's atomic replacement always holds — it degrades to a concurrent refresh (a re-prompt on
+  a rotating-refresh-token IdP), the same best-effort residual as running lock-free. To keep that
+  residual bounded, the **stamp write and the stamp-failure cleanup verify ownership** the way
+  release does: a holder stamps only a lock still empty (never overwriting a stamp a peer wrote
+  while the holder was pre-empted), and on a stamp failure drops only a lock still empty (never a
+  peer's non-empty live lock by bare path). The capture-then-verify steal below still aborts if
+  the lock acquires a stamp in the gap. The Python client MUST mirror this empty-lock grace, and
+  SHOULD mirror the ownership-verified stamp write.
 - **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