Safari seems to omit the Origin header after a certain type of cross-origin redirect. It should instead be set to null. Test: https://github.com/w3c/web-platform-tests/pull/11164. Specification change: https://github.com/whatwg/fetch/pull/594.
<rdar://problem/40694782>
We should recheck these results now that Bug 182644 has been merged.
Running the test, we indeed set correctly the Origin header in the case of no-cors POST for the initial request in FrameLoader::addHTTPOriginIfNeeded. But we are not doing anything for redirections. We might need to update SubresourceLoader::checkRedirectionCrossOriginAccessControl and NetworkLoadChecker::continueCheckingRequest.
Created attachment 359515 [details] Patch
Created attachment 359611 [details] Patch
Comment on attachment 359611 [details] Patch Attachment 359611 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10811324 New failing tests: http/tests/security/contentSecurityPolicy/form-action-src-redirect-allowed.html http/tests/navigation/post-308-response.html http/tests/security/cors-post-redirect-307-pson.html http/tests/security/cors-post-redirect-308.html http/tests/security/contentSecurityPolicy/form-action-src-redirect-allowed2.html http/tests/security/cors-post-redirect-307.html http/tests/loading/redirect-methods.html http/tests/navigation/post-307-response.html
Created attachment 359613 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 359611 [details] Patch Attachment 359611 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10811343 New failing tests: http/tests/security/contentSecurityPolicy/form-action-src-redirect-allowed.html http/tests/navigation/post-308-response.html http/tests/security/cors-post-redirect-307-pson.html http/tests/security/cors-post-redirect-308.html http/tests/security/contentSecurityPolicy/form-action-src-redirect-allowed2.html http/tests/security/cors-post-redirect-307.html http/tests/navigation/post-307-response.html
Created attachment 359614 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 359611 [details] Patch Attachment 359611 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10811485 New failing tests: http/tests/security/contentSecurityPolicy/form-action-src-redirect-allowed.html http/tests/security/cors-post-redirect-307-pson.html http/tests/security/contentSecurityPolicy/form-action-src-redirect-allowed2.html http/tests/security/cors-post-redirect-307.html http/tests/loading/redirect-methods.html http/tests/navigation/post-307-response.html
Created attachment 359615 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 359611 [details] Patch Attachment 359611 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10811468 New failing tests: http/tests/security/contentSecurityPolicy/form-action-src-redirect-allowed.html http/tests/navigation/post-308-response.html http/tests/security/cors-post-redirect-307-pson.html http/tests/security/cors-post-redirect-308.html http/tests/security/contentSecurityPolicy/form-action-src-redirect-allowed2.html http/tests/security/cors-post-redirect-307.html http/tests/loading/redirect-methods.html http/tests/navigation/post-307-response.html
Created attachment 359616 [details] Archive of layout-test-results from ews117 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 359611 [details] Patch Attachment 359611 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10811476 New failing tests: http/tests/security/contentSecurityPolicy/form-action-src-redirect-allowed.html http/tests/navigation/post-308-response.html http/tests/security/cors-post-redirect-307-pson.html http/tests/security/cors-post-redirect-308.html http/tests/security/contentSecurityPolicy/form-action-src-redirect-allowed2.html http/tests/security/cors-post-redirect-307.html http/tests/navigation/post-307-response.html
Created attachment 359617 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 359618 [details] Patch
Comment on attachment 359618 [details] Patch Attachment 359618 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10812467 New failing tests: http/tests/security/cors-post-redirect-307-pson.html http/tests/security/cors-post-redirect-308.html http/tests/security/cors-post-redirect-307.html
Created attachment 359619 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 359618 [details] Patch Attachment 359618 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10812482 New failing tests: http/tests/security/cors-post-redirect-307-pson.html http/tests/security/cors-post-redirect-308.html http/tests/security/cors-post-redirect-307.html
Created attachment 359620 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 359618 [details] Patch Attachment 359618 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10812523 New failing tests: http/tests/security/cors-post-redirect-307-pson.html http/tests/security/cors-post-redirect-308.html http/tests/security/cors-post-redirect-307.html
Created attachment 359621 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 359618 [details] Patch Attachment 359618 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10812592 New failing tests: http/tests/security/cors-post-redirect-307-pson.html http/tests/security/cors-post-redirect-307.html
Created attachment 359622 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment on attachment 359618 [details] Patch Attachment 359618 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10812540 New failing tests: http/tests/security/cors-post-redirect-307-pson.html http/tests/security/cors-post-redirect-308.html http/tests/security/cors-post-redirect-307.html
Created attachment 359624 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 359638 [details] Patch
Comment on attachment 359638 [details] Patch Attachment 359638 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10820274 New failing tests: http/tests/security/cors-post-redirect-308.html
Created attachment 359640 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 359642 [details] Patch
Comment on attachment 359642 [details] Patch Attachment 359642 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10821936 New failing tests: http/tests/security/cors-post-redirect-308.html
Created attachment 359654 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment on attachment 359642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359642&action=review LGTM but do you know why the Windows tests are failing? I wonder if a similar change should be done in windows-specific code. > LayoutTests/imported/w3c/ChangeLog:8 > + Update improved test result. This is also chaning expectations in http/tests/security to match the new behavior.
(In reply to Frédéric Wang (:fredw) from comment #33) > Comment on attachment 359642 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359642&action=review > > LGTM but do you know why the Windows tests are failing? I wonder if a > similar change should be done in windows-specific code. I don't know unfortunately. I assume curl is used here, so I got the sources and did a quick grep for 307 and 308, and could not find any specific code handling that. Basuke would you have an idea how 307 and 308 behavior could be different here?
(In reply to Rob Buis from comment #34) > (In reply to Frédéric Wang (:fredw) from comment #33) > > Comment on attachment 359642 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=359642&action=review > > > > LGTM but do you know why the Windows tests are failing? I wonder if a > > similar change should be done in windows-specific code. > > I don't know unfortunately. I assume curl is used here, so I got the sources > and did a quick grep for 307 and 308, and could not find any specific code > handling that. Basuke would you have an idea how 307 and 308 behavior could > be different here? The failure is on `apple-win`, not `WinCairo`. They uses apple's CFNetwork. > The attached test failures were seen while running run-webkit-tests on the win-ews. > Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 377817 [details] Patch
Created attachment 377819 [details] Patch
Comment on attachment 377819 [details] Patch Attachment 377819 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12989075 New failing tests: http/tests/security/cors-post-redirect-308.html
Created attachment 377821 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 379342 [details] Patch
Comment on attachment 379342 [details] Patch Attachment 379342 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13058589 New failing tests: http/tests/security/cors-post-redirect-308.html
Created attachment 379345 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 385964 [details] Patch
Created attachment 386542 [details] Patch
Created attachment 386552 [details] Patch
Created attachment 386557 [details] Patch
Comment on attachment 386557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386557&action=review > Source/WebCore/loader/SubresourceLoader.cpp:612 > + request.setHTTPOrigin(m_origin->toString()); The description of the bug is just about adding a null header value but this method is setting the origin header even though m_origin is not changed. Is it expected? Are we covering that case? Also, the method name is appendOriginIfNeeded. appendOriginHeaderIfNeeded would be better. Even with that change, the name could be improved since we are actually setting/overriding the origin header. > Source/WebCore/loader/SubresourceLoader.cpp:649 > + appendOriginIfNeeded(newRequest, redirectResponse.tainting() == ResourceResponse::Tainting::Cors, redirectingToNewOrigin && crossOriginFlag); Step 10 is only about setting the tainted origin flag, not setting the origin header. We are now changing the logic when we set m_origin to a unique origin which might be good. I am wondering whether we should use FrameLoader::addHTTPOriginIfNeeded.
*** Bug 201127 has been marked as a duplicate of this bug. ***
Created attachment 393465 [details] Patch
Created attachment 393469 [details] Patch
Comment on attachment 393469 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393469&action=review > Source/WebCore/loader/SubresourceLoader.cpp:660 > + newRequest.setHasTaintedOrigin(true); Do we need has tainted origin at all (except to get closer to fetch spec)? If we are setting m_origin to be unique in the next line, the call below to FrameLoader::addHTTPOriginIfNeeded should end up setting "null" value to the origin header.
Comment on attachment 393469 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393469&action=review >> Source/WebCore/loader/SubresourceLoader.cpp:660 >> + newRequest.setHasTaintedOrigin(true); > > Do we need has tainted origin at all (except to get closer to fetch spec)? > If we are setting m_origin to be unique in the next line, the call below to FrameLoader::addHTTPOriginIfNeeded should end up setting "null" value to the origin header. No, it is not absolutely needed, and I did it mostly to follow the spec indeed. In the spec it makes sense since the redirect sets things up for a new main fetch, but I think we do not do such explicit fetch calls in WebKit. Also I added the addHTTPOriginIfNeeded call since the Cors specific block already sets the Origin header. So I would be fine with removing the explicit tainted origin flag code.
Created attachment 393492 [details] Patch
Comment on attachment 393492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393492&action=review > Source/WebCore/loader/FrameLoader.cpp:3005 > + // FIXME: take referrer-policy into account. Maybe file a bug or provide a pointer to the spec so that it is clear what should be done here.
Created attachment 393505 [details] Patch
Comment on attachment 393505 [details] Patch Rejecting attachment 393505 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 393505, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/13340870
Comment on attachment 393505 [details] Patch Rejecting attachment 393505 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 393505, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/13340918
Created attachment 393580 [details] Patch
Comment on attachment 393580 [details] Patch Clearing flags on attachment: 393580 Committed r258465: <https://trac.webkit.org/changeset/258465>
All reviewed patches have been landed. Closing bug.
Comment on attachment 393580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393580&action=review I really appreciate all the work you are doing on this. > Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegate.cpp:141 > - if (httpMessage && CFHTTPMessageGetResponseStatusCode(httpMessage) == 307) { > + if (httpMessage && (CFHTTPMessageGetResponseStatusCode(httpMessage) == 307 || CFHTTPMessageGetResponseStatusCode(httpMessage) == 308)) { A little messy that this calls CFHTTPMessageGetResponseStatusCode twice. Makes the code both hard to read since the line is so long and one additional function call to another framework. I have an idea how to make this prettier. I’d write this helper function: static Optional<CFIndex> statusCode(CFURLResponseRef response) { auto message = CFURLResponseGetHTTPResponse(response); if (!message) return WTF::nullopt; return CFHTTPMessageGetResponseStatusCode(httpMessage); } Then we can just write: auto code = statusCode(redirectResponse); if (code == 307 || code == 308) { ... Or we can be less pedantic and write this without adding a function: auto message = CFURLResponseGetHTTPResponse(response); auto code = message ? CFHTTPMessageGetResponseStatusCode(httpMessage) : 0; if (code == 307 || code == 308) { ... I can’t look at code like this without wishing we’d crunched it a bit to make it easier to read.