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 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
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
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
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
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 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
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
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
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
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 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 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 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 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
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.
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.
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.
2019-01-18 11:29 PST, Rob Buis
2019-01-19 08:32 PST, Rob Buis
2019-01-19 09:45 PST, EWS Watchlist
2019-01-19 09:56 PST, EWS Watchlist
2019-01-19 10:18 PST, EWS Watchlist
2019-01-19 10:28 PST, EWS Watchlist
2019-01-19 10:42 PST, EWS Watchlist
2019-01-19 12:01 PST, Rob Buis
2019-01-19 13:11 PST, EWS Watchlist
2019-01-19 13:23 PST, EWS Watchlist
2019-01-19 13:52 PST, EWS Watchlist
2019-01-19 13:56 PST, EWS Watchlist
2019-01-19 14:06 PST, EWS Watchlist
2019-01-20 08:13 PST, Rob Buis
2019-01-20 10:54 PST, EWS Watchlist
2019-01-20 12:59 PST, Rob Buis
2019-01-20 14:55 PST, EWS Watchlist
2019-09-01 03:37 PDT, Rob Buis
2019-09-01 04:48 PDT, Rob Buis
2019-09-01 06:27 PDT, EWS Watchlist
2019-09-22 04:56 PDT, Rob Buis
2019-09-22 06:57 PDT, EWS Watchlist
2019-12-18 05:34 PST, Rob Buis
2019-12-30 11:45 PST, Rob Buis
2019-12-31 03:15 PST, Rob Buis
2019-12-31 05:18 PST, Rob Buis
2020-03-13 01:13 PDT, Rob Buis
2020-03-13 02:57 PDT, Rob Buis
2020-03-13 09:45 PDT, Rob Buis
2020-03-13 10:51 PDT, Rob Buis
2020-03-13 23:48 PDT, Rob Buis