Subprotocol negotiation #140
Conversation
WalkthroughAdds WebTransport subprotocol negotiation support. web-transport-proto: new protocol_negotiation module with header name constants, SFV header parsing/serialization, ConnectRequest.protocols: Vec, ConnectResponse.protocol: Option, and ConnectError::HeaderError. web-transport-quiche and web-transport-quinn: propagate new fields through open/respond paths, update request/response construction, add Client::connect_with_subprotocols and Session subprotocol tracking/accessor, implement Deref helpers, and add Request::ok_with_protocol. New multiproto server/client examples and README updates included. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)web-transport-quinn/src/connect.rs (1)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @web-transport-quinn/examples/multiproto-server.rs:
- Around line 155-162: After closing the session for an incorrect command,
immediately return to stop the function from continuing to accept streams on the
closed session; specifically, after the call to session.close(1, b"unknown
command") in the loop that checks ping_msg (where tracing::info!(command =
%ping_msg, ... ) and session.close are invoked), add a return to exit the
function so the code does not continue to accept or operate on a closed session.
In @web-transport-quinn/examples/README.md:
- Line 24: Update the three fenced code blocks that contain shell commands so
they include a language identifier for syntax highlighting: replace the opening
triple backticks before "cargo run --example multiproto-server" , before "cargo
run --example multiproto-client -- --protocol echo/0 \"any test string\"" and
before "cargo run --example multiproto-client -- --protocol ping/0 ping" with
```bash (or ```shell) so each block starts with a language tag instead of a
plain ```; leave the content and closing backticks unchanged.
- Line 21: Update the warning sentence to fix the article: change "an
dangerously insecure client setup" to "a dangerously insecure client setup" so
the text reads "This example uses a self-signed certificate and a dangerously
insecure client setup for easy bootstrapping. Don't use that at home!"
🧹 Nitpick comments (8)
web-transport-quiche/src/h3/connect.rs (2)
63-66: Subprotocol negotiation not implemented in quiche.The server always responds with
subprotocol: None, meaning it cannot select or communicate a negotiated subprotocol to the client. While the client's proposed subprotocols are available inself.request.subprotocols, there's no mechanism to use them.Per the PR objectives, full subprotocol support is currently limited to the quinn implementation. Consider adding a parameter or dedicated method in the future if quiche needs this capability.
💡 Optional future enhancement
Add an optional parameter to support subprotocol selection:
-pub async fn respond(&mut self, status: http::StatusCode) -> Result<(), ConnectError> { +pub async fn respond(&mut self, status: http::StatusCode, subprotocol: Option<String>) -> Result<(), ConnectError> { let response = ConnectResponse { status, - subprotocol: None, + subprotocol, };Or add a dedicated method:
pub async fn respond_with_subprotocol( &mut self, status: http::StatusCode, subprotocol: Option<String> ) -> Result<(), ConnectError> { let response = ConnectResponse { status, subprotocol }; response.write(&mut self.send).await?; Ok(()) }
83-86: Client cannot propose subprotocols in quiche.The client always sends an empty
subprotocolsvector, preventing subprotocol negotiation. Additionally, the server's selectedresponse.subprotocol(line 91) is read but not stored or returned, so the client has no way to access the negotiated subprotocol even if the server were to select one.Per the PR objectives, full subprotocol support is currently limited to the quinn implementation. Consider extending the API if quiche requires this feature in the future.
💡 Optional future enhancement
Add a dedicated method to support client-side subprotocol proposals:
pub async fn open_with_subprotocols( conn: &ez::Connection, url: Url, subprotocols: Vec<String> ) -> Result<(Self, Option<String>), ConnectError> { let (mut send, mut recv) = conn.open_bi().await?; let request = ConnectRequest { url, subprotocols }; request.write(&mut send).await?; let response = web_transport_proto::ConnectResponse::read(&mut recv).await?; if response.status != http::StatusCode::OK { return Err(ConnectError::Status(response.status)); } let selected_subprotocol = response.subprotocol.clone(); Ok((Self { request, send, recv }, selected_subprotocol)) }web-transport-quinn/Cargo.toml (1)
55-55: Consider using minor version pinning for dev dependency.For dev-dependencies, using
rcgen = "0.14"would allow compatible patch updates while still ensuring API stability. Pinning to a specific patch version (0.14.6) is more restrictive than typically needed for development dependencies.♻️ Suggested change
-rcgen = "0.14.6" +rcgen = "0.14"web-transport-proto/src/connect.rs (2)
117-125: Consider filtering empty subprotocol entries.If the header value is empty or contains consecutive commas (e.g.,
"echo/0,,ping/0"), this will produce empty strings in thesubprotocolsvector. Consider filtering them out.♻️ Suggested change
let subprotocols = if let Some(subprotocols) = headers.get(protocol_negotiation::AVAILABLE_NAME) { subprotocols .split(',') - .map(|s| s.trim().to_string()) + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) .collect::<Vec<_>>() } else { Vec::new() };
210-217: Consider trimming the subprotocol value.For consistency with the request parsing (which trims whitespace), consider trimming the response subprotocol value as well.
♻️ Suggested change
let subprotocol = headers .get(protocol_negotiation::SELECTED_NAME) - .map(|s| s.to_string()); + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty());web-transport-quinn/examples/multiproto-server.rs (1)
128-130: Remove duplicate logging.Lines 129 and 130 both log the sent message. Remove the redundant log statement.
♻️ Suggested change
send.write_all(&msg).await?; tracing::info!(msg = %String::from_utf8_lossy(&msg), " ➡️ sent"); - tracing::info!("send: {}", String::from_utf8_lossy(&msg));web-transport-quinn/src/session.rs (1)
315-317: Consider returningOption<&str>for more idiomatic API.Returning
Option<&str>instead ofOption<&String>is more idiomatic in Rust and provides more flexibility for callers.♻️ Suggested change
- pub fn subprotocol(&self) -> Option<&String> { - self.subprotocol.as_ref() + pub fn subprotocol(&self) -> Option<&str> { + self.subprotocol.as_deref() }web-transport-quinn/src/connect.rs (1)
70-85: Implementation is correct; consider minor optimization.The logic correctly ensures transactional semantics by only updating
self.subprotocolafter the write succeeds. However, the subprotocol is handled twice (cloned on line 77, then the original assigned on line 82).♻️ Optional optimization to avoid cloning subprotocol parameter
- let resp = ConnectResponse { - status, - subprotocol: subprotocol.clone(), - }; + self.subprotocol = subprotocol.clone(); + let resp = ConnectResponse { + status, + subprotocol, + }; tracing::debug!(?resp, "sending CONNECT response"); resp.write(&mut self.send).await?; - self.subprotocol = subprotocol;Note: This still requires one clone but moves it earlier. The current implementation is also correct and the performance difference is negligible for
Option<String>.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
web-transport-proto/src/connect.rsweb-transport-quiche/src/h3/connect.rsweb-transport-quinn/Cargo.tomlweb-transport-quinn/examples/README.mdweb-transport-quinn/examples/multiproto-client.rsweb-transport-quinn/examples/multiproto-server.rsweb-transport-quinn/src/client.rsweb-transport-quinn/src/connect.rsweb-transport-quinn/src/server.rsweb-transport-quinn/src/session.rs
🧰 Additional context used
🧬 Code graph analysis (5)
web-transport-quinn/src/client.rs (1)
web-transport-quinn/src/session.rs (4)
url(311-313)new(56-96)new(368-390)connect_with_subprotocols(127-143)
web-transport-quinn/examples/multiproto-client.rs (3)
web-transport-quinn/src/server.rs (3)
url(167-169)new(34-40)new(106-111)web-transport-quinn/src/session.rs (3)
url(311-313)new(56-96)new(368-390)web-transport-quinn/src/client.rs (2)
new(40-45)new(210-212)
web-transport-proto/src/connect.rs (4)
web-transport-quinn/src/session.rs (4)
new(56-96)new(368-390)url(311-313)subprotocol(315-317)web-transport-quiche/src/h3/connect.rs (1)
url(112-114)web-transport-quinn/src/connect.rs (2)
url(131-133)subprotocol(126-128)web-transport-quiche/src/h3/request.rs (1)
url(30-32)
web-transport-quinn/src/connect.rs (1)
web-transport-proto/src/connect.rs (1)
subprotocols(119-122)
web-transport-quinn/src/server.rs (3)
web-transport-quinn/src/connect.rs (1)
deref(46-48)web-transport-quinn/src/session.rs (3)
deref(323-325)new(56-96)new(368-390)web-transport-quiche/src/ez/connection.rs (1)
deref(214-216)
🪛 markdownlint-cli2 (0.18.1)
web-transport-quinn/examples/README.md
24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (20)
web-transport-quinn/examples/README.md (1)
17-40: Documentation is clear and well-structured.The "Multiprotocol negotiation" section effectively documents the new subprotocol negotiation feature with clear instructions for running the server and client examples. The reference to draft-14 of the WebTransport spec is helpful, and the warning about the insecure setup is appropriate for an example.
web-transport-proto/src/connect.rs (3)
11-20: LGTM - protocol negotiation header constants.The module correctly defines the WebTransport subprotocol negotiation headers per draft-14. The constants are appropriately scoped within the module.
159-164: LGTM - subprotocols encoding.The encoding correctly emits the header only when subprotocols are present and uses proper comma-separated format.
240-242: LGTM - subprotocol response encoding.The encoding correctly emits the selected protocol header only when a subprotocol was negotiated.
web-transport-quinn/examples/multiproto-client.rs (2)
42-54: Protocol negotiation validation looks correct.The client properly validates that:
- The server responded with a protocol
- The selected protocol matches what was requested
This is good defensive coding for subprotocol negotiation.
1-79: LGTM - well-structured example client.The example demonstrates the complete subprotocol negotiation flow clearly, with proper error handling for negotiation failures.
web-transport-quinn/src/client.rs (2)
214-217: LGTM - backward-compatible API change.The existing
connect()method cleanly delegates to the newconnect_with_subprotocols()with an empty subprotocols list, maintaining backward compatibility.
219-260: LGTM - new subprotocol connection method.The new
connect_with_subprotocols()method properly threads the subprotocols parameter through to the session layer.web-transport-quinn/src/server.rs (3)
94-100: LGTM - Deref implementation for Server.The
Dereftoquinn::Endpointprovides ergonomic access to the underlying endpoint, consistent with patterns used elsewhere in the codebase (e.g.,Sessionderefs toquinn::Connection).
141-147: LGTM - Deref implementation for Request.The
DereftoConnectallows direct access to connection details likesubprotocolswithout needing additional accessor methods.
172-183: LGTM - protocol-aware session acceptance.The split between
ok()(no protocol) andok_with_protocol()provides a clean API for servers that do and don't participate in subprotocol negotiation.web-transport-quinn/examples/multiproto-server.rs (1)
71-81: LGTM - protocol selection logic.The server correctly iterates through client-proposed protocols and selects the first one it supports, returning
BAD_REQUESTif none match.web-transport-quinn/src/session.rs (2)
51-52: LGTM - subprotocol field addition.The new field correctly captures the negotiated subprotocol from the connect handshake.
119-143: LGTM - connect methods with subprotocol support.The refactoring cleanly separates the subprotocol-aware path while maintaining backward compatibility through delegation.
web-transport-quinn/src/connect.rs (6)
1-2: LGTM!Import is correctly added to support the new Deref implementation below.
33-34: LGTM!The new field is well-documented and appropriately typed to store the negotiated subprotocol.
43-49: LGTM!The Deref implementation provides ergonomic access to ConnectRequest fields. While Deref is typically reserved for smart pointer types, using it here for convenience is an acceptable design choice that improves API usability.
62-66: LGTM!Correctly initializes
subprotocoltoNonesince the server hasn't selected a subprotocol yet at this stage.
87-115: LGTM!The client-side subprotocol negotiation flow is correctly implemented:
- Sends the list of proposed subprotocols in the request
- Receives the server's selected subprotocol in the response
- Stores the negotiated subprotocol in the Connect struct
125-128: LGTM!The accessor returns an owned
Option<String>, which is ergonomic for callers and avoids lifetime complexity. The clone cost is negligible for this use case.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @web-transport-quinn/examples/multiproto-server.rs:
- Around line 128-130: Two duplicate logs are emitted after sending data: the
tracing::info! call with msg = %String::from_utf8_lossy(&msg) and the subsequent
tracing::info!("send: {}", String::from_utf8_lossy(&msg)); remove one of them
(keep the structured tracing::info! with the msg field or the plain formatted
one) so only a single log remains immediately after send.write_all(&msg). Ensure
the remaining log still uses String::from_utf8_lossy(&msg) for safe UTF-8
handling.
🧹 Nitpick comments (5)
web-transport-proto/src/connect.rs (1)
119-127: Consider filtering empty subprotocol entries.When parsing the header, malformed input like
"echo/0,,ping/0"or",echo/0"would produce empty strings in the subprotocols vector. Consider filtering these out.♻️ Suggested improvement
let subprotocols = if let Some(subprotocols) = headers.get(protocol_negotiation::AVAILABLE_NAME) { subprotocols .split(',') - .map(|s| s.trim().to_string()) + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) .collect::<Vec<_>>() } else { Vec::new() };web-transport-quinn/examples/multiproto-server.rs (3)
26-29: Consider using?instead ofunwrap()for consistency.The certificate generation uses
unwrap()which would panic on failure, while other error cases in the example use?for propagation.♻️ Suggested improvement
- let cert = rcgen::generate_simple_self_signed(vec!["localhost".into()]).unwrap(); + let cert = rcgen::generate_simple_self_signed(vec!["localhost".into()]) + .context("failed to generate self-signed certificate")?;
59-69: Consider using&strinstead of&Stringfor more flexibility.Implementing
TryFrom<&str>is more idiomatic and flexible, as it works with both&Stringand&strvia deref coercion.♻️ Suggested improvement
-impl TryFrom<&String> for Protocol { +impl TryFrom<&str> for Protocol { type Error = (); - fn try_from(value: &String) -> Result<Self, Self::Error> { - match value.as_str() { + fn try_from(value: &str) -> Result<Self, Self::Error> { + match value { "echo/0" => Ok(Protocol::EchoV0), "ping/0" => Ok(Protocol::PingV0), _ => Err(()), } } }Then update the usage at line 76:
- .filter_map(|subprotocol| Protocol::try_from(subprotocol).ok()) + .filter_map(|subprotocol| Protocol::try_from(subprotocol.as_str()).ok())
83-110: Consider extracting protocol strings to avoid duplication.The protocol strings
"echo/0"and"ping/0"are duplicated between theTryFromimplementation and theok_with_protocolcalls. Consider adding a method onProtocolto return the string representation.♻️ Suggested improvement
impl Protocol { fn as_str(&self) -> &'static str { match self { Protocol::EchoV0 => "echo/0", Protocol::PingV0 => "ping/0", } } }Then use it:
Protocol::EchoV0 => { let session = request - .ok_with_protocol("echo/0".to_owned()) + .ok_with_protocol(protocol.as_str().to_owned())web-transport-quinn/src/connect.rs (1)
129-132: Minor API inconsistency with Session::subprotocol().
Connect::subprotocol()returnsOption<String>(owned), whileSession::subprotocol()returnsOption<&String>(reference) as shown in the relevant snippets. Consider aligning the return types for API consistency.If intentional (e.g.,
Connectis typically short-lived whileSessionis long-lived), this is fine. Otherwise:♻️ Suggested improvement for consistency
- pub fn subprotocol(&self) -> Option<String> { - self.subprotocol.clone() + pub fn subprotocol(&self) -> Option<&String> { + self.subprotocol.as_ref() }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
web-transport-proto/src/connect.rsweb-transport-quinn/examples/README.mdweb-transport-quinn/examples/multiproto-server.rsweb-transport-quinn/src/connect.rsweb-transport-quinn/src/server.rs
✅ Files skipped from review due to trivial changes (1)
- web-transport-quinn/examples/README.md
🧰 Additional context used
🧬 Code graph analysis (3)
web-transport-quinn/examples/multiproto-server.rs (3)
web-transport-quinn/src/session.rs (2)
url(311-313)subprotocol(315-317)web-transport-quinn/src/connect.rs (2)
url(135-137)subprotocol(130-132)web-transport-quinn/src/recv.rs (1)
read_to_end(60-62)
web-transport-quinn/src/server.rs (3)
web-transport-quinn/src/connect.rs (1)
deref(46-48)web-transport-quinn/src/session.rs (5)
deref(323-325)new(56-96)new(368-390)close(263-273)close(550-552)web-transport-quiche/src/ez/lock.rs (1)
deref(49-51)
web-transport-proto/src/connect.rs (3)
web-transport-quinn/src/session.rs (2)
url(311-313)subprotocol(315-317)web-transport-quinn/src/connect.rs (2)
url(135-137)subprotocol(130-132)web-transport-quiche/src/h3/connect.rs (1)
url(112-114)
🔇 Additional comments (13)
web-transport-proto/src/connect.rs (4)
11-20: Well-structured protocol negotiation constants.The module cleanly encapsulates the header names per the WebTransport draft-14 spec reference. The documentation linking to the spec is helpful for future maintainers.
161-166: LGTM!Correctly emits the
wt-available-protocolsheader only when subprotocols are present, using comma-space separation which is idiomatic for HTTP list headers.
214-221: LGTM!The response decoding correctly extracts the optional
wt-protocolheader and stores it in the response struct.
244-246: LGTM!The response encoding correctly emits the
wt-protocolheader only when a subprotocol was selected.web-transport-quinn/examples/multiproto-server.rs (2)
1-12: LGTM!Clean imports and CLI argument structure. The default IPv6 wildcard address with a non-privileged port is appropriate for an example.
146-164: LGTM!The ping protocol implementation correctly handles the ping/ack flow and gracefully closes the session on invalid commands.
web-transport-quinn/src/server.rs (4)
94-100: LGTM!The
Derefimplementation forServerprovides convenient access to the underlyingquinn::Endpoint, consistent with patterns used elsewhere in the codebase (e.g.,Sessionderefs toquinn::Connection).
141-147: LGTM!The
DereftoConnectenables ergonomic access to request properties likesubprotocolsandurl(), as demonstrated in the multiproto-server example.
172-183: LGTM!The
ok()andok_with_protocol()methods provide a clean API for accepting sessions with or without subprotocol selection. The separation is intuitive for consumers.
186-189: LGTM!The
close()method correctly passesNonefor the protocol parameter when rejecting a session.web-transport-quinn/src/connect.rs (3)
43-49: LGTM!The
Derefimplementation enables ergonomic access toConnectRequestfields, aligning with the pattern used elsewhere in the codebase.
70-85: LGTM!The
respond()method correctly constructs the response with the optional subprotocol and stores it locally for later retrieval viasubprotocol().
87-119: LGTM!The
open()method correctly propagates subprotocols in the request and captures the server-selected subprotocol from the response. The documentation clearly explains the subprotocol behavior.
gnunicorn
force-pushed
the
ben-protocols-2
branch
from
70ba7ad to
9d3f895
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @web-transport-proto/src/connect.rs:
- Around line 119-127: When building subprotocols from
headers.get(protocol_negotiation::AVAILABLE_NAME), filter out empty protocol
names after splitting and trimming to avoid inserting "" entries (which happen
for values like "foo,,bar" or "foo, , bar"); change the chain that currently
does .split(',').map(|s| s.trim().to_string()).collect::<Vec<_>>() to trim each
piece, then filter out empty strings (e.g., .map(|s| s.trim()).filter(|s|
!s.is_empty()).map(|s| s.to_string()) ) so the resulting subprotocols Vec
contains only non-empty protocol names.
- Around line 161-166: The header value for protocol_negotiation::AVAILABLE_NAME
is currently set from self.subprotocols.join(", ") which yields unquoted tokens;
update the encode method so subprotocols are serialized as a Structured Field
List (quoted-string items separated by ", ") before calling headers.set; either
use the sfv crate to build a List of SFV quoted-strings from self.subprotocols
or manually escape/quote each protocol and join with ", " to produce a value
like "\"proto1\", \"proto2\"", then pass that string to headers.set to comply
with RFC 9651.
🧹 Nitpick comments (2)
web-transport-quinn/src/server.rs (1)
141-147: Consider the deref chain implications.Request derefs to Connect, which itself derefs to ConnectRequest (per web-transport-quinn/src/connect.rs lines 43-49). While this provides convenient access to all layers, it creates a three-level deref chain that might be less discoverable than explicit accessor methods.
This is acceptable but worth considering for API clarity.
web-transport-quinn/src/connect.rs (1)
129-132: Consider returning a reference to avoid cloning.The method clones the String on every call. Since subprotocol strings are typically small and accessed infrequently, the performance impact is minimal. However, returning
Option<&str>would be more idiomatic and avoid unnecessary allocations.♻️ Alternative implementation returning a reference
- pub fn subprotocol(&self) -> Option<String> { - self.subprotocol.clone() + pub fn subprotocol(&self) -> Option<&str> { + self.subprotocol.as_deref() }Note: This would be a breaking change to the public API, so consider for a future refactor if the clone overhead becomes a concern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
web-transport-proto/src/connect.rsweb-transport-quinn/examples/README.mdweb-transport-quinn/examples/multiproto-server.rsweb-transport-quinn/src/connect.rsweb-transport-quinn/src/server.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- web-transport-quinn/examples/multiproto-server.rs
🧰 Additional context used
🧬 Code graph analysis (2)
web-transport-proto/src/connect.rs (3)
web-transport-quinn/src/session.rs (2)
url(311-313)subprotocol(315-317)web-transport-quinn/src/connect.rs (2)
url(135-137)subprotocol(130-132)web-transport-quiche/src/h3/connect.rs (1)
url(112-114)
web-transport-quinn/src/server.rs (1)
web-transport-quiche/src/h3/request.rs (1)
close(41-44)
🔇 Additional comments (13)
web-transport-quinn/examples/README.md (1)
17-40: Documentation accurately reflects the implementation.The "Multiprotocol negotiation" section is well-structured and comprehensive. The reference to draft-14 is valid and accessible, the port 56789 default matches the example implementations, and the security warnings are appropriately placed. Instructions for running both server and client are clear with concrete examples.
web-transport-proto/src/connect.rs (4)
11-20: LGTM! Clear specification reference.The header constants are well-documented with a direct reference to the spec section.
76-80: LGTM! Breaking change is expected.The new field is properly documented and the Vec type is appropriate for the protocol list.
188-192: LGTM! Clean implementation of response field.The optional subprotocol field is appropriately typed and the decode logic correctly handles the presence or absence of the header.
Also applies to: 214-221
244-246: LGTM! Proper handling of optional field.The conditional emission of the header when a protocol is selected is correct.
web-transport-quinn/src/server.rs (3)
94-100: LGTM! Ergonomic access to underlying endpoint.The Deref implementation allows convenient access to
quinn::Endpointmethods while maintaining the wrapper abstraction.
172-183: LGTM! Clear API for protocol negotiation.The separation between
ok()(no protocol) andok_with_protocol()(with protocol) provides a clean, explicit API for accepting sessions with or without protocol negotiation.
186-189: LGTM! Correctly updated for new signature.The close method appropriately passes None for the protocol parameter when rejecting the session.
web-transport-quinn/src/connect.rs (5)
1-1: LGTM! Provides convenient access to request fields.The Deref implementation allows Connect to be used as a ConnectRequest, providing ergonomic access to the URL and subprotocols.
Also applies to: 43-49
33-34: LGTM! Appropriate field for storing negotiated protocol.The Option type correctly represents an optional negotiated subprotocol.
52-67: LGTM! Correct initialization for server-side accept.The subprotocol is appropriately initialized to None since negotiation hasn't occurred yet at accept time.
70-85: LGTM! Proper protocol negotiation in response.The method correctly accepts the subprotocol parameter, includes it in the response, and stores it for future access.
87-119: LGTM! Complete client-side protocol negotiation.The open method properly handles the full negotiation flow: sending proposed protocols and receiving the server's selection. The documentation clearly explains the behavior.
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Can you double check that coderabbit feedback, otherwise LGTM.
web-transport-proto/src/connect.rs
Outdated
| /// The URL to connect to. | ||
| pub url: Url, | ||
| /// The subprotocols requested (if any). | ||
| pub subprotocols: Vec<String>, |
There was a problem hiding this comment.
nit: I would call this protocols instead, which also matches the header name.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-transport-quinn/src/connect.rs (1)
105-118: Validate the server-selected protocol against the offered list.Lines 113–116 accept any
response.protocolwithout validation. Draft-14 specifies that the server MUST select a protocol from the client's WT-Available-Protocols list; if the server selects a protocol not in that list, the WT-Protocol field MUST be ignored. The current code violates this requirement.Proposed fix (ignore invalid server selection)
- Ok(Self { - request, - protocol: response.protocol, - send, - recv, - }) + let mut protocol = response.protocol; + if let Some(p) = protocol.as_ref() { + if !request.protocols.iter().any(|offered| offered == p) { + protocol = None; // ignore invalid selection + } + } + + Ok(Self { + request, + protocol, + send, + recv, + })
🤖 Fix all issues with AI agents
In `@web-transport-proto/src/connect.rs`:
- Around line 123-146: The parsing currently uses filter_map over the parsed
sfv::List and silently drops non-string entries; change this so that if any list
member is not a string the whole WT-Available-Protocols field is treated as
invalid (i.e., cause the outer option to be None). Concretely, in the block that
binds protocols (the code using
headers.get(protocol_negotiation::AVAILABLE_NAME) ->
sfv::Parser::new(...).parse::<sfv::List>() and then protocols.into_iter()),
replace the filter_map iteration with a pass that checks every sfv::ListEntry:
if any entry is not an sfv::ListEntry::Item whose bare_item is
sfv::BareItem::String, abort and return None for the parsed field; otherwise
collect the strings into the Vec<String> as before. This preserves the existing
parse-ok behavior but enforces the draft-14 rule to invalidate the entire field
on any non-string member.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
web-transport-proto/Cargo.tomlweb-transport-proto/src/connect.rsweb-transport-quiche/src/h3/connect.rsweb-transport-quinn/examples/multiproto-server.rsweb-transport-quinn/src/connect.rsweb-transport-quinn/src/session.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- web-transport-quinn/examples/multiproto-server.rs
🧰 Additional context used
🧬 Code graph analysis (3)
web-transport-proto/src/connect.rs (2)
web-transport-quinn/src/session.rs (1)
url(311-313)web-transport-quiche/src/h3/connect.rs (1)
url(112-114)
web-transport-quiche/src/h3/connect.rs (4)
web-transport-quinn/src/connect.rs (1)
url(135-137)web-transport-quinn/src/server.rs (1)
url(167-169)web-transport/src/quinn.rs (1)
url(154-156)web-transport-quiche/src/h3/request.rs (1)
url(30-32)
web-transport-quinn/src/session.rs (2)
web-transport-quinn/src/client.rs (4)
connect(215-217)connect_with_subprotocols(220-260)new(40-45)new(210-212)web-transport-quinn/src/connect.rs (2)
url(135-137)open(91-119)
🔇 Additional comments (8)
web-transport-proto/src/connect.rs (4)
11-84: Protocol negotiation wiring looks coherent.
The header constants, new error variant, and request struct extension align cleanly with the negotiation flow.
172-201: Encoding path is solid.
Error propagation and conditional header emission look good.
251-301: Response protocol handling is consistent.
Optional protocol is decoded/encoded cleanly with the new error propagation.
328-433: Great coverage of encode/decode edge cases.
The tests cover empty, quoted, and non-compliant protocol values well.web-transport-proto/Cargo.toml (1)
19-20: No review comment for this manifest change.web-transport-quiche/src/h3/connect.rs (1)
63-86: Quiche handshake updated correctly for protocol negotiation.
Defaults are wired in cleanly.web-transport-quinn/src/connect.rs (1)
33-35: Protocol state is plumbed through accept/respond/open cleanly.
Storing and exposing the negotiated protocol is straightforward.Also applies to: 69-83, 129-132
web-transport-quinn/src/session.rs (1)
51-84: Session subprotocol tracking is consistent and well integrated.
New connect path and accessor are clear.Also applies to: 119-137, 307-317
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
gnunicorn
force-pushed
the
ben-protocols-2
branch
from
bf2a90a to
1ffa0a5
Compare
…b protocol negotiation. With parsing tests
gnunicorn
force-pushed
the
ben-protocols-2
branch
from
1ffa0a5 to
6b174ff
Compare
2 participants
A fresh attempt to implement webtransport protocol negotiation according to draft 14 of the spec in the quinn implementation. Refs #16 .
This is a much simpler PR compared to #139 this superseeds, not touching the qpack but just expects there to be one header (last one wins) with a comma separated list of entries or a single entry in case of the response, on the initial
CONNECTmethod.Implementation notes:
pub subprotocols: Vec<String>,fieldConnectRequestof the proto crate, that if not empty is included in the header (as a single entry, the protocols are concatenated with a comma)pub subprotocol: Option<String>,is available on theConnectResponseof the proto crate