1793822 - Server rejecting WebTransport sessions: HTTP_REQUEST_REJECTED error code or HTTP_REQUEST_REJECTED error code

Status: NEW

(Core :: Networking: HTTP, task, P3)

Description

[Dragana Damjanovic [:dragana]]

Make sure to write tests for: https://datatracker.ietf.org/doc/html/draft-ietf-webtrans-http3-03#section-3.4

Comment 1

[Ed Guloien [:edgul]]

Hey :kershaw, just wanted to confirm my approach to this bug before getting too deep.

My current approach will be to:

1. Modify test to expect SessionCloseReason::Error(Error::HttpRequestRejected.code()). Similar to this test

2. Modify implementation:
   Server calls events.send_stream_stop_sending() with Error::HttpRequestRejected.code(),
   here, probably in place of.

Currently, I'm assuming:

1. The error will propagate to and through client as expected. Though the client may need a bit of re-work for this.
2. send_headers() does not automatically send response to client and needs something to drive the communication. If this is not the case can probably just determine XHR or Http ahead of time to avoid pre-emptive :status 404 broadcast.

Also,
(In reply to Dragana Damjanovic [:dragana] from comment #0)

Make sure to write tests for: https://datatracker.ietf.org/doc/html/draft-ietf-webtrans-http3-03#section-3.4

Is this meaning to write additional cargo tests for this specific case, or to expand our CI tests with xpcshell/wpt? or perhaps both?

Comment 2

[Kershaw Chang [:kershaw]]

(In reply to Ed Guloien [:edgul] from comment #1)

Hey :kershaw, just wanted to confirm my approach to this bug before getting too deep.

My current approach will be to:

1. Modify test to expect SessionCloseReason::Error(Error::HttpRequestRejected.code()). Similar to this test

I think we should not modify the current test. We should add a new test to expect Error::HttpRequestRejected.
This means we need to modify webtransport_session_accept a bit to make the server support return Error::HttpRequestRejected.

2. Modify implementation:
   Server calls events.send_stream_stop_sending() with Error::HttpRequestRejected.code(),
   here, probably in place of.

Yes, I think this is right. Note that we need to make the server support reject a session by sending headers and also by Error::HttpRequestRejected.
So, maybe when the headers in WebTransportSessionAcceptAction::Reject(headers) is empty, we call self.cancel_fetch(stream_id, Error::HttpRequestRejected.code(), conn)?;.

Currently, I'm assuming:

1. The error will propagate to and through client as expected. Though the client may need a bit of re-work for this.

I assume this line at the client side should be called, but we still need to rework a bit to make OnSessionClosed be called.

2. send_headers() does not automatically send response to client and needs something to drive the communication. If this is not the case can probably just determine XHR or Http ahead of time to avoid pre-emptive :status 404 broadcast.

Sorry, I don't understand this. Can you explain more?
Note that neqo doesn't write data to socket, it only prepare the data for you.

Also,
(In reply to Dragana Damjanovic [:dragana] from comment #0)

Make sure to write tests for: https://datatracker.ietf.org/doc/html/draft-ietf-webtrans-http3-03#section-3.4

Is this meaning to write additional cargo tests for this specific case, or to expand our CI tests with xpcshell/wpt? or perhaps both?
Yeah, both.

Comment 3

[Ed Guloien [:edgul]]

Thanks!

Sorry, I don't understand this. Can you explain more?

I think you answered my question with this:

Note that neqo doesn't write data to socket, it only prepare the data for you.

With this being the case, I was thinking maybe we can set :status 404 along with the error HttpRequestRejected. So the original test can stay and we can write an additional cargo test to explicitly test the error. But I will have a closer look. It's possible the header doesn't get sent in the circumstance we Error. If that's the case we will need to do as you suggest and modify the logic slightly to do one or the other.

Comment 4

[Ed Guloien [:edgul]]

Attachment: tests

<deleted>

Comment 5

[Ed Guloien [:edgul]]

Attachment: xpcshell tests for wt session error

<deleted>

Comment 6

[Ed Guloien [:edgul]]

With the changes mentioned in Bug 1814712 Comment 7 and the below changes, I'm not seeing status or codes make it to the WebTransportListener.

Though there are some other calls to OnSessionClosed like this one which is perhaps returning early before we actually attempt to close from server side.

Might my tests be incorrect for my additions to the http3 server?

Server extension:

@@ -441,6 +442,19 @@ impl HttpServer for Http3TestServer {
                                         [Header::new(":status", "404")].to_vec(),
                                     ))
                                     .unwrap();
+                            } else if path == "/rejectwith429" {
+                                session
+                                    .response(&WebTransportSessionAcceptAction::Reject(
+                                        [Header::new(":status", "429")].to_vec(),
+                                    ))
+                                    .unwrap();
+                            } else if path == "/myreject" {
+                                // session
+                                //     .response(&WebTransportSessionAcceptAction::Reject(
+                                //         [Header::new(":status", "429")].to_vec(),
+                                //     ))
+                                //     .unwrap(); // also tried this
+                                session.close_session(2, "test");
                             } else if path == "/closeafter0ms" {
                                 session
                                     .response(&WebTransportSessionAcceptAction::Accept)

and a sample test (not showing tests for /myreject or /RequestRejected, but they are similar):

+++ b/netwerk/test/unit/test_webtransport_simple.js
@@ -179,6 +179,83 @@ async function test_closed(path) {
   await pClose;
 }
 
+add_task(async function test_request_reject_with_429() {
+  let webTransport = NetUtil.newWebTransport();
+
+  let listener = new WebTransportListener().QueryInterface(
+    Ci.WebTransportSessionEventListener
+  );
+
+  let pReady = new Promise(resolve => {
+    listener.ready = resolve;
+  });
+  let pClose = new Promise(resolve => {
+    listener.closed = resolve;
+  });
+  webTransport.asyncConnect(
+    NetUtil.newURI(`https://${host}/rejectwith429`),
+    Services.scriptSecurityManager.getSystemPrincipal(),
+    Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_SEC_CONTEXT_IS_NULL,
+    listener
+  );
+
+  //await pReady; // ready is not called with this path
+  await pClose; // we are getting the close
+});
+

Comment 7

[Kershaw Chang [:kershaw]]

Transfer the NI to self NI and I'll look into this later.