Bug 186030 – Set Origin header value to null rather than omitting it

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Anne van Kesteren]

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.

[Radar WebKit Bug Importer]

<rdar://problem/40694782>

[Brent Fulgham]

We should recheck these results now that Bug 182644 has been merged.

[youenn fablet]

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.

[Rob Buis]

Created attachment 359515 [details]
Patch

[Rob Buis]

Created attachment 359611 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 359618 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 359638 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 359642 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

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.

[Rob Buis]

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

[Basuke Suzuki]

(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

[Rob Buis]

Created attachment 377817 [details]
Patch

[Rob Buis]

Created attachment 377819 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 379342 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Rob Buis]

Created attachment 385964 [details]
Patch

[Rob Buis]

Created attachment 386542 [details]
Patch

[Rob Buis]

Created attachment 386552 [details]
Patch

[Rob Buis]

Created attachment 386557 [details]
Patch

[youenn fablet]

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.

[Rob Buis]

*** Bug 201127 has been marked as a duplicate of this bug. ***

[Rob Buis]

Created attachment 393465 [details]
Patch

[Rob Buis]

Created attachment 393469 [details]
Patch

[youenn fablet]

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.

[Rob Buis]

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.

[Rob Buis]

Created attachment 393492 [details]
Patch

[youenn fablet]

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.

[Rob Buis]

Created attachment 393505 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Rob Buis]

Created attachment 393580 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 393580 [details]
Patch

Clearing flags on attachment: 393580

Committed r258465: <https://trac.webkit.org/changeset/258465>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.