Skip to content

feat(qwp): support browser negotiation and session authentication - #7531

Merged
bluestreak01 merged 61 commits into
masterfrom
ia_node_qwp
Sep 15, 2026
Merged

bluestreak01 merged 61 commits into
masterfrom
ia_node_qwp

Conversation

@glasstiger

@glasstiger glasstiger commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • allow same-origin browser WebSocket upgrades for QWP ingress and egress
  • authenticate browser connections through the existing qdb_session cookie flow
  • negotiate durable ACKs, result compression, ingress batch caps, and egress batch-row limits through URL parameters and QWP messages when browser WebSocket APIs cannot send custom upgrade headers
  • retain the existing header-based negotiation path for Node.js and other non-browser clients

This is the server-side companion to the JavaScript QWP client. The new negotiation fields are optional, so older clients continue to use the existing behavior.

How browser negotiation works

Browser JavaScript cannot set custom upgrade headers or read the upgrade response, so each capability gains a browser-safe carrier alongside the existing header.

Capability Non-browser carrier Browser carrier
Durable ACK request (ingress) X-QWP-Request-Durable-Ack header Sec-WebSocket-Protocol: questdb.qwp.durable-ack.v1
Ingress batch cap (server → client) upgrade response header qwp_browser_handshake=v1, or the durable-ACK subprotocol, → STATUS_SERVER_INFO frame
Egress compression request X-QWP-Accept-Encoding header qwp_accept_encoding URL parameter
Egress compression result X-QWP-Content-Encoding response header CAP_COMPRESSION plus codec/level in the SERVER_INFO frame
Egress batch-row preference X-QWP-Max-Batch-Rows header qwp_max_batch_rows URL parameter

Precedence differs per capability and is not a header-first fallback in any of them. Durable ACK is enabled by either carrier. The compression preference takes the URL parameter whenever it is present, so an intermediary cannot override the browser's own choice by injecting the header. The batch-row limit applies both carriers and keeps the stricter value, for the same reason. Out-of-range batch-row values are clamped to the server-authoritative cap rather than failing the handshake, so one buggy client cannot break the upgrade.

Origin handling is the CSWSH control: RFC 6455 browsers always send Origin and cannot let JavaScript remove it, while machine clients normally omit it. An upgrade carrying an Origin that is not same-origin with the request Host is rejected; an upgrade with no Origin is unaffected. The origin's scheme must also match the connection's security, so an http:// origin over TLS and an https:// origin over plaintext are both refused.

qwp.browser.tls.termination.enabled (new, default false) covers the topology where a reverse proxy terminates TLS and the QuestDB connection itself is plaintext: it makes the origin check treat the connection as secure, so an https:// origin is accepted and a plaintext one is still refused. The terminator must forward the browser's original Host verbatim, port included. The flag governs the origin check only and never marks the qdb_session cookie Secure; the server.conf block states the full precondition.

Session authentication reuses the existing cookie handler: HttpCookieHandler gains getSessionCookieValue(), and the ingress and egress upgrade responses, along with the 421 role reject, emit Set-Cookie when a session is created or rotated, so a browser that logged in over HTTP carries qdb_session into the WebSocket.

Coverage

  • browser origin and session-authentication tests
  • reject-response delivery under send fragmentation
  • ingress capability and batch-limit negotiation tests
  • egress query-flag and compression negotiation tests
  • WebSocket handshake coverage

Related work

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 5d48bd59-1b0f-45e4-9f09-1aaca7320620

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

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

@glasstiger

Copy link
Copy Markdown
Contributor Author

/azp run macwin

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@glasstiger

Copy link
Copy Markdown
Contributor Author

Tandem review completed at level 3 together with questdb/questdb-enterprise#1175.

Findings

  • Critical: none open
  • Moderate: none open
  • Minor: none open

The browser WebSocket authentication/origin handling, durable-ACK negotiation and polling, SERVER_INFO framing, compression negotiation, caller impacts, resource handling, concurrency behavior, and test efficacy were reviewed across both repositories.

Coverage gate: pass (0 admitted open coverage gaps). git diff --check passes, and the companion Enterprise durable-ACK retention coverage issue was fixed in questdb/questdb-enterprise@0eabda87d1d6569831e8745dae84f4b24bb5b7a3.

Submodule provenance from the Enterprise PR: questdb is OFF-DEFAULT — in scope. Reviewed OSS head: 78f0be76b1cc3650c9f2b46bbde24ac070d0a634.

Verdict: approve.

@glasstiger

Copy link
Copy Markdown
Contributor Author

/azp run macwin

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

glasstiger and others added 2 commits September 9, 2026 22:29
Three guards on the browser negotiation paths had no test that failed
when they were removed. Each new test now does, and each was checked
against the mutation it is meant to catch.

The poll-reject arm of handleBinaryMessage records a deferred
STATUS_PARSE_ERROR when the pending-ACK flush blocks, so the resumed
connection re-sends the refusal. The structurally identical arm at the
tail of the same method already had a test; the poll arm did not.
QwpIngressUpgradeProcessorResumeRecvTest now drives a refused poll on a
blocked socket through the existing fault-injection seam and pins both
the deferred status and the sequence.

onHeadersReady resolves the accept-encoding preference with the URL
parameter winning over the header, which stops a reverse proxy from
overriding the codec a browser asked for. Both arguments are
Utf8Sequence, so swapping them at the call site compiles silently, and
the unit test around negotiateAcceptEncoding pins the function rather
than the call. QwpBrowserNegotiationWireTest now upgrades with both
carriers naming different levels, in both directions, so the result
cannot be read as a higher-wins or lower-wins rule.

writeServerInfoFrame subtracts the compression trailer from the body cap
it hands writeServerInfo, which is what keeps the trailer's two bytes
inside the send buffer once the ids fill that cap exactly. Dropping the
term writes past the end and no round-trip assertion sees it.
QwpServerInfoFrameTest now sweeps every buffer size from below the
minimum to past the natural frame size with a guard region behind the
declared size.

Along the way the tests stop reinventing each other. QwpWireTestFixtures
gains the durable-ack poll frame builder that QwpIngressAckLeapfrogTest
hand-rolled, its handshake helper takes extra request headers and uses
the WEBSOCKET_KEY constant it already exposes instead of recomputing the
nonce, and readServerInfo asserts through assertQwpMessageKind rather
than a weaker inline shape check. Two helper pairs collapse: the
subprotocol handshake assertion subsumes the durable-ack one, whose
eight callers now also pin that a header-only request draws no
subprotocol echo, and a boolean frame scan gives way to the indexed scan
it duplicated.

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

Copy link
Copy Markdown
Contributor Author

/azp run macwin

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@glasstiger

Copy link
Copy Markdown
Contributor Author

Tandem review completed at level 3 across questdb/questdb-enterprise#1175 and #7531.

Reviewed revisions: Enterprise d977d89bce73521b01b90424e76f9953bd5c1512, OSS 38ac0f1604f830cc92ba51c74a617674545102c7.

Findings

  • Critical: none
  • Moderate: 4
  • Minor: 3

Verified green: mvn compile test-compile on both repos; 142 OSS browser/negotiation tests; 58 OSS QWP wire and ack tests; 6 Enterprise browser-session and TLS-origin tests; node --check, python -m py_compile, YAML parse and git diff --check. All zero failures.

The security core holds up under independent re-derivation: the same-origin check, all four TLS-flag combinations, every handshake size-versus-writer byte path, the SERVER_INFO body cap against writeUtf8Truncated, the raw Set-Cookie bytes against HttpResponseSink.setCookie, and the durable-ACK poll ordering against the unresolved-sequence gate.

Omitted as unverified

Three independent passes raised the same candidate: on /read/v1, a request carrying X-QWP-Accept-Encoding but no qwp_accept_encoding URL parameter gets a compressed wire with CAP_COMPRESSION clear and no codec/level trailer. It is omitted because no supported producer exists. Nothing in either repo injects that header, browsers cannot set it, and whether the JavaScript client always emits the URL parameter is not checkable from this tree. The behaviour is deliberate and pinned by testHeaderNegotiatedCompressionLeavesCapCompressionClear. What survives from it is M1 below.

Moderate

M1 [OSS] Javadoc claims a protection the keying does not provide.

core/src/main/java/io/questdb/cutlass/qwp/server/egress/QwpEgressUpgradeProcessor.java:301-305 contrasts keying CAP_COMPRESSION off "the URL parameter's presence" against "which value won", and states the latter "would let an injected header compress the wire while the browser was told nothing". Those are the same predicate: negotiateAcceptEncoding returns the URL value exactly when it is non-null, so "URL won" and "URL present" are identical conditions. The state the sentence warns about is reachable under the implemented keying whenever the parameter is absent and the header is present, because state.setCompression at line 528 is keyed on the negotiated result while line 486 is keyed on the parameter. Either restate the sentence to name the residual honestly, or close the gap.

M2 [ENT] The e2e harness cap is below the budget its own docstring enumerates.

questdb-ent/e2e/lib/javascript_client.py:92-101 says "Above the driver's own worst case, not below it" and then lists 60s, 60s, 60s, 10s and 10s. Those sum to 200 against a 180s cap. The retention monitor overlaps waitForDurable, but the reconnect budget at javascript_durable_ack.mjs:139 does not overlap ackTimeoutMs at line 133, so a run that reconnects once can exceed 180s. That reproduces exactly the generic-TimeoutError masking the cap was raised to eliminate.

M3 [ENT] The retention comment claims a tolerance the assertion does not implement.

questdb-ent/e2e/lib/javascript_durable_ack.mjs:75-82 says the peak and low pair "survives an appearance read that raced a client still writing its second record". With a seed of 1 and a later sample of 2, peak becomes 2 and low stays 1, so the low == peak assertion at tests/test_javascript_client_durable_ack.py:102-110 fails, and its message reads "shrank from 2 to 1" for a journal that grew. Whether one sendTables call can produce more than one .sfa record is a property of the JavaScript client and is not checkable from this tree, so this is reported as the contradiction it is, not as a proven flake.

M4 [OSS] Coverage gap: the browser-only durable-ACK carrier is never driven over a real socket.

Every Sec-WebSocket-Protocol request-header offer in the test tree is set on a MockHttpRequestHeader in QwpIngressUpgradeProcessorOnHeadersReadyTest, or is a containsWebSocketProtocol unit assertion, or asserts the response echo. The qwp_browser_handshake URL carrier is driven end to end by QwpBrowserNegotiationWireTest, precisely because route matching matters; the subprotocol carrier is not, and it is the only carrier a browser has for this capability. If anything between the socket and the header lookup dropped it, the server would echo nothing, the browser would fail the whole connection per the WHATWG algorithm, and every unit test would stay green.

Not Critical: X-QWP-Request-Durable-Ack uses the identical lookup and is exercised over a real socket by the pinned Java client, so the mechanism is proven, and the failure mode is loud rather than silent data loss. The cheap fix is an extraHeaders parameter on QwpWireTestFixtures.performWriteHandshake, which the read-side fixture already has.

Minor

  • Member ordering in the two new files. questdb-ent/src/test/java/com/questdb/acl/QwpEnterpriseBrowserSessionTestSupport.java:238 places readCrLf before openWebSocket at line 279, and loginAndAssumeServiceAccount at line 247 sits between two private statics. core/src/test/java/io/questdb/test/cutlass/qwp/QwpBrowserNegotiationWireTest.java:265 places readCapabilities before assertNegotiatedZstdLevel. Smaller insertions in QwpIngressUpgradeProcessorOnHeadersReadyTest, WebSocketHandshakeTest, QwpEgressCompressionTest, QwpIngressAckLeapfrogTest and DynamicPropServerConfigurationTest land in runs that are already unsorted at base.
  • A private helper restates an existing one. QwpIngressHttpProcessor.java:703 adds startsWithIgnoreCaseAscii, whose comparison is what Utf8s.startsWithLowerCaseAscii at Utf8s.java:1065 already performs. Adoption needs two Utf8String constants. The companion toLowerAscii at line 715 has no accessible equivalent and is still needed by the authority loop, which fuses the case-fold with its character-class rejections.
  • A timing floor uncoupled from the constant it claims to pin. test_javascript_client_durable_ack.py:112 asserts at least 500ms while the throttle is set to 3000ms at line 21, as two independent literals. The clock starts after an unbounded journal-appearance wait, so the floor does not pin the throttle in either direction.

Coverage gate

Pass. Zero admitted Critical coverage gaps, one admitted Moderate gap (M4).

Coverage is unusually strong. Every branch enumerated independently has a dedicated test, including all six equality terms of the poll-frame matcher with the superset and length relaxations pinned, all five character-class terms of the origin loop with length-matched forged hosts so the length guard cannot decide the case first, both directions of the compression and batch-row carrier precedence, and the exact Set-Cookie attribute string on both the 101 and the 421.

Scope notes

Submodule provenance: questdb is OFF-DEFAULT, in scope, present only on origin/ia_node_qwp. The nested java-questdb-client pointer did not move and is out of scope.

The Enterprise pull request contains no Enterprise production code; all five changed Java files are tests. The JavaScript lane is inert for the driver and the pipeline, but the two Python files are collected and imported by the ordinary Enterprise e2e job, so a syntax or import error there would go red on this pull request. Both compile, the driver parses, and both pipeline files parse.

Findings split: 4 in-diff, 0 out-of-diff breakage.

Verdict

ENT: approve. OSS: approve. Both gates pass. The four Moderate items are worth fixing and none of them blocks the merge.

@glasstiger glasstiger added tandem READY PR is ready for the final review labels Sep 10, 2026

@bluestreak01 bluestreak01 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glasstiger

Please address the Moderate comments (M1, M2, M3) below before this merges. This is the OSS half of a tandem review; the same report is posted on questdb/questdb-enterprise#1175. M3 is the item in this repository.


Tandem review — questdb-enterprise#1175 + #7531

Reviewed at level 3, tandem mode.

Reviewed revisions: ENT d977d89bce73521b01b90424e76f9953bd5c1512 (base 897d83ba), OSS 38ac0f1604f830cc92ba51c74a617674545102c7 (base 048a595b).

Step 2 — PR title and description

Title test(qwp): add JavaScript browser auth and durable ACK E2E follows Conventional Commits; the body repeats the verb, speaks to what is covered rather than to internals, and carries the tandem label matching the OSS counterpart. The "lane is inert until its one-time setup is done" section is an accurate account of what this PR does and does not run. No finding.

Critical

None.

Moderate

M1 [ENT] The retention comment and the failure message both describe a shrink the counters cannot detect

Problem: Peak/low extrema lose ordering; a growing journal is reported as shrinking.
Net impact: JS-client-lane CI reports a false "records retired early" failure.
Evidence: Static, d977d89bc: Math.max/Math.min at javascript_durable_ack.mjs:99-100, equality assert at test_javascript_client_durable_ack.py:104.

questdb-ent/e2e/lib/javascript_durable_ack.mjs:80-84 claims the reported pair "survives an appearance read that raced a client still writing its second record, where the seed is lower than the true starting count." It does not. initialCount comes from waitForJournal(..., count => count > 0) (:184-187), which returns on the first non-zero sample — 1 even when the client will settle at 2. The monitor then records peak = Math.max(...), low = Math.min(...) and returns only those two numbers (:112-115). The sole consumer, tests/test_javascript_client_durable_ack.py:102-110, asserts low == peak, so exactly the race the comment names is a hard failure.

Worse, the two values are order-free extrema: the sequence 1 → 2 (grew) and 2 → 1 (shrank) both yield low=1, peak=2, and the message at :106-109 unconditionally prints "the Node store-and-forward journal shrank from 2 to 1". The test comment at :95-101 makes the same claim ("this compares the count against itself").

Whether the client can actually produce two .sfa records asynchronously is a property of nodejs-questdb-client, which is not in this tree, so I am not reporting a proven flake — only that the two comments assert a robustness property the code demonstrably lacks, and that the failure text asserts a direction the data cannot carry.

Suggested fix: either record the first settled count and the minimum after it (so growth and shrink are distinguishable), or keep the extrema and assert low >= <first sample after the monitor's first tick>; and correct both comments. At minimum, make the message report peak/low without claiming a direction.

M2 [ENT] The harness timeout is below the worst case its own docstring enumerates

Problem: timeout_s=180 contradicts the 200 s budget the adjacent comment lists.
Net impact: The generic TimeoutError the cap was raised to eliminate stays reachable.
Evidence: Static, d977d89bc: javascript_client.py:92-101; javascript_durable_ack.mjs:124,126-131,168-171,175,189-192.

questdb-ent/e2e/lib/javascript_client.py:92 states the rule — "Above the driver's own worst case, not below it" — then enumerates 60 s normal ACK, 60 s reconnect, 60 s waitForDurable, and 10 s for each of the appearance and removal waits (200 s), and sets timeout_s: float = 180.0 at :101.

The one concurrency the comment claims subtracts nothing: monitorJournalUntilDurable (mjs:58-110) has no deadline of its own and is bounded only by the 60 s durable promise. The two 10 s waits are strictly sequential with respect to it (mjs:168-171 before :175; :189-192 after). The only reading that makes 180 sufficient is that reconnect.maxDurationMs is nested inside ackTimeoutMs — but the comment itself lists them as separate budgets, and mjs:123-131 passes them as sibling options whose timer semantics live in the external client module.

Nothing forces 180: the pytest step cap is 30 minutes (ci/templates/e2e-test.yml, timeoutMinutes: 30).

Suggested fix: raise the default to ≥ 210 s, or correct the comment to state the nesting and the real bound.

M3 [OSS] Coverage gap: the browser-only durable-ACK carrier is never driven over a real socket

Problem: Sec-WebSocket-Protocol opt-in is only exercised through a mock header.
Net impact: A regression between socket and header lookup breaks every browser durable-ACK client.
Evidence: Exhaustive search at 38ac0f1604; only QwpIngressUpgradeProcessorOnHeadersReadyTest:616,706 (MockHttpRequestHeader) and WebSocketHandshakeTest:203,285 (pure function / response bytes).

QwpIngressUpgradeProcessor:484-488 reads Sec-WebSocket-Protocol off the request, :530 echoes the token into the 101, and :506-508 lets it pull the browser SERVER_INFO frame. This subprotocol is the only carrier a browser has for durable-ACK opt-in (browser JS cannot set X-QWP-Request-Durable-Ack).

Every occurrence of the token in a request position across both test trees is on a MockHttpRequestHeader with a MockRawSocket that only records sentSize, or is an argument to the pure containsWebSocketProtocol, or is an expected response substring. The raw-request fixtures that could carry it — QwpWireTestFixtures.browserUpgradeRequest/performHandshake — are never called with it; performWriteHandshake hard-codes extraHeaders = "" (:280). The ENT Node e2e does not fill the gap either: javascript_durable_ack.mjs:118 opts in with requestDurableAck: true, i.e. the header carrier.

If anything between the socket and getHeader dropped it, the server would echo no subprotocol, the browser would fail the whole connection per the WHATWG "establish a WebSocket connection" algorithm, and every existing test would stay green.

Not Critical: X-QWP-Request-Durable-Ack uses the identical requestHeader.getHeader(...) lookup and is driven over a real socket (ENT ReplicationTest.testQwpDurableAckE2E, DurableAckThroughDemoteTest, SqlFailoverQwpClientLosslessTest), so the mechanism is proven; the 101 byte layout is unit-pinned by responseSize/writeResponse agreement; and the failure mode is loud, not silent data loss.

Suggested fix: add an extraHeaders parameter to QwpWireTestFixtures.performWriteHandshake (the read-side performHandshake already has one) and one QwpBrowserNegotiationWireTest case that offers the subprotocol, asserts the 101 echoes it, and reads the 6-byte STATUS_SERVER_INFO. That test class already runs under startFragmented(), so it covers send fragmentation for free.

Minor

[OSS] New test methods break alphabetical member order in runs that were sorted at base.

  • WebSocketHandshakeTest:202testContainsWebSocketProtocol inserted between testIsWebSocketUpgrade (:194) and testKeyWithAllBase64Characters (:215); that adjacency was alphabetical at 048a595b. Also testBrowserIngressServerInfoFrame (:293) precedes testBrowserEgressServerInfoCompressionTrailer (:334) — both new, inverted.
  • QwpIngressUpgradeProcessorOnHeadersReadyTest:64-150 — the seven newly added testBrowser* methods are internally unsorted.
  • QwpIngressAckLeapfrogTest:621/:649…PollRequiresNegotiation before …PollRejectClampsPipelinedTail.

Insertions into runs already unsorted at base (QwpEgressCompressionTest, DynamicPropServerConfigurationTest, and the testRejectFlushTracker…/testOnHeadersReadyAdvertises… run) are excluded.

Coverage map

Test gate: pass. Zero admitted Critical coverage gaps; one admitted Moderate gap (M3), rendered above with its recorded search and failure link. Remaining UNTESTED rows were classified private as accepted with recorded safeguards — notably effectiveMaxBatchSize > 0 (needs http.recv.buffer.size ≤ 14, unusable server), Set-Cookie combined with a URL carrier on one upgrade (each addend unit-pinned; frame offset derives from writeResponse's return value), and the ENT Secure-attribute cookie on a QWP 101.

Coverage is otherwise strong. Each of isSameOrigin's four character-class terms is pinned with a length-matched forged Host so the length guard cannot decide the case first (QwpBrowserOriginTest:116-140), the strict-prefix port case is pinned separately, and both scheme/TLS directions are covered; the ENT QwpBrowserOriginTlsTest is the only place isTlsSessionStarted() returns true. Both carrier-precedence rules are pinned at the call site (testBrowserUrlAcceptEncodingWinsOverProxyInjectedHeader in both directions), which a unit test on same-typed arguments could not do. All six equality terms of isDurableAckPoll have a dedicated assertion.

Summary

  • Verdict: changes requested on both PRs — please address M1, M2 and M3. There are no Critical findings and the test gate passes; the block is on the three Moderate items, all of which are cheap to close.
  • Submodule provenance: questdb OFF-DEFAULT — in scope (38ac0f16 exists only on origin/ia_node_qwp). The bump is a fast-forward from fef82615; the fef82615 → 048a595b portion is on origin/master and is UPSTREAM-SYNC — out of scope, so only the 048a595b → 38ac0f16 range was reviewed. java-questdb-client and every other nested pointer did not move — out of scope.
  • Findings split: 4 in-diff, 0 out-of-diff breakage. Severity distribution: Critical 0, Moderate 3, Minor 1.
  • Admission audit. Falsification: each rendered item was re-checked by an independent pass that received only the neutral proposition and revision identity — no narrative, severity, or fix — and each returned SURVIVES with quoted evidence. Producer: M1/M2 are rendered strictly as static comment-versus-code contradictions; their behavioural consequences have producer: unknown (the nodejs-questdb-client tree is not present) and are omitted rather than downgraded. M3's producer is the browser client the OSS PR exists to serve. Dynamic evidence: no dynamic behavioural finding is admitted, so no runtime artifact is load-bearing. Dependency: no rendered item depends on an omitted premise.
  • Verified green here: node --check on the new driver, Python compilation of both new modules, TOML parse of .gitleaks.toml (rule generic-api-key carries exactly one allowlist), YAML parse of both pipeline files. I did not execute the Maven suites; no finding depends on that.
  • .gitleaks.toml checked against the tool, not the comment. The claim that declaring [[rules]] id = "generic-api-key" with no regex extends the built-in rule is correct: gitleaks config/config.go extend() keeps baseRule.Regex when currentRule.Regex == nil and appends currentRule.Allowlists. generic-api-key detection is unaffected elsewhere, and the two allowlist regexes are ^…$-anchored to the exact nonces.
  • Security core re-derived independently and held. The CSWSH relaxation fails closed for non-ASCII, whitespace, null, degenerate, prefix-port and forged-path origins; the raw 101/421 Set-Cookie cannot carry CR/LF (session ids are "qs1" + base64url from TokenGeneratorImpl); responseSize/writeResponse enumerate identical optional blocks in identical order on every path, so no Unsafe under-count exists; isDurableAckPoll short-circuits on length == HEADER_SIZE before any read; FLAG_DURABLE_ACK_POLL, STATUS_SERVER_INFO and CAP_COMPRESSION occupy previously unused values in their namespaces, and no Enterprise provider sets 0x04. The Host-based same-origin check is DNS-rebinding-weak, but /exec performs no origin check at all at the same revision, so the relaxation grants a rebinding attacker no capability they lack — net-neutral, omitted.
  • No Enterprise production code changed; all five ENT Java files are tests. The OSS PR adds no new SQL statement and no state-mutating operation, so no new Permission is required — but it does newly route the existing processServiceAccountCookie hook onto /write/v4 and /read/v1, and all three Enterprise refusal arms are covered on both routes by QwpBrowserSessionRestAuthTest.

@bluestreak01 bluestreak01 removed the READY PR is ready for the final review label Sep 11, 2026
glasstiger and others added 5 commits September 14, 2026 00:15
QwpBrowserNegotiationWireTest gains a real-socket test for the
Sec-WebSocket-Protocol durable-ACK opt-in, the only durable-ACK carrier
a browser has. Until now the offer reached onHeadersReady only through
MockHttpRequestHeader, so a regression between the socket and
getHeader() would have dropped the echo, failed every browser
connection, and left the suite green. The test offers the token under
startFragmented(), asserts that the 101 echoes it, reads the 6-byte
STATUS_SERVER_INFO frame, and checks that a connection without the
offer gets no echo. With the header lookup forced to null, the test
fails on the echo assertion.

QwpWireTestFixtures.performWriteHandshake gains an extraHeaders
overload that returns the 101 response headers, and the two ingress
SERVER_INFO checks now share one helper.

Members added on this branch to WebSocketHandshakeTest,
QwpIngressUpgradeProcessorOnHeadersReadyTest, QwpIngressAckLeapfrogTest
and QwpBrowserNegotiationWireTest move into alphabetical order. The
moves change no lines.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A27128icNkuoXhFai85Hfv
The negotiateAcceptEncoding Javadoc and its call-site comment contrasted
keying CAP_COMPRESSION off the URL parameter's presence with keying it
off which value won, and credited the former with stopping an injected
header from compressing a browser's wire unannounced. The two keyings
are one predicate, since the URL value wins whenever it is present, and
the protection does not exist for a request without the parameter: an
injected header still compresses that wire while SERVER_INFO reports
nothing. The comments now state the rule, name that gap and give the
mitigation: always send the parameter, raw included.
testBrowserUrlAcceptEncodingWinsOverProxyInjectedHeader now pins that
mitigation: a raw URL value beats an injected zstd header, and
SERVER_INFO still carries CAP_COMPRESSION with a raw codec.

QwpIngressHttpProcessor.isSameOrigin matches its scheme prefixes with
Utf8s.startsWithLowerCaseAscii and two lower-case constants, replacing
the private startsWithIgnoreCaseAscii that repeated it. Only the secure
prefix had case-insensitive coverage, so QwpBrowserOriginTest gains an
upper-case HTTP:// origin as well.

Both new assertions were checked against the mutation each one pins:
a case-sensitive plaintext prefix, and CAP_COMPRESSION withheld for a
raw codec. Each mutation fails its own assertion and no other test.

The tests this branch added to QwpEgressCompressionTest and
DynamicPropServerConfigurationTest move into alphabetical order. The
moves change no lines.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A27128icNkuoXhFai85Hfv
Member sorting moved WEBSOCKET_PROTOCOL_QWP_DURABLE_ACK nine lines below
the comment that describes it, leaving that comment on top of the
URL_PARAM_* constants in QwpIngressHttpProcessor. The comment now sits
on the subprotocol constant, and the URL-decoding paragraph stays with
the URL parameters.

Two test comments described intermediate states of this branch that
never reached master. The assertDurableAckStateAfterHandshake Javadoc
said its two expectations "no longer track each other", and
WebSocketHandshakeTest called the subprotocol echo "now unconditional".
Both now state the current rule: the server echoes the subprotocol
whenever the client offers it, while enablement follows the registry.

testHeaderNegotiatedCompressionLeavesCapCompressionClear created a WAL
table that nothing used. The test only connects and reads SERVER_INFO,
which needs no table, so the DDL goes and the unused server variable
becomes ignored.

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

ideoma commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

[PR Coverage check]

😍 pass : 203 / 211 (96.21%)

file detail

path covered line new line coverage
🔵 io/questdb/cutlass/http/HttpCookieHandler.java 0 1 00.00%
🔵 io/questdb/cutlass/http/HttpCookieHandlerImpl.java 7 8 87.50%
🔵 io/questdb/cutlass/qwp/server/egress/QwpEgressUpgradeProcessor.java 45 49 91.84%
🔵 io/questdb/cutlass/qwp/server/QwpIngressUpgradeProcessor.java 52 53 98.11%
🔵 io/questdb/cutlass/qwp/server/QwpIngressHttpProcessor.java 88 89 98.88%
🔵 io/questdb/cutlass/http/HttpServerConfigurationWrapper.java 1 1 100.00%
🔵 io/questdb/PropertyKey.java 1 1 100.00%
🔵 io/questdb/PropServerConfiguration.java 2 2 100.00%
🔵 io/questdb/cutlass/qwp/protocol/QwpMessageHeader.java 6 6 100.00%
🔵 io/questdb/cutlass/http/DefaultHttpServerConfiguration.java 1 1 100.00%

@glasstiger

Copy link
Copy Markdown
Contributor Author

/azp run macwin

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@glasstiger

Copy link
Copy Markdown
Contributor Author

Tandem review completed at level 3 across questdb/questdb-enterprise#1175 and #7531.

Reviewed revisions:

  • Enterprise: 75d77e5ea80ccd6360b2b1a88354e5ee144e9e5e (base 0b88f7d908e09de107c92990f9b4d52ba9818160)
  • OSS: b56b7980855d1e517623b7113a41270bfc97a62e (base dbf0a87b517f24959a07c6274ebef2569799e878)

Findings

  • Critical: none
  • Moderate: none
  • Minor: none

Validation

  • OSS targeted QWP suites: 198 tests passed
  • Enterprise browser-session/TLS suites: 6 tests passed
  • Exact companion JavaScript client head built successfully
  • Enterprise durable-ACK JavaScript E2E: 1 test passed
  • Python compilation, Node syntax, and both repositories' git diff --check passed

Coverage gate: pass (0 admitted coverage gaps).

Submodule provenance: questdb is OFF-DEFAULT — in scope; nested java-questdb-client is unchanged and out of scope.

Verdict: approve both PRs.

@bluestreak01 bluestreak01 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdicts

  • ENT #1175 (75d77e5): approve
  • OSS #7531 (b56b798): approve

No admitted findings: 0 Critical, 0 Moderate, 0 Minor; none in-diff or out-of-diff breakage.

Correctness gate: pass. Test gate: pass — 0 admitted coverage gaps.

Existing CI checks are green. No local tests were run; the new dispatch-only JavaScript E2E lane is not exercised by normal PR CI.

Provenance: questdb: OFF-DEFAULT — in scope. No nested submodule pointer changes.

Source files unchanged; no GitHub review posted.

@bluestreak01 bluestreak01 added the QUEUED FOR MERGE Approved PR in the merge queue. Do not merge master into this PR. label Sep 15, 2026
@bluestreak01
bluestreak01 merged commit f9d4f51 into master Sep 15, 2026
56 checks passed
@bluestreak01
bluestreak01 deleted the ia_node_qwp branch September 15, 2026 16:42
eugenels added a commit that referenced this pull request Sep 18, 2026
Upstream brought f9d4f51 feat(qwp): support browser negotiation and session
authentication (#7531), a6f64f5 fix(sql): covering index nulling included
columns, and fc7a28b perf(sql): latest-by with partition-key filters.

All four conflicts were this branch's durable-ack tier negotiation meeting
upstream's browser work on the same handshake path. Neither side was a superset,
so the branch's tier logic is reapplied inside upstream's new structure rather
than either side being taken wholesale.

QwpIngressUpgradeProcessor, the negotiation block: the request is now detected
from both carriers (header and the questdb.qwp.durable-ack.v1 subprotocol);
the subprotocol has no tier parameter, so it requests REPLICATED | LEGACY_TRUE
-- the legacy "true" meaning -- which keeps it behind the same
isTierSetAvailable gate and echoes the historical "enabled" token, so the 101
stays byte-identical for pre-tier clients. A browser therefore cannot request
the weaker "local" tier. The all-or-nothing grant is unchanged: a client asking
for "replicated" is never silently downgraded. Upstream's ungated subprotocol
echo is preserved -- gating it on durableAckEnabled would make a browser drop
the connection before it could be told durable ACK is unavailable -- along with
the SERVER_INFO frame, the qwp_browser_handshake URL param and the session
cookie.

QwpIngressHttpProcessor: responseSize and writeResponse take the union
signature, keeping this branch's Utf8Sequence durableAckConfirmToken (upstream
still had the boolean this branch replaced) and adding upstream's
sessionCookieValueBytes and durableAckWebSocketProtocol, with the delegating
overloads the egress processor calls.

QwpConstants: kept upstream's new subprotocol sentence but this branch's
"2-byte nameLen". Upstream reflowed the paragraph around the pre-fix 1-byte
figure; the encoder writes a short (QwpIngressProcessorState.java:1663, sized
at :519).

Two repairs in files git merged textually but not semantically, both caused by
this merge:

- QwpIngressAckLeapfrogTest:476 overrode getDurablyUploadedSeqTxn, renamed to
  getReplicatedDurableSeqTxn on this branch by 3c65204. Upstream's new
  testDurableAckPollFlushesDurableProgress carried the old name in, leaving the
  tree uncompilable.
- The same new test set only setDurableAckEnabled(true). This branch split that
  into (enabled, tiers), so tiers stayed NONE, collectDurableProgress took the
  LOCAL branch, and the frame was correctly withheld. Added
  setDurableAckTiers(REPLICATED), the only tier its fixture registry implements.
  Production was never affected: QwpIngressUpgradeProcessor:566-567 sets both.

Verified: full-reactor test-compile clean; 158 tests green across
QwpIngressUpgradeProcessorOnHeadersReady (28), WebSocketHandshake (34),
QwpIngressUpgradeProcessorResumeRecv (35), QwpServerInfoFrame (13),
QwpUpgradeRejectFragmentation (13), QwpIngressHttpProcessor (10),
QwpIngressAckLeapfrog (10), QwpBrowserSessionAuth (6),
QwpBrowserNegotiationWire (6), QwpBrowserOrigin (3). QwpBrowserSessionAuthTest
is the guard that the 101 still carries Set-Cookie: qdb_session, the one
failure mode this resolution could have made silent.
puzpuzpuz added a commit that referenced this pull request Sep 18, 2026
Bring in three upstream commits: covering index column-top fixes
(#7559), QWP browser negotiation and session auth (#7531), and the
latest-by partition-key filter speedup (#7615).

SqlCodeGenerator conflicted because this branch added
getHashJoinGroupByCandidate() at the same spot where upstream added
the covering-index backup helpers (canKeyBeNull, isBackupNeeded,
buildSingleSymbolIndexScan, buildLatestByIndexScan, ...). The
resolution keeps both sets of methods unchanged. PropertyKey,
server.conf, ServerMainTest and PropServerConfigurationTest merged
cleanly, and each now carries both the new QWP property and this
branch's cairo.sql.parallel.hash.join.groupby.enabled.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

QUEUED FOR MERGE Approved PR in the merge queue. Do not merge master into this PR. READY PR is ready for the final review tandem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants