Bug 185840 – Migrate From-Origin to Cross-Origin-Resource-Policy

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[youenn fablet]

And update its behavior as per latest developments in https://github.com/whatwg/fetch/issues/687

[Radar WebKit Bug Importer]

<rdar://problem/40430267>

[youenn fablet]

Created attachment 340906 [details]
WIP

[youenn fablet]

Created attachment 340927 [details]
WIP

[EWS Watchlist]

Comment on attachment 340927 [details]
WIP

Attachment 340927 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7759145

New failing tests:
http/wpt/cross-origin-read-policy/script-loads.html
http/wpt/cross-origin-read-policy/image-loads.html
http/wpt/cross-origin-read-policy/fetch.html
http/wpt/cross-origin-read-policy/fetch-in-iframe.html
http/wpt/cross-origin-read-policy/iframe-loads.html

[EWS Watchlist]

Created attachment 340932 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340927 [details]
WIP

Attachment 340927 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7759346

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 340933 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 340935 [details]
Patch

[EWS Watchlist]

Comment on attachment 340935 [details]
Patch

Attachment 340935 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7759857

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 340943 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340935 [details]
Patch

Attachment 340935 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7760019

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 340946 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 340935 [details]
Patch

Attachment 340935 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7762748

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html

[EWS Watchlist]

Created attachment 340966 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[youenn fablet]

Created attachment 341151 [details]
Patch

[EWS Watchlist]

Comment on attachment 341151 [details]
Patch

Attachment 341151 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7783302

New failing tests:
http/wpt/cross-origin-resource-policy/fetch.html

[EWS Watchlist]

Created attachment 341156 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 341151 [details]
Patch

Attachment 341151 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7783175

New failing tests:
http/wpt/cross-origin-resource-policy/fetch.html

[EWS Watchlist]

Created attachment 341158 [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.4

[youenn fablet]

Created attachment 341165 [details]
Patch

[EWS Watchlist]

Comment on attachment 341165 [details]
Patch

Attachment 341165 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7784698

New failing tests:
http/wpt/cross-origin-resource-policy/fetch.html

[EWS Watchlist]

Created attachment 341171 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 341165 [details]
Patch

Attachment 341165 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7784697

New failing tests:
http/wpt/cross-origin-resource-policy/fetch.html

[EWS Watchlist]

Created attachment 341174 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[youenn fablet]

Created attachment 341196 [details]
Patch

[EWS Watchlist]

Comment on attachment 341196 [details]
Patch

Attachment 341196 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7789842

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 341212 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Chris Dumez]

Comment on attachment 341196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341196&action=review

> Source/WebCore/platform/network/HTTPParsers.cpp:900
> +CrossOriginResourcePolicy parseCrossOriginResourcePolicyHeader(const String& header)

Since we're modifying it, the parameter should be a StringView (by value, not reference), like the parseCrossOriginOptionsHeader() below.

> Source/WebCore/platform/network/HTTPParsers.cpp:913
> +    return CrossOriginResourcePolicy::Invalid;

Do we really need to distinguish Invalid? I think this could just return the default (aka None).

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:139
> +    auto policy = WebCore::parseCrossOriginResourcePolicyHeader(response.httpHeaderField(WebCore::HTTPHeaderName::CrossOriginResourcePolicy));

No need for all these WebCore::

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:141
> +    case WebCore::CrossOriginResourcePolicy::None:

No need for all these WebCore::

[John Wilander]

Comment on attachment 341196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341196&action=review

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:135
> +{

It is a little bit weird to have validate() function return a boolean unless the boolean means valid/invalid. In this particular case, the function returns true for an invalid header which is extra weird. This is why I called the corresponding function shouldCancelCrossOriginLoad(). I think that's a better name.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:152
> +    }}

I think we might need a RELEASE_ASSERT_NOT_REACHED() for GTK and other non-clang built ports to be happy without a default: in the switch clause.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:458
> +        auto error = m_networkLoadChecker->validateResponse(m_response);

This is a more appropriate use of "validate" in a function name, i.e. an error condition for invalid values.

> LayoutTests/TestExpectations:372
> +http/wpt/cross-origin-resource-policy/ [ Skip ]

Nice to see these move to WPT. Good job.

[youenn fablet]

Thanks for all comments, I will update the patch accordingly.
As of switch/default, it seems all bots are happy so I plan to leave it as is.

[youenn fablet]

> Nice to see these move to WPT. Good job.

Thanks.
What might be missing in terms of coverage so far:
- 401/407 WPT tests
- CORP header value parsing WPT tests to the level of our unit tests

[John Wilander]

(In reply to youenn fablet from comment #30)
> Thanks for all comments, I will update the patch accordingly.
> As of switch/default, it seems all bots are happy so I plan to leave it as
> is.

IIRC, the bots were fine when I landed a similar patch without the release assert. So there's something else that breaks in GCC land when we have switch clauses without a default:.

I guess we'll see. :)

[youenn fablet]

> IIRC, the bots were fine when I landed a similar patch without the release
> assert. So there's something else that breaks in GCC land when we have
> switch clauses without a default:.
> 
> I guess we'll see. :)

Looking at https://github.com/WebKit/webkit/commit/0a927e472fa65284bd7d92db8cd3225283ea8f6e, it seems you are right.
I'll add it back then.

[Daniel Bates]

Is there a reason we do not support this feature in WebKit1?

[John Wilander]

(In reply to Daniel Bates from comment #34)
> Is there a reason we do not support this feature in WebKit1?

Yes, a large part of why we want cross-origin-resource-policy is to leverage process separation.

[youenn fablet]

I think we might want to implement it in WK1. To me, it is mostly a question of priority, like finishing the implementation, ironing out all details...

[youenn fablet]

Created attachment 341316 [details]
Patch for landing

[youenn fablet]

Created attachment 341319 [details]
Fixing WPT lint issues

[Daniel Bates]

Comment on attachment 341319 [details]
Fixing WPT lint issues

View in context: https://bugs.webkit.org/attachment.cgi?id=341319&action=review

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:148
> +        return m_origin->isUnique() || ResourceRequest::partitionName(m_origin->host()) != ResourceRequest::partitionName(response.url().host());

For your consideration, I suggest we add registrableDomainsAreEqual() overload (in ResourceRequestBase.h) for the comparison performed in the second disjunct.

[Daniel Bates]

(In reply to Daniel Bates from comment #39)
> Comment on attachment 341319 [details]
> Fixing WPT lint issues
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341319&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:148
> > +        return m_origin->isUnique() || ResourceRequest::partitionName(m_origin->host()) != ResourceRequest::partitionName(response.url().host());
> 
> For your consideration, I suggest we add registrableDomainsAreEqual()
> overload (in ResourceRequestBase.h) for the comparison performed in the
> second disjunct.

To elaborate further, the term of art here is "registrable domain" not "partition name". At the time of writing the partition name for a URL is its registrable domain. This is an implementation detail. The partition name just needs to be unique per registrable domain. That is, it does not need to actually be the registrable domain.

[Daniel Bates]

Comment on attachment 341319 [details]
Fixing WPT lint issues

View in context: https://bugs.webkit.org/attachment.cgi?id=341319&action=review

>>> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:148
>>> +        return m_origin->isUnique() || ResourceRequest::partitionName(m_origin->host()) != ResourceRequest::partitionName(response.url().host());
>> 
>> For your consideration, I suggest we add registrableDomainsAreEqual() overload (in ResourceRequestBase.h) for the comparison performed in the second disjunct.
> 
> To elaborate further, the term of art here is "registrable domain" not "partition name". At the time of writing the partition name for a URL is its registrable domain. This is an implementation detail. The partition name just needs to be unique per registrable domain. That is, it does not need to actually be the registrable domain.

I have not had a chance to catch up on the GitHub thread. Is the isUnique() check here correct? I know we are calling this state SameSite. I take it that this definition was agreed in the thread despite it being different from the definition used in the Same-Site cookies RFC that first defined "same-site".

[John Wilander]

(In reply to Daniel Bates from comment #40)
> (In reply to Daniel Bates from comment #39)
> > Comment on attachment 341319 [details]
> > Fixing WPT lint issues
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=341319&action=review
> > 
> > > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:148
> > > +        return m_origin->isUnique() || ResourceRequest::partitionName(m_origin->host()) != ResourceRequest::partitionName(response.url().host());
> > 
> > For your consideration, I suggest we add registrableDomainsAreEqual()
> > overload (in ResourceRequestBase.h) for the comparison performed in the
> > second disjunct.
> 
> To elaborate further, the term of art here is "registrable domain" not
> "partition name". At the time of writing the partition name for a URL is its
> registrable domain. This is an implementation detail. The partition name
> just needs to be unique per registrable domain. That is, it does not need to
> actually be the registrable domain.

Perhaps this is changing, but WebKit has tried to use either partition or topPrivatelyControlledDomain to represent eTLD+1. There are instances in ResourceLoadStatistics where we call it primaryDomain and highLevelDomain but those are not desirable.

[Daniel Bates]

Comment on attachment 341319 [details]
Fixing WPT lint issues

View in context: https://bugs.webkit.org/attachment.cgi?id=341319&action=review

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:139
> +    auto policy = parseCrossOriginResourcePolicyHeader(response.httpHeaderField(HTTPHeaderName::CrossOriginResourcePolicy));

This variable is referenced exactly once in this function. I would inline its value in the line below.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:154
> +    RELEASE_ASSERT_NOT_REACHED();

We need a return statement after this line for compilers that are not convinced that the enum handles all cases.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:174
> +            return ResourceError { errorDomainWebKitInternal, 0, m_url, makeString("Cancelled load to ", response.url().string(), " because it violates the resource's Cross-Origin-Resource-Policy response header."), ResourceError::Type::AccessControl };

Is it intentional to include the full URL in this error message? Typically we use URL::stringCenterEllipsizedToLength() to truncate/pretty-print URLs. Would using such pretty-printing mechanics affect Web Platform Test portability? or need to be negotiated with other browser vendors?

[youenn fablet]

> For your consideration, I suggest we add registrableDomainsAreEqual()
> overload (in ResourceRequestBase.h) for the comparison performed in the
> second disjunct.

registrableDomainsAreEqual sounds more readable, I'll use an existing overload for now.

> I have not had a chance to catch up on the GitHub thread. Is the isUnique()
> check here correct? I know we are calling this state SameSite. I take it
> that this definition was agreed in the thread despite it being different
> from the definition used in the Same-Site cookies RFC that first defined
> "same-site".

It would seem odd to me that a unique origin could same-site-match any URL.
But I do not know how Same-Site cookies RFC differ from fetch Same-Site.
Ideally they should behave the same, right?

> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:174
> > +            return ResourceError { errorDomainWebKitInternal, 0, m_url, makeString("Cancelled load to ", response.url().string(), " because it violates the resource's Cross-Origin-Resource-Policy response header."), ResourceError::Type::AccessControl };
> 
> Is it intentional to include the full URL in this error message? Typically
> we use URL::stringCenterEllipsizedToLength() to truncate/pretty-print URLs.
> Would using such pretty-printing mechanics affect Web Platform Test
> portability? or need to be negotiated with other browser vendors?

Will use stringCenterEllipsizedToLength()

[Daniel Bates]

(In reply to youenn fablet from comment #44)
> It would seem odd to me that a unique origin could same-site-match any URL.
> But I do not know how Same-Site cookies RFC differ from fetch Same-Site.
> Ideally they should behave the same, right?
> 

Where is "fetch 'Same-Site'" defined? I briefly tried searching on <https://fetch.spec.whatwg.org> and using Google to no avail.

[youenn fablet]

Created attachment 341336 [details]
Rebasing + updating according dan comments

[youenn fablet]

> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:139
> > +    auto policy = parseCrossOriginResourcePolicyHeader(response.httpHeaderField(HTTPHeaderName::CrossOriginResourcePolicy));
> 
> This variable is referenced exactly once in this function. I would inline
> its value in the line below.

I kept it the way it is. I like this switch(policy) and I do not think it matters in terms of optimization.

> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:154
> > +    RELEASE_ASSERT_NOT_REACHED();
> 
> We need a return statement after this line for compilers that are not
> convinced that the enum handles all cases.

Maybe there would be some warning saying that this return statement is not reachable?
I left it the way Michael fixed it for GTK builds.

[Michael Catanzaro]

(In reply to John Wilander from comment #32)
> IIRC, the bots were fine when I landed a similar patch without the release
> assert. So there's something else that breaks in GCC land when we have
> switch clauses without a default:.

Guess: probably -Wreturn-type. Just make sure the function returns a value in every case. E.g. if you are returning values inside the switch, one return statement for each case, there should be a RELEASE_ASSERT_NOT_REACHED() to avoid GCC warning that there's no return value when none of the switch cases are reached.

There is also -Wswitch: you need to either list all possible enum values in the switch, or else add a default case to suppress the warning. This one is useful because it's so easy to forget to update switches for new enum values otherwise. I don't see this warning as often, though.

Neither of these would break the build, but they'd introduce warnings that we'd have to follow up and fix.

[youenn fablet]

> There is also -Wswitch: you need to either list all possible enum values in
> the switch, or else add a default case to suppress the warning. This one is
> useful because it's so easy to forget to update switches for new enum values
> otherwise. I don't see this warning as often, though.

This warning is probably turned into build errors on some EWS bots.

[Michael Catanzaro]

(In reply to youenn fablet from comment #49) 
> This warning is probably turned into build errors on some EWS bots.

It was for EFL, but should not be for GTK or WPE bots.

[WebKit Commit Bot]

Comment on attachment 341336 [details]
Rebasing + updating according dan comments

Clearing flags on attachment: 341336

Committed r232217: <https://trac.webkit.org/changeset/232217>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.