[glasstiger]

Summary

Adds native-backed OIDC device-flow authentication for QuestDB Enterprise. The
Python API runs OAuth 2.0 Device Authorization Grant (RFC 8628) through the
native QuestDB client, including from remote Jupyter kernels where the browser
and Python process are on different machines.

Discovery, endpoint validation, token selection, caching, refresh, persistence,
and concurrency control use the native implementation. Python provides the
public authentication API, Jupyter/terminal rendering, PG-wire adapters, and
integration with QuestDB transports.

It also adds rotating Bearer-token providers, for machine credentials that
rotate: Microsoft Entra ID managed identities and service principals, OAuth2
client credentials, a secrets vault. bearer_token_provider= accepts a native
RefreshingTokenProvider around a token source you write, or any callable or
object that returns the current token. They follow the cross-language
dynamic-credential specification shared with the Java client; see Rotating
Bearer tokens.

Merge order

This branch temporarily carries the UUID / fixed-size-binary changes that belong
to #140 (a363777). They are here because bumping the c-questdb-client
submodule past questdb/c-questdb-client#186 — which added reverse_uuid_bytes
and has been on main since 18 Aug — obliges the Python side to supply
canonical RFC 4122 bytes; without that adaptation every UUID would be silently
stored byte-reversed. The QWP wire bytes themselves are unchanged and still
match the Java client.

#140 should land first. This branch's copy then collapses into the merge
base and this PR reduces to the OIDC and token-provider features alone. This
branch currently documents the combined changes under 6.0.0, with a
5.0-to-6.0 migration guide; reconcile those headings when merging #140.

User-visible breaking changes

The 5.0-to-6.0 migration guide records eleven changes that may require action:

• UUID values use canonical RFC 4122 bytes on every API boundary, including
  Arrow-backed query results; remove any application-side byte reversal.
• Fixed-size-binary columns no longer claim UUID/LONG256 from width alone;
  use the arrow.uuid label, preserved whole-table/stream Arrow field
  metadata (questdb.column_type=uuid or questdb.column_type=long256),
  or schema_overrides as documented.
• A two-item schema_overrides tuple is accepted only for
  ('geohash', bits); values such as ('symbol', 16), whose argument was
  previously discarded, must become 'symbol'. The public
  questdb.SchemaOverride alias describes accepted values.
• Callback inboxes, including the error_inbox_capacity config-string key,
  reject capacities above 65,536.
• Config strings above 1 MiB (UTF-8 bytes) are rejected before parsing, with
  QuestDBError(InvalidApiCall), on every entry point: Sender.from_conf,
  Sender.from_env / QDB_CLIENT_CONF, questdb.connect and
  QuestDB.from_conf.
• Committing a SenderTransaction after its sender was closed now raises
  QuestDBError(InvalidApiCall) instead of an internal TypeError. A commit
  whose flush fails leaves the sender's buffer empty and the transaction
  completed, exactly as in 5.0 -- that is not a change.
• When warning filters promote an abandoned QueryResult's ResourceWarning
  to an exception, the finalizer now lets it reach sys.unraisablehook instead
  of swallowing it, after releasing the cursor. Otherwise normal warning filters
  apply as before. Close results deterministically (preferably with with).
• Interpreter shutdown suppresses new OIDC callbacks and drains callbacks
  already admitted, including those paused before acquiring the GIL. Renderer
  and persistence-warning callbacks must return promptly: a blocked callback
  can delay process exit, even on a daemon sign-in thread.
• A query failover that runs out of failover_max_duration_ms while attempts
  remain, after a last round rejected on role, at the WebSocket upgrade or at
  TLS, now raises that RoleMismatch / HandshakeError / TlsError instead
  of the connection failure that started the failover.
• QuestDB.close() raises QuestDBError(InvalidApiCall) instead of waiting
  while the calling thread holds a pool operation or lease. That includes a
  sender lease handed to a worker that has not used it yet, which 5.0 waited
  for; have the worker use the lease before closing on the borrowing thread.
• A sender's own callbacks can no longer append to it: called from the
  sender's own connection_listener or error_handler, row(),
  dataframe(), new_buffer(), establish(), transaction() and the
  SenderTransaction operations raise QuestDBError(InvalidApiCall), as
  flush() and close() already did. 5.0 accepted row() there; hand
  listener events to another sender or to the owning thread instead.
• A QWP/WebSocket sender that has connected no longer stops when a server
  rejects its credential with HTTP 401 or 403 on a reconnect, a fixed
  token= or username=/password= included: it keeps reconnecting with
  capped backoff, keeps its unacknowledged rows, and reports each attempt as
  ConnectionEventKind.AuthFailed. 5.0 stopped the sender at the first
  rejection and raised AuthError from its next call. A connection that is
  waited for -- a sender's first connection, an eager questdb.connect --
  still fails at once. Code that relied on the sender stopping must close it
  itself.

See CHANGELOG.rst and docs/migration.rst for exact affected entry points and
migration examples.

Usage

Interactive sign-in is explicit. sign_in() is the only operation that may
prompt or open a browser; token(), headers(), and transport connect/reconnect
paths only use cached or silently refreshable credentials and otherwise raise
OidcInteractionRequired.

import questdb
from questdb.auth import OidcDeviceAuth

auth = OidcDeviceAuth.from_questdb(
    "https://questdb.example.com:9000")
auth.sign_in()

with questdb.connect(
        "wss::addr=questdb.example.com:9000;",
        oidc_auth=auth) as db:
    result = db.query("select * from trades limit 10")
    with db.sender() as sender:
        sender.row(
            "events", columns={"value": 42},
            at=questdb.ServerTimestamp)

with questdb.Sender.from_conf(
        "https::addr=questdb.example.com:9000;",
        oidc_auth=auth) as sender:
    ...

questdb.connect, Sender, and Sender.from_conf retain the shared native
provider and request a current token on every connect or reconnect. A fixed
token= is not required and is mutually exclusive with oidc_auth=.

For PG-wire, sqlalchemy_engine() injects a current token as the _sso
password on each new pooled connection, while psycopg_connect() captures the
current token for one connection. Other clients can use auth.token() or
auth.headers() after explicit sign-in.

Configuration and lifecycle

• OidcDeviceAuth.from_questdb() discovers configuration from QuestDB's public
  /settings endpoint; explicit constructor arguments can override discovery,
  and the direct constructor supports fully explicit IdP endpoints.
• groups_in_token=True selects the ID token; otherwise the access token is
  used. It does not modify scope: the configured scope is sent verbatim on
  the initial device-authorization request, including in groups mode. Refresh
  requests intentionally omit scope, matching the Java client, so the IdP
  preserves the scope originally granted. Include openid explicitly when the
  identity provider requires it to issue an ID token.
• Credentials are in memory by default. On supported Unix platforms,
  FileTokenStore.at_default_location() enables opt-in native persistence with
  atomic files, owner-only permissions, and cross-process refresh coordination.
  On non-Unix platforms the bundled store rejects mutation-required operations
  before creating credentials or transient coordination metadata; use
  memory-only authentication there. Custom Python token stores are not
  supported (the native Rust API alone accepts a custom/keychain-backed store).
• The default renderer uses rich Jupyter output in ZMQ kernels and plain text in
  terminals, with optional QR output. Renderer callbacks receive bounded,
  native display-normalized but still untrusted text. Custom renderers must
  encode it for their output sink and use only the native-vetted
  browser_target for actionable URLs.
• Persistence warnings are logged synchronously on the questdb logger,
  often on a token-refresh thread. A QWP/WebSocket ACK wait made inside such a
  handler raises OidcInteractionRequired promptly once its sender's
  reconnect needs a token the provider cannot supply until the handler
  returns; the frame stays queued. ACK waits on other threads are not
  rejected: they wait for the handler to return.
• close() permanently closes a provider and cancels a device flow, refresh
  coordination, or token-store lock wait running on another thread; it is safe
  from any thread, including a renderer callback, so a notebook "Cancel" button
  can call it. OidcDeviceAuth is a context manager. Ctrl-C during
  sign_in() cancels only the current attempt and raises KeyboardInterrupt;
  the provider and its attached transports remain usable. Operations on a
  closed provider -- including attaching it to a new Sender, pool or reader
  -- raise OidcCancelledError, except clear(), which stays
  available so the persisted credential can still be removed;
  cancel_sign_in(), which remains an idempotent no-op when no flow is
  running; and config, which remains readable.
• Typed failures are exposed through OidcError subclasses, including
  OidcConfigError, OidcNetworkError, OidcInteractionRequired,
  OidcDeviceFlowError, OidcTimeoutError, and OidcCancelledError, all
  QuestDBError subclasses. Their code reports the failing call's native
  category and is not a recovery instruction: with no usable credential,
  auth.token() reports AuthError while an HTTP sender flush reports
  SocketError, yet retrying cannot succeed until someone signs in. Catch
  OidcInteractionRequired before any retry logic keyed on code; its
  acquisition_busy flag tells a wait-and-retry case (another thread is
  acquiring) from one that needs sign_in().
• IdP credential endpoints require HTTPS except for loopback development.
  insecure=True applies only to QuestDB discovery transport. Attach the
  provider to https:: / wss:: transports: over plaintext http:: / ws::
  to a non-loopback host the token travels in cleartext.

The public surface comprises OidcDeviceAuth, OidcConfig, FileTokenStore,
Renderer, the PG-wire adapter functions, and the typed exceptions. There is no
public TokenSet; token() returns the selected token string.

OIDC itself adds no optional Python dependency. SQLAlchemy, psycopg/psycopg2,
qrcode, Pillow (for notebook PNG QR output), and IPython are imported lazily
for their respective integrations.

Rotating Bearer tokens

import questdb
from questdb.auth import ExpiringToken, RefreshingTokenProvider

def fetch_token():          # any OAuth2 flow, Azure Identity, a secrets vault
    grant = issue_token()
    return ExpiringToken.expires_in(grant["access_token"], grant["expires_in"])

with RefreshingTokenProvider(fetch_token) as tokens, \
        questdb.connect("wss::addr=questdb.example.com:9000;",
                        bearer_token_provider=tokens) as db:
    ...

• bearer_token_provider= works on questdb.connect, QuestDB.from_conf,
  Sender, Sender.from_conf and Sender.from_env, and is mutually
  exclusive with oidc_auth=, token=, username= and password=.
  Passing an OidcDeviceAuth there raises TypeError pointing at
  oidc_auth=, which is unchanged.
• questdb.auth.RefreshingTokenProvider is the native refreshing cache. Its
  source -- a callable or an object with fetch_token() -- returns an
  ExpiringToken, a (token, expires_at) tuple, or any object with token
  and expires_on. Azure Core's AccessToken and AccessTokenInfo are such
  objects, so Azure Identity works without this package importing it. The
  source runs only on the provider's own thread; token(), await_ready()
  and on_rejected() release the GIL.
• Any callable returning a token, or an object with token() and an optional
  on_rejected(token, http_status), works as an application provider. It is
  called on every connection attempt, possibly on a native thread.
• questdb.auth.TokenUnavailableError classifies a source's failure as
  retryable or permanent; any other exception counts as retryable. A
  connection that is waited for retries only a retryable failure, while a
  connected sender keeps retrying either kind. A provider's failure surfaces
  as a TokenUnavailableError, a QuestDBError that keeps the transport's
  error code.
• On a 401 the client tells the provider which token was rejected and retries
  the endpoint once with a new one, unless the WWW-Authenticate challenge
  names an error a new token cannot fix. A rejection that persists is handled
  by phase: a connected sender rides it out (see the breaking change above),
  a store-and-forward slot left by an earlier sender rides it out and is then
  left on disk for the next session, and a query fails, with its pooled
  connection replaced for the next one.
• Native code never holds a Python reference to a source or provider: they
  live in an id-keyed registry owned by their Python wrappers, which clients
  keep alive, and every callback passes the process-wide callback gate that
  is drained before interpreter finalization. A provider is not usable across
  fork(). A provider used over http:: or ws::, an oidc_auth= one
  included, is warned about once per process on the questdb logger.
• No provider is selected from a configuration string: the token_provider
  and azure_* keys are rejected on every protocol, and there is no azure
  extra. The token-provider guide shows Entra ID and OAuth2
  client-credentials recipes in application code instead. This deviates from
  the specification, which proposes token_provider=azure with an optional
  Python extra; the note asking it to make connect-string providers optional
  is still to be raised on feat(qwp): add refreshing token providers for Entra ID authentication java-questdb-client#102.

Documentation and example

• Adds the OIDC guide in docs/auth.rst and API reference entries in
  docs/api.rst.
• Adds the rotating Bearer token guide in docs/token_providers.rst, with
  Entra ID and OAuth2 client-credentials recipes and a section on rejections
  that persist, plus API reference entries.
• Updates installation guidance and the changelog.
• Adds examples/oidc_device_auth.py, including native transport, PG-wire,
  manual-token, and opt-in persistence examples. The example requires a live
  OIDC-enabled QuestDB Enterprise deployment and a human sign-in, so it is not
  part of the automated example suite.

Test scope

test/test_auth.py contains the Python binding/integration test suite and
requires the compiled questdb._client extension. It is imported by
test/test.py for the aggregate CI run.

The focused tests cover:

• constructor/config validation, documented optional values, native config
  views, typed errors, and the interactive/non-interactive lifecycle split;
• terminal versus ZMQ renderer selection, native-vetted browser targets,
  renderer interface validation, and provider/renderer ownership and GC;
• FileTokenStore acceptance and default-location configuration;
• provider attachment and lifetime for Sender, Sender.from_conf, and lazy
  questdb.connect, including invalid types and fixed-token conflicts;
• preservation of typed OIDC errors through DataFrame auto-flush;
• psycopg and SQLAlchemy token injection plus rejection of unsafe adapter URLs
  before token retrieval or connection creation;
• the cancellation lifecycle: cross-thread close(), closing from a renderer
  callback, Ctrl-C during sign_in(), clear() after close(), and
  rejection of an explicitly closed provider at attach;
• interactive-context detection (ZMQ kernel subclasses such as Colab and
  Spyder, and stderr-vs-stdout redirection), token-store path resolution, and
  propagation of the native error code onto OidcError.

These Python tests use OidcTestServer to simulate discovery, device
authorization, polling, token acquisition, and refresh. The human-in-the-loop
live-IdP example remains intentionally outside the automated suite.

test/test_token_provider.py, also imported by test/test.py, covers the
token providers:

• ExpiringToken forms and validation, and TokenUnavailableError
  classification;
• RefreshingTokenProvider: every source shape, classified and unclassified
  failures, a forced refresh through on_rejected, close() failing every
  holder, the GIL released while waiting, the source released with the
  provider, argument validation, and the protocol classes;
• attaching a refreshing provider, a callable and an application provider to
  a Sender and to a pool against a QWP/WebSocket mock that checks the
  Authorization header: a 401 heard by on_rejected and retried once, a
  permanent failure ending a synchronous connect and a retryable one
  retried, conflicting arguments, the reserved configuration keys, the
  holder's lifetime, and the cleartext warning;
• that a sentinel token never appears in the repr of a token, a provider
  or its status.

Additional maintenance

• Keeps 32-bit wheel jobs on the pandas 2 / NumPy 1 path because pandas 3 has no
  32-bit wheels, and makes the parquet dtype expectation compatible with pandas
  3's string dtype.
• Supplies the module-access flags needed when the QuestDB-master CI leg runs on
  JDK 25.
• Suppresses expected mock-server disconnect tracebacks without hiding unrelated
  server errors.
• Removes an obsolete Python 3.8 cibuildwheel comment.

Interactive detection and browser opening

sign_in() prompts by default, wherever it is called from. There is deliberately
no TTY check: a missing TTY is not evidence of a missing human, and refusing on
it turned away sign-ins that work — prog 2>&1 | tee log at a real terminal, a
process supervisor, an IDE run configuration. The one exception is a notebook
executed headlessly (papermill, nbclient, nbconvert --execute), whose kernel
states at protocol level that it accepts no input; there sign_in() raises
OidcInteractionRequired immediately. Pass interactive=False to get that
fail-fast behaviour anywhere else, such as cron or CI.

open_browser is tri-state for the same reason: the default None opens a
browser except inside a Jupyter kernel, where it may be on a different machine
from the reader; True opens one anyway — correct for a local jupyter lab,
where the previous unconditional suppression was simply wrong and had no
override — and False never does.

Submodule pin

This branch pins c-questdb-client at 3dc38c73896fa05623aba1c3870888cd5c7a2743,
the current head of questdb/c-questdb-client#183. It adds the refreshing token
providers and phase-specific credential-failure handling on top of the OIDC
integration. Compared with the earlier c3c58866 pin, it also includes the C++
test and gitleaks updates and a merge of native main (6317e5d3, #195:
maximum GEOHASH values and Arrow FFI validation). The two must merge together:
line_sender.pxd binds
that PR's oidc.h, client.h and token_provider.h surface and does not
compile against the current main. That pin also carries changes to API
already present on native main: ReaderConfig::upgrade_headers now returns
Result<Vec<_>> rather than Vec<_> for direct Rust callers, callback inbox
capacities are capped at 65,536, config strings are capped at 1 MiB, a
QWP/WebSocket sender that has connected rides out an HTTP 401 or 403 instead
of becoming terminal, and the reserved token-provider configuration keys are
rejected on every protocol. All but the Rust signature change reach Python;
CHANGELOG.rst documents the caps and the 401/403 change under Breaking
changes and the reserved keys under Other changes. The TTY behavior and
non-copyable C++ device_auth concern APIs introduced inside the native PR
itself, so they do not break a pre-existing API.

[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.

• Last reviewed commit: ae92178
• Latest commit, not reviewed: cea1fe9

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

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		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
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes the primary changes: adding OIDC authentication and rotating bearer-token support.

✨ 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).}

[glasstiger]

Tandem review: py-questdb-client #133 + c-questdb-client #183

Level 3, multi-agent, adversarial. These two PRs are one change: py #133 adds the native-backed OIDC device-flow binding and pins the c-questdb-client submodule at exactly c #183's head (91b516a2), whose Rust engine + C ABI it binds through oidc.pxi and line_sender.pxd. Reviewed as a pair across ten parallel agents (correctness, Cython memory/refcount/crash surface, C-ABI/FFI panic-safety, GIL/concurrency, resource lifecycle, performance, tests, API/stubs/docs, cross-context caller impact, and a fresh-context adversarial pass), with every finding verified against source on both sides of the ABI.

Verdict: Approve. No Critical issues — no memory-unsafety, refcount imbalance, double-free, GIL violation, ABI mismatch, auth-bypass, secret-leak, or terminal-injection. This is an unusually well-hardened change. The two Moderate items are now resolved on the branch; the remainder are Minor.

---

✅ Resolved in fa924d7

• M1 — OIDC error path dropped in_doubt. The OIDC branch of c_err_to_py/c_err_to_py_fmt returned an OidcError that always reported in_doubt=False, dropping the native flag the non-OIDC path propagates (c_err_to_fields → in_doubt=tup[3]). Not reachable today (OIDC errors classify BeforeWrite → NotDelivered, never combined with the DuringWrite/ACK-wait failures that set in_doubt=true), but a latent data-duplication risk for an except QuestDBError retry/dead-letter handler if the native classification ever raised an OIDC error after a partial write. Fixed: _oidc_err_to_py_unowned now reads questdb_error_in_doubt(err) and threads it into every constructor; OidcError/OidcDeviceFlowError accept and forward in_doubt to the QuestDBError base. Regression test added.
• M2 — Missing Python-side from_questdb discovery tests. Three documented cross-context contracts had zero Python coverage. Added: groups_in_token inherit-vs-override, explicit-kwarg override of discovered values (the builder-override-after-discovery path, previously only ever run with None), and the non-OIDC-server → OidcConfigError mapping (the chore: add requirements.txt and review DEV_NOTES.md #1 real-world discovery failure). Full test_auth.py suite passes (109 tests).

---

Minor (non-blocking)

1. Reentry guard over-rejects benign concurrent calls — c #183, diagnostic-quality. SharedOidcAuth::{token,sign_in,clear} check a process-global per-handler AtomicBool before the cache fast-path (questdb-rs-ffi/src/oidc.rs:206-238), so a concurrent token() from a background flush thread — not reentrant at all — is rejected with InvalidApiCall("...re-entered from its event callback") whenever any renderer callback is momentarily active. The pure-Rust core does this correctly with a non-blocking try_lock → InteractionRequired. Low impact (the active window is one brief callback invocation, and the transport path self-heals via retryable-error reclassification), but the error class/message is wrong for a benign concurrent call. Consider mirroring the core's try_lock semantics.
2. clear() blocks for the whole device flow — c #183 core. clear() → try_clear takes the blocking lock an in-progress sign_in() holds for the entire flow, so auth.clear() on another thread (the natural "abort a stuck sign-in" gesture) blocks until the code expires instead of cancelling. Consider documenting that clear() is not an abort mechanism.
3. Finalization TOCTOU in the trampolines — py, pre-existing pattern, accepted. _oidc_event_trampoline/_oidc_user_data_release_trampoline check qdb_py_is_finalizing() then acquire the GIL; a shutdown starting in between can hang on a native release thread. Identical to the already-shipped connection-event/sender-error trampolines; the standard best-effort mitigation with no better C-ABI-layer fix. Deterministic close() before teardown avoids it.
4. OidcConfig has no class docstring (src/questdb/auth/_config.py) — the only public questdb.auth class that renders as a bare signature in the API docs.
5. CHANGELOG.rst omits Sender.from_env from the oidc_auth=-accepting list (it forwards oidc_auth= to from_conf). Incompleteness, not a wrong claim.
6. Narrower test gaps — the OidcDeviceFlowError non-str field-coercion defense is only reachable by a direct unit test; the QR renderer path (qr=True), the insecure/ca_bundle flag effects, and adapter OidcInteractionRequired propagation are untested; error-view status/retry_after carry-through is asserted only for the NETWORK/DEVICE_FLOW kinds.
7. Doc/stub cosmetics — examples/oidc_device_auth.py isn't surfaced in docs/examples.rst; :mod:questdb.auth`` won't render as a link (no .. py:module:: directive; harmless, `nitpicky` off); minor stub docstring drift; the `.pyi` imports OIDC types from private `.auth._*` modules rather than the public re-export.

---

Investigated and cleared (false positives / non-issues)

• PyWeakref_GetRef on the >=3.10 floor — not a build-breaker. This PR replaces PyWeakref_GetObject with PyWeakref_GetRef (CPython 3.13+), but Cython 3.2.5 ships an inline __Pyx_PyWeakref_GetRef backport over PyWeakref_GetObject for Python < 3.13, and both call sites preserve the -1/0/1 tri-state despite the bint binding.
• 196 Rust OIDC tests silently not running in CI — they run. oidc is in almost-all-features and forced on by the FFI crate; ci/run_all_tests.py runs cargo test --features=almost-all-features[,...].
• c_err_to_py OIDC branch double-free/leak — err is freed exactly once on both branches (mutually exclusive via early return); typedef line_sender_error questdb_error makes the type-punning correct. ~200 callsites of the shared error helpers verified unaffected (the native gate returns false for every non-OIDC error, so the contract change is invisible to pre-existing callers).
• Also verified: all four OIDC structs + both enums match .pxd ↔ header field-for-field; symbol_dict_full=37 consistent with an updated exhaustiveness test; the _finish_builder weakref refcount balances on all paths (Py_DECREF(<object>user_data) compiles to exactly one net decref); token() frees on the decode-exception path; the FFI is panic-free on caller-reachable paths under panic=abort (NULL-guarded, 1 MiB input cap, checked UTF-8); the C++ RAII layer and Rust token-store lock/temp lifecycles are sound; display fields are sanitized on both the native and Python sides; the .pyi stub matches the implementation.

---

Summary

• No regressions. The change is purely additive; the OIDC branch is gated by a native getter that returns false for every pre-existing error type, so the blast radius on the shared c_err_to_py helpers is nil. token=/oidc_auth= mutual exclusion is native-enforced at both set-time and build-time. The PyWeakref_GetObject → PyWeakref_GetRef swap is a refcount-correct improvement.
• Tandem coherence. The .pxd ↔ oidc.h/client.h binding matches the pinned c #183 headers exactly (31 functions, 2 enums, 4 structs), so the two must merge together — the submodule bump in feat(auth)!: add OIDC and rotating bearer tokens #133 is meaningless without #183, and the binding would not compile against the old headers.
• Both Moderate items resolved in py feat(auth)!: add OIDC and rotating bearer tokens #133 fa924d7; the remaining Minor items (incl. the two c #183 diagnostic/UX items) are non-blocking.

_{🤖 AI-assisted multi-agent review (Claude Code, level-3 tandem). Findings verified against source, but please sanity-check before acting.}

[glasstiger]

Note: this PR temporarily carries UUID / fixed-size-binary changes that belong to #140.

Commit a363777 adapts the Python side to canonical RFC 4122 UUID byte order and drops the width-based UUID/LONG256 inference. That work is owned by #140 (Fixes #138), which documents it properly under a new 5.1.0 → Breaking changes heading, with a migration guide and the exact to_bytes(16, 'little') → .bytes before/after.

It appears here because this PR bumps the c-questdb-client submodule past questdb/c-questdb-client#186, which added reverse_uuid_bytes and has been on main since 18 Aug. Any bump past that commit must carry the Python adaptation: without it Python emits reverse(UUID.bytes), native reverses it again, and every UUID is silently stored byte-reversed. So it cannot be deferred to a follow-up — it is a prerequisite, and #140 is that prerequisite.

To be precise about what did and did not change: the QWP wire bytes are unchanged and still match the Java client — QwpColumnWriter stores UUIDs "lo+hi contiguously, matching wire order" and QwpBindValues.setUuid writes putLong(getLeastSignificantBits()); putLong(getMostSignificantBits()). Before and after, the wire carries reverse(UUID.bytes); the swap simply moved from Python into native. uuid.UUID columns and query binds are unaffected. Only the raw-byte input contract moved.

Merge order: #140 should land first. This branch's copy then collapses into the merge base and this PR's diff reduces to the OIDC feature alone.

Watch the CHANGELOG.rst conflict. This branch had written the UUID notes into the already-published 5.0.0 section, rewriting the history of a shipped release; I have reverted that. #140's version, under a new 5.1.0 Breaking changes heading, is the one to keep when the two branches meet.

[glasstiger]

/azp run questdb.py-questdb-client-release

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[glasstiger]

superseded by #165