feat: OIDC device-flow authentication

[glasstiger]

Summary

New questdb.auth module that lets you sign in interactively to OIDC-secured
QuestDB Enterprise from Python — including from remote kernels (JupyterHub,
SageMaker, Colab, VS Code-remote, containers) that have no local browser.

It runs the OAuth 2.0 Device Authorization Grant (RFC 8628)
entirely client-side: you authorize in any browser (laptop or phone), while the
kernel only makes outbound calls to your identity provider (IdP). The resulting
token is presented to QuestDB over the auth paths it already supports — HTTP
Authorization: Bearer or PG-wire _sso — so no server change is required.

Pure Python, built on the standard library (urllib); questdb.ingress is
never imported at module load, and all optional deps are lazy.

Two ways to use it

Just the token — no extra dependencies; works with PG-wire, raw HTTP, the
ingestion Sender, or any client:

from questdb.auth import OidcDeviceAuth

auth = OidcDeviceAuth.from_questdb("https://questdb.example.com:9000")
token = auth.token()        # device flow on first use, else cached / refreshed
headers = auth.headers()    # {"Authorization": "Bearer <token>"}

On first use you get a sign-in prompt (clickable link in Jupyter, plain text on
a terminal). Re-running is silent: the token is cached and refreshed silently as
it nears expiry.

PG-wire adapters — feed the auto-refreshed token in as the _sso password
(require acl.oidc.pg.token.as.password.enabled=true on the server):

from questdb.auth import OidcDeviceAuth, sqlalchemy_engine, psycopg_connect

url = "https://questdb.example.com:9000"
auth = OidcDeviceAuth.from_questdb(url)
auth.token()                          # sign in once up front, before the pool opens

engine = sqlalchemy_engine(auth, url) # fresh token injected on every new connection
conn = psycopg_connect(auth, url)     # raw psycopg / psycopg2 connection

For REST (Authorization: Bearer) and ingestion (the ILP Sender), there is no
dedicated adapter — take auth.headers() / auth.token() and wire it in
yourself, e.g. Sender.from_conf("https::addr=…;", token=auth.token()).

Highlights

• Config auto-discovery from the QuestDB /settings endpoint (acl.oidc.*),
  falling back to the IdP .well-known/openid-configuration. Anything passed
  explicitly overrides discovery; discovery can also be skipped entirely.
• Correct token selection mirroring QuestDB's own logic
  (groupsEncodedInToken ? id_token : access_token), requesting the openid
  scope automatically when the id_token is sent. A completed grant that is
  missing the required token kind fails once with a clear error rather than
  caching an unusable token.
• Token lifecycle: in-process cache with silent refresh via the
  refresh_token; re-prompts only when the refresh token is missing/rejected.
  A transient failure — a network blip or an IdP 5xx/429, on a poll or a
  refresh — is surfaced and retried, not re-prompted, while a terminal IdP
  rejection fails fast with a clear error. A lock serializes refresh so parallel
  cells/threads don't double-prompt. The cache is an always-on, process-global
  in-memory store: re-running a notebook cell reuses the token, a kernel
  restart re-prompts once. By default nothing is written to disk; pass
  token_store=FileTokenStore.at_default_location() to opt into persistence
  so a restarted process resumes from a saved refresh token — an owner-only
  (0600) plaintext file per identity under ~/.questdb/oidc-tokens/ — instead
  of re-prompting.
• PG-wire connection adapters: SQLAlchemy and psycopg / psycopg2, injecting
  the auto-refreshed token as the _sso password (sqlalchemy_engine
  re-supplies a fresh token on every new pooled connection). That per-connection
  injection is non-interactive — it reuses / silently refreshes the cached
  token but refuses to start the device flow from a pool thread (raising
  OidcInteractionRequired rather than blocking the pool on a browser prompt),
  so you sign in once up front. REST and the ingestion Sender are
  bring-your-own-client via headers() / token().
• Non-interactive detection: raises OidcInteractionRequired instead of
  hanging under papermill / cron / CI.
• Jupyter-first prompt (clickable link, optional QR code via qrcode) with a
  plain-text terminal fallback that degrades gracefully on a non-UTF-8 console
  (the verification URL and code always print, even if the emoji can't be
  encoded — it no longer silently swallows the whole prompt). A device-
  authorization response missing the verification URI is rejected rather than
  rendering a blank prompt, and the "expires in N min" message reflects the
  token's real lifetime (its JWT exp), not the cache's re-validation clamp.
• Typed errors: every failure path raises an OidcError subclass
  (OidcConfigError, OidcNetworkError, OidcInteractionRequired,
  OidcDeviceFlowError, OidcTimeoutError) — malformed config, HTTP/URL errors
  and bad server payloads are mapped to these rather than leaking raw
  ValueError / AttributeError / http.client exceptions. Even a hostile
  non-string error / error_description in an IdP response is coerced, so it
  can't escape the contract as a raw TypeError.

Security

• No IdP passwords are entered in the notebook; MFA/SSO happen at the IdP.
• https is required; plaintext is allowed only to a loopback address.
  insecure=True permits plaintext to a non-loopback QuestDB host only — it
  never downgrades the IdP (so the device code and refresh token are never
  sent in cleartext) and never disables certificate verification.
• The credential (device-authorization / token) endpoints must share one
  origin, and HTTP redirects are refused — urllib doesn't strip the
  Authorization header across a cross-origin 30x, and the origin check only
  sees the pre-redirect URL, so a silently-followed redirect could leak the token
  or refresh token. A 30x on those endpoints surfaces as an error instead (and
  redirect-refusal is pinned on the HTTPS opener, the production path that
  carries the bearer / refresh token).
• Out-of-band IdP pin — pass issuer= / discovery_url= to pin the IdP. The
  discovery origin is never derived from a server-supplied token endpoint, so a
  tampered /settings can't redirect the device-code / refresh-token POSTs. The
  pin is required before trusting credential endpoints that arrive over an
  untrusted plaintext /settings (only reachable with insecure=True), and
  before discovering endpoints from the IdP. On a path-based multi-tenant IdP
  (e.g. Keycloak …/realms/{realm}) it also scopes endpoints to the issuer
  path, rejecting .. traversal even when hidden by percent-/double-encoding,
  a backslash, or a matrix ;param.
• Only server-authoritative /settings config is trusted — discovery reads
  the nested config object and ignores the user-writable preferences sibling
  (which the web console persists via PUT /settings), so a user who can write a
  preference can't smuggle an acl.oidc.* override (e.g. a redirected token
  endpoint) into the resolved config.
• Hardened device-flow poll — the IdP-supplied expires_in / interval are
  clamped (sane lifetime cap, interval in [5s, 60s], never sleeps past the
  deadline), so a hostile or buggy IdP can't time the flow out before the first
  poll or stall the loop while it holds the acquisition lock. Transient poll
  failures (network, IdP 5xx/429, or a non-JSON proxy error) keep polling to
  the deadline; a terminal rejection — including a non-JSON 4xx from a WAF or
  proxy — stops immediately rather than polling on to a misleading timeout. A
  plain 429/5xx honours the IdP's Retry-After when present — even on a
  non-JSON body from a proxy/WAF — else the RFC 8628 +5s step, both clamped to
  the poll-interval bounds; a slow_down only ever increases the interval, so
  it never polls faster right after the IdP asked it to slow down (RFC 8628 §3.5).
• The sign-in prompt can't be spoofed — C0/C1 control characters (incl. ESC)
  and bidi/zero-width characters are stripped from untrusted device-response
  fields (verification_uri, user_code, IdP error_description, JWT-derived
  identity) before they reach the TTY or notebook, so a MITM'd response can't
  inject ANSI escapes or reorder a URL's host. The verification link is shown
  with its host in IDNA/punycode form, and is only made clickable / opened in
  a browser / encoded as a QR when that host is a plain ASCII name — so a
  homoglyph dot (a fullwidth ． that IDNA folds to a real ., resolving to a
  different registrable domain) or an embedded user@host can't masquerade as
  the trusted host, and the link shown, the URL opened, and the QR all use one
  vetted target so they can't diverge. The Jupyter renderer additionally
  HTML-escapes its output and scheme-vets the link (no javascript:/data:).
• Credentials kept out of logs — TokenSet is immutable (frozen) and keeps
  the access/id/refresh tokens (and the sub) out of its repr, so a token
  can't leak into a log line or traceback. The PG adapters avoid logging the
  token / DSN and validate the PG-wire host and port: a host carrying
  connection-string metacharacters (; / = / whitespace / control / an IPv6
  zone-id %) or a non-integer port is rejected with a typed error, so a
  tampered URL can't inject libpq conninfo parameters.
• Honours HTTPS_PROXY, REQUESTS_CA_BUNDLE, SSL_CERT_FILE, and ca_bundle=.

Dependencies & Python support

token() / headers() need nothing beyond the standard library. sqlalchemy,
psycopg / psycopg2, qrcode and IPython are imported lazily, only when
used. The declared minimum Python is corrected to 3.10 (setup.py >=3.8
→ >=3.10, docs/installation.rst >=3.9 → >=3.10), matching the floor
4.1.0 already adopted when it dropped Python 3.9; stale 3.8 references are
removed (RELEASING.rst, proj.py).

Docs, examples & tests

• New guide docs/auth.rst (:ref:oidc_auth), API reference in docs/api.rst
  (OidcDeviceAuth, the two PG-wire adapters, OidcConfig, TokenSet, and the
  typed-error classes), index/installation updates, and a CHANGELOG.rst entry.
• Runnable example examples/oidc_device_auth.py.
• test/test_auth.py — pure-Python (no compiled extension required), wired into
  the suite via test/test.py. Covers the device flow, refresh, non-interactive
  contexts, discovery, the insecure-/settings guard, PG adapters (host/port
  validation), config helpers, endpoint validation, cache-key derivation, the
  HTTPS redirect-refusal path, multi-threaded and cross-instance cache-contention
  stress tests, and transport/renderer security (control/bidi/homoglyph/userinfo
  handling).

Also in this PR (CI & build robustness)

Bundled fixes to keep the matrix green, independent of the auth feature:

• pandas 3 / numpy 2: derive the expected dtype in test_parquet_roundtrip
  instead of hardcoding object (pandas 3 reads back the new string dtype); keep
  32-bit wheel targets on the consistent pandas 2 / numpy 1 stack (pandas 3 ships
  no 32-bit wheels) so the dataframe tests actually run there.
• QuestDB-master leg on JDK 25: build/run questdb master on JDK 25, build its
  -SNAPSHOT java client via the local-client Maven profile (new
  ci/templates/detect-local-client.yml), invoke Maven directly (the Maven task
  can't parse JDK 25), and pass the JPMS module-access flags the server needs.
• Windows: silence mock-server tracebacks on abrupt client disconnect
  (narrowed to the specific broken-pipe / connection-reset / connection-aborted
  errors, not all ConnectionError), and skip the readonly AZP_ENHANCED* agent
  var in the wheel-build env re-export.
• Updates the internal review-pr review skill (.claude/skills/review-pr/).

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR introduces the questdb.auth package implementing client-side OAuth 2.0 Device Authorization Grant (RFC 8628) OIDC authentication for QuestDB Enterprise. The package includes a stdlib HTTP client, token cache backends, OIDC endpoint discovery, device-flow rendering for terminal and Jupyter, the core OidcDeviceAuth token manager, a QuestDB session with REST/SQLAlchemy/psycopg/ILP adapters, a full unit test suite, and comprehensive documentation. The minimum Python version is raised to 3.10. An unrelated Claude PR review skill document is also added.

Changes

questdb.auth OIDC Device Authorization Grant

Layer / File(s)	Summary
Exception hierarchy and Python version bump src/questdb/auth/_errors.py, setup.py, docs/installation.rst	Defines OidcError and seven typed subclasses; raises python_requires to >=3.10 in setup.py and updates the installation docs accordingly.
Stdlib HTTP client with TLS and security enforcement src/questdb/auth/_http.py	Implements build_ssl_context, HttpResponse, loopback detection, HTTPS-only enforcement for non-loopback hosts, request(), get_json(), and post_form() using only Python stdlib.
TokenSet and cache backends src/questdb/auth/_cache.py	Defines TokenSet with clock-skew-aware validity, TokenCache interface, and three implementations: MemoryCache (thread-safe), NullCache, and FileCache (atomic JSON writes, cross-process fcntl/msvcrt locking) plus make_cache() factory.
OIDC configuration discovery and endpoint validation src/questdb/auth/_discovery.py	Implements OidcConfig, QuestDB /settings fetching/flattening, IdP .well-known fallback, relative endpoint resolution, same-origin and issuer-pin validation, and resolve_config() orchestrator.
Device-flow prompt rendering src/questdb/auth/_render.py	Adds scheme-safe link validation, TerminalRenderer (plain-text + ASCII QR), JupyterRenderer (updatable HTML display + base64 PNG QR), environment detection, and make_renderer() factory.
OidcDeviceAuth token manager src/questdb/auth/_device.py	Implements OidcDeviceAuth with from_questdb() classmethod, token lifecycle (cache gate → silent refresh → RFC 8628 device flow with poll loop), _tokenset_from_response, secure IdP transport, URL normalization, and browser-open safety.
QuestDB session and connection adapters src/questdb/auth/_questdb.py	Adds QuestDB wrapper with sql() (REST /exec → pandas DataFrame), sqlalchemy_engine() (per-connection token injection), psycopg() (_sso auth), sender() (ILP bearer), and top-level connect().
Public API, example, changelog, and docs src/questdb/auth/__init__.py, examples/oidc_device_auth.py, docs/auth.rst, docs/api.rst, docs/index.rst, CHANGELOG.rst	Wires __all__ for the public surface; adds a runnable example; adds comprehensive Sphinx documentation covering usage modes, discovery, caching, adapters, IdP requirements, and security; registers the auth toctree entry and updates the changelog.
Unit test suite, test infrastructure, and CI compatibility test/test_auth.py, test/test.py, test/test_dataframe.py, test/mock_server.py, ci/pip_install_deps.py, ci/cibuildwheel.yaml	Adds 1167 lines covering device flow, non-interactive, refresh, file cache, discovery, REST adapter, concurrency, adapter mocks, config helpers, endpoint validation, cache-key normalization, transport security, and renderer security; registers all test classes; updates pandas dtype compatibility and pandas 3/32-bit wheel checks; makes HTTP test server quiet on abrupt connection closures; extends Windows env-var filtering.

Claude PR Review Skill

Layer / File(s)	Summary
review-pr SKILL.md .claude/skills/review-pr/SKILL.md	Defines a 4-step PR review workflow with 10 parallel agents, review level controls, change-surface mapping, verification requirements, comprehensive checklists (correctness, Cython/C-ABI, GIL, performance, tests), and structured output format for findings.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant connect as questdb.auth.connect()
  participant OidcDeviceAuth
  participant resolve_config
  participant QuestDB_Settings as QuestDB /settings
  participant IdP
  participant Renderer
  participant TokenCache
  participant QuestDB_Session as QuestDB session

  User->>connect: connect(url, eager=True)
  connect->>OidcDeviceAuth: from_questdb(url)
  OidcDeviceAuth->>resolve_config: url, overrides
  resolve_config->>QuestDB_Settings: GET /settings
  QuestDB_Settings-->>resolve_config: acl.oidc.* config
  resolve_config->>IdP: GET /.well-known/openid-configuration (if needed)
  IdP-->>resolve_config: device_authorization_endpoint
  resolve_config-->>OidcDeviceAuth: OidcConfig
  connect->>QuestDB_Session: QuestDB(url, auth)
  connect->>OidcDeviceAuth: token() [eager]
  OidcDeviceAuth->>TokenCache: load(cache_key)
  TokenCache-->>OidcDeviceAuth: miss
  OidcDeviceAuth->>IdP: POST device_authorization_endpoint
  IdP-->>OidcDeviceAuth: device_code, user_code
  OidcDeviceAuth->>Renderer: on_prompt(resp)
  Renderer-->>User: display verification URI + user code
  loop Poll IdP
    OidcDeviceAuth->>IdP: POST token_endpoint (device_code grant)
    IdP-->>OidcDeviceAuth: authorization_pending / tokens
  end
  OidcDeviceAuth->>TokenCache: store(cache_key, TokenSet)
  OidcDeviceAuth->>Renderer: on_success(identity, expires_in)
  connect-->>User: QuestDB session
  User->>QuestDB_Session: sql("SELECT ...") / sender() / sqlalchemy_engine()

Loading

🎯 4 (Complex) | ⏱️ ~75 minutes

🐇 Hoppity-hop through device flows and grants,
A token appears after suitable chants!
The IdP polls, the cache stores with care,
A Bearer in headers floats freshly through air.
Jupyter glows, the terminal prints neat—
OIDC for QuestDB, the login's complete! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.81% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the primary change: introduction of interactive OIDC device-flow authentication. It is concise, specific, and directly reflects the main feature addition across the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ia_oidc_device_flow

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (1)

src/questdb/auth/_questdb.py (1)

82-94: 💤 Low value

Chain the import exception for better diagnostics.

When both psycopg and psycopg2 fail to import, the raised ImportError loses the underlying cause. Chaining the exception preserves the diagnostic context.

Suggested fix

 def _pg_module():
     try:
         import psycopg  # type: ignore  # psycopg v3
         return psycopg
     except ImportError:
         pass
     try:
         import psycopg2  # type: ignore
         return psycopg2
-    except ImportError:
+    except ImportError as e:
         raise ImportError(
             'A PostgreSQL driver is required: install `psycopg` (v3) or '
-            '`psycopg2-binary`.')
+            '`psycopg2-binary`.') from e

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/questdb/auth/_questdb.py` around lines 82 - 94, The second except
ImportError block in the _pg_module() function should preserve the exception
chain for better diagnostics. Capture the ImportError exception in the second
except block using `as e` syntax, then use `raise ... from e` syntax when
raising the new ImportError to chain the exception, which preserves the
underlying cause and provides better debugging context.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.claude/skills/review-pr/SKILL.md:
- Around line 157-162: The Step 3 preamble at lines 157-162 states that every
agent receives the full change-surface map, but Agent 10's input contract at
lines 231-240 explicitly limits it to only the PR diff and changed file names.
Add an explicit exception clause in the Step 3 preamble section to clarify that
Agent 10 does not receive the full change-surface map and instead receives only
the diff and changed file names, ensuring consistency between the general rule
and the specific Agent 10 behavior documented at lines 231-240.
- Around line 39-42: The table at lines 39-42 states that level 0 (default)
skips Step 2.5, but there is a later reference (around lines 144-155) that marks
Step 2.5e as mandatory at every level, creating a contradiction. Fix this
inconsistency by clarifying the level 0 description in the table: either modify
the "Skip Step 2.5" statement to specify that Step 2.5e is an exception and is
still performed, or restructure it to list exactly which substeps (2.5a, 2.5b,
2.5c, 2.5d, 2.5e) are executed or skipped. Ensure the description at level 0 and
the mandatory requirement stated at lines 144-155 are aligned so the default
path is no longer self-contradictory.

In `@docs/auth.rst`:
- Around line 65-76: The code example in the documentation is missing the import
statement for TimestampNanos, which is used in the sender.row() method call. Add
an import statement at the top of the code snippet to include TimestampNanos
from the questdb.ingress module (or appropriate module) so that the example can
be executed successfully when copied verbatim.

In `@src/questdb/auth/__init__.py`:
- Around line 75-92: The `__all__` list in the module needs to be sorted
alphabetically to comply with Ruff's RUF022 rule and maintain consistency in the
public API exports. Reorder all the items in the `__all__` list (which includes
'connect', 'QuestDB', 'OidcDeviceAuth', 'OidcConfig', 'TokenCache', 'TokenSet',
'MemoryCache', 'FileCache', 'NullCache', and the various OidcError types) in
alphabetical order while ensuring all items remain in the list.

In `@src/questdb/auth/_http.py`:
- Around line 123-129: The `_opener()` function doesn't receive the `insecure`
parameter, so it cannot enforce HTTPS-only redirect policies. To fix this, add
an `insecure` parameter to the `_opener()` function signature. When
`insecure=False`, create a custom redirect handler that validates redirects do
not downgrade from HTTPS to HTTP for non-loopback hosts (similar to the
validation in `_require_secure()`), and pass this handler to
`urllib.request.build_opener()` instead of using the default. When
`insecure=True`, use urllib's default redirect handling. Update all callers of
`_opener()` to pass the `insecure` parameter value.

In `@test/test_auth.py`:
- Around line 600-605: The timed joins with a 10-second timeout do not verify
that threads actually completed, which can cause non-daemon threads to remain
running and hang the test process if a regression causes a deadlock. After
calling t.join(10) on each thread in both the loop at lines 600-605 and at lines
828-835, add a verification check that confirms each thread has terminated by
asserting that t.is_alive() returns False, or raise an exception if any thread
is still alive after the timeout. This ensures the test fails cleanly rather
than leaving hanging threads.

---

Nitpick comments:
In `@src/questdb/auth/_questdb.py`:
- Around line 82-94: The second except ImportError block in the _pg_module()
function should preserve the exception chain for better diagnostics. Capture the
ImportError exception in the second except block using `as e` syntax, then use
`raise ... from e` syntax when raising the new ImportError to chain the
exception, which preserves the underlying cause and provides better debugging
context.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bf4af383-9022-4ae5-b189-73691f4ccd00

📥 Commits

Reviewing files that changed from the base of the PR and between a523b3a and 4559587.

📒 Files selected for processing (18)

• .claude/skills/review-pr/SKILL.md
• CHANGELOG.rst
• docs/api.rst
• docs/auth.rst
• docs/index.rst
• docs/installation.rst
• examples/oidc_device_auth.py
• setup.py
• src/questdb/auth/__init__.py
• src/questdb/auth/_cache.py
• src/questdb/auth/_device.py
• src/questdb/auth/_discovery.py
• src/questdb/auth/_errors.py
• src/questdb/auth/_http.py
• src/questdb/auth/_questdb.py
• src/questdb/auth/_render.py
• test/test.py
• test/test_auth.py

[glasstiger]

Code review — feat: interactive OIDC device-flow authentication (questdb.auth)

Reviewed at level 3 (full multi-agent pass + per-finding source verification). This PR is pure Python — a new questdb.auth package (5,319 lines) touching no Cython/C-ABI code — so the review targeted the actual mission-critical surface: security (token leakage, MITM, header/conninfo injection, origin/redirect validation, homoglyph/control-char prompt spoofing) and correctness (token lifecycle, concurrency, cross-process persistence).

Headline: no Critical or Moderate findings. This is unusually hardened, defensively-written, and thoroughly-tested code (316 tests). Every credential-leak / injection / spoofing / race vector I could construct was already defended and covered by a test. What remains are Minor hardening and test-robustness items.

Critical / Moderate

None.

Minor

1. Persisted non-served token skips the control-char / CRLF gate — _device.py:1176-1184
_tokenset_from_persisted gates only the served token with _has_only_token_chars; the non-served token gets only _str_or_none (no char screen). The network path (_tokenset_from_response:1306-1307) gates both access_token and id_token via _safe_token_or_none. So a groups-mode entry whose access_token carries \r\n is adopted un-screened and re-persisted verbatim by _snapshot.
Not exploitable today (_select only returns the served kind, and both cache_key and the store fingerprint bucket by groups_in_token, so the tainted field can never become the served token), but it's an asymmetry in a security boundary and the fix is one line — _safe_token_or_none is already in the same file (_device.py:294):

access_token = _safe_token_or_none(persisted.access_token)
id_token = _safe_token_or_none(persisted.id_token)

This makes the persisted and network ingestion paths apply an identical screen, matching the parity the :1301-1303 comment already claims.

2. Head-read watchdog can tear down a healthy socket in the open()→release() window — _http.py:445-456
The head_timer fires watch.shutdown() at start+timeout. When a head read completes right at the deadline (T_head ≈ timeout), the timer can fire in the gap between open() returning (line 450) and watch.release() (line 455) — where _done is still False and _sock is set — shutting down the live socket _read_body is about to use, producing a spurious OidcNetworkError. This contradicts the docstring guarantee at :452-454 / :192-195 that a late fire "can't tear down the healthy socket." Impact is low and recoverable (poll treats it as transient and retries; refresh retries later) and the trigger is narrow, but it is a real bug. Fix: neutralize a fire that races a successful return (e.g. release before the body read can begin, or drop a shutdown once open() has returned).

3. Discovery document's own issuer field is never validated — _discovery.py:578-592
resolve_config reads only token_endpoint / device_authorization_endpoint from the .well-known doc; doc.get('issuer') is never checked against the pinned issuer (RFC 8414 §3.3 issuer validation). The well-known test fixtures all carry an 'issuer' field that is dead payload. Not a live vulnerability — the doc is fetched over TLS directly from the out-of-band-pinned issuer origin (insecure=False), so it can't be substituted, and a hostile pinned origin would echo a matching issuer anyway. It's a spec-conformance / misconfiguration-diagnostic gap; worth adding the check (reject when doc['issuer'] is present and ≠ the pinned issuer) plus a test.

4. Concurrency tests can hang CI instead of failing — test_auth.py:5677 (+ siblings)
test_in_lock_serializes_concurrent_acquirers uses an unbounded t.join() with no is_alive() assertion — a serialization/deadlock regression in the O_EXCL lock protocol would hang the whole suite indefinitely rather than fail fast. Several sibling concurrency tests use join(timeout) without a following assertFalse(t.is_alive()) (e.g. :5583, :600, :828). Add a bounded join + is_alive() check, matching the well-built ones (test_concurrent_steal_does_not_hang_or_leak:5616, test_renderer_reentrant_call_raises_not_deadlocks:2589).

5. The only behavioral TLS-rejection test silently skips in CI — test_auth.py:4296
test_untrusted_server_certificate_is_rejected skipTests when cryptography is absent, and this PR's ci/pip_install_deps.py does not install cryptography. So in CI, request-path cert verification is guarded only by the static posture assertion, not the behavioral handshake test that would catch a verification silently bypassed on the request path. Either add cryptography to the CI test deps or accept the skip explicitly. Low risk (build_ssl_context wraps stdlib ssl.create_default_context()).

Minor nits (optional)

• FileTokenStore.save leaks the mkstemp fd if os.fdopen() itself raises before the with takes ownership (_store.py:564-571). Theoretical; the temp file is still cleaned. Guard with a closed flag in the finally to be airtight.
• _steal_stale_lock can leave an orphaned fresh lock if a live holder releases exactly mid-steal (_store.py:812-843). Safety-neutral (atomic writes hold); the identity degrades to lock-free until the orphan ages past lock_stale. Worth a one-line comment at the restore site.
• _normalize_url (cache_key) collapses an explicit :0 port while _canonical_endpoint (store key) keeps it (_device.py:1718 vs _store.py:237) — a hair's-breadth deviation from the "same identity distinctions" contract. :0 is unconnectable, so cosmetic.
• _coerce_port silently truncates a float port (int(8812.9)→8812, _adapters.py:123) rather than rejecting a non-integer; result is still a valid port.
• FileTokenStore.load/save/clear/in_lock lack their own docstrings (_store.py:514+), inheriting the ABC's; may render doc-less on the concrete class page since docs/api.rst doesn't set :inherited-members:.

Dismissed after verification (false positives / over-rated)

• __all__ not sorted (RUF022) — it is sorted (codepoint order, classes before lowercase functions).
• _pg_module() doesn't chain the ImportError — it already does raise ImportError(...) from e (_adapters.py:67-70).
• docs/auth.rst example missing TimestampNanos import — present at docs/auth.rst:96.
• _opener() should take insecure to enforce redirect policy — moot: _NoRedirect is installed unconditionally on every opener variant and refuses all redirects; _require_secure is enforced in request().
• Stale _questdb.py / QuestDB class / top-level connect() references — the module was reorganized into _adapters.py+_store.py; no doc/changelog/example references a removed symbol.
• OidcError can escape if an arg's __str__ raises — not reachable; all untrusted fields come from json.loads, whose outputs have total str().
• Memoize cache_key to avoid per-call URL parsing — refuted: cache_key is evaluated zero times on the valid-cached fast path (read only in the tokens is None branch, past the fast-path return). The fast path is genuinely lock-free and allocation-free.

Verdict

Approve — no blocking issues. The five Minor items are hardening/test-robustness improvements, none security-critical given the mitigations. #1 (one-line gate symmetry) and #4 (bounded join) are the two most worth doing before merge.

Notable strengths (relevant to the verdict): the parse-vs-connect authority defense, the unconditional redirect refusal, the issuer origin+path pinning with the discovery-confirmed exemption, the _strip_control Unicode-category sanitizer, the generation-CAS cache protocol, and the O_EXCL cross-process lock are all correctly implemented, match real stdlib behavior, and are backed by adversarial tests.

_{🤖 Level-3 review via the repo's review-pr skill (Claude Code). ~14 draft findings → 5 Minor + 5 nits confirmed, ~9 dismissed on source verification.}

[glasstiger]

Code review — feat: interactive OIDC device-flow authentication (questdb.auth)

Reviewed at level 3 (full multi-agent pass + per-finding source verification). This PR is pure Python — a new questdb.auth package (~5,300 lines, 10 modules) touching no Cython/C-ABI code — so the review targeted the actual mission-critical surface: security (token leakage, MITM/origin confusion, header/conninfo injection, prompt spoofing) and correctness (token lifecycle, concurrency, cross-process persistence). I read all ten src/questdb/auth/*.py modules in full, mapped the change surface, ran parallel adversarial passes (security, correctness, concurrency/resources, tests, API/docs, cross-context, fresh-context), and source-verified every draft finding.

This is a re-review at the current tip (4559587). The earlier level-3 review on this PR was posted at an older commit — I re-verified each of its items against current HEAD, and all 5 Minor findings and the fd-leak nit are already fixed (see Downgraded).

Headline: no Critical or Moderate findings. Every credential-leak / injection / spoofing / origin-confusion / race vector I could construct was already defended and backed by an adversarial test. What remains are Minor correctness-robustness and test/doc items.

Critical / Moderate

None.

Minor

1. Rotated refresh token is silently discarded on the "200-but-missing-required-kind" refresh branch — _device.py:1085-1103 (_try_refresh_locally).
When a silent refresh returns HTTP 200 carrying a newly rotated refresh_token but without the required token kind (groups mode, IdP re-issues access_token+refresh_token but no id_token), _refresh (_device.py:1370-1377) preserves the rotated token into refreshed, but _try_refresh_locally then hits if self._has_required_token(refreshed): … return None and drops it without calling _store. _acquire (:1005-1006) evicts the old token and either re-prompts (interactive) or raises OidcInteractionRequired (non-interactive pool path).
Nuance (why Minor): this only triggers in groups mode with an IdP that doesn't re-issue the id_token on refresh — a case the code already anticipates (:1097-1099). For a deterministic IdP, silent id_token maintenance is fundamentally impossible there, so a re-prompt is the correct, unavoidable outcome, and preserving the rotated token wouldn't change it. The discard only produces a worse-than-correct result for a pathological non-deterministic IdP. The regression test test_refresh_without_id_token_non_interactive_does_not_loop (test_auth.py:1717) covers the no-id_token path but with a response that omits refresh_token, so the rotation-discard sub-case is untested.
Fix (optional): before return None, if refreshed.refresh_token != tokens.refresh_token, persist/adopt the rotated refresh token so a later attempt refreshes with the live token rather than replaying the (server-rotated, likely-revoked) old one — plus a regression test with a rotating no-id_token refresh response.

2. Two public __init__ methods lack docstrings — _errors.py:38 (OidcError.__init__) and _errors.py:90 (OidcDeviceFlowError.__init__).
These are the only public (non-underscore) API members with no docstring — the entirety of what drags the docstring-coverage metric to 15.81%. Impact is limited to the metric: docs/api.rst documents these via .. autoexception:: without :members:, so the signatures aren't rendered anyway. One-line docstrings resolve it.

3. Minor test-coverage gaps (test/test_auth.py). Real defenses with no direct test:

• build_ssl_context env-var path (REQUESTS_CA_BUNDLE/SSL_CERT_FILE) and the capath directory branch (_http.py:82-83) — the documented TLS-intercepting-proxy mechanism is never exercised; only an explicit ca_bundle= path is.
• The non-string-file-value branch of _audience_matches/_issuer_matches (_store.py:914-937) — the load-side type gate against a hostile "audience": 123 / "issuer": ["x"] — is untested (only None-vs-str and str-vs-str are).
• _reject_confusable_authority with a bare C0-control/DEL (\x01, \x7f) inside the netloc (_discovery.py:319) — tested for \t\n\r/%/@/non-ASCII, but not the control chars urlparse keeps in .netloc.
• Adapter host guard _ILLEGAL_HOST_CHARS (_adapters.py:55) with a NUL/DEL host — ;/=/space/% are tested, the \x00-\x1f/\x7f range is not.

Minor nits (optional, borderline)

• _strip_control keeps category-Mc spacing marks uncapped (_render.py:389; only Mn is Zalgo-capped). A deliberate tradeoff (comment at :331-336 keeps Mc for legitimate Indic text) and cosmetic-only (the host is separately IDNA-normalized) — near-dismissible.
• An issuer= carrying a query/fragment produces a malformed .well-known URL via well_known_url's string-concat (_discovery.py:421); not exploitable (host unchanged → discovery 404s).
• questdb_url's authority isn't confusable-checked before the /settings GET (_discovery.py:139), unlike issuer/endpoints. Low risk: that GET carries no secret and every returned endpoint is re-vetted before any credential POST.
• cache_key excludes device_authorization_endpoint while the on-disk TokenStoreKey.hash includes it (_device.py:765 vs _store.py:329). Benign: the token_endpoint (where credentials are exchanged) is keyed by both; asymmetry costs at most one extra silent refresh, never a cross-identity serve.

Downgraded (verified false positives / non-issues)

Earlier review's findings — all resolved at current HEAD:

• "Persisted non-served token skips the control-char/CRLF gate" — FIXED: _tokenset_from_persisted now runs both tokens through _safe_token_or_none (_device.py:1184-1185).
• "Discovery doc's own issuer never validated" — FIXED: RFC 8414 §3.3 check at _discovery.py:464-472.
• "Concurrency tests can hang CI" — RESOLVED: zero unbounded join()s; all 12 non-daemon worker joins are bounded and paired with assertFalse(t.is_alive()); the only assertion-less joins are on daemon=True reapers.
• "TLS-rejection test silently skips in CI" — RESOLVED: ci/pip_install_deps.py:116 now installs cryptography, so test_untrusted_server_certificate_is_rejected runs the real handshake.
• nit "FileTokenStore.save leaks the mkstemp fd if os.fdopen raises" — FIXED: fd_owned guard (_store.py:581-603).

Other candidates dismissed on verification:

• MemoryCache.release() remaining < 0 strands a generation entry — unreachable in current code (every generation() is paired with one release()); a documented future-caller guard, bounded to one entry even if reached.
• _backoff_interval no forward progress at the 60s cap under repeated slow_down — by design; the device-code deadline terminates the loop. RFC technicality only.
• Head-read watchdog open()→release() race — accurately documented (_http.py:191-198/:456-464) as irreducible for a wall-clock timer and safe-degrading (retryable OidcNetworkError, never a wrong/truncated token).
• :0-port cache_key vs store-key asymmetry — not present; both _normalize_url (:1730) and _canonical_endpoint (:237) compare port against None and keep :0.
• _normalize_url vs _canonical_endpoint render a path-less endpoint differently — benign: separate key spaces (never cross-compared), and RFC 8628 endpoints always carry a path.
• settings_config on a non-dict /settings — safe (returns {}).
• Leading/trailing spaces survive _safe_token_or_none — not an injection vector (space can't break a header/conninfo field; CR/LF/;/= are already rejected).

Verdict

Approve — no blocking issues. Unusually hardened, defensively-written, thoroughly-tested code (319 tests). Verified defenses include: TLS-can-never-be-disabled, insecure=True never downgrading the IdP, the repr=False + _strip_control credential-leak defenses, the _safe_token_or_none CR/LF gate on both network and persisted paths, _reject_confusable_authority's urlparse-vs-http.client divergence coverage, issuer origin+path pinning with the discovery-confirmed exemption and full ..-traversal decoding, the plaintext-/settings guard, unconditional redirect refusal, the preferences-vs-config split, the RFC 8414 doc-issuer check, the display/open/QR single-vetted-target unification, the generation-CAS resurrection defense, and the O_EXCL cross-process lock. The two documented irreducible races degrade to "one wasted round-trip / one re-prompt, never a wrong or torn token."

All findings are in-diff; out-of-diff is legitimately zero (the auth package is a self-contained leaf, imported only by the new tests/examples, and the PR modifies no existing exported API — ingress.pyx/.pyi, Sender/Buffer, src/questdb/__init__.py are untouched). The "also in this PR" CI/build changes (connection-error scoping in mock_server.py, version-agnostic dtype in test_dataframe.py, the 32-bit pandas guard, cryptography added non-fatally, the 3.8→3.10 bump across setup.py/docs/RELEASING.rst/proj.py) are all low-risk and correct.

Most worth doing before merge: #1 (persist the rotated refresh token on the missing-kind branch + regression test) and #3 (the build_ssl_context env-var and _*_matches non-string-file test gaps). Neither is security-critical given the mitigations.

_{🤖 Level-3 review via the repo's review-pr skill (Claude Code). 10 source modules read in full; parallel adversarial agents; per-finding source verification. Earlier review re-verified as stale (all items fixed).}

[glasstiger]

Code review — feat: OIDC device-flow authentication (questdb.auth)

Reviewed at level 3 (full multi-agent adversarial pass + per-finding source verification) at the current tip e50f490. This is effectively a re-review: the two earlier level-3 reviews were posted before a history rewrite (the last-reviewed commit 4559587 is no longer in HEAD's ancestry), and there are 5 newer fix(auth): commits since — so I re-verified everything against current source and treated prior line numbers as stale.

This PR is pure Python — a new self-contained questdb.auth package (9 modules, ~5,700 LOC + ~7,000 LOC of tests) touching no Cython/C-ABI code — so the review targeted the real surface: transport/TLS, credential-endpoint origin pinning, token lifecycle, cross-process concurrency, and injection/spoofing. Ten parallel agents read current source (not just the diff); every draft finding was then source-verified, including a direct code trace to adjudicate a conflict between agents on the one security-relevant Moderate candidate.

Headline: no Critical or Moderate findings. Every attacker-controlled vector I constructed (tampered /settings, MITM'd IdP, hostile JWT/token fields, conninfo/header injection, homoglyph/control-char prompt spoofing, path traversal, redirect-based token leak) was already defended and backed by an adversarial test (346 auth tests, all green). What remains are three Minor items and a few optional Nits.

Critical / Moderate

None.

Minor

1. Rotated refresh_token discarded on the "200-but-missing-required-kind" refresh branch — _device.py:1148-1151 (_try_refresh_locally).
When a silent refresh returns HTTP 200 carrying a newly rotated refresh_token but without the required token kind (groups mode; IdP re-issues access_token+refresh_token but no id_token), the if self._has_required_token(refreshed): gate is False, so it return None without ever calling _store/_persist_if_rotated. The rotated token is thrown away; _tokens keeps the old refresh token, which the IdP just rotated (hence spent). _acquire evicts and re-prompts, and on a later attempt/restart the spent old token is replayed → IdP refresh-token-reuse rejection → an extra re-prompt.
Nuance (why Minor): only bites groups mode against an IdP that rotates the refresh token but doesn't re-issue id_token on refresh. For a deterministic IdP, silent id_token maintenance is impossible there anyway, so a re-prompt is the correct unavoidable outcome and preserving the rotated token wouldn't change it — it only produces a worse-than-correct result for a pathological non-deterministic IdP. Recoverable (a re-prompt, never a wrong or served token). test_refresh_without_id_token_non_interactive_does_not_loop covers the no-id_token path but with a response that omits refresh_token, so the rotation-discard sub-case is untested.
Fix: before return None, if refreshed.refresh_token != tokens.refresh_token, adopt/persist the rotated refresh token (so the next attempt refreshes with the live token rather than the revoked one), then still return None to drive the interactive fallback — plus a regression test with a rotating, no-id_token refresh response.

2. A non-string ca_bundle= escapes the typed-error contract as a raw TypeError — _http.py:82-85 (build_ssl_context).
build_ssl_context catches only (OSError, ssl.SSLError) (line 85), but a non-string ca_bundle (a list/dict/float, or a stray int) makes os.path.isdir(ca) / ssl.create_default_context(cafile=ca) raise a bare TypeError that propagates out of OidcDeviceAuth.__init__ and from_questdb, despite the documented contract (lines 79-80) to map bad-bundle errors to OidcConfigError. Careless-caller only — not attacker-reachable (the REQUESTS_CA_BUNDLE/SSL_CERT_FILE env fallbacks are always str), and the other constructor args are type-validated in __init__, so this is an inconsistency, not a vuln. test_bad_ca_bundle_raises_config_error covers only string paths, so it's untested.
Fix: add TypeError to the except at line 85, or isinstance-check ca_bundle in __init__ alongside the existing typed-arg validation.

3. build_ssl_context's env-var and capath directory branches are untested — _http.py:75-76, 82-83.
The only test exercises an explicit string ca_bundle= file path. The documented TLS-intercepting-proxy mechanism — CA resolution from REQUESTS_CA_BUNDLE then SSL_CERT_FILE, and the isdir → capath= branch — has no test. A regression that read the wrong env var, dropped the SSL_CERT_FILE fallback, or passed a directory as cafile= (a silent no-trust) would not be caught. This is TLS trust configuration, so worth a test.
Fix: add a test that points each env var at the suite's runtime-generated cert and asserts the returned context trusts it, plus a capath directory case.

Minor nits (optional)

• slow_down in a non-conformant 5xx body without Retry-After isn't honored — _device.py:1690-1702. A 500 {"error":"slow_down"} with no Retry-After enters the 5xx arm but the inner guard status == 429 or retry_after is not None is False, so the RFC 8628 §3.5 +5s step is skipped and the interval stays flat. Doubly-non-conformant (RFC returns slow_down with HTTP 400, handled correctly by the dedicated arm at 1723); the poll continues and the deadline bounds it. Optional: fold body.get('error') == 'slow_down' into the guard.
• well_known_url string-concatenates — _discovery.py:446. An issuer= carrying a query/fragment yields a malformed .well-known URL. Not exploitable (host unchanged → 404, and the RFC 8414 issuer-match would reject any doc); build via urlunparse dropping query/fragment for cleanliness.
• Wrong-type host=/url in the PG adapters raise AttributeError/TypeError — _adapters.py:93,106. Same class as Ma/examples only #2 (careless caller, nothing injectable reaches the driver — the call crashes before producing a host). Add an isinstance guard for contract consistency.
• Deliberate, documented Unicode tradeoffs (not bugs): _strip_control leaves category-Mc spacing marks uncapped, and e50f490 strips Khmer U+17B4/U+17B5 which could alter a legitimate Khmer identity string — both are documented, visually-inert choices.
• Cosmetic: CHANGELOG.rst:5 has a stray blank line before its underline; the frozen-dataclass fields in docs/api.rst render as bare field: type (no attribute docstrings).

Downgraded (false positives / over-rated, dismissed after source verification)

• _last_persisted_refresh_token desync → refresh-token reuse (flagged Moderate by the fresh-context agent) — FALSE POSITIVE. Claim: when the shared-cache CAS in _store drops (a concurrent clear() bumped the generation), _tokens advances to the rotated token but the marker doesn't, and the _refresh_under_lock gate (:1170) then replays a peer-rotated token. Traced and disproved: when the CAS drops, store_if_current returns False and _persist_if_rotated is skipped (:1073), so the rotated token lives only in self._tokens — never written to the shared cache or to disk, so no peer/process can obtain or spend it. Refreshing with it later is therefore guaranteed-live and correct; the gate's "marker ≠ in-mem ⇒ in-mem is strictly newer/un-shared ⇒ don't regress to disk" logic is right. Independently cleared by the two agents that deep-audited _device.py and _store.py, and the flagging agent's own experiment showed a targeted marker-sync fix did not change the measured reuse rate — i.e. the desync is not the cause. The residual multi-process reuse it measured is the separately-documented best-effort cross-process file-lock limitation (two processes sharing an identity when the O_EXCL lock degrades), whose consequence is a recoverable re-prompt, not credential compromise.
• clear()'s orphan-temp sweep deletes a concurrent save()'s live temp (flagged Moderate) — over-rated to Nit. In the normal single-auth-instance flow, clear and save serialize through the O_EXCL in_lock; only in the degraded lock-free window can the sweep unlink a live temp, whereupon save's os.replace raises OSError → swallowed (best-effort) → continues with the valid in-memory token. Integrity-safe (the atomic-write invariant holds); costs one lost persist = one extra silent refresh next restart.
• body.get('error') at _device.py:1702 could hit body=None — unreachable in production (post_form guarantees a dict; only a mock returning body=None triggers it).
• CodeRabbit's actionable items — all already correct at HEAD: __all__ is codepoint-sorted (RUF022-clean); _pg_module does raise ImportError(...) from e; the docs/auth.rst example does import TimestampNanos; _opener refuses redirects unconditionally on every variant (so passing insecure into it is moot).
• Docstring coverage 15.81% (CodeRabbit pre-merge warning) — every public symbol in __all__ (all 16, plus their public methods) has a docstring; the metric counts private _-prefixed helpers, consistent with the repo's convention (ingress.pyx). Non-blocking.

Verdict

Approve — no blocking issues. Unusually hardened, defensively-written, thoroughly-tested code. Verified defenses include the layered parse-vs-connect authority defense, unconditional redirect refusal, issuer origin+path pinning with full ..-traversal decoding (percent/double-encoded/backslash/;param), the _safe_token_or_none CR/LF gate on both the network and persisted token paths, the _strip_control Unicode-category sanitizer (with the display/open/QR single-vetted-target unification), the preferences-vs-config split, the plaintext-/settings pin requirement, the generation-CAS cache protocol, and the O_EXCL cross-process lock — all correctly implemented, matching real stdlib behavior, and backed by adversarial tests.

All findings are in-diff; out-of-diff is legitimately zero — questdb.auth is a brand-new self-contained leaf package imported only by its own new tests/example, and the diff modifies no existing exported API (ingress.pyx/.pyi, Sender/Buffer, src/questdb/__init__.py are untouched). The "also in this PR" CI/build changes (mock-server connection-error scoping, version-agnostic dtype, the 32-bit pandas guard, cryptography added to CI deps, the 3.8→3.10 bump across setup.py/docs/RELEASING.rst/proj.py) are all low-risk and consistent.

Most worth doing before merge: #1 (persist the rotated refresh token on the missing-kind branch + a rotating-no-id_token regression test). #2 and #3 are quick TLS-config hardening. None is security-critical given the mitigations.

_{🤖 Level-3 review via the repo's review-pr skill (Claude Code). Re-review at tip e50f490 after history rewrite; 9 source modules read in full; 10 parallel adversarial agents; per-finding source verification. ~15 draft candidates → 3 Minor + ~5 Nits confirmed, ~7 dropped (incl. the one Moderate-flagged reuse candidate, disproved by code trace).}