Skip to content

feat(core): add OIDC sign-in via device flow (RFC 8628) - #52

Merged
bluestreak01 merged 259 commits into
mainfrom
ia_oidc_device_flow
Aug 31, 2026
Merged

feat(core): add OIDC sign-in via device flow (RFC 8628)#52
bluestreak01 merged 259 commits into
mainfrom
ia_oidc_device_flow

Conversation

@glasstiger

@glasstiger glasstiger commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Adds interactive OIDC sign-in to the Java client using the OAuth 2.0 Device Authorization Grant (RFC 8628). A process with no local browser — a remote notebook kernel, a container, a headless job — can sign a human in against QuestDB Enterprise: the user authorizes on any device (laptop or phone) while the process only makes outbound calls to the identity provider.

On first use it prints a verification URL and a short code (and, by default, also tries to open the URL in a local browser); once the user authorizes, the token is cached in memory and refreshed silently on later calls.

import io.questdb.client.Sender;
import io.questdb.client.cutlass.auth.OidcDeviceAuth;

// Discover the client id, scope and endpoints from the QuestDB server's /settings:
try (OidcDeviceAuth auth = OidcDeviceAuth.fromQuestDB("https://questdb.example.com:9000")) {
    auth.signIn(); // sign in once: prompts on first use, then caches and refreshes

    // Pass a token provider, not a fixed string: the sender pulls a freshly refreshed token on each
    // request, so a long-lived sender keeps working as the token rotates. getToken() refreshes
    // silently and never prompts on the flush path.
    try (Sender sender = Sender.builder(Sender.Transport.HTTP)
            .address("questdb.example.com:9000")
            .enableTls()
            .httpTokenProvider(auth::getToken)
            .build()) {
        sender.table("trades")
                .symbol("symbol", "ETH-USD")
                .doubleColumn("price", 2615.54)
                .atNow();
    }
}

What's new

OidcDeviceAuth (io.questdb.client.cutlass.auth) — runs the flow and owns the token:

  • OidcDeviceAuth.fromQuestDB(url) discovers the client id, scope, audience and IdP endpoints from the server's unauthenticated /settings; OidcDeviceAuth.fromQuestDB(url, DiscoveryOptions) adds an identity-provider pin (.issuer(...)), a TLS config, an allowInsecureTransport opt-in, and the prompt (see Discovery and trust below); OidcDeviceAuth.builder() configures the identity provider explicitly.
  • signIn() signs in interactively on first use, then serves a cached token and refreshes it silently; getToken() never prompts and never waits behind an interactive sign-in (safe on a request/flush path); getAuthorizationHeaderValue() returns the full Bearer … value; clearCache() drops the cached token so the next signIn() re-signs-in; close() cancels an in-flight sign-in (observed between polls, so it can take up to one HTTP request timeout to return). Calls are serialized by a ReentrantLock; getToken() uses tryLock and fails fast rather than wait behind an interactive sign-in. Token state is in-memory only by default; pass a TokenStore to persist it across restarts (see Token persistence below).

Sender integration — new HttpTokenProvider interface and Sender.builder(...).httpTokenProvider(auth::getToken). The sender pulls a freshly refreshed token on every request, so a long-lived sender keeps working as the token rotates — unlike a fixed httpToken(...), which is captured once and eventually starts returning 401s. Mutually exclusive with httpToken/httpUsernamePassword. Supported over HTTP and WebSocket transport (a WebSocket sender re-queries the provider on every (re)connect/upgrade); rejected for TCP and UDP. The two transports differ in mechanism but both keep the producer alive across a sustained token outage: over HTTP a failed pull leaves the request token-pending and is retried on the next row; over WebSocket the token must be obtainable when build() runs (the initial handshake fails fast otherwise), after which a pull that keeps failing on later reconnects is retried indefinitely, with the buffered rows held in store-and-forward, until a token is available again — a token outage does not terminate a running WebSocket sender, just as a persistent transport reconnect failure does not (store-and-forward Invariant B). The first pull is deferred off the build path to the first row, so the documented construct → signIn() → send ordering works and a provider that throws leaves the request retriable instead of corrupting the sender.

QWP egress query clientQwpQueryClient.withBearerTokenProvider(HttpTokenProvider) accepts the same on-demand provider, so OidcDeviceAuth::getToken plugs into the egress query path as well as ingress. The provider is queried at every WebSocket upgrade — the initial connect() and each failover reconnect — so a long-lived query client follows token rotation; each returned token is validated before it reaches the header, and a provider that throws fails that connection attempt (matching the ingress sender). Mutually exclusive with withBearerToken/withBasicAuth.

DeviceCodePrompt / DeviceAuthorizationChallenge — how the verification URL and user code are shown. The default, DeviceCodePrompt.openBrowser(), prints the instructions to System.out and also tries to open the verification URL in the local default browser; the browser open is best-effort (skipped on a headless JVM, without the java.desktop module, or for a non-http(s) URL, and disabled by -Dquestdb.client.oidc.open.browser=false) and never blocks or fails sign-in. Use DeviceCodePrompt.SYSTEM_OUT to print only, or supply your own to render a clickable link or a QR code, e.g. in a notebook.

audiencebuilder().audience(...) / discovered from acl.oidc.audience. When set, the audience parameter is sent on the device-authorization and refresh requests, for providers that require it to stamp the aud claim QuestDB expects.

The token can be presented to QuestDB over any auth path the server already validates:

  • REST / ingestionAuthorization: Bearer <token>.
  • PG-wire — connect as _sso with the token as the password (requires acl.oidc.pg.token.as.password.enabled=true on the server).

Discovery and trust

fromQuestDB(...) takes the IdP endpoints from the server's unauthenticated /settings, so by default it trusts that server to designate where the user signs in: a spoofed, compromised, or man-in-the-middled server could otherwise redirect the sign-in — and the long-lived refresh token — to an attacker-controlled identity provider. An optional DiscoveryOptions.issuer(...) pin addresses this, and also covers servers that do not advertise a device-authorization endpoint. The pin separates two sources of endpoints and trusts them differently — an endpoint the untrusted /settings advertised is constrained to the issuer, while an endpoint read from the identity provider's own .well-known is trusted wherever the provider hosts it:

  • .well-known discovery fallback. Current servers do not advertise the device-authorization endpoint. When it (and/or the token endpoint) is missing, a pinned issuer reads it from {issuer}/.well-known/openid-configuration. The discovery origin comes only from the caller-supplied issuer, never from a /settings-supplied value, so a tampered /settings cannot choose where discovery — and the credential POSTs it resolves — are aimed. Without a pin, discovery is refused rather than guessed.
  • Co-location pin. validateEndpointOrigins, enforced on every construction path (discovery and the explicit builder()), requires the token and device-authorization endpoints to share one origin (RFC 8628 co-locates them on a single authorization server), so a tampered /settings or discovery document cannot siphon one of the two credential POSTs off to a different origin.
  • /settings-advertised endpoints are pinned to the issuer. An endpoint the untrusted /settings response supplied must sit on the pinned issuer's origin, and — when the issuer has a path — under that path (compared segment by segment, rejecting ./.., percent-encoded traversal, and a percent-encoded path separator such as %2f or %5c, at every decode level). The path check matters for a path-based provider that shares one origin per tenant (e.g. a Keycloak realm path /realms/<realm>), where the origin check alone cannot stop a tampered /settings from steering credentials to a sibling tenant. The issuer is supplied out of band and cannot be forged.
  • IdP-discovered endpoints are trusted where the issuer hosts them. An endpoint read from the issuer's own .well-known is neither origin-pinned nor path-scoped: that document is fetched from the pinned issuer origin and is authoritative for wherever the provider hosts its endpoints. This is deliberate — some providers (e.g. Google, Azure AD) serve their token and device endpoints from a different origin or path than the issuer, and discovery against them signs in normally. The co-location pin above still applies.
  • Plaintext-channel pin. A /settings response fetched over plaintext http to a non-loopback host (only reachable with allowInsecureTransport) is MITM-able, so its advertised endpoints are not trusted to route credentials without an issuer pin.

Without a pin, the behaviour against an https server that advertises its endpoints is unchanged: that server is trusted, as before.

Security

  • https is required by default for both the QuestDB server and the IdP endpoints; http is rejected unless the caller opts in with allowInsecureTransport(true). That opt-in relaxes only the QuestDB /settings link — the IdP device-authorization and token endpoints always require https (loopback excepted), so the device code and refresh token never cross the network in cleartext (matching the Python client).
  • Tokens never leak into logs or exceptions. Only the HTTP status of a token/device response is captured; the body — which carries access, id and refresh tokens — is never retained or surfaced in a message. The captured status is validated to be exactly three digits, so a malformed or hostile status line cannot splice ANSI/control bytes into a later [httpStatus=…] echo, nor can a short all-digit status (2, 5) be misread as a 2xx/5xx class — a malformed-length status falls through to the terminal reject path.
  • Untrusted IdP text is sanitized before it is shown in a prompt or an exception message: per-code-point stripping of control characters, ANSI escapes, CR/LF, and bidirectional / zero-width / Unicode-format characters (including supplementary-plane "tag" characters that arrive as surrogate pairs, and unpaired surrogates), so an attacker-influenced field cannot reorder, hide, or forge what a human reads — e.g. a right-to-left override that makes the displayed verification URL differ from the one the browser opens. A user code or verification URL that is non-empty on the wire but sanitizes to nothing is rejected (or, for verification_uri_complete, treated as absent) rather than shown as a blank line or handed to the browser launcher.
  • The token itself is validated before use. The token QuestDB will actually receive — the id token when the server encodes groups in the token, the access token otherwise — is rejected if it carries a control or non-ASCII character (outside 0x200x7e) before it is cached, placed in the Authorization: Bearer header, or used as the PG-wire password — so a tampered or hostile identity provider cannot smuggle a CR/LF into the request the client then sends to the trusted QuestDB server. Only the served kind is checked; a stray character in the unused token kind, which never reaches the wire, no longer aborts an otherwise usable grant. (The JsonLexer change below decodes JSON escapes, which is what turns a \r/\n in a token into a real byte rather than two literal characters.)
  • URLs are validated up front. Endpoint.parse rejects control characters, whitespace and display-unsafe code points anywhere in the url (so a tampered endpoint cannot inject a CR/LF into the request line or a bidi char into a log line), rejects bracketed IPv6 literals rather than mis-parsing them, rejects userinfo (user@host) — which the HTTP layer would otherwise try to connect to literally — terminates the authority at the first /, ? or # so a query or fragment is never folded into the host, and range-checks the port to 1..65535.
  • Bounded against a hostile or stalled server. Response reads are capped by a 4 MiB byte limit and a monotonic (System.nanoTime) deadline that bounds the whole read — covering both a chunked response whose chunk-size line is dribbled a byte at a time and a Content-Length body dribbled through stalled TLS records, either of which previously could keep a single read running well past the deadline. After such a bounded-read abort the half-read poll connection is dropped so the next poll reconnects on a clean socket, rather than the loop spinning on the stalled response's leftover bytes until the device code expires. The device-code lifetime, the poll interval, and the token TTL are all clamped (defaults applied for absent/zero values, hard caps for absurd ones). A 429 with no OAuth error is treated as a transient back-off (a 429 that also carries a terminal error such as access_denied still aborts on the error); a transient transport failure or 5xx during polling keeps polling until the device-code deadline rather than failing the sign-in (RFC 8628; matches the Python client), while a definitive OAuth error or a terminal 4xx aborts immediately. The trade-off is that a persistently flaky network is no longer cut short by a separate error budget — it polls to the device-code deadline.

Token persistence (opt-in)

By default token state is in-memory only, so a restarted process re-runs the interactive device flow. Passing a TokenStore persists it, so the restarted process resumes from the saved refresh token (one silent token-endpoint round-trip) instead of re-prompting — getToken() then even works as the first call, with no explicit signIn().

  • TokenStore SPI (io.questdb.client.cutlass.auth) — load/save/clear keyed by a non-secret TokenStoreKey (endpoints, client id, scope, audience, groups-in-token mode), plus an optional inLock hook for cross-process coordination. Wire it in with builder().tokenStore(...) or DiscoveryOptions.tokenStore(...). Persistence is best-effort: a store failure logs a warning through SLF4J at WARN and the in-memory token is used regardless — and the library ships slf4j-api with no binding, so that warning, like every other client warning, is discarded unless the application supplies one.
  • FileTokenStore (the default) — one plaintext JSON file per OIDC configuration under ${user.home}/.questdb/oidc-tokens/ (override with questdb.client.oidc.token.store.dir), the refresh token protected at rest by file permissions (0600 file, 0700 directory on POSIX) rather than encryption — the same approach gcloud, aws and gh take. The file name is a SHA-256 of that configuration (endpoints, client id, scope, audience, groups-in-token mode), so it leaks neither endpoint nor client id, and different servers, providers or client configurations stay in separate files. FileTokenStore.atDefaultLocation() / FileTokenStore.at(dir).
  • One store, one active login. The key names a configuration, and no field of it names a subject, so the hashed file name is not a per-person partition: two people signing in through the same configuration address the same file and the later sign-in overwrites the earlier. Signing several application users in at once needs a store each — FileTokenStore.at(dir) on a per-user directory, or a per-user questdb.client.oidc.token.store.dir — rather than a reliance on the name to separate them. The default location is per OS user already, so this only arises inside one OS user: a shared service account, or a process signing in on behalf of several people.
  • Treated as untrusted on load. A persisted file is attacker-writable, so on load it is size-bounded, parsed defensively (a corrupt/oversized/garbage file is ignored, not fatal), its fingerprint re-checked against the live config (a token minted for one identity is never served for another), and the served token re-validated for control/non-ASCII characters before it can reach a header — the same CR/LF / non-ASCII rejection the device flow applies to IdP responses. A tampered far-future expiry is clamped, not trusted. A bad file degrades to a refresh or an interactive sign-in.
  • Integrity and cross-process coordination. Each update is written to a temp file then atomically renamed, so a concurrent reader never sees a half-written credential. When the IdP rotates the refresh token on each refresh, the read-refresh-write is serialized across processes with an O_CREAT|O_EXCL lock file (not an OS advisory lock, which Java FileLock and Python flock cannot share); a process that cannot acquire the lock degrades to a lock-free refresh rather than stall.
  • The on-disk format is a frozen cross-language contract (design/oidc-token-persistence.md): the file name, JSON schema, atomic-write and lock-file protocols are specified so the Python client (and others) can share one file.

Supporting changes

  • JsonLexer now resolves JSON string escape sequences (\", \\, \/, \b \f \n \r \t, \uXXXX; lenient on malformed input), so string values arrive fully decoded. This also reaches the existing ILP error-response parser, which now sees decoded message/code/line/errorId fields.
  • Response.recv(int timeout) — a default method delegating to recv(), so an implementation of this exported interface written before the overload existed keeps compiling and linking, and keeps its previous behaviour. Both implementations here override it. It bounds the whole read to the timeout in total, not per socket read, so a server that dribbles the body — the chunk-size line of a chunked response, or Content-Length bytes behind stalled TLS records — cannot keep a single read running past the caller's deadline. A non-positive timeout keeps the legacy unbounded behaviour. Every ILP flush-path body read now passes an explicit timeout — actualTimeoutMillis, the base request timeout plus the throughput extension, not the raw request_timeout — so a tuned-low request_timeout paired with request_min_throughput cannot abort a large, still-progressing chunked body. That bounds each recv() call in total rather than the body cumulatively across calls; the ILP server is trusted, unlike the identity provider, whose reads OidcDeviceAuth.parseBody additionally caps by total bytes and one monotonic deadline. A response that completes within the per-call bound is unaffected, while one that dribbles a single fragment for longer than it — previously tolerated as long as each socket read made progress — now aborts. The only no-arg recv() left is the construct-time /settings protocol-version probe, whose retry loop already catches the abort. Both flush-path consequences of that newly reachable abort are handled — see the third review round below.
  • AbstractLineHttpSender plumbs the token provider through with a deferred, retriable per-request pull (a throwing or blank-returning provider leaves the request token-pending for the next row instead of corrupting the half-built request), so the very first send already carries a provider-sourced token. Its error rendering now routes every untrusted server-supplied string through putAsPrintable — the decoded JSON error body, the [http-status=…] field, and the line-protocol-version detection probe body — escaping control and Unicode format characters (bidi overrides, zero-width joiners, the BOM), so a hostile or proxied endpoint cannot reorder, hide, or forge the text shown in a LineSenderException (or spliced into a log line or terminal).
  • QwpWebSocketSender sources its auth header from the token provider too, re-querying it on every connect/reconnect so a rotating token keeps a long-lived WebSocket sender authenticated.

Tradeoffs and limitations

  • The origin pin behaves differently on the two construction paths. fromQuestDB(...) discovery trusts an endpoint read from the issuer's .well-known wherever the provider hosts it, so an off-origin provider (e.g. Google) signs in normally through a pinned issuer; only an endpoint the /settings response itself advertised is held to the issuer's origin and path. The explicit builder().issuer(...) pin is stricter — a plain sanity check that both supplied endpoints sit on the issuer origin — so an off-origin provider configured that way must have its issuer omitted, or its endpoints supplied to match. This matches the Python client.

  • A failed token pull is handled differently per transport, but neither drops buffered rows: over HTTP it leaves the request token-pending and retries on the next row; over WebSocket the initial handshake must obtain a token at build() (it fails fast otherwise), after which a pull that keeps failing on later reconnects is retried indefinitely with the buffered rows held in store-and-forward, until a token is available again — a token outage does not terminate a running WebSocket sender (store-and-forward Invariant B). A getToken() provider that fails only transiently recovers on both transports; a long WebSocket outage grows store-and-forward (and eventually applies backpressure) rather than ending the sender.

  • The co-location check requires the token and device-authorization endpoints to share an origin. A test that previously pointed the token endpoint at a dead second port to simulate an unreachable endpoint was reworked to drop a co-located connection instead (MockOidcServer.dropConnection()).

  • The plaintext-channel pin's firing path is exercised end to end by reaching the loopback mock through a short-form 127.x address that the loopback classifier deliberately rejects as non-loopback. That trick relies on the OS resolver expanding the short form (BSD inet_aton, on Linux/macOS), which Windows getaddrinfo does not do, so that one end-to-end test is skipped on Windows; the loopback classifier itself is covered cross-platform.

  • Persistence writes a long-lived refresh token to disk in plaintext, protected only by file permissions — anyone who can read the file holds a credential until the IdP expires or revokes it. This is why persistence is opt-in; for at-rest encryption, supply a TokenStore backed by an OS keychain or a secrets manager instead of FileTokenStore. On Windows POSIX permissions cannot be enforced, so the file currently relies on the user-profile directory's default ACL (owner-only ACL hardening is a follow-up); the client logs a one-line SLF4J warning at WARN the first time it cannot enforce them — which an application with no SLF4J binding never sees, so this boundary has to be read here rather than watched for at runtime.

  • Two stampede guards make a credential failure sticky for a few seconds. getToken() runs once per ILP flush and once per WebSocket (re)connect, so a failing credential path would otherwise cost one token-endpoint round trip — or one blocking store read, two stack-trace fills and a WARN line — per flush, on the producer thread and under this instance's lock, which is enough to trip an identity provider's rate limits and lengthen the very outage being retried. So a silent refresh that fails is not re-attempted for 5 s: calls inside that window fail immediately with the cached-token-expired error instead of re-hitting the provider, and only a real attempt re-arms the latch, which an explicit signIn() or clearCache() clears outright. A TokenStore.load that throws is retried once immediately (a one-shot fault, notably a carried interrupt flag, must recover on the next call), then backed off 5 s, doubling to a 60 s cap; a store that simply has nothing to return reports that by returning null and is unaffected. Both are deliberately short — a stampede guard, not a circuit breaker — but the cost is that a credential which recovers inside a window is picked up on the first call after that window rather than the first call after it recovers.

  • The HTTP chunk-size line is bounded where it is read. An overflowing chunk size was reading as valid framing (see the second review round below). The bound lives in AbstractChunkedResponse, which counts significant hex digits and rejects more than 15 BEFORE parsing — the only form that works, since the worst residue (10000000000000000 wrapping to zero, read as the terminal chunk) is indistinguishable from a genuine 0 afterwards. Numbers.parseHexLong keeps its two's-complement contract: ffffffffffffffff is still -1, matching parseHexInt beside it and the server-side io.questdb.std.Numbers of the same name, whose Long256 decoding depends on the wrap. io.questdb.client.std is exported, so that contract is shipped and is deliberately left alone; a caller parsing a count a remote peer chose must bound the digits itself, which is what the chunk parser now does. The javadoc says so and points at it as the worked example.

  • TokenStore.inLock may now return false without running the action. An implementation that waits for its lock must make that wait interruptible and abandon it on an interrupt, because the wait can outlast a caller's shutdown budget (see the second review round below). The false return reads as "no refresh happened", which OidcDeviceAuth already handled, but a third-party TokenStore inherits the new obligation.

  • A store-coordinated getToken() may briefly wait to acquire the cross-process lock before a silent refresh (a few seconds at most for FileTokenStore — the acquire budget is capped — then it proceeds without the lock). It still never waits behind an interactive sign-in; this is a quick silent refresh, not an interactive wait.

  • The Response.recv(int) bound (see Supporting changes) also tightens existing, non-OIDC ILP flushes. Each flush-response body read is now bounded in total rather than re-arming its timeout per socket read, so a response that legitimately dribbles a single fragment for longer than the per-flush budget (request_timeout plus the request_min_throughput extension) — previously tolerated as long as each socket read made progress — now aborts. The bound is per recv() call, not cumulative across the whole body, so it is a ceiling on one stalled fragment rather than on the response as a whole. A healthy response is unaffected. Making that abort reachable inside flush0's retry scope had two consequences, both fixed in the third review round below rather than accepted: a drain abort after a 2xx no longer re-sends a batch the server had already committed, and a body-read abort under an error status no longer reclassifies a definitive 401/403/405 as a transport failure. The pre-existing ILP-over-HTTP at-least-once window is therefore not widened; the only observable effect is that a pathologically slow single fragment is cut short instead of tolerated indefinitely.

  • A malformed HTTP response head now fails the flush instead of being retried. This is the second change in this branch that existing, non-OIDC ILP-over-HTTP users can observe (the Response.recv(int) bound above is the first). HttpHeaderParser rejects a response head past its fixed 4096-byte buffer, a malformed Content-Length, or a non-HTTP/1.x status line. Reaching one needs an intermediary — QuestDB's own /write answers 204 with a small head — but where one does, the flush now fails immediately with a non-retryable LineSenderException naming the malformed head, rather than spending the retry budget re-sending a batch the server had already answered. A healthy response is unaffected.

  • The JsonLexer escape decoding (see Supporting changes) also changes how a QuestDB row error renders. This is the third change in this branch that existing, non-OIDC ILP-over-HTTP users can observe, and the one they are most likely to meet — it is the ordinary bad-line rejection. QuestDB builds that error with a real newline (LineHttpProcessorState.formatError) and escapeJsonStr puts it on the wire as the JSON escape \n. The client used to copy those two characters through verbatim, so LineSenderException.getMessage() read ...on line(s):\nerror in line 1: ...; the lexer now decodes the escape to a newline and putAsPrintable re-escapes it for display, so the same failure reads ...on line(s):\u000aerror in line 1: .... Neither form lets a raw newline into the message — that is what putAsPrintable is there for — and decoded message/code/line/errorId fields are the point of the change; but the text an operator reads, or a log scraper matches on, is not the text it was. LineHttpSenderErrorResponseTest#testQuestDbRowErrorRendersTheDecodedNewlineAsAnEscape pins the new rendering. No assertion in questdb or questdb-enterprise breaks: each matches a substring that does not span the escape.

  • The response-head read is bounded on elapsed time as well, and that is the fourth change existing, non-OIDC ILP-over-HTTP users can observe. Same mechanism as the first, one read earlier: ResponseHeaders.await(int) now bounds the whole head read instead of re-arming its timeout per socket read, so a head that dribbles but keeps making progress aborts where base ran on with it. It precedes every response, including the 204 QuestDB's own /write answers with, so reaching it needs an intermediary exactly as the body case does. What separates it from the other two flush-path reads is how little it can conclude: at the point it aborts no status has been read, so unlike the 2xx drain — which knows the server committed and reports the success it was — and unlike the error arm — which has a verdict to surface — it says nothing about whether the batch landed. It falls to flush0's transport arm and is retried, which is the only answer available, and that retry spends the pre-existing ILP-over-HTTP at-least-once window: against a table without DEDUP keys, a peer that dribbles a head past the budget duplicates rows. The behaviour is kept — the alternative is the unbounded read the bound exists to remove — but the callsite now says so and LineHttpSenderErrorResponseTest#testDribbledResponseHeadFailsTheFlushWithinTheRetryBudget holds it (restoring the per-read re-arm turns it red on the test timeout, which is what unbounded looks like from the flush path).

  • Widening the lock-hold multiple turns a previously-accepted configuration into a build() failure. FileTokenStore's staleness window must now exceed six times httpTimeoutMillis rather than four, so httpTimeoutMillis(120_000) — the cap — paired with a default store (600s window) is rejected at build() where it used to be accepted, and the comment that blessed that pairing was wrong: a hold can reach 720s. The error names lockStaleMillis and the fix is to raise it, but this is a break for anyone already on that pairing, and the only one in this branch that a caller's own configuration rather than their traffic can trigger. The default 30s timeout is unaffected (180s of a 600s window). The same figure caps acquireForGetToken()'s in-process wait, which grows with it — a peer waiting behind another instance's refresh now fails fast after six times the timeout rather than four.

Tests & docs

OidcDeviceAuthTest (~123 cases) + MockOidcServer, BrowserLauncherTest, LineHttpSenderTokenProviderTest, WebSocketTokenProviderTest + TestWebSocketServer, SenderBuilderErrorApiTest, JsonLexerTest, LineHttpSenderErrorResponseTest, DisplaySafeTest, ChunkedResponseTest/ResponseTest, QwpQueryClientTokenProviderTest, FileTokenStoreTest, OidcDeviceAuthPersistenceTest, WebSocketCredentialCancellationTest, SenderPoolSfTokenProviderTest, BackgroundDrainerCredentialOutageReportTest, NumbersTest, HttpClientConstructorLeakTest; runnable OidcDeviceFlowExample / OIDCAuthExample; and README "OIDC Sign-In (Device Flow)" and "Persisting the Token Across Restarts" sections. Coverage includes:

  • .well-known discovery via a pinned issuer; a discovery document that omits the device-authorization endpoint
  • the co-location check rejecting split-origin endpoints; the builder().issuer(...) origin pin rejecting off-origin endpoints; a /settings-advertised endpoint rejected when off the issuer origin; an endpoint discovered from the issuer's .well-known accepted even when off the issuer origin (the Google case)
  • issuer path scoping: endpoints under the issuer path accepted; a sibling realm, percent-encoded traversal, an encoded path separator (%2f), and a percent-encoded backslash (%5c) rejected
  • the plaintext-channel pin requiring an issuer pin for advertised endpoints over http (firing path skipped on Windows)
  • the audience parameter discovered from /settings and sent on the device and refresh requests
  • token-provider support over HTTP and WebSocket (initial upgrade and re-queried per reconnect; static token / username-password still work over WebSocket); rejected for TCP/UDP; mutual exclusion with other auth; null/empty provider token rejected; deferred build-time pull; no sender corruption when the provider throws after a flush; the QWP egress query client's token provider — header synthesis, per-resolve re-query, token validation, and mutual exclusion
  • a token with a control or non-ASCII character rejected rather than sent (the served kind validated); tokens never echoed in messages
  • bounded reads: a stalled body and an oversized body aborting on the deadline / 4 MiB cap; a chunked body read aborting when the chunk-size line is dribbled; a bounded-read abort dropping the dirty poll connection so the next poll reconnects and signs in
  • the poll model: a 429 and a transient 5xx/transport failure keep polling to the deadline; a terminal 4xx and an OAuth error fail fast (including a 429 that also carries a terminal error); slow_down growth and the 60 s interval clamp; device-code-lifetime and clock-skew clamps
  • Endpoint.parse rejecting a malformed url: userinfo (user@host), a bracketed IPv6 literal, an out-of-range port, and control/whitespace/display-unsafe characters
  • a malformed HTTP status code rejected on both the device-authorization and the token-poll path: a non-numeric status, and a short all-digit status (2, 5) that must not be read as a 2xx/5xx class
  • display sanitizing: bidi/zero-width, lone-surrogate and supplementary-plane format characters stripped from the challenge and OAuth error; a server's JSON error body, its HTTP status field, and the protocol-version probe body with such characters escaped, not rendered raw; a user code or verification URL that sanitizes to empty rejected, and a verification_uri_complete that sanitizes to empty treated as absent
  • JsonLexer escape decoding, including a \uXXXX escape split across two parse fragments, and the lenient/exotic escape arms
  • getToken() failing fast while another thread holds the lock in an interactive sign-in or a silent refresh; native-memory cleanup on the error/rejection construction paths
  • discovery gated on status: a /settings body and a .well-known body under an HTTP error status refused as configuration, and a malformed status rejected without echoing its bytes
  • a dribbled response body under a 2xx not re-sending the batch, and under an error status still surfacing the status rather than a transport timeout
  • a TokenStore throwing before its action degrading to exactly one uncoordinated refresh, and throwing after it keeping the completed refresh
  • HttpClient constructor rollback at four failure points, asserted through assertMemoryLeak
  • token persistence: round-trip save/load with 0600/0700 permissions and control-char JSON escaping; a corrupt, empty, oversized, schema-version-mismatched, or per-field fingerprint-mismatched file ignored; a tampered far-future expiry clamped and a CR/LF served token rejected on load; a restart serving a valid persisted token (or silently refreshing an expired one) without re-running the device flow; rotating vs non-rotating refresh write behaviour; a swallowed save not replaying a revoked token; the cross-process lock-file protocol (acquire, mutual exclusion, stale-steal, degrade); getToken() degrading to a lock-free refresh when a peer holds the lock; the HTTP-timeout cap; the file-name hash pinned as a cross-language contract
  • the trust sentinel under a symlink squatting its name and under a mark that cannot be lifted; a rotated refresh token adopted from a refresh whose served token was rejected; clear() reclaiming a write temp stamped in the future; a dribbled response head failing an ILP flush inside its retry budget

Review follow-ups

A level-3 review of this branch surfaced the issues below; all are fixed here, each production fix with a regression test proven to fail without it (see the commit history for detail).

Confirmed defects:

  • Blank served token: adopt()/storeTokens() accepted a whitespace-only served token (it passed isEmpty()/hasOnlyTokenChars() vacuously), so signIn() reported success and getToken() served a blank Bearer header the server only answers with 401, never falling back. Now rejected via Chars.isBlank; a blank served kind is folded to absent so selectToken() surfaces the actionable error.
  • getToken() lock contention: the unconditional tryLock() failed fast on any lock hold, so concurrent callers sharing one OidcDeviceAuth threw on every token refresh. It now waits briefly behind a peer's silent refresh (bounded by httpTimeoutMillis) and fails fast only behind an interactive sign-in.
  • SYNC initial connect: a token-provider failure was treated as a transport outage and retried for the whole reconnect budget (5 min default) then wrapped. It now fails fast with the provider's own exception, matching the OFF-mode and background-reconnect paths.

Hardening and coverage:

  • Discovery parsers reject array-wrapped JSON, so {"config":[{...}]} can no longer surface fields at the trusted config depth.
  • The cross-process lock file is created with its owner stamp in one atomic exclusive open, removing the create-then-stamp gap a GC pause could straddle (design doc updated; the Python client must mirror it).
  • The ILP token-provider request is built once per flush rather than twice. (validateToken still re-scans every pulled token by design - a provider may mutate a reused buffer between flushes - but that scan is O(token length), runs once per flush, and is dwarfed by the network round-trip.)
  • Added regression tests for previously untested load-bearing guards: the parseHex4 non-ASCII guard, the raw .. issuer-path reject, the FileTokenStore size caps, the store-and-forward credential-timer reset, and the ILP flush whole-read timeout bound.
  • Docs: HttpTokenProvider.getToken() now discloses the OS-bounded connect stall; TokenStore.inLock documents its no-reentrancy contract; FileTokenStore states the concurrent-refresh residual (token-family revocation on a reuse-detecting IdP) instead of understating it.

Review follow-ups (second round)

A further review round surfaced the issues below. As before, every production fix carries a regression test shown to fail without it; the commit messages record the counterfactual output for each.

Confirmed defects:

  • A device grant inherited the previous user's refresh token. storeTokens() kept the current refresh token whenever a response omitted one, for refresh grants and fresh device grants alike. For a refresh that is right — RFC 6749 §6 makes the field optional and the same authorization is continuing — but a device grant is a new authorization and may be a different human. So user A's refresh fails, user B completes the device flow without a refresh token, B's access token expires, and the next silent refresh presents A's retained token and resumes as A: no prompt, no error, nothing in any log recording that the identity changed. Persisted it is worse — the refresh token is unchanged, so persistIfRotated() sees no rotation and skips the save, leaving A's whole entry on disk for the next process start to adopt. The omission policy is now grant-specific: a device grant that returns no refresh token clears it, so getToken() asks for an interactive sign-in instead. Covered end to end, including a restart over the same store.
  • The token store's lock waits could not be interrupted. A credential pull owns no socket, so closeTraffic() cannot reach it; the only lever is an interrupt, sent by the send loop's connect cancellation on the foreground path and by BackgroundDrainerPool's shutdownNow() on the orphan-drainer path. Neither reached the built-in path: the in-process lock was taken with lock(), and the lock file was polled through Os.sleep, which catches InterruptedException and keeps sleeping to its own deadline. Since the acquire budget caps at 30 s — the same as the QWP shutdown budget — a sender closing while another same-identity instance held the lock burned the whole budget and then gave up on its I/O thread, delegating teardown of the native client, the cursor engine and the store-and-forward slot lock; on the drainer path it abandoned the drainer still holding the orphan slot's lock. inLock now takes the process lock interruptibly, polls with Thread.sleep, and checks for an interrupt before running the critical section. Every prior test blocked the pull in an interruptible test double, so the shipped path was untested; the new tests block it in a real OidcDeviceAuth over a real FileTokenStore whose lock a peer holds.
  • An overflowing chunk size was accepted as valid framing. The guard rejected only a negative parse result, which is one of three ways an unchecked val << 4 accumulation wraps, and the least harmful — a negative size matches neither the data branch nor the terminator, so the state machine spins, which is at least visible. The other two report success with the wrong bytes: 10000000000000000 wraps to zero and reads as the terminal chunk, so the caller gets a complete-looking body that is truncated and the connection's framing is lost for the next keep-alive response, while a longer value wraps to a short positive count that mis-frames everything after it. Truncated JSON parses. The size line is chosen by the server, untrusted for a discovery or token response. The size line is now bounded in the chunk parser itself, before the parse, so all three residues are refused at the point the untrusted digits are read — see the chunk-size note in Tradeoffs.
  • An orphan drainer reported a credential outage nowhere, and mislabelled it. The drainer's error sink was never wired into its drain loop, so the loop's credential-unavailable report was dispatched into a null; at initial connect the exception matched none of the typed arms and landed in the generic transport arm, whose warning says the cluster is unreachable. A revoked token therefore surfaced as a network fault while rows accumulated in store-and-forward, pointing an operator at disk sizing rather than at their credentials. The sink is now wired and the condition named. TERMINAL is filtered on the way through: on an orphan loop a terminal is the loop handing the slot back to the drainer to decide, so forwarding it would announce a dead producer for a rotating credential the next sweep accepts, and double-report every quarantine.

Exported API compatibility:

  • Three public signatures had been replaced rather than added to, in packages module-info.java exports and that ship a javadoc jar, with no japicmp gate to catch it: Response.recv(int) arrived as an abstract interface method, two QwpWebSocketSender.connect(..., String, ...) overloads were retyped to Supplier<String>, and the multi-host AbstractLineHttpSender.createLineSender gained a parameter in place. An existing caller would fail with NoSuchMethodError, an external Response implementation with AbstractMethodError. No affected caller exists in this repo, questdb, or questdb-enterprise, so this is a latent break rather than an observed one. The exact old signatures are restored as delegates; the supplier-backed connect entry points are renamed connectWithCredentialSupplier rather than left as overloads, because a String and a Supplier<String> parameter of equal arity make a bare null credential argument ambiguous and would trade a link error for a compile error. Verified by compiling a caller written against the pre-branch signatures against both the unfixed and fixed classes.

Coverage:

  • Token providers on store-and-forward pooled senders — the pooled WebSocket + SF + OIDC combination this client is built for, and the one no test covered. SenderPool applies the provider on two legs and only the non-SF one was exercised. Unwired, every SF pooled sender's upgrade would go out unauthenticated and take a 401, surfacing later as ring backpressure or a quarantined slot rather than at connect time; on the recovery leg a recovery delegate replays the previous run's data, so it would quarantine the slot and report DATA_LOSS for replayable rows.
  • The fixed-versus-rotating credential tag, which decides whether a 401 during an orphan drain may quarantine a slot. Nothing connected the builder half to the drainer half. A mis-tag is silent at build time and asymmetric: tagging a rotating credential as fixed drops a .failed sentinel that nothing in production clears, permanently abandoning replayable rows over a token the next pull would have refreshed, while the other direction only delays the operator's signal. isCredentialDynamic() exposes the tag on a built sender, asserted alongside the header the server actually received and the value the drainer's reconnect factory reports.

Review follow-ups (third round)

A third review round surfaced the issues below. As before, every production fix carries a regression test shown to fail without it; the commit messages record the counterfactual output for each.

Confirmed defects:

  • Discovery trusted a response body whatever its status. fetchJson awaited the response headers and went straight to parsing, so both discovery paths read configuration out of a non-2xx body. A discovery document decides where the user signs in and where the long-lived refresh token is POSTed, and an error body can easily carry the keys — an error envelope, a proxy's branded page, a captive portal, a tenant-not-found stub. Black-box proofs constructed a working instance from an HTTP 500 /settings response and from an HTTP 404 .well-known response. The token and device-authorization paths already gated on status; this one did not. requireSuccessStatus now runs before parseBody: it validates the status is exactly three bare digits before echoing any of it — the header parser copies the status-line token verbatim apart from SP/CR/LF, so a non-digit byte means a malformed or hostile line that must not splice ESC or other control bytes into a message, a log or a terminal — and then requires a leading 2. A short all-digit status is malformed too and must not be read as a class by its leading digit, matching isHttpStatusSuccess elsewhere in the class. On rejection the body is drained within the usual bound so the keep-alive connection stays usable, and the connection is dropped when the drain cannot finish, mirroring readResponse on the token path. Each call site passes its own message, so a /settings failure and a .well-known failure are told apart.
  • A response-body read abort could re-send a committed flush. Both body reads in flush0 sit inside the try whose only catch treats HttpClientException as a retryable network error. Base could not throw there for a dribbling-but-progressing server, because recv() re-armed its timeout on every socket read; bounding the whole call (see Supporting changes) made it reachable, and the two branches fail differently. On the success branch a 2xx is the commit — the server already has the rows — so draining its body afterwards is only bookkeeping to keep the connection reusable, and an abort there re-sent a batch the server had accepted, with a retry budget that kept trying. The drain is now wrapped: on abort the connection is dropped, since unconsumed bytes would mis-frame the next response, and the flush is reported as the success it was. On the error branch the status is the verdict and the body is only detail for the message; an abort escaping into the catch reclassified a definitive 401, 403 or 405 as a transport failure, burned the whole retry budget against an endpoint that would keep refusing, and finally reported "Connection Failed: timed out" with the real status nowhere in it. throwOnHttpErrorResponse now wraps its body reads — all four branches at once — and falls back to a status-only exception. Reaching either needs a chunked, slowly dribbled body, which QuestDB's own /write does not produce (it answers 204 non-chunked), so exposure is through intermediaries. The pre-existing testFlushResponseBodyDribbleAbortsOnRequestTimeout asserted that a dribbled body fails the flush, against a mock that answers 200 — so it was pinning this defect rather than guarding against it. It is reworked rather than left in place: it still proves the whole-read bound, since an unbounded read would hang to the test timeout, and now also proves the batch is sent exactly once, against a retry budget a re-send would visibly spend.
  • A throwing TokenStore took the whole sign-in down with it. TokenStore is a user-implemented SPI and persistence is documented best-effort, but tryRefreshCoordinated called inLock bare, so a store that threw before running its action refreshed nothing even though the client held a perfectly good refresh token. What the right degrade is depends entirely on whether the refresh already ran, which only the action can report, so the call now tracks whether it entered and completed. Threw before the action: nothing was refreshed, so run one — exactly one — uncoordinated refresh; the point of the lock is that a rotating refresh token must not be POSTed twice, and a reuse-detecting provider answers a replay by revoking the whole family. Threw after the action completed, releasing a lock or closing a handle: the refresh happened and the token is live, so report what the action returned; re-running it is that same double-POST, and throwing tells the caller a completed sign-in failed. The action itself threw: that is the refresh's own failure, not the store's, so it propagates untouched — never swallowed, never replayed. Error is deliberately not caught; an OutOfMemoryError is not a store fault to degrade around. FileTokenStore also let unchecked exceptions escape its own lock bookkeeping — SecurityException from a SecurityManager, UnsupportedOperationException from a filesystem that cannot carry POSIX permissions — so the acquire path now degrades to lock-free on those as it already did on IOException, and the release path, which runs in a finally after the critical section, absorbs them so bookkeeping cannot replace a completed result. The guard above already contains such an escape, so this second half is about the quality of the degrade — coordination is kept for that refresh rather than lost — and about the reference implementation honouring the contract TokenStore.inLock publishes.

Pre-existing defect newly exposed:

  • HttpClient construction leaked when it failed partway. A constructor that fails partway leaves an object nobody can close: it never reaches the caller, so no finally, no try-with-resources and no close() ever runs on it, and whatever it had already taken is lost for the life of the process. HttpClient's base constructor takes a socket and two native buffers, then each platform subclass builds its poller, and neither step guarded the earlier ones. What makes it worth fixing is the trigger: epoll_create and kqueue fail on fd exhaustion, and the mallocs fail under memory pressure, so the failure arrives exactly when resources are already scarce, and a caller that retries compounds the loss each time. The root predates this branch; OIDC discovery newly exposes it by building a client per fetch. The base constructor now stages the socket and both buffers in locals, assigns the fields only once ResponseHeaders has succeeded, and frees in reverse order under catch (Throwable); each platform subclass wraps its poller construction and calls super.close() before rethrowing. Kqueue already guarded its own constructor this way, so this is the same pattern applied one level out; Epoll, Kqueue and FDSet each free their own allocations on failure already, and what leaked was purely what the caller had taken before calling them. HttpClientConstructorLeakTest covers four failure points through assertMemoryLeak — removing the rollback leaks 65536 bytes on the base path and 131072 on the poller path. The base case injects a negative response-buffer size and runs everywhere; the poller cases are Assume-guarded so only the running platform's executes, leaving epoll and FDSet to CI, which the class javadoc records. The pollers are failed through their facades rather than through a failing size, even though a size would need no facade: Kqueue's own failure path calls close() with its descriptor still zero, so an allocation failure there would close the test JVM's stdin — a facade returning a negative descriptor is the shape fd exhaustion actually takes and touches no real descriptors.

Review follow-ups (fourth round)

A fourth review round surfaced the issues below. As before, every production fix carries a regression test shown to fail without it; the commit messages record the counterfactual output for each.

Confirmed defects:

  • A carried interrupt failed a store-and-forward delegate close. PoolHousekeeper.stop() and SenderPool.stopStartupRecoveryDriver() escalate to Thread.interrupt() when their join times out, to break a recovery build's credential pull — and the thread they interrupt is the same one that then runs senderPool.reapIdle() and the startup-recovery step's finally, both of which close a delegate. CountDownLatch.await(t, u) tests Thread.interrupted() before it ever consults the latch, so the shutdown await returned having waited 0 ms, close() took the failed-stop branch, and the slot was reported with its flock still held — precisely the outcome the interrupt was added to prevent. That branch re-asserts the flag, so in a reap sweep every remaining delegate failed the same way and QuestDB.close() returned still holding their slots. QwpWebSocketSender.close() is now interrupt-neutral, the shape QwpQueryClient.close() and QueryWorker.shutdown() already use — the query half of the pool got that treatment when the escalation landed; the ingest half, which reapIdle() reaches first, did not. An interrupt delivered during the close still takes the failed-stop branch, which is correct. PoolHousekeeper's comment claimed every wait on the pull path is interruptible; the token POST's connect, send, await and parse run on the native HTTP client, which no interrupt breaks, so it now says what the escalation does and does not buy.
  • An unparseable response head was retried as a transport failure. HttpException had been added to flush0's transport catch, which was right about the disconnect and the exception type — uncaught it escaped flush0 entirely, leaving the next flush on a connection holding a half-read response and throwing a raw HttpException past every caller's catch (LineSenderException). But it must not be retried. HttpHeaderParser only runs on bytes that arrived, so the exception is positive evidence the server answered — the same evidence the 2xx drain arm treats as decisive — and the head is chosen by an intermediary, so the next attempt parses the same block and fails identically. Measured against a mock returning a 5000-byte head: 16 sends over 10.8 s per flush at the default retry budget, where the same trigger sent once before. It now disconnects, reports a non-retryable LineSenderException naming the malformed head, and does not re-send. The existing test asserted the retry, so it was pinning this rather than guarding against it.
  • A recovered store read reverted a completed sign-in. maybeLoadFromStore() deliberately leaves its latch unset when a read throws, so a transient fault is retried — but it runs at the top of getToken(), ahead of the cache check, and adopt() assigns the served kind, the expiry and the ttl unconditionally, never comparing the file against what is already in memory. A store directory unavailable across signIn() and readable afterwards (an unmounted home, a container started before its volume attaches) therefore undid it: the read failed, the human authenticated, the save failed the same way and was swallowed, and the next getToken() — one per ILP flush — installed the previous entry over the grant just obtained. persistIfRotated() now latches the flag, above the rotation check so it holds whether or not the save succeeds, and covering the refresh-only path through adoptRotatedRefreshToken() with the same line. A store that never yielded a token is unaffected.
  • The client wrote a store entry adopt() refuses to read back. adopt() rejects a refresh token carried with neither token kind as positive evidence of a foreign writer — the guard that stops someone who can write the store directory swapping in their own refresh token. Its justification rested on a callsite count ("persistIfRotated runs solely at the tail of storeTokens"), and that count was wrong: under groupsInToken, a stored entry carrying only an access token takes adopt()'s own served-kind-absent branch, which nulls both kinds and keeps the refresh token, so a refresh that rotates the refresh token but still returns no id token reaches adoptRotatedRefreshToken()persistIfRotated() with both null. The file it wrote was one it would never read back, so every restart re-ran the device flow over a live refresh token on disk. persistIfRotated() now declines that shape; the previous entry stays, its burned refresh token costs one silent round trip on the next start, and the rejection keeps its teeth.

Review follow-ups (fifth round)

A fifth review round found no new defect in the shipping code. It produced coverage for two paths that ran only in production, two accuracy fixes, and one candidate that was examined and deliberately left as it is.

Coverage:

  • The token store's lock-restore arm ran only in production. stealIfStale captures a lock it judged stale into a private name, then re-reads it to confirm it captured the lock it judged rather than one a peer recreated in the gap. When that check says no, the capture has to go back — hard-linked rather than renamed, so a third party that claimed the freed path keeps its live lock, and byte for byte, because releaseLock verifies the owner stamp before deleting. None of that ran under test. Reaching it through stealIfStale needs a peer to replace the lock file between the staleness read and the ATOMIC_MOVE, an interleaving no test can force without a production seam, so testConcurrentStealersLeaveExactlyOneWinner only ever drove the confirmed-stale path — and its three observables (no stealer threw, the lock is gone, no capture temp survives) all hold under the bare deleteIfExists(lock) that the method's own comment says must never be used, because a bare delete also removes the lock and leaves no temp. So roughly thirty lines guarding a peer's live lock could be deleted or inverted without a red test, and the failure they prevent is two holders POSTing the same rotating refresh token, which a reuse-detecting identity provider answers by revoking the whole family. The restore is split into restoreCapturedLock(lock, captured) — pure code motion — and driven directly by two deterministic cases that need no interleaving: one asserts the peer's owner stamp survives byte for byte, the other has a third party already holding the path and asserts its lock is untouched. Dropping the capture instead of restoring it fails the first; replacing createLink with a REPLACE_EXISTING move fails the second.
  • The sidecar's ERR sanitizing had no test that could fail. test_a_failed_connect_oidc_leaves_the_protocol_in_sync claimed to drive the newline case through a real failure. It cannot: the connect it aims at a closed port fails inside fetchJson, which throws OidcAuthException carrying a fixed literal, and that class builds every message from literals plus putSanitized, which strips CR and LF — Throwable.toString() never appends the cause either. So the reply it inspects has no newline to remove whether or not the OIDC verbs sanitize, and both its assertions hold with the sanitizing deleted (the second, pulls == 0, is true by construction on a fresh sidecar as well). QwpSidecarErrReplyTest now supplies a multi-line message directly — the nested-cause-and-stack-frame shape a Throwable actually carries a break in — and fails when sanitize() is dropped or when a null message renders as the word null. The interpolating ERR replies route through one err(out, message) helper that sanitizes, prints and flushes, so a new verb inherits the sanitizing instead of having to remember it; the ERR replies left writing directly are fixed literals with nothing to interpolate. The e2e test's docstring now claims only what it proves: that a real device-flow failure replies ERR and leaves the stream in sync for the next command.

Accuracy:

  • AbstractResponse.recv(int) and AbstractChunkedResponse.recv(int) now carry @Override, which the recv() beside them in the same two classes already had. It matters for this pair specifically: the two methods delegate in opposite directions, so an implementation whose signature drifts from the interface does not fail to compile — it silently inherits the default that discards the bound and defers to recv(), and recv() here is implemented as recv(defaultTimeout), which recurses.
  • OidcDeviceAuthTlsTest's javadoc named the partial-record read as its reason for existing. It does not script one — both canned responses are a few hundred bytes and arrive whole, so recvOrDie never returns 0 without consuming its timeout, and removing the whole-call bound leaves all three assertions green. It now states what it does cover, which is real and worth having (the whole device flow — handshake, record framing, two JSON bodies read back — over a real TLS socket, the only shape a production sign-in takes and one nothing else exercised), and names where the bound is actually pinned: ResponseTest#testRecvHonoursTotalTimeoutWhenNoApplicationBytesArrive and ChunkedResponseTest#testRecvHonoursTotalTimeoutWhileChunkSizeDribbles. That matters because OidcDeviceAuthTransportBudgetTest points this way for the end-to-end half, and a reader following the pointer would otherwise arrive and find nothing.

Examined and deliberately left unchanged:

  • The rotating-401 dwell anchor across an accepted connect. connectWithDurableAckRetry() clears firstDynamicCredentialAuthFailureNanos on its three transient arms but not on a successful clientFactory.reconnect(), so the anchor keeps ageing across the wire session that follows — and a session that durably acks nothing never reaches noteAckProgress() either. A later 401 can therefore arrive with the attempt threshold already met from an earlier run and a dwell measured from a rejection that ended minutes ago, and quarantine on the first sweep of what should have been a fresh ride-out. Clearing the anchor on a successful connect is the obvious fix and is the wrong one: testFlappingCredentialEscalatesAcrossMidDrainRecycles pins the opposite contract — an accepted connect does not end a run of rejections; only real ack progress does — and the change turns a flap that quarantines after six rejections into one that quarantines after 256, reopening in slower motion the recycle-forever hole the field promotion closed. Closing the gap properly means routing the loop's own transient observations through to the drainer's anchor, which trades a longer ride-out for a later operator signal. That is a design decision rather than a bug fix, and the current behaviour is in any case strictly better than the pre-branch one, which quarantined on the first 401 unconditionally.

Review follow-ups (sixth round)

A sixth review round found one defect in the shipping code, one bounded cost, one arithmetic error in a safety bound, one documentation gap with a security consequence, and two paths whose guards no test could fail on. It also corrected an overstatement this branch's own analysis had made.

Fixes:

  • AWTError escaped both guards on the browser launch and aborted signIn(). BrowserLauncher.open() caught Exception and DeviceCodePrompt.openBrowser() catches LinkageError; java.awt.AWTError extends Error directly and is neither, so it passed through both. Toolkit.getDefaultToolkit() raises it whenever assistive_technologies — from $JAVA_HOME/conf/accessibility.properties or the matching system property — names a class the runtime cannot load, which is the stock configuration on several Linux distributions that point at org.GNOME.Accessibility.AtkWrapper without shipping the package; a set DISPLAY that answers no X server reaches the same error by another route. Desktop.isDesktopSupported() calls the toolkit unconditionally with no headless short-circuit, so the default prompt met it on the way to a best-effort browser open, and an interactive sign-in died with an AWT error after the verification URL and code had already been printed — as a type the caller's documented catch (OidcAuthException) does not handle. open() now catches Throwable, rethrowing LinkageError first so the missing-java.desktop degrade stays where DesktopFreeModulePathTest pins it. Two offsets already existed and still apply: -Djava.awt.headless=true skips the assistive-technology loading entirely, and questdb.client.oidc.open.browser=false returns before Desktop is touched. BrowserLauncherAwtErrorTest drives the real prompt in a forked JVM whose toolkit cannot initialise; it forks twice because the failure is one-shot (the toolkit throws on the first getDefaultToolkit() and then completes, so a probe sharing the process would consume the only throw), and the probe half is also what keeps the launch half from reaching a real browser on a developer machine. Reverting the guard to catch (Exception) turns it red.
  • The lock-hold multiple left out two of the six timeout budgets a refresh can spend. LOCK_HOLD_HTTP_TIMEOUT_MULTIPLE sizes the worst-case time a coordinated refresh holds the token store's cross-process lock, and build() enforces it as the floor under FileTokenStore's staleness window; a hold that outruns that window is judged abandoned, so a peer steals a live holder's lock mid-refresh and both processes POST the same rotating refresh token. It was 4, counting send, await, parse and the parse-failure drain. The comment beside it said httpConfig() bounds the connection phase "by httpTimeoutMillis too", but HttpClient spends the TCP connect and the TLS handshake as separate budgets — it anchors a fresh tlsHandshakeStartNanos and grants the handshake its own connectTimeout rather than continuing the connect's. Six is the count of independently bounded phases. See the tradeoff below for what widening it costs.
  • The drainer's dispatcher close busy-spun on a deliberately-interrupted thread. run()'s finally closes the loop error dispatcher this branch added, on a thread whose interrupt flag stopRequestedOrInterrupted() leaves set so loop.close()'s latch await throws rather than blocking on a wedged I/O thread. SenderErrorDispatcher.close() drains by joining its delivery thread against a refreshed deadline and re-asserts the flag in its catch, so Thread.join(millis) threw on arrival on every pass and the loop never parked. The scope is narrower than it first looked, and the earlier draft of this note overstated it: join() returns normally the moment the thread is no longer alive, so neither the wait's duration nor which errors get delivered changes — measured, an interrupted and an uninterrupted close of the same dispatcher finish within a millisecond of each other and reach the same verdict on whether the thread survived. The cost is CPU alone: 15k–53k join attempts measured, one core pinned for that window, per closing drainer, with max_background_drainers defaulting to 4, on shutdown only. Clearing the flag around that one call and restoring it afterwards is the same clear-and-restore QueryWorker.shutdown(), QwpQueryClient.close() and QwpWebSocketSender.close() already use, which is why the pre-existing foreground caller never hit it. No test: the spin is invisible to wall-clock and to delivery, so the only discriminator is CPU time, and a threshold assertion on that would be more fragile than the three lines it guards.
  • Os.sleep destroyed an interrupt in the store's rename retry. replaceTarget() backed its retry off with Os.sleep(), which catches InterruptedException, sleeps on to its deadline and never re-asserts the flag — the hazard acquireLock()'s poll in the same file already avoids and documents in as many words. save() parks and restores only the flag it saw on entry, so an interrupt arriving mid-save had to survive this sleep on its own, and that interrupt is what PoolHousekeeper.stop() delivers to break a recovery step blocked in a credential pull. Swallowed, the stop signal is gone: the housekeeper's second join times out and close() can return with the recoverer still holding its store-and-forward slot flock, so an immediate reopen fails with "sf slot already in use". The retry now sleeps interruptibly, re-asserts the flag and abandons; the denial it was retrying still propagates, and persistence is best-effort either way. testReplaceTargetPreservesAnInterruptDeliveredDuringItsBackoff denies the rename the way its sibling does and proves the denial bites on the host before resting on it.

Documentation:

  • tlsConfig also governs identity-provider certificate validation, and nothing said so. Builder.tlsConfig() carried no javadoc at all, while the two allowInsecureTransport() javadocs beside it — and the README — promise that the identity provider endpoints are "never relaxed" and that the device code and refresh token "never cross the network in cleartext". That is true of the scheme and silent about the trust anchor: one OidcDeviceAuth holds a single ClientTlsConfiguration and threads it through the /settings fetch, the .well-known fetch and the device-authorization and token POSTs alike, so a user who reaches for INSECURE_NO_VALIDATION to talk to a QuestDB server with a self-signed certificate also stops the client authenticating the token endpoint — the leg that carries the refresh token — while the neighbouring sentence tells them that leg is protected. Both tlsConfig() setters, both allowInsecureTransport() setters and the README's OIDC section now say what the scope is and recommend a trust store over disabling validation whenever an identity provider is in play. No behaviour change; INSECURE_NO_VALIDATION still means what its name says.

Coverage:

  • noteAckProgress()'s guard was unpinned, and the whole BackgroundDrainer suite stayed green without it. The method ends an escalation episode when the wire durably acks something past the watermark. The delivering case had a test; the non-delivering one did not, and run()'s poll loop calls the method on every 50 ms tick — so weakening acked <= watermark to acked <, or dropping the guard, refilled all three escalation counters twenty times a second for as long as the drain stayed connected, and an orphan drainer swept forever with no ack progress: no .failed sentinel, no DATA_LOSS report, the slot lock held and one of max_background_drainers workers pinned for the life of the process. testConnectingWithoutDeliveringDoesNotGrantAFreshSettleBudget is the negative twin of the delivering test — the same two gap windows, neither reaching the threshold alone, with a session between them that connects, takes a frame and closes without acking — and expects the quarantine. The weakened guard turns it red.
  • QueryWorker.shutdown()'s interrupt handling had no test, while both its siblings did. testShutdownHandsBackACarriedInterrupt pins the hand-back and the thread teardown; removing the restore turns it red. It deliberately does not pin the clear: the only observable difference there is whether join() waited, and thread.interrupt() fires immediately before it, so the dispatch thread is already leaving and any "still alive on return" assertion would be a race rather than a check. Pinning it honestly needs a dispatch thread with a controllable exit latency, which is a production seam this does not justify.

Housekeeping:

  • Seven members moved back into alphabetical order within their group (isRetryable, the two MIN_* constants, requireSecureIdpEndpoint/requireSecureTransport, warnPersistence, and the readBounded..retainProcessLock run in FileTokenStore, where lifting the first two stranded two more). Pure moves.
  • Three comments described code that is not there. Two referenced invokeIsLoopbackHost, a reflection helper that went away when a build()-driven test replaced it, one of them stranded above an unrelated stack-inspection helper. CursorWebSocketSendLoop's cancel() claimed the credential-pull interrupt "fires ONLY while a pull is in flight, so a sender with no token provider is untouched"; the connect walk publishes that marker whenever it carries a cancellation, before it looks at the supplier, so a background sender with no provider configured is in the window too. It costs nothing — close() sets running = false before cancel(), so every path a late interrupt can reach is already winding down and the I/O thread's exit is native frees plus a latch countdown — but the comment now says that rather than denying the window exists.

Review follow-ups (seventh round)

A seventh review round found no Critical defect — both the correctness and the test gate passed — and six Moderate ones, all fixed here rather than deferred. As before, every production fix carries a regression test shown to fail without it, and the commit messages record the counterfactual output for each. The whole set is 6 production edits across 4 files; the rest is test and spec.

Confirmed defects:

  • A symlink switched the token store's trust sentinel off. The .untrusted sentinel carries the "another local user could have planted an entry here" verdict across the chmod that erases the evidence, and both halves of it read the name in a way that fails OPEN — against the one party who can write that directory and therefore choose what stands at the name. restrictToOwner tested !Files.exists(sentinel), which follows symlinks, so a dangling link planted at the name reported absent and the directory read as trusted on its permission bits alone: no race to win, nothing in any log, for as long as the link stood. markUntrusted could not repair it either, because its exclusive create answers FileAlreadyExistsException for a symlink exactly as it does for a peer's mark, so it read the squatter as "already marked". The verdict is now Files.notExists(sentinel, NOFOLLOW_LINKS) — positive evidence of absence, so a link, a directory or an indeterminate stat all leave the directory distrusted — and markUntrusted treats only a regular file as a mark, displacing anything else. Both rules are in the frozen cross-language contract now, so the Python client mirrors the fix rather than the bug.
  • An untrusted mark that could not be lifted killed persistence in silence. Retaining the sentinel after an incomplete sweep is deliberate and fail-closed — the next caller re-sweeps — but that reasoning assumes the failure goes away. When it does not, nothing lifts the mark: the sweep skips the sentinel by design, and markUntrusted only runs while the directory is still other-writable, which the chmod has ended. So every later load() returns null over an entry save() has just written, for the life of the directory, and the only line that appeared reported a different condition with a different fix. One entry this process cannot unlink is enough (another UID under a sticky-bit parent, a persistent EPERM/EIO/ESTALE), and a non-empty directory squatting the sentinel's name reaches the same state and survives the chmod. The distrust stays — it is the fail-closed direction — but both paths now warn once per JVM, naming the fault kind and the entry to remove. A headless getToken() consumer that re-prompts every restart is the case persistence exists to serve, and its operator had nothing to act on.
  • A rejected served token discarded the rotated refresh token that arrived with it. tryRefresh() has two branches over one clean 2xx, and only one applied the rule adoptRotatedRefreshToken() exists to state: a 2xx with no OAuth error means the provider ACCEPTED the token we presented, so a rotating provider has already burned it and the refresh_token in that body is the live one. The branch where the served kind is absent took it; the branch where it arrives and is unusable — rejected by validateTokenChars — returned first and dropped the rotation, leaving the spent token cached. This branch made that reachable: before it, JsonLexer did not decode escapes, so ACCESS\r2 arrived as nine printable characters and the rotation was recorded. signIn() hides the loss because its device-flow fallback overwrites the refresh token; getToken() is where it bites, because it never prompts — the spent token goes back on the wire on the next flush, and a reuse-detecting provider answers a replay by revoking the whole family, which with a TokenStore reaches every process sharing the identity. The catch now adopts the rotation before returning.
  • clear()'s "at ANY age" temp sweep was conditional on a clock. A crash between createTempFile and the atomic rename orphans a temp holding the full serialized entry — access, id and refresh tokens in plaintext — and clear() passes minAgeMillis 0 to reclaim it however fresh it is. That went through the same now - mtime >= minAgeMillis comparison save()'s staleness-bounded sweep uses, and the comparison is not age-neutral at zero: an mtime ahead of now yields a negative left-hand side, which is not >= 0. So the one sweep written to ignore the clock was the one a clock could veto, and save()'s sweep skips the same file against a larger threshold — nothing in the class would ever reclaim it. A future mtime needs no attacker: a network home whose server clock leads the client's (already documented as in scope), or any wall-clock step backwards between the crash and the clear. The sweep now short-circuits on minAgeMillis <= 0.

Documentation:

  • BackgroundDrainerPool's javadoc described a fast path this branch removed. It states that an interrupted close() skips the graceful window and shuts the executor down hard. That stopped being true when QwpWebSocketSender.close() was made interrupt-neutral — it has to be, because a carried flag made every wait beneath it throw on arrival, which is how a reap sweep came to report slots with their store-and-forward flock still held. drainerPool.close() runs inside that window, so only an interrupt delivered during the wait still takes the fast path, and the common caller (a task cancelled by ExecutorService.shutdownNow()) arrives with one rather than delivering one. A cancelled close therefore spends up to GRACEFUL_DRAIN_MILLIS + STOP_GRACE_MILLIS per sender whose orphan drainer is actively delivering. The behaviour is right and stays — the split stop already exempts drainers that are only retrying a connect, so what the window waits on is a drainer with rows on the wire — and the javadoc now says which of the two interrupts it means.
  • The response-head read was the third flush-path read this branch bounded and the only one it did not name. See the tradeoff above; the callsite now records what the abort can and cannot conclude, and a test holds the bound.

Tandem

OSS: questdb/questdb#7331
Ent: https://github.com/questdb/questdb-enterprise/pull/1090

@glasstiger glasstiger changed the title feat(core): OIDC device flow feat(core): OIDC sign-in via device flow (RFC 8628) Jun 17, 2026
glasstiger and others added 11 commits June 18, 2026 16:29
isUnsafeForDisplay() inspected one UTF-16 code unit at a time, so a
supplementary-plane (>= U+10000) format or control character - an
invisible U+E00xx "tag" char, for instance - arrived as a surrogate
pair whose halves are each neither a control nor category Cf and so
passed the filter unstripped. Because the JSON lexer reassembles such
😀-style escapes, a hostile or man-in-the-middled identity
provider could smuggle invisible/spoofing characters into a user_code,
a verification_uri, or an error_description and on into the terminal
prompt and exception messages.

Judge a Unicode code point instead: isUnsafeForDisplay() takes an int,
and both sanitizers (putSanitized for exception messages,
sanitizeForDisplay for the prompt) walk the text by code point with
Character.codePointAt/charCount, so Character.getType classifies a
supplementary char as one character. A legitimate astral character
(an emoji) is still preserved.

Make the assertNoUnsafeDisplayChars test helper code-point-aware too -
it shared the blind spot - and add a regression test that fails (the
U+E0001 tag char survives) without the fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pollOnce() checked for a token before the HTTP status and the OAuth
error field, so a response that carried a token alongside an error, or
under a non-2xx status, was cached as a valid grant. tryRefresh() had
the same flaw: it accepted the refreshed token on token presence alone.
Both contradict RFC 6749 - 5.1 makes a grant a 2xx response carrying a
token, and 5.2 says an error response must not be treated as a grant.

Handle the OAuth error first in pollOnce(), so a token smuggled
alongside an error never counts, and accept a token only when the
status is 2xx; a token under a non-2xx status goes to the transport-
error budget instead of being trusted. Guard tryRefresh() the same way:
cache the refreshed token only from a clean 2xx response with no error,
otherwise fall back to the interactive flow.

The happy path and the existing pending/slow_down/access_denied/empty-
body outcomes are unchanged. Add regression tests for a token alongside
an error, a token under a non-2xx status, and a refresh that smuggles a
token with an error - each fails without the fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
newRequest() passed the token from httpTokenProvider.getToken() straight
to authToken(), which does not null- or empty-check it. A provider that
returned null, "", or whitespace therefore produced a malformed
"Authorization: Bearer " header that the server only answered with a 401
far from the cause - no client-side error at all. The HttpTokenProvider
contract forbids such a return but nothing enforced it, and httpToken()
already rejects a blank token, so the provider path was the weaker spot.

Validate the pulled token with Chars.isBlank (as httpToken does) and
throw a clear LineSenderException instead. The check sits inside the
deferred pull, so a rejected token leaves the stamp pending and the next
row retries cleanly, just like a throwing provider does. OidcDeviceAuth
never returns a blank token, so this guards custom providers.

Add tests that a null, an empty, and a whitespace-only provider token is
rejected at first use - each fails without the fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JsonLexer.getCharSequence rescanned every decoded value and name from
the start to look for a backslash, even though the parse loop already
detects one when it sets ignoreNext. Record that in a sawEscape flag
(carried across parse() fragments) and resolve escapes only when it is
set, so the common no-escape value returns the assembled sink without a
second pass.

OidcDeviceAuth.Endpoint.parse now rejects a host that contains control
characters or whitespace - a smuggled CR/LF would otherwise flow into
the outbound Host header.

Add the tests these paths lacked: a cross-fragment escape; the lexer's
lenient and exotic escape arms (surrogate pairs, \b/\f, unknown and
malformed escapes, lone surrogates); the version-probe settings parser
reading an escaped key through unescape; HTTP-token-provider rejection
for UDP and WebSocket (not just TCP); and the control-character host
cases above.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Port the issuer feature from py-questdb-client (PR #133) onto
OidcDeviceAuth, so the device flow keeps working against servers that
do not advertise their device-authorization endpoint, and so the
device code and refresh token are only sent where the caller pins.

The issuer plays three roles:

- Discovery fallback: when /settings omits the device (and/or token)
  endpoint, fromQuestDB(url, issuer) reads it from the issuer's
  .well-known/openid-configuration document. The discovery origin comes
  only from the out-of-band issuer (or an explicit discoveryUrl), never
  from a /settings-supplied value, so a tampered /settings cannot
  redirect discovery. Without a pin, discovery is refused.

- Plaintext-channel pin: a /settings response fetched over plaintext
  http to a non-loopback host (only reachable with
  allowInsecureTransport) cannot route credentials to its advertised
  endpoints without a pin.

- Endpoint-origin pin: validateEndpointOrigins, enforced in
  Builder.build() on every construction path, requires the token and
  device endpoints to share one origin (RFC 8628 co-location) and, when
  an issuer is set, to belong to it.

Config surface: Builder.issuer(...); new fromQuestDB overloads
(url, issuer), (url, issuer, allowInsecure), and a 5-arg master taking
issuer, discoveryUrl and a TLS config.

Tradeoffs:

- The co-location check makes the token and device endpoints share an
  origin. testPersistentTransportFailureDuringPollingAborts simulated
  an unreachable token endpoint with a dead second port; it now uses a
  new MockOidcServer.dropConnection() against a co-located path.

- The origin pin compares scheme/host/port and ignores the path, so an
  identity provider that hosts its endpoints on a different origin than
  its issuer must be configured without an issuer. This matches the
  Python client.

- allowInsecureTransport still relaxes the identity provider endpoints
  too (unchanged); the Python client always forces https/loopback for
  the IdP. Left as-is to avoid changing settled transport behavior.

Adds 7 tests and updates the README OIDC section.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Endpoint.parse now rejects control characters and whitespace anywhere
in the url before splitting it. The host was already checked, but the
path was not, so a tampered /settings or discovery document could carry
a CR/LF in an endpoint path that the JSON lexer decodes and postForm
writes verbatim onto the request line via .url(endpoint.path) - a
header-injection / request-smuggling vector that the origin pin (which
compares scheme/host/port only) does not catch. Validating the whole
url up front also keeps it safe to echo in the parse error messages.

fromQuestDB now derives the pin origin from a caller-supplied
discoveryUrl when no issuer was resolved. Previously a discoveryUrl pin
only took effect when discovery actually ran (an endpoint missing from
/settings); when /settings advertised both endpoints the discovery
branch was skipped and validateEndpointOrigins ran with a null issuer,
so a compromised server could advertise both endpoints at an attacker
origin and slip past the pin. The discoveryUrl pin now behaves like the
issuer pin on every construction path.

Adds regression tests for both: a CR/LF-injected advertised endpoint,
path and query cases in Endpoint.parse, and discoveryUrl-pin accept and
reject against on- and off-origin endpoints.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Endpoint.parse already rejected control characters and whitespace in
the url, which kept it safe to echo into the exception messages once it
passed validation. That scan did not catch bidi, zero-width or other
format characters (U+202E, U+200B, U+FEFF, the Cf category, and the
supplementary-plane tag characters), so a tampered /settings or
discovery endpoint url could still smuggle one into an OidcAuthException
message and reorder, hide or forge the log line it lands in.

The url scan now runs per code point and also rejects anything
isUnsafeForDisplay flags, so an OIDC url may carry no control,
whitespace or display-unsafe character. Every raw url echo in
Endpoint.parse, requireSecureTransport and fromQuestDB is therefore
safe on screen as well as on the wire, and the rejection message
sanitizes the url it reports.

Adds a regression test covering a right-to-left override, a zero-width
space, the BOM and a supplementary-plane tag character.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
isUnsafeForDisplay now treats an unpaired UTF-16 surrogate as unsafe,
so a lone surrogate half - which JsonLexer emits verbatim for a single
backslash-u-XXXX escape and which codePointAt surfaces as a SURROGATE
code point - is stripped from a user_code, verification_uri or error
string before it reaches a terminal or a log line. A valid high+low
pair is still reassembled by codePointAt and judged on its real
category, so a legitimate emoji survives. The method comment is
corrected too: codePointAt in the callers reassembles pairs, not the
lexer.

close() and the class Javadoc no longer claim an in-flight sign-in is
cancelled "promptly". The cancel flag is observed between polls (within
about 100ms) but a poll request already in flight is not interrupted,
so close() can take up to one HTTP request timeout to return - still
far short of the device-code lifetime. The docs now say so.

Adds tests: lone high and low surrogates are stripped from the device
challenge while an emoji survives; and the private isLoopbackHost
classifier (which gates the plaintext-channel MITM pin) is pinned for
localhost and the 127.0.0.0/8 block, and against non-loopback and
spoofing hosts such as 127.evil.com, localhost.evil.com, 127.1 and
127.0.0.256.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The poll loop now clamps the slow_down-inflated interval to the same
MAX_POLL_INTERVAL_SECONDS cap the initial interval already respects, so
repeated slow_down responses from the identity provider cannot grow the
wait without bound.

The device-authorization, token and well-known parsers now reset their
current field to FIELD_NONE after each value, matching
SettingsDiscoveryParser. The parsers are not currently confusable - in
well-formed JSON a name event always sets the field before the next
value, array elements arrive as EVT_ARRAY_VALUE, and nested values are
filtered by the depth check - so this is a defensive consistency fix
that removes a latent field-confusion foot-gun rather than a behavior
change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JsonLexer.unescape no longer re-scans the value from the start to
re-find the backslash the lexer already flagged via hasEscape; it walks
the value once, copying plain characters and resolving escapes in place.
That drops the now-dead "no escapes" early return and the separate
prefix copy, so an escaped value is traversed about twice (decode then
unescape) instead of three times. parseHex4 looks the hex digit up in
the shared Numbers.hexNumbers table instead of Character.digit, keeping
the same -1-on-non-hex contract. All of this is on the cold
error/discovery/auth parse path, never on ingestion.

Reorders pollForToken ahead of pollOnce so the private methods stay in
alphabetical order; no behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@glasstiger glasstiger added enhancement New feature or request security labels Jun 19, 2026
glasstiger and others added 16 commits June 19, 2026 15:42
The 4 MiB response-body cap (MAX_RESPONSE_BODY_BYTES) that bounds the
OIDC device flow against a hostile or MITM'd server streaming an
endless body had no test coverage on the parseBody path.

Add an oversizedJson() mode to MockOidcServer that streams a chunked,
mostly-whitespace body past the cap, and a test that drives discovery
against it and asserts the bounded read aborts with the size-limit
error - which also confirms the token-bearing body never reaches the
message. The body is whitespace so the lexer keeps consuming until the
byte cap trips, instead of hitting its per-value length limit first.

Verified both ways: the test passes with the 4 MiB cap and fails when
the cap is disabled, where the full body is read and parsing fails with
"Unterminated object" instead.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three small fixes to the OIDC device authorization flow, all in
OidcDeviceAuth:

- runDeviceFlow now rejects a non-2xx device authorization response.
  Previously it trusted any body that carried device_code/user_code/
  verification_uri and no OAuth error, so a non-2xx response would
  prompt the user and start polling. It now applies the same 2xx gate
  pollOnce and tryRefresh already use before trusting a body.

- pollForToken checks the device-code deadline at the top of the loop
  and never sleeps past it, so an expiry that elapses during a sleep
  times out promptly instead of after one more wasted poll and up to a
  full extra poll interval.

- tryRefresh drops an unreachable branch that rethrew on an OAuth
  error. postForm only throws on a parse failure here, and a real
  OAuth error arrives in tokenParser.error (handled by the
  hasRequiredToken check), so the branch was dead. No behaviour change.

Add testNonSuccessDeviceAuthorizationResponseRejected covering the new
2xx gate; it fails without the check (the 403 is accepted, the user is
prompted, and polling fails later instead).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A discoveryUrl pins the identity provider, yet fromQuestDB adopted the
issuer the discovery document declared about itself and validated the
token and device endpoints against that, never against the pinned
discoveryUrl origin. A document served at the pinned url could therefore
name an attacker issuer, co-locate both endpoints under it, and route the
device code and the long-lived refresh token there while the co-location
and issuer checks passed trivially - so the discoveryUrl pin did not in
fact pin the provider, contradicting its documented guarantee.

Reject a document whose own issuer sits on a different origin than the
pinned discoveryUrl (RFC 8414 section 3.3), and derive the endpoint pin
from the discoveryUrl origin rather than the document's self-declared
issuer. An identity provider that serves its discovery document on a
different origin than its endpoints must instead be configured with
explicit endpoints via OidcDeviceAuth.builder().

The issuer-pinned path is unchanged: it already binds the endpoints to
the caller-supplied issuer. testFromQuestDbDiscoveryUrlPinRejectsForeign
IssuerInDocument covers the new rejection and fails without the fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
readResponse copied the response status code into a sink that later
appears in OidcAuthException messages. A well-formed status code is bare
digits, but the HTTP header parser keeps the status-line token verbatim
apart from SP/CR/LF, so a hostile or MITM'd identity provider could
splice ESC or other control bytes into it - smuggling ANSI sequences
into a log or terminal, or fabricating a leading digit that passes the
2xx success gate.

Validate the status code as it is captured: on any non-digit byte, drain
the body so the keep-alive connection stays usable, then reject the
response with a message that echoes none of its bytes. A clean status is
copied digit by digit, so every later [httpStatus=...] echo is bare digits.

testNonNumericStatusCodeRejected drives a status code with a spliced ANSI
reset and asserts the rejection; it fails without the fix. The new
MockOidcServer.raw() helper writes a verbatim response so a test can craft
a malformed status line.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JsonLexer now resolves JSON string escapes, so the message and errorId
fields a QuestDB endpoint returns in a JSON error body arrive at the
sender fully decoded. The JSON error parser put them into the
LineSenderException verbatim, so a hostile or proxied endpoint could
inject real control characters or ANSI escapes that forge a log line or
rewrite a terminal when the exception text is printed.

Render the server-supplied message, id, code and line through
putAsPrintable - the same escaping the column-name errors in this class
already use - so a decoded control byte arrives escaped.

LineHttpSenderErrorResponseTest flushes against a server returning a
chunked JSON error whose message and errorId carry an ESC and a newline,
and asserts they reach the exception escaped; it fails without the fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The plaintext-channel pin refuses /settings-supplied OIDC endpoints
fetched over a non-loopback http channel unless the identity provider is
pinned out of band, so a tampered response cannot route the device code
and refresh token to an attacker. Only its loopback exemption was
exercised end to end, because the test mock binds to 127.0.0.1; the
firing branch had no integration coverage.

Reach the loopback mock through "127.1": the OS resolver expands the
short form to 127.0.0.1 so the mock answers, but the loopback classifier
deliberately rejects the short form, so the server host is non-loopback
and the pin fires. Assert that a plaintext /settings advertising both
endpoints without a pin is refused, and that pinning the issuer over the
same channel is accepted - proving the pin, not an unrelated rejection,
is the gate. The test fails if the firing check is removed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Skip testPlaintextSettingsWithAdvertisedEndpointsRequiresPin on
Windows: it reaches the loopback mock through the "127.1" short-form
address, which Linux/macOS getaddrinfo expands to 127.0.0.1 but Windows
getaddrinfo rejects, so discovery cannot connect there. No host string
is both reachable at the loopback mock and classified non-loopback on
Windows, so the end-to-end firing path cannot run there; the classifier
stays covered cross-platform by
testLoopbackHostClassifierRejectsNonLoopbackAndSpoofing.

Wrap every OidcDeviceAuth construction in try-with-resources so the
native JSON lexer and HTTP clients are always released, including the
rejection paths where build()/fromQuestDB() throws.

Also replace manual StringBuilder fills with String.repeat, switch
index loops to enhanced-for, and collapse the split-value test helper
to a single lexer cache-limit parameter.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A token whose JSON value carries an escaped CR/LF now decodes to real
control bytes (the lexer resolves string escapes), and getToken() serves
it verbatim as an "Authorization: Bearer <token>" header value and as
the PG-wire _sso password. A control character would break out of the
header and inject into the request line sent to the trusted QuestDB
server; a non-ASCII character is silently truncated by the ASCII header
writer.

storeTokens now validates the access and id tokens and rejects any
character outside printable ASCII (0x20-0x7E) before caching them, so a
tampered or corrupt credential from a hostile or man-in-the-middled
identity provider never reaches the wire. The refresh token is left
unchecked: it is only ever sent URL-encoded. The token bytes are never
embedded in the error message.

Add testTokenWithControlCharsRejected, which fails without the guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AbstractChunkedResponse.recv re-armed the full timeout on every internal
read while scanning an incomplete chunk-size line, so a server that
dribbles that line one byte per timeout window - or fills the buffer with
a CRLF-less chunk size - kept a single recv() running without bound. That
defeats a caller's wall-clock deadline, e.g. OidcDeviceAuth.parseBody,
whose comment claims a dribbling server cannot wedge the thread.

recv(int) now bounds the whole call to the given timeout when it is
positive: it tracks elapsed time, shrinks the per-read budget, and throws
once the budget is exhausted. The first read still gets the full budget;
a non-positive timeout keeps the legacy unbounded behaviour, so the
existing test harness is unaffected. The Response.recv javadoc is updated
to match.

Add testRecvHonoursTotalTimeoutWhileChunkSizeDribbles, which hangs and
trips its JUnit timeout without the bound.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
putAsPrintable rendered untrusted text - an ILP server's JSON error body,
a column name - into a LineSenderException message escaping only C0
controls and DEL. Bidi overrides, zero-width joiners and the BOM passed
through raw, so a hostile or proxied endpoint (whose JSON escapes the
lexer now decodes to real code points) could reorder or hide the text a
human reads in a terminal or a log line. It also truncated any escaped
char above U+00FF to its low byte.

putAsPrintable now escapes control characters and Unicode format
characters, matching the OIDC display sanitizer's threat model, and emits
the full four hex digits. Escaping rather than stripping keeps the
original visible for diagnosis. For characters up to U+00FF the output is
unchanged. This is the client's own Utf16Sink copy.

Also close OIDC test-coverage gaps:
- reject a malformed status code on the token-poll path, not only the
  device-authorization path
- getTokenSilently fails fast while another thread holds the lock in a
  silent refresh, not only an interactive sign-in
- a backslash-u escape split across parse() fragments still decodes
- tighten the stalled-body timeout assertion to prove the configured 1s
  limit fired

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Condense the verbose comments and javadoc the device-flow PR added,
across the new auth classes (OidcDeviceAuth, OidcAuthException,
DeviceAuthorizationChallenge, DeviceCodePrompt, HttpTokenProvider) and
the comments added to JsonLexer, Response, Utf16Sink,
AbstractChunkedResponse, AbstractLineHttpSender and Sender. Drop filler,
use active voice, and collapse wrapped lines while preserving every
technical fact - the security rationale, RFC references, invariants, and
ordering/locking notes. Comments only; no code changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DeviceCodePrompt.openBrowser() renders the device-code challenge and
then opens the verification URL in the local default browser,
best-effort: a new package-private BrowserLauncher allowlists http(s)
schemes (rejecting javascript:/data:/file: from a hostile or MITM'd
identity-provider response) and skips silently on a headless JVM or a
runtime without the java.desktop module, so sign-in never breaks and
the URL and code are always printed.

Collapse OidcDeviceAuth.fromQuestDB's seven overloads into two:
fromQuestDB(url) and fromQuestDB(url, DiscoveryOptions).
DiscoveryOptions carries the issuer, discovery URL, TLS config, the
insecure-transport opt-in, and the device-code prompt. Threading the
prompt through the discovery path is the point: a custom prompt (such
as openBrowser) previously worked only via the explicit builder(),
which forgoes /settings discovery.

Migrate the OidcDeviceAuth test call sites to the options form and add
BrowserLauncherTest, which reaches the package-private allowlist by
reflection (the client is an open module). Update the example and the
README, including two now-removed overload references.

Tests: OidcDeviceAuthTest (90) and BrowserLauncherTest (3) pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The default device-code prompt is now openBrowser() in both the builder
and DiscoveryOptions, so an interactive sign-in prints the verification
URL and code and also opens the URL in the local default browser when
one is available. SYSTEM_OUT becomes the explicit print-only opt-out.

A new questdb.client.oidc.open.browser system property (default true)
gates the launch in BrowserLauncher, so a server, automation or CI host
can suppress it process-wide. OidcDeviceAuthTest sets it false so no
device-flow test launches a real browser, under maven or an IDE - the
default prompt would otherwise pop a tab for every flow that reaches
the prompt.

Update the javadocs, README and example accordingly.

Tests: OidcDeviceAuthTest (90) and BrowserLauncherTest (4) pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SettingsDiscoveryParser now reads acl.oidc.audience from the trusted
config object, and fromQuestDB threads it into the builder, so the
audience is discovered from the server rather than only set through
builder().

tryRefresh() now appends the audience form parameter - the device
authorization request already sent it - so both the device grant and
the refresh request carry it, matching the Python client. The
device-code poll does not, also matching Python.

Tests: testDiscoveryReadsAudience and testAudienceSentOnRefresh.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The identity provider endpoints (device authorization, token, and the
.well-known discovery URL) now require https unless they are loopback,
regardless of allowInsecureTransport. The flag relaxes only the QuestDB
/settings link; it no longer downgrades the identity provider, so the
device code and refresh token are never sent in cleartext. Loopback
http endpoints are accepted without the flag, for local development.

When the pinned issuer carries a path, an endpoint from /settings must
now be under that path, not just on the issuer's origin. A path-based
multi-tenant provider (Keycloak /realms/<realm>) shares one origin per
tenant, so the origin check alone could not stop a tampered /settings
from steering credentials to a different realm. The check decodes
repeatedly (%252e -> ..), folds backslashes, scans matrix params, and
rejects any . or .. segment. Endpoints from IdP discovery or configured
explicitly are not scoped, since some providers place endpoints outside
the issuer path.

Both changes match the behaviour of the Python client (py #133).

Tests: testIdpEndpointsRequireHttpsExceptLoopback and three
testIssuerPathScoping* tests; OidcDeviceAuthTest (95) and
BrowserLauncherTest (4) pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The device-code lifetime clamp now matches the Python client. A missing
or zero expires_in in the device-authorization response defaults to
600s (was 300s), and an absurd value is capped at 1800s (was 3600s) via
a new MAX_DEVICE_CODE_TTL_SECONDS, so a hostile or buggy provider cannot
make the client poll for an absurd duration.

The token-cache clamp is unchanged (300s default, 3600s cap); it
previously shared the cap constant with the device-code clamp, now split
so the two are independent.

Test: testDeviceCodeLifetimeClamped.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
glasstiger and others added 19 commits August 25, 2026 17:10
Re-read the durable acknowledgement after a recoverable terminal is observed. This prevents a wire recycle from carrying an exhausted capability-gap or rotating-auth budget across progress published by the I/O thread.
Use interrupt-neutral timed joins in PoolHousekeeper.stop so a fresh cancellation cannot skip the worker interrupt. Extend the token-provider recovery coverage with a closer interrupted while it is inside the join.
Both sit in FileTokenStore, both were found in review, and neither needs
an attacker to reach.

Reclaim a lock name this store cannot have written. stealIfStale
returned the moment readLockHolder threw, so a directory, a symlink or a
mode-000 file occupying .store.lock was never reclaimed: CREATE_NEW
reports EEXIST for all three exactly as it does for a peer's live lock,
so acquireLock spent its whole budget and threw, and threw again on
every later call. Nothing else reclaims that name - the untrusted sweep
skips it for want of a hash prefix, and sweepTempFiles globs *.tmp - so
load() and save() failed for as long as the shape stood. Persistence
then died silently: OidcDeviceAuth degraded to "continuing without
persistence" and every process start re-ran the interactive device flow,
which is a hard failure for the headless getToken() consumer
persistence exists for. It also contradicted this file's own claim,
where discardUntrustedDirectoryContents deliberately leaves .lock names
in place, that acquireLock already treats a hostile or stale one as
stealable.

The shapes split into two cases. A directory or a symlink is a squatter
that no wait turns into a lock, since createLockFile only ever produces
a regular file, so displaceLockSquatter removes it on sight - the way
markUntrusted already displaces a squatted .untrusted name. It captures
atomically before deciding, so a peer that creates a real lock in the
gap gets it restored rather than deleted. An unreadable regular file is
genuinely ambiguous: a run under a different uid killed while holding
the lock leaves one, and it may equally be a live holder's. stealIfStale
therefore carries "stamp unreadable" as a third state beside "stamp" and
"empty", ages it on the full staleness window rather than the short
empty-lock grace, and folds readability into the capture-verify so a
name that could not be read before the capture but yields a stamp after
it is restored, not stolen.

Two supporting changes fall out. The mtime reads move to NOFOLLOW_LINKS,
because the link-following default threw on a dangling symlink - a
second route into the same wedge - and because it keeps the before and
after mtimes describing one object across the rename. The after-capture
stamp and mtime reads split into separate try blocks: folded together,
an unreadable stamp left afterModified null and would have made every
such capture unconfirmable.

Honour the untrusted sentinel on a non-POSIX filesystem. restrictToOwner
returned a bare true from its UnsupportedOperationException branch, so a
directory a POSIX peer had marked untrusted and not finished sweeping -
its sweep latches whenever one entry resists deletion - read as trusted
on Windows. This client then adopted entries out of it and presented
their tokens, and neither swept nor cleared the mark. The sentinel is
deliberately permission-independent, which is exactly what lets it carry
a verdict across a filesystem whose mode bits this client cannot read;
design/oidc-token-persistence.md requires honouring it whatever the
permission bits say, and canUseShortDirectoryLockLease already evaluates
it on the same catch.

FileTokenStoreTest gains four tests. Three fail against the previous
code with the reported OidcAuthException and pass now; the fourth pins
the safety property the ageing buys - a fresh unreadable lock may be a
live holder's and must survive. The non-POSIX branch stays untested:
reaching it needs a Windows agent or a synthetic FileSystemProvider, as
the class javadoc already records.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P7VQ956HEEVH9fci5SF4QL
@glasstiger

Copy link
Copy Markdown
Contributor Author

Tandem review — client #52 / OSS #7331 / ENT #1090

Reviewed as one logical change at client 3e534eb8, OSS bc26767e, ENT eda397ea.

Submodule provenance. questdb and java-questdb-client pointers are both OFF-DEFAULT (branch-only), so all three diffs are this change's work — no upstream content is being attributed here.

Verdict: approve on all three. No Critical findings. Three Moderate items and a Minor bundle remain open below; none blocks. The test gate passes (one admitted coverage gap, Moderate).

Compile compatibility is settled: public signature sets diffed base-vs-head show zero removals, both new interface methods are default, and mvn -pl core test-compile exits 0. Full client suite is green at 3452 tests, 0 failures, 0 errors, 5 skipped.


Resolved since the first pass

Two findings from the initial review are fixed in 3e534eb8 (disclosure: I wrote that commit, so it warrants a second pair of eyes rather than my own sign-off):

  1. An unreadable .store.lock permanently wedged the token store. stealIfStale returned as soon as readLockHolder threw, so a directory, a symlink or a mode-000 file at the lock name was never reclaimed — CREATE_NEW reports EEXIST for all three exactly as for a peer's live lock, and nothing else sweeps that name. load()/save() then failed permanently, OidcDeviceAuth degraded to "continuing without persistence", and every process start re-ran the interactive device flow — a hard failure for the headless getToken() consumer persistence exists for. Reachable without an attacker: a run under a different uid killed while holding the lock leaves a 0600 file this uid cannot read.

    The fix splits the shapes. A directory or symlink is a squatter no wait turns into a lock, so it is displaced on sight (mirroring markUntrusted's handling of a squatted .untrusted name), capturing atomically first so a peer's real lock created in the gap is restored rather than deleted. An unreadable regular file may be a live holder's, so it is not displaced: stealIfStale carries "stamp unreadable" as a third state, ages it on the full staleness window, and folds readability into the capture-verify.

  2. The non-POSIX trust check ignored the .untrusted sentinel. restrictToOwner()'s UnsupportedOperationException branch returned a bare true, so a directory a POSIX peer had marked untrusted and not finished sweeping read as trusted on Windows — entries adopted, mark never cleared. It now evaluates the sentinel, matching its sibling canUseShortDirectoryLockLease() and design/oidc-token-persistence.md:325 ("whatever the permission bits say").

Four regression tests were added. The three that pin the wedge fail against the previous code with the reported OidcAuthException and pass now; the fourth pins the safety property the ageing buys — a fresh unreadable lock may be a live holder's and must survive. The non-POSIX branch remains untested (needs a Windows agent or a synthetic FileSystemProvider), as the class javadoc already records.


Open — Moderate

M1. clearCache() is not published as the token-store waiter

Problem: clearCache() enters TokenStore.inLock without publishing itself, so close() cannot interrupt it.
Net impact: close() blocks for a peer's full refresh budget, against its own documented bound.
Evidence: Static. Sole publishTokenStoreWaiter() call site is OidcDeviceAuth.java:2271; clearCache() calls the store at :488; the contract is claimed at :504-505.

close() states: "Only while an operation is waiting for a peer instance's shared token-store lock is its thread interrupted, abandoning that wait immediately." clearCache() is such an operation and isn't covered — close() reads tokenStoreWaiterThread as null (:529) and then blocks on the plain lock.lock() at :534. FileTokenStore.inLock's wait is lockInterruptibly() with no budget.

Provenance: f99e9c1b removed publishLockHolder() from five sites including clearCache(), re-added it only in tryRefreshCoordinated(), and strengthened this javadoc in the same diff. OidcDeviceAuthPersistenceTest:399-401 pins the getToken() shape; there is no clearCache() counterpart.

Suggested fix: a tightly scoped publish/clear pair around :488. Note FileTokenStore.clear captures wasInterrupted == false at :369, so a naive pair leaves a stray flag on the caller.

M2. drainOnClose()'s re-asserted interrupt defeats close()'s own interrupt-neutrality

Problem: A consumed interrupt is re-asserted, so the next wait fails at 0 ms.
Net impact: QuestDB.close() reports a stopped I/O thread as not stopped and skips the drainer graceful window.
Evidence: Reproduced 6/6 at head (LineSenderException: cursor I/O thread did not stop…, isSlotLockReleased()==false on return). CountDownLatch.await(t,u) throws at 0 ms with a preset flag even when the latch is already at zero — measured.

QwpWebSocketSender.java:3959-3963:1435/:1452. close() clears a carried flag once at :1295, but drainOnClose()'s finally re-asserts on every exit, and close0() then runs cursorSendLoop.close() and drainerPool.close() on the same stack with no intervening clear. The flag's provenance is lost: PoolHousekeeper.stop():115 interrupts to break a credential pull, and BackgroundDrainerPool.close() then reads it as caller intent and skips both the 2.5 s graceful and 0.5 s stop windows — documented there as deliberate for an application interrupt. PoolHousekeeper.stop()'s own comment states the invariant broken here: "The flag must not outlive the interrupt's target."

Not Critical: for the identical trigger, base produced the same failed-stop throw plus a full-core busy-spin, so head is better there; only the producer is new. Consequences are bounded — SenderPool.reapIdle swallows the throw, cleanup completes ~50 ms later via delegateClose, and a retired slot has three recovery paths. No data loss.

Suggested fix: clear once more at the top of close0()'s teardown and let close()'s outermost finally restore. A second route exists — an interrupt landing after the final iteration's Thread.interrupted() check is never consumed — so removing the re-assert alone does not close it.

M3. Coverage: nothing pins the new 401 terminal as orphan-only

Problem: No test asserts the rotating-credential 401 terminal is unreachable from the live drainer.
Net impact: A future change moving it onto the foreground path would drop a live producer silently.
Evidence: connectWithDurableAckRetry callers are only BackgroundDrainer.java:1075 and :1246; ReconnectPolicy.ORPHAN is constructed only at :1143.

Every other aspect of the ride-out is pinned well — attempt threshold without dwell, constant-credential immediate quarantine, the clamped dwell at the call site, one test per transient arm proving it rewinds the dwell but not the attempt count, both noteAckProgress resets separated, credential tagging across all four shapes. The orphan-only scoping is the one property enforced purely by construction, and a regression there is the SF data-loss class the carve-out exists to prevent.

Suggested test (~60 lines, seams already exist): in WebSocketTokenProviderTest, arm TestWebSocketServer.setRejectWithStatus(401, …) (:299) after the first ACK on a live WS Sender with httpTokenProvider, sleep past reconnectMaxDurationMillis, then assert the sender still accepts rows, no .failed sentinel appears under sf_dir, and no DATA_LOSS reaches the handler.


Open — Minor

  • FileTokenStoreTest.java:2551store.inLock(key, …)'s boolean is discarded and the sole assertion lives inside the lambda; inLock has three early returns that never enter it. The file's own convention captures the boolean at 14 sibling sites.
  • OidcDeviceAuthPersistenceTest.java:381 — asserts Thread.State.WAITING as a proxy for "queued on the shared process lock"; any untimed park satisfies it. The stronger form already ships here (FileTokenStoreTest:3146 awaitWaitingInside, OidcDeviceAuthTest:4298 awaitInside).
  • BackgroundDrainerMidDrainAuthRejectTest.java:396 and :494 — byte-identical durableAckFrame/okFrame pairs in one compilation unit; hoist to file scope.
  • WebSocketTokenProviderTest.java:706connectionsAccepted written, never read (copied from ReconnectTest, where it is read), along with the firstClient branch feeding it.
  • BackgroundDrainerMidDrainAuthRejectTest.java:176 — comment says the session "sleeps 800ms before it acks"; 01fe9c99 moved the sleep after the ack, and the helper's javadoc at :429 says the opposite. No efficacy impact.

Coverage

Verified COVERED with real failure links: validateToken CR/LF and both boundary pairs; the snapshot-before-validate contract at all three production pull sites via the deterministic HandOffCharSequence; POSIX 0600/0700 with a negative control preventing vacuous passes; cross-process lock contention, stale-lock takeover and crash-during-save (unit plus the two-sided ENT kill-9 e2e); the interrupt escalation across three modes including caller-already-interrupted and interrupt-during-join; requires static java.desktop degrading on a genuinely desktop-free module path; the JsonLexer value cap driven through the production instance; and token secrecy, including reflective sweeps with positive preconditions so an empty buffer cannot pass. Windows/non-POSIX arms are an accepted gap, disclosed in the class javadoc and the design doc's open questions.

Notes

Three behaviours existing non-OIDC ILP-over-HTTP users can observe — JSON escape decoding reaching the error-response parser, whole-call read bounds, and a FileTokenStore lock-hold multiple that turns a previously-accepted config into a build() failure — are all disclosed in the PR body with their pinning tests named, which is why none is filed above.

Java 8 compatibility verified by compilation: 421 classes at -source 8 -target 8, zero errors; --release 8 flags only pre-existing sun.misc usage.

Reviewed with Claude Code.

@mtopolnik

Copy link
Copy Markdown
Contributor

[PR Coverage check]

😍 pass : 2475 / 2758 (89.74%)

file detail

path covered line new line coverage
🔵 io/questdb/client/cutlass/line/http/LineHttpSenderV1.java 0 4 00.00%
🔵 io/questdb/client/cutlass/http/client/HttpClientWindows.java 0 7 00.00%
🔵 io/questdb/client/cutlass/http/client/HttpClientOsx.java 0 7 00.00%
🔵 io/questdb/client/cutlass/line/http/LineHttpSenderV2.java 0 4 00.00%
🔵 io/questdb/client/cutlass/auth/BrowserLauncher.java 15 24 62.50%
🔵 io/questdb/client/cutlass/qwp/protocol/QwpTableBuffer.java 2 3 66.67%
🔵 io/questdb/client/impl/QueryWorker.java 3 4 75.00%
🔵 io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java 108 131 82.44%
🔵 io/questdb/client/cutlass/qwp/client/QwpCredentialUnavailableException.java 5 6 83.33%
🔵 io/questdb/client/cutlass/auth/FileTokenStore.java 577 679 84.98%
🔵 io/questdb/client/cutlass/http/client/HttpClient.java 31 36 86.11%
🔵 io/questdb/client/cutlass/line/http/AbstractLineHttpSender.java 91 102 89.22%
🔵 io/questdb/client/cutlass/auth/DeviceCodePrompt.java 22 24 91.67%
🔵 io/questdb/client/std/str/Utf16Sink.java 22 24 91.67%
🔵 io/questdb/client/cutlass/auth/OidcDeviceAuth.java 1093 1187 92.08%
🔵 io/questdb/client/cutlass/auth/TokenStoreKey.java 42 45 93.33%
🔵 io/questdb/client/impl/SenderPool.java 18 19 94.74%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java 91 95 95.79%
🔵 io/questdb/client/Sender.java 31 32 96.88%
🔵 io/questdb/client/cutlass/qwp/client/QwpQueryClient.java 34 35 97.14%
🔵 io/questdb/client/impl/PoolHousekeeper.java 22 22 100.00%
🔵 io/questdb/client/cutlass/auth/TokenStore.java 1 1 100.00%
🔵 io/questdb/client/QuestDB.java 4 4 100.00%
🔵 io/questdb/client/std/str/DirectUtf8Sink.java 9 9 100.00%
🔵 io/questdb/client/std/str/StringSink.java 4 4 100.00%
🔵 io/questdb/client/cutlass/qwp/client/QwpUdpSender.java 1 1 100.00%
🔵 io/questdb/client/cutlass/http/client/HttpClientLinux.java 7 7 100.00%
🔵 io/questdb/client/QuestDBBuilder.java 7 7 100.00%
🔵 io/questdb/client/cutlass/line/LineSenderException.java 5 5 100.00%
🔵 io/questdb/client/cutlass/http/client/Response.java 1 1 100.00%
🔵 io/questdb/client/impl/QueryClientPool.java 5 5 100.00%
🔵 io/questdb/client/cutlass/auth/DeviceAuthorizationChallenge.java 12 12 100.00%
🔵 io/questdb/client/cutlass/http/client/AbstractChunkedResponse.java 17 17 100.00%
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java 32 32 100.00%
🔵 io/questdb/client/cutlass/json/JsonLexer.java 72 72 100.00%
🔵 io/questdb/client/cutlass/auth/PersistedToken.java 12 12 100.00%
🔵 io/questdb/client/impl/QuestDBImpl.java 2 2 100.00%
🔵 io/questdb/client/cutlass/auth/OidcAuthException.java 42 42 100.00%
🔵 io/questdb/client/cutlass/http/client/AbstractResponse.java 8 8 100.00%
🔵 io/questdb/client/std/str/DisplaySafe.java 20 20 100.00%
🔵 io/questdb/client/HttpTokenProvider.java 7 7 100.00%

@glasstiger

Copy link
Copy Markdown
Contributor Author

Level 3 tandem review

Reviewed in tandem with questdb/questdb#7331 and questdb/questdb-enterprise#1090.

Moderate

Problem: Final post-ACK interrupt capture lacks failure-linked coverage.
Net impact: Late-interrupt teardown regressions can escape CI.
Evidence: Deleting lines 3963-3966 leaves all 18 close-drain tests green at 3423784.

The final interrupt capture in QwpWebSocketSender.java handles an interrupt arriving after the loop observes the final ACK. The new successful-drain test injects through closeDrainWaitingHook, before the pre-loop capture, so it does not exercise this last-iteration window.

A scratch mutation removing only the final capture passed the complete CloseDrainTest class (18/18) and the adjacent carried-interrupt test. The supplied base did not contain this guard or its tests. Add a deterministic post-final-ACK hook/barrier and assert both interrupt restoration and completed teardown.

Coverage map

Test gate passes with 1 admitted Moderate coverage gap for this PR. The focused OIDC/QWP suite passed 177 tests, and the full JDK 8/25, Linux, macOS, Windows, coverage, and leak-check matrices are green.

Summary

  • Client PR feat(core): add OIDC sign-in via device flow (RFC 8628) #52: approve
  • OSS PR #7331: approve
  • Enterprise PR #1090: approve
  • Correctness gate: pass
  • Test gate: pass
  • Severity: 0 Critical, 1 Moderate, 0 Minor on this PR
  • Attribution: 1 in-diff, 0 out-of-diff breakage
  • questdb pointer in enterprise: OFF-DEFAULT — in scope
  • java-questdb-client pointer in OSS: OFF-DEFAULT — in scope

Both tandem submodule contents were independently reviewed because their PRs were explicitly supplied. No other submodule contents were expanded.

@bluestreak01
bluestreak01 merged commit 0b9b576 into main Aug 31, 2026
16 checks passed
@bluestreak01
bluestreak01 deleted the ia_oidc_device_flow branch August 31, 2026 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants