Harden OIDC token validation, lock and issuer-path pin

Three defects in the OIDC token-provider work.

The ILP HTTP sender skipped token validation whenever the provider
returned the same CharSequence instance it had validated before, on
the theory that an instance never changes content. HttpTokenProvider
makes no such promise: a provider that reuses one buffer (the idiomatic
zero-alloc style) and mutates it in place between flushes had its
mutated token spliced verbatim into the Authorization header, which
request.authToken writes with no CR/LF filtering - a header-injection
bypass of the very check validateToken exists to enforce. The sender
now validates every pulled token; the scan is O(token length) and is
dwarfed by the flush's network round-trip, matching what the WebSocket
auth path already does on every pull.

getToken() acquired its lock with the interruptible timed tryLock even
on the uncontended fast path. That overload throws InterruptedException
the moment the calling thread merely carries a set interrupt flag -
even on a free lock - and the handler re-arms the flag, so every later
getToken() on that thread failed with a valid token sitting in the
cache. ILP producers commonly run on pooled or managed threads where
interrupt is the standard cancellation signal. An untimed tryLock now
handles the uncontended case; the timed poll remains only for genuine
contention behind a peer's silent refresh.

The issuer-path pin rejected a "." or ".." path segment but not "..;":
a server or proxy that strips RFC 3986 matrix parameters resolves
/realms/acme/..;/evil to /realms/evil, a different realm on the same
host, redirecting the device code and refresh token to a sibling
tenant. The dot-segment check now strips a ";suffix" from each decoded
segment before comparing.

Each fix gets a regression test proven to fail when the fix is
reverted.

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

Author: glasstiger

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

@@ -797,7 +797,16 @@ private static boolean isEndpointUnderIssuerPath(String endpointUrl, String issu
         // a "." or ".." segment is rejected outright: the server normalizes it away, so a naive prefix test
         // would pass /realms/acme/../evil/token yet it resolves to a different realm
         for (int i = 0; i < endpointSegs.length; i++) {
-            if (".".equals(endpointSegs[i]) || "..".equals(endpointSegs[i])) {
+            // strip an RFC 3986 ";matrix" parameter suffix before the dot-segment test: a server or proxy that
+            // drops matrix params resolves "..;" (or "..;x") to "..", so /realms/acme/..;/evil/token would
+            // otherwise slip the issuer-path pin to a sibling realm. decodePathSegments already percent-decoded,
+            // so a "%3b"-encoded ";" is a literal ";" here too.
+            String seg = endpointSegs[i];
+            int semi = seg.indexOf(';');
+            if (semi >= 0) {
+                seg = seg.substring(0, semi);
+            }
+            if (".".equals(seg) || "..".equals(seg)) {
                 return false;
             }
         }
@@ -1036,15 +1045,25 @@ private static String wellKnownUrl(String issuer) {
     }
 
     private void acquireForGetToken() {
-        // Never wait behind an interactive signIn(): it holds the lock for the whole device-code lifetime
-        // (up to 30 min) with no token to serve until it completes, so fail fast and let the caller retry. A
-        // peer holding the lock for a quick cached read or a silent refresh (bounded, usually well under a
-        // second) is different - the HttpTokenProvider contract permits a brief wait behind such a refresh - so
-        // poll for the lock in short slices rather than fail every concurrent caller sharing this instance on
-        // each token refresh (the old unconditional tryLock() did exactly that). Polling, not one blocking
-        // acquire, lets us still fail fast the moment an interactive sign-in - or close() - begins while we
-        // wait. Bound the total wait by httpTimeoutMillis so a stuck or pathologically slow holder degrades to
-        // a retryable failure instead of stalling the flush path without bound.
+        throwIfClosed();
+        // Uncontended fast path: a plain CAS. It deliberately bypasses the interruptible timed tryLock in the
+        // loop below, which throws InterruptedException the moment the calling thread merely carries a set
+        // interrupt flag - even on a FREE lock - and then re-arms that flag, so every later getToken() on the
+        // same thread would fail with a valid token sitting in the cache. An ILP producer on a pooled or
+        // managed thread, where interrupt is the standard cancellation signal, is the common case. An
+        // uncontended acquire cannot be behind an interactive sign-in (which holds the lock), so it is correct.
+        if (lock.tryLock()) {
+            return;
+        }
+        // Contended - a peer holds the lock. Never wait behind an interactive signIn(): it holds the lock for
+        // the whole device-code lifetime (up to 30 min) with no token to serve until it completes, so fail fast
+        // and let the caller retry. A peer holding the lock for a quick cached read or a silent refresh
+        // (bounded, usually well under a second) is different - the HttpTokenProvider contract permits a brief
+        // wait behind such a refresh - so poll for the lock in short slices rather than fail every concurrent
+        // caller sharing this instance on each token refresh (the old unconditional tryLock() did exactly that).
+        // Polling, not one blocking acquire, lets us still fail fast the moment an interactive sign-in - or
+        // close() - begins while we wait. Bound the total wait by httpTimeoutMillis so a stuck or pathologically
+        // slow holder degrades to a retryable failure instead of stalling the flush path without bound.
         final long deadline = System.currentTimeMillis() + httpTimeoutMillis;
         while (true) {
             throwIfClosed();

core/src/main/java/io/questdb/client/cutlass/line/http/AbstractLineHttpSender.java

@@ -94,9 +94,6 @@ public abstract class AbstractLineHttpSender implements Sender {
     private boolean isTokenPending;
     private JsonErrorParser jsonErrorParser;
     private boolean lastFlushFailed;
-    // the last provider token instance validated in stampTokenIfPending(); lets an unchanged multi-KB token
-    // skip re-validation (a full scan) on every flush. Identity only, never dereferenced for content.
-    private CharSequence lastValidatedToken;
     private long pendingRows;
     private int rowBookmark;
     private RequestState state = RequestState.EMPTY;
@@ -835,19 +832,14 @@ private void stampTokenIfPending() {
             // The throwing operations run BEFORE the request is mutated: a getToken()/validateToken() throw
             // (not signed in yet, a failed refresh, or a rejected token) leaves isTokenPending set and the
             // request untouched at the header stage, so the next row retries cleanly - the sender is never left
-            // corrupted. validateToken (which scans the whole token) is skipped when the provider returned the
-            // same instance already validated: an unchanged multi-KB id token is otherwise re-scanned every
-            // flush. A provider returning the same instance must not mutate its content (HttpTokenProvider
-            // contract), so the skip cannot let a changed token bypass validation.
+            // corrupted. Validate EVERY pulled token, not just a changed instance: HttpTokenProvider.getToken()
+            // makes no immutability promise, so a provider that reuses one CharSequence buffer (the idiomatic
+            // zero-alloc style) and mutates its content between flushes must be re-checked, or a mutated token
+            // could splice a CR/LF into the "Authorization: Bearer" header (request.authToken writes it verbatim,
+            // with no CR/LF filtering). The scan is O(token length) and is dwarfed by the flush's network
+            // round-trip; the WebSocket auth path validates on every pull for the same reason.
             CharSequence token = httpTokenProvider.getToken();
-            // always validate a null token (it must be rejected, and null is the initial lastValidatedToken
-            // value); otherwise skip re-validation only for the exact same instance already validated.
-            // lastValidatedToken is assigned only after a successful validation, so it never holds a rejected
-            // (null/blank/bad-char) token - a re-returned bad token is therefore re-validated and re-rejected.
-            if (token == null || token != lastValidatedToken) {
-                HttpTokenProvider.validateToken(token);
-                lastValidatedToken = token;
-            }
+            HttpTokenProvider.validateToken(token);
             request.authToken(token);
             request.withContent();
             rowBookmark = request.getContentLength();

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

@@ -1541,6 +1541,38 @@ public void testGetTokenDoesNotBlockBehindInteractiveSignIn() throws Exception {
         });
     }
 
+    @Test(timeout = 30_000)
+    public void testGetTokenSucceedsWhenCallingThreadIsInterrupted() throws Exception {
+        assertMemoryLeak(() -> {
+            // getToken()'s uncontended lock acquire must NOT fail merely because the calling thread carries a
+            // set interrupt flag. An ILP producer on a pooled/managed thread commonly does (interrupt is the
+            // standard cancellation signal), and the old timed tryLock threw InterruptedException even on a FREE
+            // lock and then re-armed the flag, so every getToken() on that thread failed with a valid token
+            // sitting in the cache. The untimed fast-path acquire fixes it.
+            MockOidcServer.Handler handler = (method, path, body) -> {
+                if (DEVICE_PATH.equals(path)) {
+                    return MockOidcServer.json(200, deviceAuthorizationJson(1, 300));
+                }
+                return MockOidcServer.json(200, tokenJson("ACCESS-1", null, "REFRESH-1", 3600));
+            };
+            try (MockOidcServer server = new MockOidcServer(handler);
+                 OidcDeviceAuth auth = newAuth(server, false, noopPrompt())) {
+                Assert.assertEquals("ACCESS-1", auth.signIn()); // seed a valid cached token
+
+                Thread.currentThread().interrupt(); // the calling (producer) thread carries a pending interrupt
+                try {
+                    // uncontended lock, valid cached token: getToken() must return it, not throw on the interrupt
+                    Assert.assertEquals("ACCESS-1", auth.getToken());
+                    // and it must not silently swallow the caller's interrupt (the untimed acquire preserves it)
+                    Assert.assertTrue("getToken() must not clear the caller's interrupt flag",
+                            Thread.currentThread().isInterrupted());
+                } finally {
+                    Thread.interrupted(); // clear so the flag does not leak into later tests sharing this fork
+                }
+            }
+        });
+    }
+
     @Test(timeout = 30_000)
     public void testGetTokenWaitsBehindSilentRefreshInsteadOfFailing() throws Exception {
         assertMemoryLeak(() -> {
@@ -1971,6 +2003,34 @@ public void testIssuerPathScopingRejectsEncodedTraversal() throws Exception {
         });
     }
 
+    @Test(timeout = 30_000)
+    public void testIssuerPathScopingRejectsMatrixParamTraversal() throws Exception {
+        assertMemoryLeak(() -> {
+            // the device endpoint hides a parent traversal as an RFC 3986 ";matrix" segment (..;): a server or
+            // proxy that strips matrix params resolves /realms/acme/..;/evil to /realms/evil, a DIFFERENT realm.
+            // The plain "." / ".." dot-segment check does not match "..;", so the check must strip the ";suffix"
+            // first and reject it - the origin pin alone cannot stop a sibling-tenant redirect on one host.
+            AtomicReference<MockOidcServer> serverRef = new AtomicReference<>();
+            MockOidcServer.Handler handler = (method, path, body) -> {
+                MockOidcServer server = serverRef.get();
+                return MockOidcServer.json(200, "{\"config\":{"
+                        + "\"acl.oidc.enabled\":true,"
+                        + "\"acl.oidc.client.id\":\"questdb\","
+                        + "\"acl.oidc.token.endpoint\":\"" + server.httpUrl("/realms/acme/token") + "\","
+                        + "\"acl.oidc.device.authorization.endpoint\":\"" + server.httpUrl("/realms/acme/..;/evil/device") + "\""
+                        + "}}");
+            };
+            try (MockOidcServer server = new MockOidcServer(handler)) {
+                serverRef.set(server);
+                try (OidcDeviceAuth ignored = OidcDeviceAuth.fromQuestDB(server.httpUrl(""), insecure().issuer(server.httpUrl("/realms/acme")))) {
+                    Assert.fail("expected the ..;-matrix-param traversal device endpoint to be rejected");
+                } catch (OidcAuthException e) {
+                    Assert.assertTrue(e.getMessage(), e.getMessage().contains("not under the pinned issuer"));
+                }
+            }
+        });
+    }
+
     @Test(timeout = 30_000)
     public void testIssuerPathScopingRejectsSiblingRealm() throws Exception {
         assertMemoryLeak(() -> {

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

@@ -133,10 +133,10 @@ public void testCancelRowWithPendingTokenDoesNotCorruptRequest() throws Exceptio
     @Test(timeout = 30_000)
     public void testChangedProviderTokenIsRevalidated() throws Exception {
         assertMemoryLeak(() -> {
-            // the per-flush token validation is skipped only for the SAME instance already validated, so a
-            // token that CHANGES to a bad one must still be re-validated and rejected - the identity guard must
-            // not cache a previously-valid result past a token change. First flush a valid token, then return a
-            // CR/LF token and require the next flush to reject it rather than splice it onto the wire.
+            // every pulled token is validated per flush, so a token that CHANGES to a bad one must be rejected.
+            // First flush a valid token, then return a distinct CR/LF token and require the next flush to reject
+            // it rather than splice it onto the wire. (The same-instance-mutated case - a reused buffer whose
+            // content changes - is covered by testMutatedSameInstanceProviderTokenIsRevalidated.)
             AtomicInteger calls = new AtomicInteger();
             try (MockOidcServer server = new MockOidcServer((method, path, body) -> MockOidcServer.json(204, ""))) {
                 HttpTokenProvider provider = () -> calls.incrementAndGet() == 1
@@ -211,6 +211,44 @@ public void testFailedFlushReSendsSameTokenWithoutRePull() throws Exception {
         });
     }
 
+    @Test(timeout = 30_000)
+    public void testMutatedSameInstanceProviderTokenIsRevalidated() throws Exception {
+        assertMemoryLeak(() -> {
+            // A provider may reuse one CharSequence buffer (the idiomatic zero-alloc style) and return the SAME
+            // instance every call. HttpTokenProvider.getToken() makes no immutability promise, so the sender
+            // must re-validate EVERY pulled token, not trust instance identity: a token mutated in place to
+            // carry a CR/LF between flushes must be rejected, not spliced verbatim into the "Authorization:
+            // Bearer" header (authToken writes it with no CR/LF filtering). This pins the fix that dropped the
+            // identity-cache skip; before it, the second flush injected a header past the auth line.
+            StringBuilder token = new StringBuilder("GOODTOKEN"); // one instance, mutated in place below
+            try (MockOidcServer server = new MockOidcServer((method, path, body) -> MockOidcServer.json(204, ""))) {
+                try (Sender sender = Sender.builder(Sender.Transport.HTTP)
+                        .address("127.0.0.1:" + server.port())
+                        .protocolVersion(Sender.PROTOCOL_VERSION_V1)
+                        .disableAutoFlush()
+                        .httpTokenProvider(() -> token) // always the SAME instance
+                        .build()) {
+                    sender.table("t").longColumn("v", 1L).atNow();
+                    sender.flush(); // first flush: GOODTOKEN validated and sent
+                    // mutate the SAME instance to inject a CR/LF header break
+                    token.setLength(0);
+                    token.append("abc").append((char) 0x0d).append((char) 0x0a).append("X-Injected: pwned");
+                    try {
+                        sender.table("t").longColumn("v", 2L).atNow();
+                        sender.flush();
+                        Assert.fail("a mutated same-instance token carrying CR/LF must be re-validated and rejected");
+                    } catch (LineSenderException e) {
+                        Assert.assertTrue(e.getMessage(), e.getMessage().contains("control or non-ASCII character"));
+                    }
+                }
+                // only the first (valid) flush reached the wire; the injected token was rejected before any send
+                List<String> auth = server.requestAuthHeaders();
+                Assert.assertEquals("only the valid first flush must reach the server", 1, auth.size());
+                Assert.assertEquals("Bearer GOODTOKEN", auth.get(0));
+            }
+        });
+    }
+
     @Test
     public void testNullOrEmptyProviderTokenIsRejected() throws Exception {
         assertMemoryLeak(() -> {