feat(qwp): support browser negotiation and session authentication - #7531
Open
glasstiger wants to merge 56 commits into
Open
feat(qwp): support browser negotiation and session authentication#7531glasstiger wants to merge 56 commits into
glasstiger wants to merge 56 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/azp run macwin |
|
Azure Pipelines successfully started running 1 pipeline(s). |
CLAUDE.md groups class members by kind and sorts them alphabetically, and the browser negotiation work inserted several members out of place. QwpConstants now lists FLAG_DURABLE_ACK_POLL after FLAG_DELTA_SYMBOL_DICT and STATUS_SERVER_INFO after STATUS_SECURITY_ERROR, so both blocks read in order again. QwpIngressHttpProcessor moves WEBSOCKET_PROTOCOL_QWP_DURABLE_ACK after VALUE_WEBSOCKET, slots containsWebSocketProtocol and getSessionCookieValueBytes into place among the public statics, and lifts startsWithIgnoreCaseAscii and toLowerAscii out of the middle of that block into the trailing private-static block beside buildVersionBytes and containsUpgrade. The three test classes the same work added are sorted too. misdirectedRequestWithRoleSize(byte[]) and writeMisdirectedRequestWithRole(long, int, byte[]) were left behind when the 421 path moved to the overloads that carry the session cookie. A search across both repositories and the client submodule, over every file type, finds no caller, so they go. The SERVER_INFO_BODY_MAX_BYTES javadoc still described the bound as 26 fixed bytes after the constant grew to 28 for the browser compression trailer; it now matches the value and says what the extra bytes cover. No behaviour changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QffWppgMiw9YUKTaYnBinm
The IntelliJ formatter check on CI reformats the repo and diffs the result, and QwpIngressHttpProcessor tripped it: two javadoc blocks had lost their blank line separator and one had picked up three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QffWppgMiw9YUKTaYnBinm
QwpBrowserOriginTest exercised the authority byte loop only through its three accepting cases. Every rejecting case returned earlier, on the scheme branch or on the authority-length check, so nothing asserted that isSameOrigin rejects an equal-length authority whose bytes differ -- the direction that matters for the CSWSH gate. Neutralising that comparison so it always reports a match left the class green at 3/3, as did dropping the explicit '/' rejection and the host == null guard. Add three assertions covering those clauses: an equal-length authority with different bytes, a forged Host that reproduces a path-bearing Origin byte for byte, and an absent Host. Each one fails when the clause it pins is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QffWppgMiw9YUKTaYnBinm
Three paths added by the browser negotiation work had no test that could fail if they regressed. The qwp_accept_encoding and qwp_max_batch_rows URL carriers had no test reference at all, so swapping either getUrlParam key in onHeadersReady left every suite green. QwpBrowserNegotiationWireTest upgrades a real read socket with each carrier and asserts the wire effect: CAP_COMPRESSION plus the codec/level trailer for the first, three single-row RESULT_BATCH frames for the second, each against a control connection that omits the carrier. Both key swaps now fail. QwpWireTestFixtures.performReadHandshake gains a query-string overload so its six existing callers keep their signature. The durable-ack poll never produced a STATUS_DURABLE_ACK in any test: the leapfrog suite runs against a disabled registry, so nothing covered the frame's stated purpose. Arm the registry between the data frame and the poll, leaving the poll as the only flush point able to carry the new watermark. Finally, isDurableAckPoll's magic, version and payload-length conjuncts were never falsified; deleting any of the three failed no test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QffWppgMiw9YUKTaYnBinm
The SERVER_INFO_BODY_MAX_BYTES javadoc enumerated 26 bytes of fixed body fields, the 2-byte CAP_ZONE length prefix and the 2-byte browser compression trailer, but the constant summed them to 28. Spell the breakdown out in the expression itself so it cannot drift again, and drop the "tight bound" claim along with it: EntQwpServerInfoProvider populates a third u16-capped id (zone_id), so the reservation is a floor the ids share rather than an upper bound on the body. Nothing functional moves - writeServerInfo truncates inside the cap it is handed, and the check already reserved ~128 KB of headroom. QwpWireTestFixtures grows a readHttpHeaders helper and turns public. That retires five hand-rolled CRLFCRLF readers: two in OSS tests, two in Enterprise ones, and a fifth inlined in performReadHandshake. The shared reader tolerates a stream that ends early and returns what arrived, which is what three of the four copies did; the callers that printed only the status line now print the whole response, so a truncated upgrade still says something. server.conf's comment for qwp.browser.tls.termination.enabled named one prerequisite of the same-origin gate but not the other: isSameOrigin compares Origin against the raw Host header, so a terminator that rewrites Host - nginx's proxy_pass defaults to $proxy_host - fails every browser upgrade with "Origin header not allowed on QWP WebSocket". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYfQvZ6aCkMN9jnEw1uKfb
The qwp.browser.tls.termination.enabled documentation named "proxy_set_header Host $host" as the safe nginx form. nginx resolves $host to the hostname alone and drops the port, while a browser serializes any non-default port into Origin. isSameOrigin compares the two authorities byte for byte, so a TLS terminator listening on any port other than 443 rejects every browser upgrade with "Origin header not allowed on QWP WebSocket" -- on a deployment configured exactly as the property documented. The comment now prescribes $http_host, which forwards the header verbatim, and states the requirement the check imposes: the terminator must preserve Host including its port. It also names both nginx traps as traps -- proxy_pass defaulting to $proxy_host, and the $host in the widely copied WebSocket snippet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYfQvZ6aCkMN9jnEw1uKfb
The durable-ack-poll reject arm of handleBinaryMessage sent STATUS_PARSE_ERROR without first flushing the connection's pending cumulative ack, unlike the error arm at the tail of the same method. Both frames go out either way, but a store-and-forward sender that treats the error as terminal tears the connection down on reading it and never sees the ack covering the frames already committed in that pass, so its reconnect replays and duplicates them. The arm now flushes first, with the same onErrorBlocked handling its sibling uses. QwpEgressUpgradeProcessor read the browser's qwp_accept_encoding URL parameter only when X-QWP-Accept-Encoding was absent, so an injected header won outright and left browserCompressionNegotiation false: the server compressed the wire while withholding CAP_COMPRESSION and the codec/level trailer, and the browser decoded compressed frames as raw. negotiateMaxBatchRows already guards against exactly that injection by taking the stricter of its two carriers. A new negotiateAcceptEncoding gives compression the same protection -- the URL parameter wins, since only the client's own connect URL can carry it -- and the CAP_COMPRESSION bit now keys off which carrier the client used rather than which value won. A header-only native client and a URL-only browser both resolve as before. server.conf now records that qwp.browser.tls.termination.enabled governs the origin check alone and does not mark qdb_session Secure. That attribute follows QuestDB's own TLS setting, which is off by definition in the very topology the flag exists for. New tests close the gaps those fixes exposed: - A non-rotating upgrade must emit no Set-Cookie, on both endpoints. Dropping the empty-session guard in getSessionCookieValueBytes otherwise answers every 101 and 421 with "qdb_session=" plus a 30-day Max-Age, clearing the browser's live session. - The browser SERVER_INFO frame stays absent when qwp_browser_handshake is missing, unknown or empty, and the positive case pins the advertised cap against X-QWP-Max-Batch-Size instead of merely asserting it positive. - A real socket upgrade to /write/v4?qwp_browser_handshake=v1 proves the parameter survives route matching on the ingress path; the previous coverage drove it through a mock request header only. performReadHandshake and the new performWriteHandshake now share one body in the fixture. - The 101 does not name the durable-ack subprotocol to a client that offered none, which RFC 6455 s4.1 makes such a client fail on. - negotiateAcceptEncoding resolves all four carrier combinations. - The poll reject puts its cumulative ack ahead of the error. Every new assertion fails against a mutation of the line it guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYfQvZ6aCkMN9jnEw1uKfb
The browser carrier mirrors X-QWP-Accept-Encoding, whose documented value grammar is "zstd;level=N", but only the header is delivered verbatim. The query string goes through HttpHeaderParser.urlDecode, which re-keys a parameter on every unescaped '='. So ?qwp_accept_encoding=zstd;level=5 parses as the parameter "zstd;level" with value "5", the qwp_accept_encoding key is absent entirely, and the request reads as "no preference" -- the client asked for compression and silently got a raw wire. The parser behaviour predates this work, but this is the first QWP URL parameter whose own value grammar contains '=', so the branch only became reachable now. State the percent-encoding requirement on negotiateAcceptEncoding and on the URL_PARAM_QWP_ACCEPT_ENCODING declaration, and pin both halves on the wire: the encoded form must reach the negotiator with its level intact, and the raw form must be dropped whole rather than half-applied. A bare "zstd" clamps to level 1, so asserting level 5 proves the parameter survived the round trip. These two files also carry the member-order and dead-overload cleanups described in the following commits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ak82JZCcANpEtK85fvnUgK
handleBinaryMessage marks a refused durable-ack poll's sequence unresolved before flushing, so the cumulative OK ack cannot cover it or anything pipelined behind it. Nothing exercised that: every existing poll test either ends at the poll or sends nothing after it, so deleting the marker left the whole suite green. Drive data, poll, data through one pass with durable ack switched off and assert the watermark stops at the frame before the refused poll and the tail never commits. Without the marker the ack jumps to 2 and the tail row lands: a store-and-forward sender that treats STATUS_PARSE_ERROR as terminal tears down before reading that ack, replays from its old watermark, and duplicates the tail frame's rows. Also reattach the javadoc that described hasErrorResponseForSeq. A helper added earlier landed between the comment and its method, leaving the comment on hasDurableAckFrame, which takes no sequence and tests for equality with STATUS_DURABLE_ACK rather than inequality with STATUS_OK. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ak82JZCcANpEtK85fvnUgK
server.conf told the reader that the qdb_session Secure attribute "follows QuestDB's own TLS setting". That describes Enterprise. Open source never sets Secure at all -- HttpCookieHandlerImpl writes the SESSION_COOKIE_ATTRIBUTES constant, which carries only HttpOnly, Path, SameSite and Max-Age -- so an operator reading this file and enabling QuestDB's own TLS would over-trust the cookie. Say what each edition actually does; the paragraph's advice for the TLS-terminator topology is unchanged. Promoting effectiveMaxBatchSize to a field put an int under a comment describing "header bytes ... Null when the cap collapses to zero". Give each field its own comment. Sort the members this branch added into the positions the project's arrangement rules put them in: CAP_COMPRESSION before CAP_QUERY_FLAGS, isDurableAckPoll before isMessageMagic, getSessionCookieValue ahead of parseCookies (the interface already had it there), RESPONSE_WEBSOCKET_PROTOCOL_DURABLE_ACK after RESPONSE_SUFFIX, the public static writeBrowserServerInfoFrame out of the instance-method run, and in the egress processor the private parseMaxBatchRows out of the public block and the now-public writeServerInfoFrame out of the private one. Also record on isDurableAckPoll why every term is an equality: relaxing the length to >= or the flags to a bitmask test would let the server ack a sequence whose payload it never processed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ak82JZCcANpEtK85fvnUgK
QwpWireTestFixtures exists so the OSS and Enterprise test trees cannot drift apart on the QWP byte layouts, but the browser-shaped upgrade request was still stamped out by hand at every call site, and the one-row ingress message and QWP header assertion had no shared home at all. Add browserUpgradeRequest, encodeSingleLongRow and assertQwpMessageKind to the fixture, plus the RFC 6455 handshake nonce the browser tests share, and route the fragmentation test's two upgrade builders through them. encodeSingleLongRow uses the production QwpVarint encoder rather than a fourth hand-rolled varint writer. The Enterprise support class re-derives all three today; it picks them up in the companion change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ak82JZCcANpEtK85fvnUgK
Both guards the poll path relies on were green with the guard deleted. testDurableAckPollMustNotCommitDeferredGroup asserted the cumulative ack stays at -1, but setHighestProcessedSequence already refuses that advance through its last-resort uncommitted-deferred-rows clamp, which returns without assigning and differs only by a LOG.critical line. The observable state was therefore identical with and without the hasUncommittedDeferredRows guard in handleBinaryMessage. Count the calls into setHighestProcessedSequence instead, so the test fails when the poll arm asks for the advance at all -- the clamp is documented as containment for a regression of exactly this path, so a test that leans on it proves nothing. testRecognizesExactDurableAckPollFrame mutated the length to HEADER_SIZE - 1 and the flags to FLAG_DEFER_COMMIT, and isDurableAckPoll rejects both under the relaxed forms its own contract warns about: a shorter frame fails >= too, and 0x01 & 0x02 is zero. Add the two mutations that separate them -- a longer frame and the FLAG_DURABLE_ACK_POLL | FLAG_DEFER_COMMIT superset. Verified by re-running both mutations: each now reddens its test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ak82JZCcANpEtK85fvnUgK
Two gaps on the browser upgrade response path. writeBrowserServerInfoFrame took no buffer size and wrote seven bytes unconditionally, unlike writeMisdirectedRequestWithRole and the egress writeServerInfoFrame, which both answer -1 when short. Its caller's requiredHandshakeSize reservation was the only thing keeping it inside the raw send buffer, and no test covered that term -- every buffer-too-small case omits qwp_browser_handshake. Give it the same size argument and -1 contract, fail the handshake when it reports no room, and pin the boundary from both ends: a unit case either side of the seven bytes, and an onHeadersReady case on a buffer measured to sit one byte short of them. The egress processor threads sessionCookieValueBytes through its own responseSize and writeResponse pair, but both rotation tests upgraded /write/v4, so only the null branch ever ran on /read/v1. Parameterise the rotation test over both routes. Dropping the argument from one call but not the other puts the SERVER_INFO frame at the wrong offset in the send buffer, which the null branch cannot see. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ak82JZCcANpEtK85fvnUgK
The ingress upgrade told a browser that durable acknowledgements were unavailable by withholding the questdb.qwp.durable-ack.v1 subprotocol echo from an otherwise successful 101. A browser cannot read that signal. The WHATWG "establish a WebSocket connection" algorithm fails the whole connection when the client offered a subprotocol and the response names none, so the socket never opens and the page sees an opaque error instead. The client's own QwpDurableAckUnavailableError, which inspects socket.protocol after the open, could never fire. QwpIngressUpgradeProcessor now echoes the token whenever the client offered it. The echo confirms that the server speaks the browser negotiation rather than that the capability is on, and RFC 6455 still holds because the server never names a token the client did not offer. The verdict moves to the browser SERVER_INFO frame as the new SERVER_INFO_CAP_DURABLE_ACK bit, which a browser can read on an open socket. Either browser carrier now pulls that frame, so a client that wants only durable ACK no longer has to pass qwp_browser_handshake=v1 to learn whether it got it. The frame grows from five bytes to six, and BROWSER_SERVER_INFO_PAYLOAD_BYTES and BROWSER_SERVER_INFO_WS_FRAME_BYTES now name that size so onHeadersReady's send-buffer reservation and writeBrowserServerInfoFrame cannot drift apart. The upgrade also logs a capability gap so an operator can diagnose one without a client. This changes a wire format the same branch introduces, so nothing released depends on it, but it has to land together with nodejs-questdb-client#62: until the client reads the capability bit, the unconditional echo would make it report durable ACK as enabled against a server that has it switched off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ak82JZCcANpEtK85fvnUgK
The QWP upgrade refused a cross-origin browser with "Origin header not allowed on QWP WebSocket". That was accurate while the guard rejected every Origin, but the same-origin allowance made it a misdirection: an Origin IS allowed now, as long as it matches Host. An operator reading it goes looking for a way to turn browser support on, when the cause is almost always a reverse proxy that rewrote Host or dropped its port. The message now reads "Origin is not same-origin with Host on QWP WebSocket", and ERROR_ORIGIN_HEADER_NOT_ALLOWED becomes ERROR_CROSS_ORIGIN_NOT_ALLOWED so the constant does not restate the mistake. Browsers hide the handshake response from page JavaScript, so the server log is the only place either text is read. Two coverage gaps around the same gate close with it. testValidateHandshakeRejectsOriginHeader set no Host, so isSameOrigin returned on its null-host guard and the test would have stayed green through a scheme-arm or authority-loop bug. It now sends an Origin that shares both the scheme and the authority length of Host, which leaves the byte-by-byte comparison as the only thing that can refuse it. QwpBrowserOriginTest gains the case where the Origin authority is a strict prefix of Host, the shape a page on the default port produces when it reaches QWP on another one. RFC 6454 counts the port, so that is cross-origin, and every previously asserted rejection either has the origin authority longer than Host or differs inside the compared bytes. Relaxing the length equality to reject only the longer case therefore left the whole suite green while re-opening CSWSH from any same-host web app on a different port. The empty-authority case joins it, since the byte loop never runs there and only the explicit bound rejects it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ak82JZCcANpEtK85fvnUgK
The browser SERVER_INFO capability byte was only ever asserted clear. Every assertBrowserHandshakeServerInfo call site passed expectDurableAck false, and the enabled-registry test pinned the negotiated state and the 101 echo without reading the frame, so replacing the durableAckEnabled argument of writeBrowserServerInfoFrame with a constant left the whole suite green while every browser was told durable ACK was unavailable and never entered durable mode. The frame is the browser's only carrier for that verdict. Give the helper an optional registry and add the one case that drives an enabled registry through onHeadersReady and reads the bit back. The four character-class terms of the isSameOrigin byte loop had the same problem. The userinfo case exited at the length check, 24 bytes against 19, so the '@', '?', '#' and control-byte rejections decided nothing and could all be deleted unnoticed. Add equal-length pairs that force each term to be the one that rejects.
The RESPONSE_WEBSOCKET_PROTOCOL_DURABLE_ACK comment said the token is echoed only when the client offered the subprotocol and the registry is enabled. The code echoes it whenever it was offered, on purpose, and three other places say so: the inline comment above the echo, the javadoc on SERVER_INFO_CAP_DURABLE_ACK, and a test that asserts the echo happens with the registry disabled. A maintainer who trusted the constant would re-add the gate and break every browser connection, because a browser fails the whole connection when it offered a subprotocol and the 101 names none. State what the code does instead. The durableAckWebSocketProtocolEnabled local only renamed durableAckWebSocketProtocolRequested, and its suffix contradicted the comment directly above it, which says the token carries the dialect and not the capability. Use the requested flag at both call sites. Two handshake overloads lost their last production caller when the session-cookie parameter landed and were left behind by the sweep that removed the previous pair. Fold their javadoc into the surviving forms and move the two remaining test call sites across. Sort WEBSOCKET_PROTOCOL_QWP_DURABLE_ACK after WEBSOCKET_GUID; the public constant block was in exact order before this branch. Point the WEBSOCKET_KEY javadoc at this repository's own path allowlist rather than at a per-value rule that only exists in the enterprise gitleaks config.
The pointer was ambiguous about which repository's .gitleaks.toml carries the exemption. This repository uses a blanket src/test path allowlist; the enterprise repository scans its test sources and needs a per-value entry.
Restore alphabetical member order in the QWP header, fragmented upgrade, and handshake processor tests.
Contributor
Author
|
Tandem review completed at level 3 together with questdb/questdb-enterprise#1175. Findings
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). Submodule provenance from the Enterprise PR: Verdict: approve. |
Contributor
Author
|
/azp run macwin |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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
Collaborator
[PR Coverage check]😍 pass : 207 / 215 (96.28%) file detail
|
Contributor
Author
|
/azp run macwin |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
Author
|
Tandem review completed at level 3 across questdb/questdb-enterprise#1175 and #7531. Reviewed revisions: Enterprise Findings
Verified green: 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 Omitted as unverifiedThree independent passes raised the same candidate: on ModerateM1 [OSS] Javadoc claims a protection the keying does not provide.
M2 [ENT] The e2e harness cap is below the budget its own docstring enumerates.
M3 [ENT] The retention comment claims a tolerance the assertion does not implement.
M4 [OSS] Coverage gap: the browser-only durable-ACK carrier is never driven over a real socket. Every Not Critical: Minor
Coverage gatePass. 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 Scope notesSubmodule provenance: 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. VerdictENT: approve. OSS: approve. Both gates pass. The four Moderate items are worth fixing and none of them blocks the merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
qdb_sessioncookie flowThis 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. Headers are still preferred; the URL parameter is consulted only when the header is absent.
X-QWP-Request-Durable-AckheaderSec-WebSocket-Protocol: questdb.qwp.durable-ack.v1qwp_browser_handshake=v1→STATUS_SERVER_INFOframeX-QWP-Accept-Encodingheaderqwp_accept_encodingURL parameterContent-Encodingresponse headerCAP_COMPRESSIONplus codec/level in theSERVER_INFOframeX-QWP-Max-Batch-Rowsheaderqwp_max_batch_rowsURL parameterOut-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
Originand cannot let JavaScript remove it, while machine clients normally omit it. An upgrade carrying anOriginthat is not same-origin with the requestHostis rejected; an upgrade with noOriginis unaffected. Fragmented upgrade requests are rejected rather than reassembled.Session authentication reuses the existing cookie handler:
HttpCookieHandlergainsgetSessionCookieValue(), and both the ingress and egress upgrade responses emitSet-Cookiewhen a session is created or rotated, so a browser that logged in over HTTP carriesqdb_sessioninto the WebSocket.Coverage
Related work