Conversation
WalkthroughThe PR renames CONNECT state types ( 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 1
🤖 Fix all issues with AI agents
In `@web-transport-quiche/src/h3/request.rs`:
- Around line 34-37: The close method in request.rs is calling
self.connect.ok(status) which only sends the response but doesn't finish the
send stream; change it to delegate to self.connect.close(status) so the
Connecting::close implementation (which calls connect.send.finish()) runs;
update the call in pub async fn close(self, status: http::StatusCode) ->
Result<(), ServerError> to call self.connect.close(status).await? and propagate
the Result as before, ensuring the await and error type match the existing
method signature.
🧹 Nitpick comments (6)
web-transport-quiche/examples/echo-server.rs (1)
74-76: Nit:.ok(http::StatusCode::OK)is redundant.The method name
ok()already implies acceptance, so passingStatusCode::OKexplicitly reads as a tautology. If the library API allows calling.ok()without an argument (or always implies 200), that would be cleaner. If the status code parameter exists to allow other 2xx codes, a more neutral method name (e.g.,accept) might be better — but that's a library-level design decision, not something to change here.web-transport/src/quinn.rs (1)
77-77: Consider usingConnectResponse::ok()instead ofhttp::StatusCode::OK.Since the method is already named
ok(), passingConnectResponse::ok()(or even justConnectResponse::ok()with a protocol) would be more idiomatic and self-documenting than rawhttp::StatusCode::OK. Minor nit — current code works correctly via theFrom<StatusCode>impl.web-transport-quinn/examples/echo-server.rs (1)
94-101: Consider using the newConnectResponse::ok()convenience constructor.Line 94 manually constructs
ConnectResponse::new(http::StatusCode::OK), but this PR addsConnectResponse::ok()for exactly this purpose.Suggested simplification
- let mut response = ConnectResponse::new(http::StatusCode::OK); + let mut response = ConnectResponse::ok();web-transport-quiche/src/h3/connect.rs (3)
61-76: Missing protocol validation inok()– inconsistent with quinn counterpart.The quinn version of
Connecting::ok()(web-transport-quinn/src/connect.rslines 66-70) validates that the response protocol is present in the client's request protocols list, returningErr(ConnectError::ProtocolMismatch(...))on mismatch. This quiche version skips that check entirely, which could allow the server to respond with a protocol the client never offered.Consider whether this omission is intentional. If not:
♻️ Add protocol validation to match quinn behavior
pub async fn ok( mut self, response: impl Into<ConnectResponse>, ) -> Result<Connected, ConnectError> { let response = response.into(); + // Validate that our protocol was in the client's request. + if let Some(protocol) = &response.protocol { + if !self.request.protocols.contains(protocol) { + return Err(ConnectError::Proto( + web_transport_proto::ConnectError::ProtocolMismatch(protocol.clone()), + )); + } + } + tracing::debug!(?response, "sending CONNECT"); response.write(&mut self.send).await?;
105-138: Missing protocol validation inopen()– inconsistent with quinn counterpart.Similarly, the quinn version of
Connected::open()(web-transport-quinn/src/connect.rslines 136-139) validates the server's response protocol against the request. This quiche version only checks for a 200 OK status. If the server returns a protocol the client didn't request, this would silently succeed.♻️ Add protocol validation to match quinn behavior
// 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::Proto( + web_transport_proto::ConnectError::ProtocolMismatch(protocol.clone()), + )); + } + } + Ok(Self {
27-36: Nit:recvfield onConnectingis#[allow(dead_code)]– consider if it should be used.The
recvstream is marked dead code inConnectingbut is passed through toConnectedinok(). If it's not read before the response is sent, this is fine as a placeholder for the CONNECT stream lifecycle. Just noting the asymmetry with the quinn version whererecvispub(crate).
| pub async fn close(self, status: http::StatusCode) -> Result<(), ServerError> { | ||
| self.connect.ok(status).await?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Bug: close should delegate to self.connect.close(status), not self.connect.ok(status).
Calling ok(status) sends the response but does not finish the send stream. The Connecting::close method (in web-transport-quiche/src/h3/connect.rs lines 78-82) additionally calls connect.send.finish() to properly tear down the stream. The quinn counterpart (web-transport-quinn/src/server.rs line 171) correctly delegates to self.connect.close(status).
🐛 Proposed fix
pub async fn close(self, status: http::StatusCode) -> Result<(), ServerError> {
- self.connect.ok(status).await?;
+ self.connect.close(status).await?;
Ok(())
}🤖 Prompt for AI Agents
In `@web-transport-quiche/src/h3/request.rs` around lines 34 - 37, The close
method in request.rs is calling self.connect.ok(status) which only sends the
response but doesn't finish the send stream; change it to delegate to
self.connect.close(status) so the Connecting::close implementation (which calls
connect.send.finish()) runs; update the call in pub async fn close(self, status:
http::StatusCode) -> Result<(), ServerError> to call
self.connect.close(status).await? and propagate the Result as before, ensuring
the await and error type match the existing method signature.
There was a problem hiding this comment.
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/client.rs (1)
90-102:⚠️ Potential issue | 🟠 Major
ConnectRequestfields (e.g.protocols) are silently discarded.Line 92 moves only
urlout of theConnectRequest, and the remaining fields are dropped.Connection::connect(line 147 ofconnection.rs) acceptsurl: Url, so any extra request metadata (likeprotocols) never reaches the server.Compare with the other
Client::connectin lines 216–258 of this same file, which passes the fullrequestthrough toSession::connect(conn, request), preserving all fields.
Connection::connectshould acceptimpl Into<ConnectRequest>(orConnectRequestdirectly) and forward it toh3::Connected::open, just like theSessionpath does.Proposed fix sketch
In
connection.rs, updateconnectto take the full request:- pub async fn connect(conn: ez::Connection, url: Url) -> Result<Connection, ClientError> { + pub async fn connect(conn: ez::Connection, request: impl Into<ConnectRequest>) -> Result<Connection, ClientError> { let settings = h3::Settings::connect(&conn).await?; - let connect = h3::Connected::open(&conn, url).await?; + let connect = h3::Connected::open(&conn, request).await?; let session = Connection::new(conn, settings, connect); Ok(session) }Then in
ClientBuilder::connect:- let url = request.url; - let port = url.port().unwrap_or(443); + let port = request.url.port().unwrap_or(443); - let host = match url.host() { - Some(host) => host.to_string(), - None => return Err(ClientError::InvalidUrl(url.to_string())), + let host = match request.url.host() { + Some(host) => host.to_string(), + None => return Err(ClientError::InvalidUrl(request.url.to_string())), }; let conn = self.0.connect(&host, port).await?; - Connection::connect(conn, url).await + Connection::connect(conn, request).awaitweb-transport-quiche/src/connection.rs (1)
147-152:⚠️ Potential issue | 🟠 Major
connectstill acceptsUrl, losing non-URL request fields.This is the root cause of the issue flagged in
client.rs.h3::Connected::openalready acceptsimpl Into<ConnectRequest>, so this method can be updated to forward the full request. This would also let you remove theuse url::Urlimport on line 13 if no other usage remains.Proposed fix
- pub async fn connect(conn: ez::Connection, url: Url) -> Result<Connection, ClientError> { + pub async fn connect(conn: ez::Connection, request: impl Into<ConnectRequest>) -> Result<Connection, ClientError> { // Perform the H3 handshake by sending/reciving SETTINGS frames. let settings = h3::Settings::connect(&conn).await?; // Send the HTTP/3 CONNECT request. - let connect = h3::Connected::open(&conn, url).await?; + let connect = h3::Connected::open(&conn, request).await?; // Return the resulting session with a reference to the control/connect streams. // If either stream is closed, then the session will be closed, so we need to keep them around. let session = Connection::new(conn, settings, connect); Ok(session) }
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 (2)
web-transport-quiche/src/h3/connect.rs (1)
109-142:⚠️ Potential issue | 🟡 MinorMissing response protocol validation in
Connected::open(client-side).The quiche client-side
openonly validates that the response status is 200 OK (line 132), but does not check whether the server'sresponse.protocolwas in the client's originalrequest.protocols. The quinn counterpart (web-transport-quinn/src/connect.rs, lines 135-140) performs this check and returnsProtocolMismatchif violated.This could allow a quiche client to silently accept a server response advertising a protocol it never requested.
🛡️ Proposed fix — add protocol validation
// 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. + // TODO: Add ProtocolMismatch variant to ConnectError if needed. + Ok(Self {web-transport-quinn/src/connect.rs (1)
58-81:⚠️ Potential issue | 🟡 MinorQuinn validates protocol membership before responding; quiche does not — inconsistent implementations.
The quinn
Connecting::respond(lines 66–70) validates that the response protocol is in the client's requested protocols before sending the response. The quiche counterpart (web-transport-quiche/src/h3/connect.rs, lines 65–79) omits this validation entirely, potentially allowing a quiche server to respond with a protocol the client did not request.
🤖 Fix all issues with AI agents
In `@web-transport-quiche/src/h3/request.rs`:
- Around line 41-45: Update the doc comment for the reject method in request.rs
to fix the typo "returing" → "returning" so the comment reads "Reject the
session, returning your favorite HTTP status code."; locate the doc on the pub
async fn reject(self, status: http::StatusCode) -> Result<(), ServerError>
method and correct the misspelling.
🧹 Nitpick comments (1)
web-transport-quiche/src/h3/connect.rs (1)
82-86:rejectpropagatesfinish()error, while quinn's equivalent ignores it.Line 84:
connect.send.finish()?propagates the error, whereas the quinn counterpart usesconnect.send.finish().ok()to silently discard it. Both approaches are defensible, but the inconsistency may be worth noting — if the intent is to best-effort close the stream during rejection, ignoring the error (as quinn does) may be more resilient.
| /// Reject the session, returing your favorite HTTP status code. | ||
| pub async fn close(mut self, status: http::StatusCode) -> Result<(), ServerError> { | ||
| self.connect.respond(status).await?; | ||
| pub async fn reject(self, status: http::StatusCode) -> Result<(), ServerError> { | ||
| self.connect.reject(status).await?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Typo: "returing" → "returning".
Line 41 doc comment has a typo.
📝 Proposed fix
- /// Reject the session, returing your favorite HTTP status code.
+ /// Reject the session, returning your favorite HTTP status code.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Reject the session, returing your favorite HTTP status code. | |
| pub async fn close(mut self, status: http::StatusCode) -> Result<(), ServerError> { | |
| self.connect.respond(status).await?; | |
| pub async fn reject(self, status: http::StatusCode) -> Result<(), ServerError> { | |
| self.connect.reject(status).await?; | |
| Ok(()) | |
| } | |
| /// Reject the session, returning your favorite HTTP status code. | |
| pub async fn reject(self, status: http::StatusCode) -> Result<(), ServerError> { | |
| self.connect.reject(status).await?; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In `@web-transport-quiche/src/h3/request.rs` around lines 41 - 45, Update the doc
comment for the reject method in request.rs to fix the typo "returing" →
"returning" so the comment reads "Reject the session, returning your favorite
HTTP status code."; locate the doc on the pub async fn reject(self, status:
http::StatusCode) -> Result<(), ServerError> method and correct the misspelling.
1 participant
No description provided.