Bug 222558

(In reply to Michael Catanzaro from comment #0) > But I would expect "GET /enter_bug.cgi HTTP/1.1" Sorry, changed the URL I was using for this bug report. I would expect: "GET / HTTP/1.1"

(In reply to Michael Catanzaro from comment #0) > I hope this is only a web inspector bug, but I doubt it. Running libsoup's example server and connecting via Epiphany, the server prints: $ jhbuild run ./simple-httpd Listening on http://0.0.0.0:42381/ Listening on http://[::]:42381/ Waiting for requests... GET / HTTP/1.1 Host: 0.0.0.0:42381 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Upgrade-Insecure-Requests: 1 User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0 Safari/605.1.15 Accept-Encoding: gzip, deflate Accept-Language: en-US Connection: Keep-Alive -> 200 OK So this is a WebKit bug, not a libsoup bug. What we display in the web inspector is different from what we actually send to the server!

I don't trust the request method displayed either. I have a case where Firefox sends a POST request to some internal website, receives a 302 redirect response, and then sends a GET request to the new location. In contrast, the WebKit web inspector only shows WebKit making a GET request for the original request. It doesn't show the HTTP method used for the second request, but if I copy the request headers I can see it is doing POST when it should be GET. However, the server operator says they are really receiving GET for the second request, not POST! So I think WebKit is doing the right thing, but showing the wrong request method for both the first and second request when the redirect occurs. This is a more complex issue than the example.com test, though, and it occurs on an internal domain that I can't share. I should set up a public test for it to see what is really happening. Anyway, treat it as a tangent: for the purposes of this bug, we should just ensure the web inspector displays HTTP/1.1. In the libsoup simple-httpd testcase, when I right-click the request in the web inspector and "Copy HTTP Request", the copied output is identical to what the server actually receives except for the missing HTTP/1.1.

(In reply to Michael Catanzaro from comment #3) > This is a more complex issue than the example.com test, though, and it > occurs on an internal domain that I can't share. I should set up a public > test for it to see what is really happening. OK I've confirmed this too. I'm attaching a SoupServer test case to this bug. Run the test server, open the web inspector to the network tab, check "Preserve log" so the redirect doesn't erase the logging, then point WebKit to http://127.0.0.1:8080/post_then_redirect and watch the magic happen. What actually happens is this: WebKit will send a GET to /post_then_redirect (request #1), loading a page that contains a form that will submit itself with a POST request (request #2). The test server will respond with a 302 redirect in response to the POST request. WebKit will then send a GET request (request #3) to the redirect URI. But what the network tab of the web inspector displays is different. First, the network inspector shows request #2 as a GET request: it shows "GET /redirect". I will attach a screenshot to prove this. But at *no point* did WebKit ever send "GET /redirect": that's just totally made up by the web inspector! WebKit actually sent "POST /redirect HTTP/1.1". What the server received was: POST /redirect HTTP/1.1 Host: 127.0.0.1:8080 Referer: http://127.0.0.1:8080/post_then_redirect Origin: http://127.0.0.1:8080 Content-Type: application/x-www-form-urlencoded Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Upgrade-Insecure-Requests: 1 User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0 Safari/605.1.15 Accept-Encoding: gzip, deflate Accept-Language: en-US Connection: Keep-Alive Content-Length: 7 foobar= (The Host line might be a libsoup artifact? I presume that isn't really sent. Not certain.) Next, if I select Copy Request Headers in the web inspector, it copies request #3 (seems it always copies the final request, never the redirect), which was actually a GET request. But what it copies to my clipboard is a POST request! It copies this: POST / Referer: http://127.0.0.1:8080/post_then_redirect Origin: http://127.0.0.1:8080 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Upgrade-Insecure-Requests: 1 User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0 Safari/605.1.15 Accept-Encoding: gzip, deflate Accept-Language: en-US Connection: Keep-Alive But what the server actually received for the redirected request was this: GET / HTTP/1.1 Host: 127.0.0.1:8080 Referer: http://127.0.0.1:8080/post_then_redirect Origin: http://127.0.0.1:8080 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Upgrade-Insecure-Requests: 1 User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0 Safari/605.1.15 Accept-Encoding: gzip, deflate Accept-Language: en-US Connection: Keep-Alive I decided to keep this as one bug for now, but can split it into three separate bugs if desired. Because it's really three issues: (1) web inspector doesn't display HTTP/1.1, (2) web inspector displays "GET /redirect" when WebKit really sent "POST /redirect" (screenshot will prove this), (3) web inspector copies "POST /" when WebKit really sent "GET / HTTP/1.1". Whether these are cross-platform bugs or soup backend bugs, I do not know.

Created attachment 421888 [details] Screenshot, web inspector displays "GET /redirect" but WebKit actually sent "POST /redirect"

Created attachment 421889 [details] libsoup test server

I'm not sure this is soup specific, the inspector is notified about the request being sent from the web process, the network process might change the request after that. We would need to add a requstDidSend notification from the network process back to the web process like we do for web sockets.

I asked somebody to test using Safari on macOS and determined that Safari is messing up GET and POST in the same way that WebKitGTK does. So that confirms your guess that this is mostly not platform-specific. However, Safari *does* show "HTTP/1.1", so the missing HTTP version string seems to be a soup backend bug.

This can probably be tested using the new http/wpt/fetch/navigation-post-to-get-origin.html that Youenna added in r273905.

<rdar://problem/75062111>

The problem is that we are not providing the http protocol version to the inspector via network load metrics. *** This bug has been marked as a duplicate of bug 168543 ***

Well I don't doubt that will fix the missing protocol version. I would be surprised if this fixes the wrong method type, though? Currently web inspector shows GET when WebKit actually did a POST. Shall I reopen this for the purpose of fixing that? Or does your change somehow solve that too?

I'm confused about the screenshot because the status line is not shown here without the patch, the whole line is skipped when the protocol is not present: let protocol = this._resource.protocol || ""; let urlComponents = this._resource.urlComponents; if (protocol.startsWith("http/1")) { // HTTP/1.1 request line: // https://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1 let requestLine = `${this._resource.requestMethod} ${urlComponents.path} ${protocol.toUpperCase()}`; this._requestHeadersSection.appendKeyValuePair(requestLine, null, "h1-status"); } else if (protocol === "h2") { // HTTP/2 Request pseudo headers: // https://tools.ietf.org/html/rfc7540#section-8.1.2.3 this._requestHeadersSection.appendKeyValuePair(":method", this._resource.requestMethod, "h2-pseudo-header"); this._requestHeadersSection.appendKeyValuePair(":scheme", urlComponents.scheme, "h2-pseudo-header"); this._requestHeadersSection.appendKeyValuePair(":authority", WI.h2Authority(urlComponents), "h2-pseudo-header"); this._requestHeadersSection.appendKeyValuePair(":path", WI.h2Path(urlComponents), "h2-pseudo-header"); }

Ah!, I see it's the redirect request/response, the line is indeed missing in the final request/response. Let me check...

You are right, this is a different bug. It's not GTK specific, it's an inspector bug. In case of redirection it's using the method of the new request instead of the previous one.

Created attachment 426090 [details] WIP This patch fixes it. It's WIP because I don't have time to write a test now.

Created attachment 426118 [details] Screenshot confirming the fix This patch solves the bug for me, using my manual test. Nice! Since the problem reportedly also affects Safari, maybe a web inspector developer would have time to help with a proper test?

Comment on attachment 426090 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=426090&action=review Nice catch! (In reply to Michael Catanzaro from comment #17) > Since the problem reportedly also affects Safari, maybe a web inspector developer would have time to help with a proper test? The only Web Inspector test that I know of for redirects (or more like that involves redirects) is `LayoutTests/http/tests/inspector/network/resource-timing.html`, specifically the `"Resource.TimingData.Redirect"` test case. You could probably do something like that that looks at the resulting `resource.requestMethod` (and/or other properties) after a redirect that changes it. > Source/WebInspectorUI/UserInterface/Models/Resource.js:681 > - this._redirects.push(new WI.Redirect(oldURL, request.method, oldHeaders, response.status, response.statusText, response.headers, elapsedTime)); > + this._requestMethod = request.method || null; > + this._redirects.push(new WI.Redirect(oldURL, oldMethod, oldHeaders, response.status, response.statusText, response.headers, elapsedTime)); I wonder if maybe we should also have `oldStatus` and `oldStatusText` too, just to be safe 🤔

(In reply to Devin Rousso from comment #18) > The only Web Inspector test that I know of for redirects (or more like that > involves redirects) is > `LayoutTests/http/tests/inspector/network/resource-timing.html`, > specifically the `"Resource.TimingData.Redirect"` test case. You could > probably do something like that that looks at the resulting > `resource.requestMethod` (and/or other properties) after a redirect that > changes it. Any chance a web inspector dev has time to help with a test?

Created attachment 436703 [details] WIP /w Test

Thanks Patrick!

Comment on attachment 436703 [details] WIP /w Test View in context: https://bugs.webkit.org/attachment.cgi?id=436703&action=review > LayoutTests/http/tests/inspector/network/resource-redirect-request-headers.html:22 > + test(resolve, reject) { > + WI.Resource.awaitEvent(WI.Resource.Event.ResponseReceived) NIT: You could rewrite this a bit cleaner with less indentation using `async` ``` async test() { let [event] = await Promise.all([ WI.Resource.awaitEvent(WI.Resource.Event.ResponseReceived), InspectorTest.evaluateInPage(`createRedirectRequest()`), ]); let resource = event.target; ... ``` > LayoutTests/http/tests/inspector/network/resource-redirect-request-headers.html:27 > + InspectorTest.expectEqual(resource.redirects.length, 1, "Resource should have one redirect.") Style: missing `;` (and below too)

Created attachment 440871 [details] Patch v1.0

Comment on attachment 426090 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=426090&action=review >> Source/WebInspectorUI/UserInterface/Models/Resource.js:681 >> + this._redirects.push(new WI.Redirect(oldURL, oldMethod, oldHeaders, response.status, response.statusText, response.headers, elapsedTime)); > > I wonder if maybe we should also have `oldStatus` and `oldStatusText` too, just to be safe 🤔 We actually want the more recent response to the old request we made, so pairing the old request values with the new response values is correct. I've added to the test case to make sure the response status and status text are the expected values for the redirect response we receive as well as for the final response.

Comment on attachment 440871 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=440871&action=review Nice test! > Source/WebInspectorUI/ChangeLog:1 > +2021-10-11 Michael Catanzaro <mcatanzaro@gnome.org> This is Carlos Garcia's fix, not mine! > LayoutTests/http/tests/inspector/network/resource-redirect-request-headers.html:9 > + fetch("resources/delay.py?delay=100", { Why is a delay required?

Comment on attachment 440871 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=440871&action=review >> Source/WebInspectorUI/ChangeLog:1 >> +2021-10-11 Michael Catanzaro <mcatanzaro@gnome.org> > > This is Carlos Garcia's fix, not mine! Oops! Thank you for catching this! >> LayoutTests/http/tests/inspector/network/resource-redirect-request-headers.html:9 >> + fetch("resources/delay.py?delay=100", { > > Why is a delay required? Copy/Paste oversight when writing the test -just confirmed it isn't necessary.

Created attachment 440943 [details] Patch v1.1 - For Landing

Comment on attachment 440943 [details] Patch v1.1 - For Landing View in context: https://bugs.webkit.org/attachment.cgi?id=440943&action=review > LayoutTests/http/tests/inspector/network/resource-redirect-request-headers.html:9 > + fetch("resources/delay.py", { Looks like there is already a resources/redirect.py that you can probably use instead.

Comment on attachment 440943 [details] Patch v1.1 - For Landing View in context: https://bugs.webkit.org/attachment.cgi?id=440943&action=review >> LayoutTests/http/tests/inspector/network/resource-redirect-request-headers.html:9 >> + fetch("resources/delay.py", { > > Looks like there is already a resources/redirect.py that you can probably use instead. `redirect.py` is actually the destination of the redirect that `delay.py` sends in its `Location` header. `redirect.py` does not actually redirect anywhere.

Committed r284139 (242959@main): <https://commits.webkit.org/242959@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440943 [details].