Content-Type gets wrongfully added to GET request after 303 redirects #16508

sysrqb · 2023-08-09T03:42:42Z

`9b8c53b`

Content-Type gets wrongfully added to GET request after 303 redirects
https://bugs.webkit.org/show_bug.cgi?id=258720
rdar://111899420

Reviewed by Chris Dumez.

In 252713@main we began copying the Content-Type header from the original
request if we receive a redirect with a 303 response status. However, we may be
handling a subsequent redirect, and in some cases the Content-Type header is
removed from redirect requests. For example, if we transition from a POST to a
GET request, Fetch 4.4.12 [0] says that the Content-Type header should be
deleted. WebKit currently follows this specification, but we re-introduce the
header on a second redirect because we consult the first request instead of the
previous request.

This patch introduces NetworkDataTask::m_previousRequest to keep track of the
previous request. This is duplicate information that is stored in NetworkLoad,
as well, but I'm leaving merging these two copies for a follow-up patch.

[0] https://fetch.spec.whatwg.org/#http-redirect-fetch

* LayoutTests/imported/w3c/web-platform-tests/xhr/resources/redirect.py:
(main):
* LayoutTests/imported/w3c/web-platform-tests/xhr/send-redirect-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/xhr/send-redirect.htm:
* Source/WebCore/platform/network/ResourceHandleInternal.h:
* Source/WebCore/platform/network/mac/ResourceHandleMac.mm:
(WebCore::ResourceHandle::willSendRequest):
* Source/WebKit/NetworkProcess/NetworkDataTask.h:
* Source/WebKit/NetworkProcess/NetworkLoad.h:
* Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:
(WebKit::NetworkDataTaskCocoa::willPerformHTTPRedirection):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm:
(TEST):

Canonical link: https://commits.webkit.org/266761@main

3605a61

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-08-09T03:42:51Z

EWS run on previous version of this PR (hash 00292cf)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

cdumez

This is breaking compatibility with other browsers in at least some aspects.

Consider this test change:

diff --git a/LayoutTests/imported/w3c/web-platform-tests/xhr/send-redirect.htm b/LayoutTests/imported/w3c/web-platform-tests/xhr/send-redirect.htm
index 16b3231e25b4..6e4ca3d4f89d 100644
--- a/LayoutTests/imported/w3c/web-platform-tests/xhr/send-redirect.htm
+++ b/LayoutTests/imported/w3c/web-platform-tests/xhr/send-redirect.htm
@@ -9,28 +9,45 @@
   <body>
     <div id="log"></div>
     <script>
-      function redirect(code) {
-        var test = async_test(document.title + " (" + code + ")")
+      function redirect(code, method) {
+        var test = async_test(document.title + " (" + code + ", " + method + ")")
         test.step(function() {
           var client = new XMLHttpRequest()
           client.onreadystatechange = function() {
             test.step(function() {
               if(client.readyState == 4) {
-                assert_equals(client.getResponseHeader("x-request-method"), "GET")
-                assert_equals(client.getResponseHeader("x-request-content-type"), "application/x-pony")
+                let expected_method = method;
+                if (method == "POST" && (code == "301" || code == "302" || code == "303"))
+                  expected_method = "GET";
+                assert_equals(client.getResponseHeader("x-request-method"), expected_method)
+                let expected_content_type = "application/x-pony";
+                if (method == "POST" && (code == "301" || code == "302" || code == "303"))
+                  expected_content_type = "NO";
+                assert_equals(client.getResponseHeader("x-request-content-type"), expected_content_type);
                 test.done()
               }
             })
           }
-          client.open("GET", "resources/redirect.py?location=content.py&code=" + code)
+          client.open(method, "resources/redirect.py?location=content.py&code=" + code)
           client.setRequestHeader("Content-Type", "application/x-pony")
           client.send(null)
         })
       }
-      redirect("301")
-      redirect("302")
-      redirect("303")
-      redirect("307")
+      redirect("301", "GET");
+      redirect("302", "GET");
+      redirect("303", "GET");
+      redirect("307", "GET");
+      redirect("308", "GET");
+      redirect("301", "POST");
+      redirect("302", "POST");
+      redirect("303", "POST");
+      redirect("307", "POST");
+      redirect("308", "POST");
+      redirect("301", "HEAD");
+      redirect("302", "HEAD");
+      redirect("303", "HEAD");
+      redirect("307", "HEAD");
+      redirect("308", "HEAD");
     </script>
   </body>
 </html>

This updated test passes in Chrome, Firefox and shipping Safari. However, your patch regresses the XMLHttpRequest: send() - Redirects (basics) (303, GET) subtest.

webkit-early-warning-system · 2023-08-09T23:42:03Z

EWS run on current version of this PR (hash 3605a61)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

sysrqb · 2023-08-09T23:47:09Z

Thanks, that's a good point. My reasoning for that change was actually wrong because the HTTP Semantics spec talks about sending content in a GET request, it is not talking about the Content-Type header. Therefore, sending the Content-Type header in a GET request is allowed, except where the Fetch spec says we must remove the Content-Type header on redirect.

sysrqb · 2023-08-09T23:48:23Z

Your modified test was helpful, and I adapted that in this patch. That may not be the best approach, so I'm happy to get feedback on it (maybe create a new test file?) - and I'll upstream any wpt test changes.

cdumez

LGTM

cdumez · 2023-08-09T23:52:37Z

LayoutTests/imported/w3c/web-platform-tests/xhr/send-redirect.htm

@@ -9,20 +9,28 @@
  <body>
    <div id="log"></div>
    <script>
-      function redirect(code) {
-        var test = async_test(document.title + " (" + code + ")")
+      function redirect(code, method="GET", redirectLocation="content.py") {


That's great, please make sure to upstream to WPT or it will get lost during the next re-sync.

web-platform-tests/wpt#41417

cdumez · 2023-08-09T23:55:01Z

Tools/TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm

@@ -1798,3 +1798,60 @@ HTTPServer server({
    while (navigationToExample)
        Util::spinRunLoop();
 }
+
+TEST(WKNavigation, Multiple303Redirects)


Note that in general, we prefer layout tests to API tests whenever possible. This is a case where a layout test was possible I believe.

https://bugs.webkit.org/show_bug.cgi?id=258720 rdar://111899420 Reviewed by Chris Dumez. In 252713@main we began copying the Content-Type header from the original request if we receive a redirect with a 303 response status. However, we may be handling a subsequent redirect, and in some cases the Content-Type header is removed from redirect requests. For example, if we transition from a POST to a GET request, Fetch 4.4.12 [0] says that the Content-Type header should be deleted. WebKit currently follows this specification, but we re-introduce the header on a second redirect because we consult the first request instead of the previous request. This patch introduces NetworkDataTask::m_previousRequest to keep track of the previous request. This is duplicate information that is stored in NetworkLoad, as well, but I'm leaving merging these two copies for a follow-up patch. [0] https://fetch.spec.whatwg.org/#http-redirect-fetch * LayoutTests/imported/w3c/web-platform-tests/xhr/resources/redirect.py: (main): * LayoutTests/imported/w3c/web-platform-tests/xhr/send-redirect-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/xhr/send-redirect.htm: * Source/WebCore/platform/network/ResourceHandleInternal.h: * Source/WebCore/platform/network/mac/ResourceHandleMac.mm: (WebCore::ResourceHandle::willSendRequest): * Source/WebKit/NetworkProcess/NetworkDataTask.h: * Source/WebKit/NetworkProcess/NetworkLoad.h: * Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm: (WebKit::NetworkDataTaskCocoa::willPerformHTTPRedirection): * Tools/TestWebKitAPI/Tests/WebKitCocoa/Navigation.mm: (TEST): Canonical link: https://commits.webkit.org/266761@main

webkit-commit-queue · 2023-08-10T14:13:20Z

Committed 266761@main (9b8c53b): https://commits.webkit.org/266761@main

Reviewed commits have been landed. Closing PR #16508 and removing active labels.

sysrqb requested a review from cdumez as a code owner August 9, 2023 03:42

sysrqb self-assigned this Aug 9, 2023

sysrqb added the Page Loading For bugs in page loading, including handling of network callbacks. label Aug 9, 2023

cdumez requested changes Aug 9, 2023

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 9, 2023

sysrqb removed the merging-blocked Applied to prevent a change from being merged label Aug 9, 2023

sysrqb force-pushed the eng/258720 branch from 00292cf to 3605a61 Compare August 9, 2023 23:41

cdumez approved these changes Aug 9, 2023

View reviewed changes

sysrqb added the merge-queue Applied to send a pull request to merge-queue label Aug 10, 2023

webkit-commit-queue force-pushed the eng/258720 branch from 3605a61 to 9b8c53b Compare August 10, 2023 14:13

webkit-commit-queue merged commit 9b8c53b into WebKit:main Aug 10, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content-Type gets wrongfully added to GET request after 303 redirects #16508

Content-Type gets wrongfully added to GET request after 303 redirects #16508

sysrqb commented Aug 9, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Aug 9, 2023 •

edited

cdumez left a comment

webkit-early-warning-system commented Aug 9, 2023 •

edited

sysrqb commented Aug 9, 2023

sysrqb commented Aug 9, 2023 •

edited

cdumez left a comment

cdumez Aug 9, 2023

sysrqb Aug 10, 2023

cdumez Aug 9, 2023

webkit-commit-queue commented Aug 10, 2023

Content-Type gets wrongfully added to GET request after 303 redirects #16508

Content-Type gets wrongfully added to GET request after 303 redirects #16508

Conversation

sysrqb commented Aug 9, 2023 • edited by webkit-early-warning-system

9b8c53b

webkit-early-warning-system commented Aug 9, 2023 • edited

cdumez left a comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Aug 9, 2023 • edited

sysrqb commented Aug 9, 2023

sysrqb commented Aug 9, 2023 • edited

cdumez left a comment

Choose a reason for hiding this comment

cdumez Aug 9, 2023

Choose a reason for hiding this comment

sysrqb Aug 10, 2023

Choose a reason for hiding this comment

cdumez Aug 9, 2023

Choose a reason for hiding this comment

webkit-commit-queue commented Aug 10, 2023

sysrqb commented Aug 9, 2023 •

edited by webkit-early-warning-system

`9b8c53b`

webkit-early-warning-system commented Aug 9, 2023 •

edited

webkit-early-warning-system commented Aug 9, 2023 •

edited

sysrqb commented Aug 9, 2023 •

edited