refactor!: make the CONNECT request optional for raw QUIC sessions#326
Open
kixelated wants to merge 1 commit into
Open
refactor!: make the CONNECT request optional for raw QUIC sessions#326kixelated wants to merge 1 commit into
kixelated wants to merge 1 commit into
Conversation
Raw QUIC carries no in-band request URL, but `Session::raw()` required a `ConnectRequest`. Callers had to synthesize a URL from the TLS SNI purely to satisfy the signature, and that URL was then discarded. SNI is optional (a client dialing a bare IP sends none, per RFC 6066), so the synthesis also forced a needless "SNI is required" constraint on the accept path. Drop the request from `Session::raw()` and make it optional instead: - `Session::raw(conn, response)` in the quinn, noq, and quiche backends - `Session::request() -> Option<&ConnectRequest>` - `web_transport::Session::url() -> Option<&Url>` The response stays on `raw()` because it carries the negotiated ALPN, which `protocol()` reads. The wasm router returns `Some` unconditionally so the signature is identical across targets and cross-platform callers still compile. No `path()` or `host()` accessor is added. Decomposing the URL was only ever needed to paper over the synthesized authority, and combining an unauthenticated `:authority` header with a cert-bound SNI behind one accessor would hide which of the two a caller was trusting. `ConnectRequest.url` is unchanged: it is still needed to encode the `:scheme`, `:authority`, and `:path` pseudo-headers. The FFI, Node, and browser bindings have no raw QUIC support, so their `url()` accessors are untouched. Adds tests/raw.rs, which covers raw sessions for the first time. BREAKING CHANGE: `Session::raw()` no longer takes a `ConnectRequest`, and both `Session::request()` and `web_transport::Session::url()` now return an `Option`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughRaw QUIC sessions now omit CONNECT requests, so session and connection request accessors return optional values. Raw constructors no longer accept a request argument. Native and WebAssembly URL accessors also return optional URLs. New QUIC integration tests configure a non-HTTP/3 ALPN and verify request absence, protocol propagation, and plain stream payload exchange. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
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
1 participant
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.
What
Raw QUIC (a moq ALPN with no WebTransport CONNECT) has no in-band request URL, but
Session::raw()required aConnectRequest. Callers had to synthesize one from the TLS SNI purely to satisfy the signature — and then discard it. Since SNI is optional (a client dialing a bare IP sends none, per RFC 6066), that synthesis also imposed a needless "SNI is required" constraint on the accept path.This makes the request optional so the type tells the truth:
Session::raw(conn, response)— the request parameter is gone (quinn, noq, quiche)Session::request() -> Option<&ConnectRequest>web_transport::Session::url() -> Option<&Url>Why this shape
responsestays onraw(). It carries the negotiated ALPN, whichprotocol()reads. Dropping it would silently lose the ALPN on exactly the path this change exists to serve.No
path()orhost()accessor. Decomposing the URL was only ever needed to work around the synthesized authority.host()in particular would merge an unauthenticated:authorityheader with a cert-bound SNI behind one name, hiding which of the two a caller was trusting — the security property would depend on the transport.irohalready models this correctly (Session::raw(conn),request() -> Option<..>); the other backends now match it.ConnectRequest.urlis unchanged. It's still needed to encode the:scheme,:authority, and:pathpseudo-headers, and it's what downstream reads.Bindings untouched. FFI, Node, and the browser have no raw QUIC support, so
url()is alwaysSomethere. Making it nullable would have broken the Python/JS query-string coverage for no gain.wasm returns
Someunconditionally so the signature is identical across targets and cross-platform callers still compile.Tests
Adds
rs/web-transport-quinn/tests/raw.rs, covering raw sessions for the first time: no request, ALPN surfaced viaprotocol(), and streams round-tripping without the WebTransport header.Reviewers should know these are mostly compile-time guards — the old three-argument signature wouldn't build against them at all. Only
raw_session_streams_omit_webtransport_headerexercises real behavior. There's no bug being fixed here, so there's no failing-before case to capture.The tests are gated on
#[cfg(any(feature = "aws-lc-rs", feature = "ring"))]and pass an explicit crypto provider, matching the crate's own builders:--all-featuresenables both backends, leaving rustls unable to pick a process-level default. Verified under default,--all-features,ring-only, and--no-default-features.Verification
just checkandjust testboth pass (clippy-D warningson native and wasm32,cargo fmt --check, full test suite).Follow-up
Unblocks deleting the
moqt://{sni}synthesis inmoq-native, where the raw arm collapses toSelf::Raw { connection, response }.Error::BuildUrldies with it, andError::MissingServerNameis already orphaned there.Breaking
Session::raw()no longer takes aConnectRequest;Session::request()andweb_transport::Session::url()now returnOption. Affectsweb-transport,web-transport-quinn,web-transport-noq, andweb-transport-quiche.🤖 Generated with Claude Code