Repository navigation
feat(qwp): authenticate third-party browser apps with a subprotocol credential - #68
Conversation
…redential A browser cannot set Authorization on a WebSocket upgrade, and QuestDB ignores cookies on a cross-origin QWP upgrade, so a web application served from another origin had no way to authenticate. QuestDB now accepts a credential in the upgrade's subprotocol offer from origins listed in qwp.browser.allowed.origins (questdb/questdb#7683, questdb/questdb-enterprise#1246). The new `auth` option on QwpBrowserWebSocketOptions and QwpBrowserClusterOptions takes a fixed credential, or a function the client calls before every connect, reconnect, and failover attempt. The function form lets a long-lived session pick up a refreshed OIDC access token rather than reconnect with an expired one. `auth` cannot be combined with sessionBootstrap. The credential travels as questdb.qwp.authorization.<credential>, the unpadded base64url encoding of its Authorization value: `+`, `/`, and `=` are not token characters, so the padded base64 used for Basic cannot be offered. It is paired with the dialect QuestDB selects -- durable ACK when requested, questdb.qwp.v1 otherwise -- because QuestDB refuses a credential offered without one. The query path now builds its offer the same way instead of passing protocols through. Without `auth` the offer is unchanged: a browser fails a handshake whose response selects none of its offers, so adding questdb.qwp.v1 would break older servers. A server that selects the credential has echoed the secret in its 101 response. QuestDB never does, so the client treats it as a server defect and fails the connection without retrying or walking the endpoint list.
"retires a wholly deferred recovered transaction without replaying it" waited for the journal's .sfa segments to disappear and then checked for .ack-watermark with a single readdir. The store removes both, but not together: background maintenance unlinks the segments first and deletes the watermark only once that deletion is durable, after an ownership check and a directory sync. A check that landed between the two found the watermark still present, as seen on CI (Node 20). A 100ms delay after that sync reproduces the failure deterministically. Wait for the watermark the way the test already waits for the segments. The store's ordering is deliberate -- the watermark is what stops a crash in that window from recovering the discarded frames -- so it is unchanged.
"can ingest data via TCP and run queries" failed on main (Node 20) with a 400 from its first query. The container log shows why: QuestDB answered "table does not exist [table=test_tcp]" 5ms after tables() had listed the table. ILP over TCP has no acknowledgement, so the test waits for the table to appear in tables(), but QuestDB lists a new table there before its name resolves for queries: registerName() hydrates the metadata cache that tables() reads, syncs the name to the table registry, and only then makes the name queryable. A query in between gets "table does not exist", and query() failed on any non-200 without retrying. runSelect() now treats that one answer as "not yet" and keeps polling, reporting the last such error if it times out. Any other non-200 still fails at once, now with the server's error message, which the job log lacked. This covers the three TCP tests that share the pattern; the HTTP tests are not exposed, because an HTTP flush resolves only after table creation has completed.
|
Level-3 review verdict: approve (reviewed Test gate: passed; admitted coverage gaps: 0. Full Vitest suite: 1,141 passed (using the unchanged interop submodule fixture in a disposable worktree). Browser E2E: 10 passed; dist tests: 39 passed. Typechecks, ESLint, formatting, and package checks passed. The browser E2E server is a fixture, not the tandem QuestDB server; live server interop was not run in this review. The credential subprotocol is base64url-encoded, not encrypted: use |
Level-3 review: approveReviewed
Validation ran in disposable worktrees, which were removed afterward. The primary working tree was unchanged. |
Level-3 review: approveReviewed
What was checked
Validation (at head, in disposable worktrees)
Tradeoffs (declared in the PR)
Limitations
|
Follow-up: live interop against QuestDB EnterpriseThis covers the limitation noted in the earlier review: the client was not yet tested against a live server. The browser client at Setup
Results
Checks that held in every scenario
Observations
Not testedOIDC tokens (not configured), |
bluestreak01
left a comment
There was a problem hiding this comment.
Review of PR #68: feat(qwp): authenticate third-party browser apps with a subprotocol credential
Verdict: approve with comments. No Critical findings; one Moderate documentation issue below. Reviewed at level 3 (as requested): $BASE a658130 (current origin/main) against $HEAD 3c4515d.
PR description: follows the conventions. Commit subjects are Conventional Commits, the user impact is clear, the tandem server PRs are linked, and the behaviour changes and tradeoffs are spelled out. The two test-only flake fixes are disclosed, and I reproduced both. No supported input changes a published API's errors or wire bytes, so no ! marker is needed.
Submodules: none.
Critical
None.
Moderate
M1. Root README.md and QWP.md keep cross-origin cookie advice that the PR's new text contradicts
- Problem: two docs still recommend credentialed CORS for cross-origin cookie auth.
- Net impact: anyone setting up a cross-origin browser deployment from the root docs builds one that cannot authenticate.
- Evidence: read directly from the docs at 3c4515d; an independent falsifier confirmed it.
Where it is:
- Stale sentences:
README.md:424-427: "The REST and WebSocket routes should be served from the same browser origin (or configured with credentialed CORS)…"QWP.md:974-975: "…or correctly configured credentialed CORS and cookie attributes."
- New text in the same sections that contradicts them:
README.md:430-432: "QuestDB ignores cookies on such a cross-origin upgrade."QWP.md:1002-1003: "…so session bootstrap cannot authenticate it."
Why it is wrong:
- The PR already removed this same sentence from
packages/browser-client/README.md(base lines 208-210, now lines 224-227) and replaced it with "useauthinstead". The two root documents are now inconsistent with that file and with themselves. - The advice cannot work on either server:
- The base server rejects every cross-origin QWP upgrade.
- With questdb#7683, a cross-origin upgrade ignores cookies and returns 401. Its own test,
testQwpBrowserListedOriginIgnoresAmbientCredentials, asserts this. - CORS never applies to WebSocket upgrades, so the "credentialed CORS" clause cannot rescue the WebSocket route.
Classification: in-diff. The sentences themselves predate the PR, but the contradiction comes from the text the PR added, and the PR fixed only one of the three copies.
Suggested fix:
- Replace both clauses with the browser README's wording: serve
/exec,/write/v4and/read/v1from the application's origin or through a same-origin proxy, and useauthfor an application on another origin. - Run
pnpm run docssodocs/media/QWP.mdstays in sync.test/docs-reference.test.tsenforces that.
Coverage map
Test gate: passes. Admitted coverage gaps: 0.
- Mutation testing: I made 17 deliberate breakages to the new auth paths. Each was caught by at least one new test. They covered:
- removing the echo check;
- the retry and endpoint-walk flags;
- always adding
questdb.qwp.v1; - offering durable ACK on egress;
- padded base64url;
- skipping each validation call;
- caching the provider result;
- passing the provider an unrelated signal.
- Encoding: I compared 600 randomised Basic and bearer credentials, including multi-byte UTF-8, against Node's base64 and the server's decoding rules. There were no mismatches. The
bootstrapQwpBrowserSessionheader is byte-identical to base. - Egress reconnect: there is no dedicated test, but I probed the head build. A provider that fails once is retried and the session reconnects. A provider that returns an invalid credential stops the reconnect loop.
- Flake fixes:
reconnect.test.ts: I added a 100 ms delay before.ack-watermarkis deleted. The base test then fails and the head test passes.sender.integration.test.ts: 10 of 10 tests pass against a QuestDB container.
Summary
All the CONTRIBUTING.md checks pass at head:
eslint,format:checkand the source and test type checks;- the bench type check and lint;
- the unit suite: 1119 pass, and the one failure is environmental (the
questdb-client-testsubmodule wasn't initialised in my scratch checkout; that ILP interop test is unrelated to this PR); test:distandtypecheck:dist, including TypeScript 4.9;check:packages;- the real-Chromium browser e2e suite, 10 of 10.
Behaviour for existing users: unchanged.
- Runtime exports: both packages export the same names in ESM and CJS at base and head.
- New type exports:
QwpBrowserAuthContextandQwpBrowserAuthProvider, both browser-only. - No
authoption: the subprotocol offer is the same as on base.
Not a regression: with failoverUrls, a token provider that hangs past connectTimeoutMs is called once per endpoint, and the failure reports timeouts rather than an auth problem. This is documented, the PR tests it deliberately, and a hung sessionBootstrap behaves the same way on base.
Findings:
- Admitted: 1 in-diff, 0 out-of-diff breakage.
- Severity: 0 Critical, 1 Moderate, 0 Minor.
Residual risk: I checked the client against the tandem server diff at questdb#7683 head 8da9ba8, which is still unmerged. If that PR changes how the server chooses the subprotocol or decodes the credential before it merges, recheck the offer builder.
Main's browser credential authentication (#68) changed packages/browser-client/src/index.ts, which on this branch only re-exports; the implementation moved to qwp.ts, so the change is merged there instead. auth is cluster-owned like url, failoverUrls and sessionBootstrap, and main's credential tests use this branch's single-options-object API. docs/ keeps this branch's pages until the next commit regenerates them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Tandem
Description
Let third-party web applications, served from an origin other than QuestDB's, authenticate browser QWP connections. A browser cannot set
Authorizationon a WebSocket upgrade, and with the tandem server change QuestDB ignores cookies on a cross-origin QWP upgrade. It instead accepts a credential in the subprotocol offer from origins listed inqwp.browser.allowed.origins.authoption onQwpBrowserWebSocketOptionsandQwpBrowserClusterOptions. It takes a fixedbearerorbasiccredential, or aQwpBrowserAuthProviderfunction that the client calls before every connect, reconnect, and failover attempt, passing anAbortSignalthat fires when the attempt is abandoned. The function form lets a long-lived sender or query session pick up a refreshed OIDC access token instead of reconnecting with an expired one.authcannot be combined withsessionBootstrap; in the unified client it is configured once, undercluster.client-core/_qwp/_core/durable-ack.ts,connectQwpBrowserEndpoint). Withauth, the offer is the caller's protocols, then durable ACK when requested orquestdb.qwp.v1otherwise, thenquestdb.qwp.authorization.<credential>: the unpadded base64url encoding of theAuthorizationvalue. The padded base64 used for Basic cannot be offered, because+,/, and=are not token characters. The query path now builds its offer the same way instead of passingoptions.protocolsthrough.QwpUpgradeError(capability-mismatch,tryNextEndpoint: false) and does not walk the endpoint list.QwpUpgradeErrorof kindauthenticationwhosecauseis the thrown value, without trying the remaining endpoints; reconnects retry it unless it carriesretryable: false. An invalid credential, fixed or returned by a provider, is rejected before any socket opens; bearer tokens must be visible ASCII. No error quotes the credential.Behavior changes and tradeoffs
authnothing on the wire changes.questdb.qwp.v1is offered only alongside a credential: a browser fails a handshake whose response selects none of its offers, so offering it unconditionally would break servers that predate it.auth, aquestdb.qwp.authorization.*token among the caller'sprotocolsis rejected, because QuestDB refuses an offer carrying two credentials.sessionBootstrap.authenticationnow fails with aTypeErrorthat names the option, rather than one from readingtypeofundefined. The othersessionBootstrapmessages are unchanged.authoption and theQwpBrowserAuthProviderandQwpBrowserAuthContexttypes. Runtime exports are unchanged; the_corebarrel now names the durable-ACK exports so the credential helpers stay internal.capability-mismatchkind rather than adding aQWP_UPGRADE_ERROR_KINDmember.Tests
session.test.ts: offer contents, decoded by QuestDB's rules, for bearer, Basic, non-ASCII Basic, durable ACK, and the query path; the provider called on every connect, failover, and reconnect attempt; the connect deadline and session close aborting a pending provider; provider failures and invalid credentials; conflicting options at every entry point; cluster sharing and per-side overrides; credential echo on ingress and egress.core.test.tscovers the offer builder.browser.e2e.ts, in real Chromium: a cross-origin page authenticates ingress and egress through a provider with a JWT-sized token, and a server that echoes the credential is refused.be8556a) with Basic authentication and the page's origin allow-listed, driven by real Chromium: a pooled client wrote rows and queried them back; durable ACK plus a credential passed the credential gate; a wrong password, an unlisted origin, and a missing credential got 401; the 101 named onlyquestdb.qwp.v1and set no cookie; the credential never reached the server log; and a reconnect whose first attempt offered an expired credential (401) recovered on the provider's next call.docs/is regenerated in the same commit.Also included
test(qwp): wait for the ACK watermark in the deferred-recovery testfixes a CI flake unrelated to this feature, seen onmain(run 36466605724, Node 20).retires a wholly deferred recovered transaction without replaying itchecked for.ack-watermarkwith a singlereaddir, but background maintenance deletes the watermark only after the segment files the test waited for, and after a directory sync. The test now waits for it; a 100 ms delay after that sync reproduced the failure deterministically and no longer fails it. Test-only; the store is unchanged.test: retry integration queries that race QuestDB table creationfixes a second CI flake unrelated to this feature, seen onmain(run 36716864105, Node 20).can ingest data via TCP and run queriesgottable does not exist [table=test_tcp]from QuestDB 5 ms aftertables()had listed the table: QuestDB lists a new table there before its name resolves for queries, and ILP over TCP has no acknowledgement to wait for.runSelect()now treats that one answer as "not yet"; any other error still fails at once, now with the server's message. This covers the three TCP integration tests. Test-only.