Sub-protocol negotiation + breaking API changes #143
Conversation
|
@gnunicorn does this seem reasonable? Im fine making a breaking API change for this. Also |
|
oh, I didn't see you were working on this here. I've implemented it all on the original branch earlier today... well, if we go for But I am fine with either of these ways. |
oof, I should have asked.
I'm generally fine with breaking changes when the fix is "obvious", like new struct fields. But yep we could add a Builder, or turn |
|
@gnunicorn what about this |
WalkthroughAdds the 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web-transport-quiche/src/h3/connect.rs (2)
76-104: Missing protocol validation in clientopen()compared to quinn implementation.The quinn implementation (see
web-transport-quinn/src/connect.rslines 99-132) validates that the server's selected protocol is in the client's requested protocols list:if let Some(protocol) = &response.protocol { if !request.protocols.contains(protocol) { return Err(ConnectError::ProtocolMismatch(protocol.clone())); } }This quiche implementation lacks this validation, which could allow a server to respond with a protocol the client never requested.
🔧 Suggested fix
You'll need to add a
ProtocolMismatchvariant toConnectErrorand add the validation:+ #[error("protocol mismatch: {0}")] + ProtocolMismatch(String),Then in
open():// Throw an error if we didn't get a 200 OK. if response.status != http::StatusCode::OK { return Err(ConnectError::Status(response.status)); } + // Validate that the server's protocol was in our request. + if let Some(protocol) = &response.protocol { + if !request.protocols.contains(protocol) { + return Err(ConnectError::ProtocolMismatch(protocol.clone())); + } + } + Ok(Self {
62-71: Add protocol validation to the server'srespond()method for consistency with quinn.The quinn implementation (web-transport-quinn/src/connect.rs lines 62-65) validates that the server's selected protocol is in the client's request before sending. The quiche implementation doesn't perform this validation. Although the quiche
Connectstruct does store the request, therespond()method skips the validation check that quinn performs.
🤖 Fix all issues with AI agents
In `@web-transport-quinn/examples/README.md`:
- Around line 17-40: The README mentions non-existent examples multiproto-server
and multiproto-client; either remove the "Multiprotocol negotiation" section or
add implementations named multiproto-server and multiproto-client that mirror
echo-server.rs and echo-client.rs but implement WebTransport subprotocol
negotiation (supporting "echo/0" and "ping/0") and use the same port and CLI
flags referenced in the README; ensure example file names match the cargo run
commands and update the README commands if you choose different names.
In `@web-transport-quinn/src/connect.rs`:
- Around line 29-37: Change the visibility of the Connect struct fields from pub
to pub(crate) to restrict access to the crate internals: update the fields
request, send, and recv in the Connect struct so they are declared as pub(crate)
instead of pub, leaving the struct declaration itself unchanged to preserve
internal use within the crate.
In `@web-transport-quinn/src/server.rs`:
- Line 105: Fix the typo in the doc comment above the server constructor that
mentions "manually constructed Endpoint": change "Manaully" to "Manually" in the
comment describing creation of a new server with a manually constructed Endpoint
(the docstring referencing Endpoint/new server).
In `@web-transport-quinn/src/session.rs`:
- Around line 48-52: Fix the typo in the documentation comment above the struct
fields: change "The request send by the client." to "The request sent by the
client." for the request field associated with ConnectRequest (and while there,
verify the response comment above the response field associated with
ConnectResponse is phrased correctly); update the comment that documents the
request field named request (type ConnectRequest) accordingly.
🧹 Nitpick comments (6)
web-transport/src/quinn.rs (1)
76-78: Consider plumbing protocol selection into the response.
If sub-protocol negotiation is expected at this layer, you may want to respond with aConnectResponsethat includes the selected protocol rather than always returningOK.web-transport-proto/src/connect.rs (1)
144-149: Consider preserving error details in protocol decoding.The
.map_err(|_| ConnectError::InvalidProtocol)discards the underlying error fromdecode_list. Sincedecode_listalready returnsConnectError(includingStructuredFieldError), you could simplify this and preserve error information:♻️ Suggested simplification
let protocols = headers .get(protocol_negotiation::AVAILABLE_NAME) .map(protocol_negotiation::decode_list) .transpose() - .map_err(|_| ConnectError::InvalidProtocol)? + ? .unwrap_or_default();web-transport-quiche/src/h3/request.rs (1)
43-47: Consider documenting the asymmetry betweenrespondandclose.
respond()accepts a genericConnectResponsewhileclose()only acceptshttp::StatusCode. This is fine sincecloserepresents rejection (non-200), but a brief doc comment clarifying this would help users understand when to use each method.web-transport-quiche/src/h3/connect.rs (1)
1-1: Inconsistent import style.Line 1 imports from
crate::proto, but line 91 usesweb_transport_proto::ConnectResponse::readdirectly. Consider usingConnectResponse::readsinceConnectResponseis already imported.♻️ Suggested fix
- let response = web_transport_proto::ConnectResponse::read(&mut recv).await?; + let response = ConnectResponse::read(&mut recv).await?;web-transport-quinn/src/session.rs (1)
103-117: Consider accepting only the recv stream instead of the full ConnectComplete.The
run_closedmethod only usesconnect.recv. Consider takingquinn::RecvStreamdirectly to make the dependency clearer. However, this is a minor clarity improvement.♻️ Optional refactor
- async fn run_closed(&mut self, mut connect: ConnectComplete) -> (u32, String) { + async fn run_closed(&mut self, mut recv: quinn::RecvStream) -> (u32, String) { loop { - match web_transport_proto::Capsule::read(&mut connect.recv).await { + match web_transport_proto::Capsule::read(&mut recv).await {And at the call site:
- let (code, reason) = this2.run_closed(connect).await; + let (code, reason) = this2.run_closed(connect.recv).await;web-transport-quinn/src/server.rs (1)
168-184: Consider validating status code inreject().The
reject()method accepts anyhttp::StatusCode, but passingStatusCode::OKwould be semantically incorrect for a rejection. Consider adding validation or documenting that the caller must use a non-2xx status.💡 Option 1: Add validation
/// Reject the session with the given status code. pub async fn reject(self, status: http::StatusCode) -> Result<(), ServerError> { + debug_assert!(!status.is_success(), "reject() should use a non-success status code"); self.connect.respond(ConnectResponse::new(status)).await?; Ok(()) }Alternatively, document the expectation in the docstring.
- Fix typo "send" -> "sent" in session.rs comment - Fix typo "Manaully" -> "Manually" in server.rs docstring - Change Connect struct fields from pub to pub(crate) - Remove non-existent multiproto examples from README Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
no, worries. it's your crate/work and there was radio-silence from me due to other work.
totally your call. I always find non_exhaustive structs/enums kinda annoying if you match or destruct them (which we do here) - then you end up adding do you want me to review your code? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web-transport-quinn/src/server.rs`:
- Around line 168-178: The respond method currently returns a Session even for
non-200 ConnectResponse values; update respond (in impl for the type with method
respond) to inspect the ConnectResponse.status (from the response.into()) and if
it is not 200 OK, return an appropriate ServerError (or call the existing reject
flow) instead of proceeding to create Session via self.connect.respond; only
call self.connect.respond and construct Session::new(self.conn, self.settings,
connect) when status == 200 to ensure server and client agree on session
establishment.
| /// Reply to the session with the given status code. | ||
| /// | ||
| /// If you want to reject the session, use a non-200 status code. | ||
| pub async fn respond( | ||
| self, | ||
| response: impl Into<ConnectResponse>, | ||
| ) -> Result<Session, ServerError> { | ||
| let response = response.into(); | ||
| let connect = self.connect.respond(response).await?; | ||
| Ok(Session::new(self.conn, self.settings, connect)) | ||
| } |
There was a problem hiding this comment.
Avoid returning Session for non‑200 responses.
respond() currently returns a Session even when response.status is non‑OK (the doc comment even suggests using non‑200 to reject). That creates a server/client state mismatch because the client path rejects non‑200 and won’t establish a session. Please enforce 200 OK here (or delegate to reject) and return an error otherwise.
Proposed fix
- /// Reply to the session with the given status code.
- ///
- /// If you want to reject the session, use a non-200 status code.
+ /// Reply with 200 OK to accept the session.
+ ///
+ /// Use `reject` for non-200 responses.
pub async fn respond(
self,
response: impl Into<ConnectResponse>,
) -> Result<Session, ServerError> {
let response = response.into();
+ if response.status != http::StatusCode::OK {
+ // Send the rejection but don't create a Session on the server.
+ self.connect.respond(response).await?;
+ return Err(crate::ConnectError::ErrorStatus(response.status).into());
+ }
let connect = self.connect.respond(response).await?;
Ok(Session::new(self.conn, self.settings, connect))
}🤖 Prompt for AI Agents
In `@web-transport-quinn/src/server.rs` around lines 168 - 178, The respond method
currently returns a Session even for non-200 ConnectResponse values; update
respond (in impl for the type with method respond) to inspect the
ConnectResponse.status (from the response.into()) and if it is not 200 OK,
return an appropriate ServerError (or call the existing reject flow) instead of
proceeding to create Session via self.connect.respond; only call
self.connect.respond and construct Session::new(self.conn, self.settings,
connect) when status == 200 to ensure server and client agree on session
establishment.
I hadn't thought about that before. I generally don't use destructing, but yeah that makes sense. I think If we were constructing only, then yeah the builder pattern only makes more sense. That way you would get destructing support for crate internals. Anyway, I'll merge this when I actually test it in a web browser. Thanks for the contribution and figuring this stuff out! |
|
Thanks again @gnunicorn. It would be cool to support arbitrary headers if/when Chrome adds support. |
UNTESTED
old:
connect_with_subprotocol(url, protocol)new:
connect(ConnectRequest { url, protocol })This will future-proof things a bit more because we can add new stuff to
ConnectRequestandConnectResponse.