And update its behavior as per latest developments in https://github.com/whatwg/fetch/issues/687
<rdar://problem/40430267>
Created attachment 340906 [details] WIP
Created attachment 340927 [details] WIP
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
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
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.
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
Created attachment 340935 [details] Patch
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.
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
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.
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
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
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
Created attachment 341151 [details] Patch
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
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
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
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
Created attachment 341165 [details] Patch
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
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
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
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
Created attachment 341196 [details] Patch
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
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
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::
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.
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.
> 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
(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. :)
> 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.
Is there a reason we do not support this feature in WebKit1?
(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.
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...
Created attachment 341316 [details] Patch for landing
Created attachment 341319 [details] Fixing WPT lint issues
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.
(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.
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".
(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.
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?
> 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()
(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.
Created attachment 341336 [details] Rebasing + updating according dan comments
> > 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.
(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.
> 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.
(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.
Comment on attachment 341336 [details] Rebasing + updating according dan comments Clearing flags on attachment: 341336 Committed r232217: <https://trac.webkit.org/changeset/232217>
All reviewed patches have been landed. Closing bug.