Bug 222558 – HTTP method in web inspector network tab is not what WebKit actually sent after a redirect from POST to GET

• Component: Web Inspector
• Status: RESOLVED FIXED

[Michael Catanzaro]

I hope this is only a web inspector bug, but I doubt it. Open the web inspector to the Network tab, load example.com, then copy the request headers. We do this:

GET /
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 I would expect "GET /enter_bug.cgi HTTP/1.1"

[Michael Catanzaro]

(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"

[Michael Catanzaro]

(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!

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 421889 [details]
libsoup test server

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

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

[Radar WebKit Bug Importer]

<rdar://problem/75062111>

[Carlos Garcia Campos]

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 ***

[Michael Catanzaro]

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?

[Carlos Garcia Campos]

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");
}

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 426090 [details]
WIP

This patch fixes it. It's WIP because I don't have time to write a test now.

[Michael Catanzaro]

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?

[Devin Rousso]

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 🤔

[Michael Catanzaro]

(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?

[Patrick Angle]

Created attachment 436703 [details]
WIP /w Test

[Michael Catanzaro]

Thanks Patrick!

[Devin Rousso]

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)

[Patrick Angle]

Created attachment 440871 [details]
Patch v1.0

[Patrick Angle]

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.

[Michael Catanzaro]

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
> [phone]  Michael Catanzaro  <[e-mail]>

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?

[Patrick Angle]

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
>> [phone]  Michael Catanzaro  <[e-mail]>
> 
> 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.

[Patrick Angle]

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

[Michael Catanzaro]

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.

[Patrick Angle]

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.

[EWS]

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].