The first step toward https://github.com/whatwg/fetch/issues/687 is to consume the From-Origin response header and block the resource load if the header specifies 'self' and the originating document's origin does not match the resource's origin.
<rdar://problem/38901344>
Created attachment 337851 [details] Patch
Wincairo fail seems to be a bug in some script.
I would handle that at NetworkResourceLoader level for both redirect and regular responses. NetworkRssourceLoadParameters already pass the origin of the load so no additional parameter is needed for self case. WebLoadefStrategy would need to be beefed up to pass the origin to all loads. I would also add these checks to NetworkLaodChecker which is/will be doing all fetch (Cors, csp, content blockers) of a fetch.
What about unique origins by the way?
(In reply to youenn fablet from comment #4) > I would handle that at NetworkResourceLoader level for both redirect and > regular responses. NetworkRssourceLoadParameters already pass the origin of > the load so no additional parameter is needed for self case. > WebLoadefStrategy would need to be beefed up to pass the origin to all loads. > I would also add these checks to NetworkLaodChecker which is/will be doing > all fetch (Cors, csp, content blockers) of a fetch. Oh, you mean m_parameters.sourceOrigin? I started out in NetworkLoader instead of NetworkResourceLoader which is why I ended up implementing in the task. I'll try moving the code today. Thanks!
(In reply to youenn fablet from comment #5) > What about unique origins by the way? I did do some work in that space for when frames start out in about:blank but I should test properly. Do you think a sandboxed iframe will do?
Created attachment 338186 [details] Patch
Comment on attachment 338186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338186&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:64 > + Vector<String> frameAncestorOrigins; Is there a reason we are not using SecurityOrigin? > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:359 > + if (frameOrigin != "null" && frameOrigin != response.url().protocolHostAndPort()) How did you come to the decision to treat unique origins (i.e. frameOrigin == "null") as same-origin? > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:379 > + default: Please remove this default case and move its contents outside of the switch block. The benefit of omitting a default case statement is that it allows the compiler to check that the switch block covers all enumeration values for us. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:403 > + ResourceError error = { String(), 0, url, "Cancelled load because it violates the resource's From-Origin response header." }; Would it make sense to specify ResourceError::Type::AccessControl as the error type? We should use ASCIILiteral() to construct the string for this error message as it avoids copying the characters. For you consideration I suggest using constructor syntax instead of assignment syntax to initialize this local and use uniform initializer syntax throughout the entire line for consistency: ResourceError error { String { }, 0, url, ASCIILiteral { "Cancelled load because it violates the resource's From-Origin response header." } }; > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:283 > + Vector<String> frameAncestorOrigins; Is there a reason we are not using SecurityOrigin? > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:285 > + while (currentFrame) { We are underutilizing the scope of currentFrame as we never use it outside of the while loop. I suggest that we implement this loop using a for loop: for (Frame* frame = resourceLoader.frame(); frame; frame = frame->tree().parent()) { ... } > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:286 > + if (currentFrame->document()) Can you please provide an example when a frame would not have a document when this function is invoked? > LayoutTests/http/tests/from-origin/document-from-origin-same-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/document-from-origin-same-accepted.html:19 > + setTimeout("iframeLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/document-from-origin-same-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/document-from-origin-same-blocked.html:19 > + setTimeout("iframeLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/document-from-origin-same-site-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/document-from-origin-same-site-accepted.html:19 > + setTimeout("iframeLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/document-from-origin-same-site-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/document-from-origin-same-site-blocked.html:19 > + setTimeout("iframeLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/document-nested-from-origin-same-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/document-nested-from-origin-same-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/fetch-from-origin-same-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/fetch-from-origin-same-blocked-expected.txt:6 > +PASS Fetch blocked. TypeError: Cancelled load because it violates the resource's From-Origin response header. It is disingenuous to classify this security error as a TypeError. > LayoutTests/http/tests/from-origin/fetch-from-origin-same-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/fetch-from-origin-same-site-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/fetch-from-origin-same-site-blocked-expected.txt:6 > +PASS Fetch blocked. TypeError: Cancelled load because it violates the resource's From-Origin response header. It is disingenuous to classify this security error as a TypeError. > LayoutTests/http/tests/from-origin/fetch-from-origin-same-site-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/image-from-origin-same-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/image-from-origin-same-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/image-from-origin-same-site-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/image-from-origin-same-site-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-accepted.html:19 > + setTimeout("iframeLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-blocked.html:19 > + setTimeout("iframeLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-nested-from-origin-same-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-nested-from-origin-same-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/script-from-origin-same-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/script-from-origin-same-accepted.html:18 > + setTimeout("scriptLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/script-from-origin-same-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/script-from-origin-same-blocked.html:18 > + setTimeout("scriptLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/script-from-origin-same-site-accepted.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/script-from-origin-same-site-accepted.html:18 > + setTimeout("scriptLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/script-from-origin-same-site-blocked.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/script-from-origin-same-site-blocked.html:18 > + setTimeout("scriptLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/top-frame-document-from-origin-same-accepted.php:4 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/resources/nestedIPAddressIframe.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/resources/nestedIPAddressIframe.html:15 > + setTimeout("iframeLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout? > LayoutTests/http/tests/from-origin/resources/nestedLocalhostIframe.html:1 > +<html> I do not see the need to use quirks mode for this page. Please add <!DOCTYPE html>. > LayoutTests/http/tests/from-origin/resources/nestedLocalhostIframe.html:15 > + setTimeout("iframeLoadError()", 500); This is not a good approach as it will impose a lower bound on the how long this test takes to run in the best case and introduces test flakiness in the worst case. Can we think of another way to write this test without using a timeout?
It is not clear to me why we need to go to the full list of ancestor origins. What are we gaining compared to only checking against the actual Document origin (or its parent in case of data/blob iframes)? if the header is non-existent, empty, or invalid, the patch skips it. I wonder whether we should be more restrictive in case of invalid or empty. I agree with Dan about using AccessControl errors, this is for instance used to filter or not error messages in fetch/xhr. Given this is an early experiment, I wonder whether just supporting 'same' might not be good enough for now. There is some discussions with same-site vs a list of origins, right? There were also some discussion and WPT tests IIRC for using list of origins, not sure what is the status here.
Comment on attachment 338186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338186&action=review > Source/WebCore/ChangeLog:42 > + Tests: http/tests/from-origin/document-from-origin-same-accepted.html > + http/tests/from-origin/document-from-origin-same-blocked.html > + http/tests/from-origin/document-from-origin-same-site-accepted.html > + http/tests/from-origin/document-from-origin-same-site-blocked.html > + http/tests/from-origin/document-nested-from-origin-same-accepted.html > + http/tests/from-origin/document-nested-from-origin-same-blocked.html > + http/tests/from-origin/fetch-from-origin-same-accepted.html > + http/tests/from-origin/fetch-from-origin-same-blocked.html > + http/tests/from-origin/fetch-from-origin-same-site-accepted.html > + http/tests/from-origin/fetch-from-origin-same-site-blocked.html > + http/tests/from-origin/image-from-origin-same-accepted.html > + http/tests/from-origin/image-from-origin-same-blocked.html > + http/tests/from-origin/image-from-origin-same-site-accepted.html > + http/tests/from-origin/image-from-origin-same-site-blocked.html > + http/tests/from-origin/sandboxed-sub-frame-from-origin-same-accepted.html > + http/tests/from-origin/sandboxed-sub-frame-from-origin-same-blocked.html > + http/tests/from-origin/sandboxed-sub-frame-nested-from-origin-same-accepted.html > + http/tests/from-origin/sandboxed-sub-frame-nested-from-origin-same-blocked.html > + http/tests/from-origin/script-from-origin-same-accepted.html > + http/tests/from-origin/script-from-origin-same-blocked.html > + http/tests/from-origin/script-from-origin-same-site-accepted.html > + http/tests/from-origin/script-from-origin-same-site-blocked.html > + http/tests/from-origin/top-frame-document-from-origin-same-accepted.php We should also add tests for both same-origin and cross-origin usage of XHR/fetch() from subframes and Web Workers, fetch() from Service Workers, importScripts() from Web Workers and Service Workers, EventSource, XHR()/fetch() from an about:blank main frame and XHR()/fetch() from an about:blank iframe. > Source/WebCore/platform/network/HTTPParsers.h:67 > +enum FromOriginDisposition { Please make this an enum class. We tend to reserve defining a plain enum for when the enum is part of an existing scope and the enumerators do not collide and makes sense in that scope. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:339 > +static String partitionName(const String& host) > +{ > + return ResourceRequest::partitionName(host); > +} The only benefit that this function provides over using ResourceRequest::partitionName() directly is that we do not need to class/namespace qualify it as we do with ResourceRequest::partitionName(). If we chose to keep this function then I suggest that we have it take a URL and then we can simplify the code in NetworkResourceLoader::areFrameAncestorsSameSite(). On another note, we should mark this function as inline to give a hint to the compiler that it can inline its body at each call site given that this function is simple. >> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:359 >> + if (frameOrigin != "null" && frameOrigin != response.url().protocolHostAndPort()) > > How did you come to the decision to treat unique origins (i.e. frameOrigin == "null") as same-origin? It seems reasonable to treat about:blank, about:srcdoc and the empty string as same-origin. Can you please explain how you came to the decision to treat data: URLs and path-separated file URLs as same origin? > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:378 > + case WebCore::FromOriginDisposition::SameSite: > + return !areFrameAncestorsSameSite(m_response); This does this not seem correct for XHRs, and fetch() from Web Workers and Service Workers.
(In reply to youenn fablet from comment #10) > It is not clear to me why we need to go to the full list of ancestor origins. > What are we gaining compared to only checking against the actual Document > origin (or its parent in case of data/blob iframes)? > Counter argument: good.com embeds evil.com embeds good.com > if the header is non-existent, empty, or invalid, the patch skips it. > I wonder whether we should be more restrictive in case of invalid or empty. > How do we handle these cases for Access-Control-Origin? > I agree with Dan about using AccessControl errors, this is for instance used > to filter or not error messages in fetch/xhr. > > Given this is an early experiment, I wonder whether just supporting 'same' > might not be good enough for now. There is some discussions with same-site > vs a list of origins, right? Although we do may not need to solve the problem of passing ancestor frame origins now we will need to solve it in order to move X-Frame-Options checking and CSP checking to the network process. > There were also some discussion and WPT tests IIRC for using list of > origins, not sure what is the status here. Can you please elaborate on what you mean by “list of origins” and the implications with WPT?
(In reply to Daniel Bates from comment #12) > (In reply to youenn fablet from comment #10) > > It is not clear to me why we need to go to the full list of ancestor origins. > > What are we gaining compared to only checking against the actual Document > > origin (or its parent in case of data/blob iframes)? > > > > Counter argument: good.com embeds evil.com embeds good.com Sure, but there is X-Frame-Options. Also, if we have good.com/1 which embeds good.com/2 which loads good.com/sensitive.json and good.com/1 also embeds evil.com, good.com/sensitive.json is screwed. > > if the header is non-existent, empty, or invalid, the patch skips it. > > I wonder whether we should be more restrictive in case of invalid or empty. > > > > How do we handle these cases for Access-Control-Origin? I would guess that we are doing string comparison, so empty means that it will not match. An 'invalid' value would mean that there is no match either. That said, ACAO is used to grant access, From-Origin is used to restrict access. Maybe it is fine for the default to be opposite. Probably not a big deal at this early stage. > Can you please elaborate on what you mean by “list of origins” and the > implications with WPT? See LayoutTests/imported/w3c/web-platform-tests/fetch/api/cors/cors-multiple-origins.js.
Thanks for your review, Dan! (In reply to Daniel Bates from comment #9) > Comment on attachment 338186 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338186&action=review > > > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:64 > > + Vector<String> frameAncestorOrigins; > > Is there a reason we are not using SecurityOrigin? Yes. I realized we will only be comparing for equality of the origin or its partition (possibly adding the scheme, still being discussed in WHATWG). Since this happens for every request, I wanted to make the data encoded, transferred over XPC, and decoded to be lightweight. > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:359 > > + if (frameOrigin != "null" && frameOrigin != response.url().protocolHostAndPort()) > > How did you come to the decision to treat unique origins (i.e. frameOrigin > == "null") as same-origin? Two reasons: 1) Top frame document loads are done in a frame with a null origin. 2) I check the ancestor tree for same-origin or same-site which means any intermediary null origins are under the control of the response's origin or eTLD+1. > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:379 > > + default: > > Please remove this default case and move its contents outside of the switch > block. The benefit of omitting a default case statement is that it allows > the compiler to check that the switch block covers all enumeration values > for us. Got it. Will fix. > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:403 > > + ResourceError error = { String(), 0, url, "Cancelled load because it violates the resource's From-Origin response header." }; > > Would it make sense to specify ResourceError::Type::AccessControl as the > error type? We should use ASCIILiteral() to construct the string for this > error message as it avoids copying the characters. For you consideration I > suggest using constructor syntax instead of assignment syntax to initialize > this local and use uniform initializer syntax throughout the entire line for > consistency: Sounds reasonable. Will fix. > ResourceError error { String { }, 0, url, ASCIILiteral { "Cancelled load > because it violates the resource's From-Origin response header." } }; > > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:283 > > + Vector<String> frameAncestorOrigins; > > Is there a reason we are not using SecurityOrigin? Explained above. > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:285 > > + while (currentFrame) { > > We are underutilizing the scope of currentFrame as we never use it outside > of the while loop. I suggest that we implement this loop using a for loop: > > for (Frame* frame = resourceLoader.frame(); frame; frame = > frame->tree().parent()) { > ... > } Will fix. > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:286 > > + if (currentFrame->document()) > > Can you please provide an example when a frame would not have a document > when this function is invoked? When I get a pointer, I null check it by instinct. If you don't think it's needed I'll remove the check. > > LayoutTests/http/tests/from-origin/document-from-origin-same-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. Got it. Will fix for all the HTML files. > > LayoutTests/http/tests/from-origin/document-from-origin-same-accepted.html:19 > > + setTimeout("iframeLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? Afaik, iframes elements don't fire onerror events. It's discussed here: https://bugs.chromium.org/p/chromium/issues/detail?id=365457. I quote: "I have discussed this with Adam Barth, and we have confirmed that the behaviour described here is the behaviour described in the Living HTML specification. As follows: The specification requires that all HTML elements support on onerror event. However, it does NOT require that all elements supporting network fetches raise fire a simple event called onerror. That is, elements must support allowing applications to set error handlers, but there is no (generic) requirement that the event be raised, in either HTML or the Fetch specification. The behaviour of img firing onload is explicitly specified in 4.7.1 - http://www.whatwg.org/specs/web-apps/current-work/multipage/edits.html#the-img-element , steps 10 and 14. Likewise, other elements, such as object, document specific steps to fire a simple event named error (eg: Section 4.7.4, Step 6 ). However, the iframe element lacks such requirements - see http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#the-embed-element - and describes specifically the events that should be fired. In discussing, there is some concern that there may be additional security concerns specific to iframe that may not apply to the rules for img and object, with respect to network probing and other security risks. The next step would be to discuss any such change in the WHATWG mailing list ( http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/ ), have it incorporated into the specification, and then revisit adding support. For now, this is WontFix, for Working as Intended (and as Specified)" Do you have a suggestion? Maybe doing what you did for CSP frame-src testing? The script case seemed similar but I should revisit. Maybe we do fire an onerror event there. > > LayoutTests/http/tests/from-origin/document-from-origin-same-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/document-from-origin-same-blocked.html:19 > > + setTimeout("iframeLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? > > > LayoutTests/http/tests/from-origin/document-from-origin-same-site-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/document-from-origin-same-site-accepted.html:19 > > + setTimeout("iframeLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? > > > LayoutTests/http/tests/from-origin/document-from-origin-same-site-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/document-from-origin-same-site-blocked.html:19 > > + setTimeout("iframeLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? > > > LayoutTests/http/tests/from-origin/document-nested-from-origin-same-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/document-nested-from-origin-same-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/fetch-from-origin-same-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/fetch-from-origin-same-blocked-expected.txt:6 > > +PASS Fetch blocked. TypeError: Cancelled load because it violates the resource's From-Origin response header. > > It is disingenuous to classify this security error as a TypeError. Got it. I'll see what I can make it be instead. I believe the rest of the review comments are copies of earlier ones and they will be addressed as explained above. > > LayoutTests/http/tests/from-origin/fetch-from-origin-same-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/fetch-from-origin-same-site-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/fetch-from-origin-same-site-blocked-expected.txt:6 > > +PASS Fetch blocked. TypeError: Cancelled load because it violates the resource's From-Origin response header. > > It is disingenuous to classify this security error as a TypeError. > > > LayoutTests/http/tests/from-origin/fetch-from-origin-same-site-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/image-from-origin-same-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/image-from-origin-same-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/image-from-origin-same-site-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/image-from-origin-same-site-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-accepted.html:19 > > + setTimeout("iframeLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? > > > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-from-origin-same-blocked.html:19 > > + setTimeout("iframeLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? > > > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-nested-from-origin-same-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/sandboxed-sub-frame-nested-from-origin-same-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/script-from-origin-same-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/script-from-origin-same-accepted.html:18 > > + setTimeout("scriptLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? > > > LayoutTests/http/tests/from-origin/script-from-origin-same-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/script-from-origin-same-blocked.html:18 > > + setTimeout("scriptLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? > > > LayoutTests/http/tests/from-origin/script-from-origin-same-site-accepted.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/script-from-origin-same-site-accepted.html:18 > > + setTimeout("scriptLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? > > > LayoutTests/http/tests/from-origin/script-from-origin-same-site-blocked.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/script-from-origin-same-site-blocked.html:18 > > + setTimeout("scriptLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? > > > LayoutTests/http/tests/from-origin/top-frame-document-from-origin-same-accepted.php:4 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/resources/nestedIPAddressIframe.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/resources/nestedIPAddressIframe.html:15 > > + setTimeout("iframeLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout? > > > LayoutTests/http/tests/from-origin/resources/nestedLocalhostIframe.html:1 > > +<html> > > I do not see the need to use quirks mode for this page. Please add <!DOCTYPE > html>. > > > LayoutTests/http/tests/from-origin/resources/nestedLocalhostIframe.html:15 > > + setTimeout("iframeLoadError()", 500); > > This is not a good approach as it will impose a lower bound on the how long > this test takes to run in the best case and introduces test flakiness in the > worst case. Can we think of another way to write this test without using a > timeout?
Thanks for your review comments, Youenn! (In reply to youenn fablet from comment #10) > It is not clear to me why we need to go to the full list of ancestor origins. > What are we gaining compared to only checking against the actual Document > origin (or its parent in case of data/blob iframes)? Knowing that you are in control of the top frame is important since that means you are in control of the UI and in control of the page's CSP. Since intermediary origins may be null, you also need to know the ancestors. > if the header is non-existent, empty, or invalid, the patch skips it. > I wonder whether we should be more restrictive in case of invalid or empty. Interpreting invalid headers as valid constitutes a quirk in my mind. Since this spec is still being worked on we might have behavior changes. Say a site sends "From-Origin: social.org, example.com." With the patch as it stands, that would be an invalid header. Then later we might add support for whitelisting and the same header triggers a very different behavior. > I agree with Dan about using AccessControl errors, this is for instance used > to filter or not error messages in fetch/xhr. I'll address this. > Given this is an early experiment, I wonder whether just supporting 'same' > might not be good enough for now. There is some discussions with same-site > vs a list of origins, right? > There were also some discussion and WPT tests IIRC for using list of > origins, not sure what is the status here. We had a meeting yesterday where I got convinced that we need something less strict than just same-origin to make this more useful for developers and allow for adoption.
(In reply to Daniel Bates from comment #11) > Comment on attachment 338186 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338186&action=review > > > Source/WebCore/ChangeLog:42 > > + Tests: http/tests/from-origin/document-from-origin-same-accepted.html > > + http/tests/from-origin/document-from-origin-same-blocked.html > > + http/tests/from-origin/document-from-origin-same-site-accepted.html > > + http/tests/from-origin/document-from-origin-same-site-blocked.html > > + http/tests/from-origin/document-nested-from-origin-same-accepted.html > > + http/tests/from-origin/document-nested-from-origin-same-blocked.html > > + http/tests/from-origin/fetch-from-origin-same-accepted.html > > + http/tests/from-origin/fetch-from-origin-same-blocked.html > > + http/tests/from-origin/fetch-from-origin-same-site-accepted.html > > + http/tests/from-origin/fetch-from-origin-same-site-blocked.html > > + http/tests/from-origin/image-from-origin-same-accepted.html > > + http/tests/from-origin/image-from-origin-same-blocked.html > > + http/tests/from-origin/image-from-origin-same-site-accepted.html > > + http/tests/from-origin/image-from-origin-same-site-blocked.html > > + http/tests/from-origin/sandboxed-sub-frame-from-origin-same-accepted.html > > + http/tests/from-origin/sandboxed-sub-frame-from-origin-same-blocked.html > > + http/tests/from-origin/sandboxed-sub-frame-nested-from-origin-same-accepted.html > > + http/tests/from-origin/sandboxed-sub-frame-nested-from-origin-same-blocked.html > > + http/tests/from-origin/script-from-origin-same-accepted.html > > + http/tests/from-origin/script-from-origin-same-blocked.html > > + http/tests/from-origin/script-from-origin-same-site-accepted.html > > + http/tests/from-origin/script-from-origin-same-site-blocked.html > > + http/tests/from-origin/top-frame-document-from-origin-same-accepted.php > > We should also add tests for both same-origin and cross-origin usage of > XHR/fetch() from subframes and Web Workers, fetch() from Service Workers, > importScripts() from Web Workers and Service Workers, EventSource, > XHR()/fetch() from an about:blank main frame and XHR()/fetch() from an > about:blank iframe. > > > Source/WebCore/platform/network/HTTPParsers.h:67 > > +enum FromOriginDisposition { > > Please make this an enum class. We tend to reserve defining a plain enum for > when the enum is part of an existing scope and the enumerators do not > collide and makes sense in that scope. OK. > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:339 > > +static String partitionName(const String& host) > > +{ > > + return ResourceRequest::partitionName(host); > > +} > > The only benefit that this function provides over using > ResourceRequest::partitionName() directly is that we do not need to > class/namespace qualify it as we do with ResourceRequest::partitionName(). > If we chose to keep this function then I suggest that we have it take a URL > and then we can simplify the code in > NetworkResourceLoader::areFrameAncestorsSameSite(). OK. > On another note, we should mark this function as inline to give a hint to > the compiler that it can inline its body at each call site given that this > function is simple. I've started to avoid inlining since the build time discussion we had. Maybe I shouldn't do that. > >> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:359 > >> + if (frameOrigin != "null" && frameOrigin != response.url().protocolHostAndPort()) > > > > How did you come to the decision to treat unique origins (i.e. frameOrigin == "null") as same-origin? > > It seems reasonable to treat about:blank, about:srcdoc and the empty string > as same-origin. Can you please explain how you came to the decision to treat > data: URLs and path-separated file URLs as same origin? Are they null or are you saying they have a host and or origin? Either way, since I check the ancestor tree, the resource response is in control of all intermediary documents that may have non network origins. > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:378 > > + case WebCore::FromOriginDisposition::SameSite: > > + return !areFrameAncestorsSameSite(m_response); > > This does this not seem correct for XHRs, and fetch() from Web Workers and > Service Workers. I don't doubt that you're right, but can you explain? Thanks!
(In reply to John Wilander from comment #16) > > On another note, we should mark this function as inline to give a hint to > > the compiler that it can inline its body at each call site given that this > > function is simple. > > I've started to avoid inlining since the build time discussion we had. Maybe > I shouldn't do that. > I do not recall such a general avoidance of using the "inline" keyword. I mean, it's only a hint to the compiler. We may want to clarify use of "inline" on webkit-dev. For a simple function that is effectively an alias for another function it seems acceptable to mark the function as inline. > > >> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:359 > > >> + if (frameOrigin != "null" && frameOrigin != response.url().protocolHostAndPort()) > > > > > > How did you come to the decision to treat unique origins (i.e. frameOrigin == "null") as same-origin? > > > > It seems reasonable to treat about:blank, about:srcdoc and the empty string > > as same-origin. Can you please explain how you came to the decision to treat > > data: URLs and path-separated file URLs as same origin? > > Are they null or are you saying they have a host and or origin? > What is the reason that you check if frameOrigin is not "null"? > > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:378 > > > + case WebCore::FromOriginDisposition::SameSite: > > > + return !areFrameAncestorsSameSite(m_response); > > > > This does this not seem correct for XHRs, and fetch() from Web Workers and > > Service Workers. > > I don't doubt that you're right, but can you explain? Thanks! Disregard my remark about Web Workers. For Service Workers, is this logic consistent with how same-site is applied to a request (I know you are applying it to the response) initiated by a Service Worker itself: <https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-2.1.2>?
(In reply to Daniel Bates from comment #17) > (In reply to John Wilander from comment #16) > > > On another note, we should mark this function as inline to give a hint to > > > the compiler that it can inline its body at each call site given that this > > > function is simple. > > > > I've started to avoid inlining since the build time discussion we had. Maybe > > I shouldn't do that. > > > > I do not recall such a general avoidance of using the "inline" keyword. I > mean, it's only a hint to the compiler. We may want to clarify use of > "inline" on webkit-dev. For a simple function that is effectively an alias > for another function it seems acceptable to mark the function as inline. I just recall that inlining caused increased build time. > > > > >> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:359 > > > >> + if (frameOrigin != "null" && frameOrigin != response.url().protocolHostAndPort()) > > > > > > > > How did you come to the decision to treat unique origins (i.e. frameOrigin == "null") as same-origin? > > > > > > It seems reasonable to treat about:blank, about:srcdoc and the empty string > > > as same-origin. Can you please explain how you came to the decision to treat > > > data: URLs and path-separated file URLs as same origin? > > > > Are they null or are you saying they have a host and or origin? > > > > What is the reason that you check if frameOrigin is not "null"? "null" is not considered cross-origin so areFrameAncestorsSameOrigin() should not return false. Otherwise we will block just because the request was made in a frame ancestor chain that includes a "null" origin. Also, top frame document loads are done in the null origin. > > > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:378 > > > > + case WebCore::FromOriginDisposition::SameSite: > > > > + return !areFrameAncestorsSameSite(m_response); > > > > > > This does this not seem correct for XHRs, and fetch() from Web Workers and > > > Service Workers. > > > > I don't doubt that you're right, but can you explain? Thanks! > > Disregard my remark about Web Workers. For Service Workers, is this logic > consistent with how same-site is applied to a request (I know you are > applying it to the response) initiated by a Service Worker itself: > <https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section- > 2.1.2>? I believe same-site in same-site cookies means a host match which is not aligned with same-site in my patch. However, the WHATWG issue discusses this and the current decision is to have "From-Origin: same-site" mean "same eTLD+1." We might change that to "same scheme-eTLD+1" depending on what we believe developers will prefer and how important the scheme is to protect against Spectre attacks. The mixed content blocker allows http images on https pages. If the server responds with "From-Origin: same-site" for the image load and we make same-site mean scheme+eTLD+1, we will block the image loads. This may cause too much breakage and make developers not adopt the header.
John and I are hitting the same issue. The error type might stick to General. One possibility would be to update the IPC serialization of ResourceError. diff --git a/Source/WebKit/Shared/WebCoreArgumentCoders.cpp b/Source/WebKit/Shared/WebCoreArgumentCoders.cpp index 3a1bccf9072..cdf35cc64f8 100644 --- a/Source/WebKit/Shared/WebCoreArgumentCoders.cpp +++ b/Source/WebKit/Shared/WebCoreArgumentCoders.cpp @@ -1270,11 +1270,18 @@ bool ArgumentCoder<ResourceRequest>::decode(Decoder& decoder, ResourceRequest& r void ArgumentCoder<ResourceError>::encode(Encoder& encoder, const ResourceError& resourceError) { encodePlatformData(encoder, resourceError); + encoder.encodeEnum(resourceError.type()); } bool ArgumentCoder<ResourceError>::decode(Decoder& decoder, ResourceError& resourceError) { - return decodePlatformData(decoder, resourceError); + if (!decodePlatformData(decoder, resourceError)) + return false; + ResourceError::Type type; + if (!decoder.decodeEnum(type)) + return false; + resourceError.setType(type); + return true; }
Created attachment 338347 [details] Patch
Comment on attachment 338347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338347&action=review > Source/WebCore/Modules/fetch/FetchResponse.cpp:244 > + responseCallback(Exception { (error.isFromOrigin() ? SecurityError : TypeError), error.localizedDescription() }); We could add a new error type if we want to raise a SecurityError. In that case, I would not make it isFromOrigin specific. We could add a new ResourceError::Type::Security type. But it is not very clear to me what would be the difference between Security and AccessControl. Or we could add a new DOMException type in ResourceError that would be used to fine tune our exception throwing. Maybe, this could be done in a follow-up patch though. Shipping this patch with TypeError instead of SecurityError might be fine? > LayoutTests/http/tests/from-origin/document-from-origin-same-accepted.html:1 > +<!DOCTYPE html> Some of these tests would be very legitimate as WPT tests. If we want to push for that feature and get adoption from other browsers, updating these tests to use testharness.js and upstreaming them as WPT tests would be a nice move.
(In reply to youenn fablet from comment #21) > Comment on attachment 338347 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338347&action=review > > > Source/WebCore/Modules/fetch/FetchResponse.cpp:244 > > + responseCallback(Exception { (error.isFromOrigin() ? SecurityError : TypeError), error.localizedDescription() }); > > We could add a new error type if we want to raise a SecurityError. > In that case, I would not make it isFromOrigin specific. > > We could add a new ResourceError::Type::Security type. > But it is not very clear to me what would be the difference between Security > and AccessControl. I considered AccessControl but it has a very specific meaning in CORS/Fetch and From-Origin is not the same thing. But I could switch to a more generic Security type. The only reason I’m a little hesitant there is that From-Origin is a server-enforced security boundary whereas a security error within the engine is about platform security boundaries such as the same-origin policy. Maybe we don’t make that distinction, though? > Or we could add a new DOMException type in ResourceError that would be used > to fine tune our exception throwing. > > Maybe, this could be done in a follow-up patch though. > Shipping this patch with TypeError instead of SecurityError might be fine? > > > LayoutTests/http/tests/from-origin/document-from-origin-same-accepted.html:1 > > +<!DOCTYPE html> > > Some of these tests would be very legitimate as WPT tests. > If we want to push for that feature and get adoption from other browsers, > updating these tests to use testharness.js and upstreaming them as WPT tests > would be a nice move.
AccessControl is used for more than CORS. It is used for CSP, content blockers, probably port checks as well. From-Origin seems to fit well with these.
(In reply to John Wilander from comment #14) > Thanks for your review, Dan! > > (In reply to Daniel Bates from comment #9) > > Comment on attachment 338186 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=338186&action=review > > > > > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:64 > > > + Vector<String> frameAncestorOrigins; > > > > Is there a reason we are not using SecurityOrigin? > > Yes. I realized we will only be comparing for equality of the origin or its > partition (possibly adding the scheme, still being discussed in WHATWG). > Since this happens for every request, I wanted to make the data encoded, > transferred over XPC, and decoded to be lightweight. > Serialization of origins erases information about unique origins and file URLs because all of these serialize to "null" (*). With regard to unique origins, serialization means we cannot differentiate between a data: and about:. The latter is a unique origin and serializes to "null", but the origin of an about:blank document is "the one it was assigned when its browsing context was created." (**). So, we want to be cognizant of this when perform the From-Origin check. In contrast, data URLs are unique and data URL documents do not inherit their origin. From-Origin should never match a data URL. (*) File URLs conditionally serial to "null". This is another reason why it is problematic to pass strings instead of SecurityOrigin objects. (**) <https://html.spec.whatwg.org/multipage/origin.html#origin>
(In reply to youenn fablet from comment #23) > AccessControl is used for more than CORS. > It is used for CSP, content blockers, probably port checks as well. > From-Origin seems to fit well with these. Just as I suspected, switching to AccessControl triggers a bunch of existing Console error messages that seem to be hardcoded and not pick up the message I set. CONSOLE MESSAGE: Cross-origin image load denied by Cross-Origin Resource Sharing policy. CONSOLE MESSAGE: Cross-origin script load denied by Cross-Origin Resource Sharing policy. CONSOLE MESSAGE: XMLHttpRequest cannot load http://localhost:8000/from-origin/resources/xhr.php?fromOrigin=same due to access control checks.
Note that we added Document::originIdentifierForPasteboard to differentiate unique origins across processes & computers (for Universal Clipboard).
I have most things worked out. Remaining: 1) Figure out how to allow a main frame document to load with a From-Origin response header. The main frame starts out in the unique origin which currently blocks the load. 2) Pipe through console error messages for blocked document loads. 3) File follow-up bug for worker and web sockets tests.
Created attachment 338494 [details] Patch
Some comments on the latest patch: 1) Youenn and I iterated on the error reporting part and finally decided to stick with AccessControl to not risk any hangs due to lost client error reporting. This leads to some superfluous console error messages about CORS. I will file a follow-up bug to revisit how we do this. It should be done through the error's localized message. An additional effect of this is that the Fetch errors are still TypeErrors and not SecurityErrors as requested by Dan. Being able to distinguish these errors should be part of the follow-work. 2) The SecurityOrigins are passed as a vector of RefPtrs. We should switch to a vector of Refs and I will file a follow-up bug for that. 3) As requested by Dan, we need additional tests for WebWorkers and ServiceWorkers. I also mentioned WebSockets. This will be done in a follow-up bug. 4) The tests should be converted to Web Platform tests. This is follow-up work.
Created attachment 338499 [details] Patch
The new patch just removed old changelog comments about the null origin being accepted. It no longer is.
Created attachment 338509 [details] Patch
Fixed a uint64 -> uint64_t typo. I have no idea how it passed locally.
Comment on attachment 338509 [details] Patch Attachment 338509 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7392711 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus-sync.htm imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-redirect.https.html
Created attachment 338519 [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 338509 [details] Patch Attachment 338509 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7394256 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus-sync.htm
Created attachment 338525 [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
We probably need to suppress console messages in expectations for imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-bogus-sync.htm. I don't know what to do with the extra console message in iOS Sim results for imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-redirect.https.html.
It's probably only the iOS bot that runs the service worker tests which explains why the result diff only happens there. However, imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-redirect.https.html expects a console output so I can't suppress them.
Created attachment 338529 [details] Patch
Comment on attachment 338529 [details] Patch This is almost good to go to me. Can you rebase the patch based on other changes that landed today. This should simplify your patch and fix some From-Origin cases that should trigger more errors. Some nits below. View in context: https://bugs.webkit.org/attachment.cgi?id=338529&action=review > Source/WebCore/platform/network/HTTPParsers.cpp:902 > + auto strippedHeader = header.stripWhiteSpace(); We should probably be using stripLeadingAndTrailingHTTPSpaces which has the benefit of not creating a new String. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:412 > + return ShouldContinueDidReceiveResponse::No; This is kind of ok but does not cover the case of cached entries. Instead of doing that check here, it should be done in NetworkLoadChecker::validateResponse to cover the cached entry case and the case of 304 responses that may not contain From-Origin but the cached response does. > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1274 > + encoder.encodeEnum(resourceError.type()); This is no longer needed as another patch fixed it already. > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2841 > + origins.reserveCapacity(dataSize); I would use reserveInitialCapacity so that we ensure that origins is empty. > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2846 > + origins.uncheckedAppend(decodedOriginRefPtr); decodedOriginRefPtr.releaseNotNull() > Source/WebKit/Shared/WebCoreArgumentCoders.h:705 > +}; If we add template<> struct ArgumentCoder<RefPtr<WebCore::SecurityOrigin>>, we might be able to use the regular Vector encoder. If it cannot be done in that patch, we should fix it as a follow-up when moving to a Vector<Ref<SecurityOrigin>> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:303 > + loadParameters.isMainFrameNavigation = resourceLoader.frame() && resourceLoader.frame()->isMainFrame() && resourceLoader.options().mode == FetchOptions::Mode::Navigate; Maybe we should merge these two parameters. If isMainFrameNavigation is true, shouldEnableFromOriginResponseHeader could be set to false. > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:312 > + loadParameters.frameAncestorOrigins = frameAncestorOrigins; WTFMove()
Thanks for your review comments, Youenn! (In reply to youenn fablet from comment #41) > Comment on attachment 338529 [details] > Patch > > This is almost good to go to me. > Can you rebase the patch based on other changes that landed today. > This should simplify your patch and fix some From-Origin cases that should > trigger more errors. > Some nits below. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338529&action=review > > > Source/WebCore/platform/network/HTTPParsers.cpp:902 > > + auto strippedHeader = header.stripWhiteSpace(); > > We should probably be using stripLeadingAndTrailingHTTPSpaces which has the > benefit of not creating a new String. Will fix. > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:412 > > + return ShouldContinueDidReceiveResponse::No; > > This is kind of ok but does not cover the case of cached entries. > Instead of doing that check here, it should be done in > NetworkLoadChecker::validateResponse to cover the cached entry case and the > case of 304 responses that may not contain From-Origin but the cached > response does. I did this change which requires that I return true in NetworkResourceLoader's shouldUseNetworkLoadChecker() for when parameters.shouldEnableFromOriginResponseHeader is true. This effectively means the checker is turned on as soon as we do subresource loads because the response may contain a From-Origin header. The checker being run much more often results in test failures (I only ran http tests) … http/tests/cache/disk-cache/disk-cache-redirect-to-data.html failed unexpectedly (text diff) http/tests/eventsource/eventsource-cors-basic.html failed unexpectedly (text diff) http/tests/eventsource/eventsource-cors-with-credentials.html failed unexpectedly (text diff) http/tests/fetch/caching-with-different-options.html failed unexpectedly (text diff) http/tests/security/cannot-read-cssrules-redirect.html failed unexpectedly (text diff) http/tests/security/contentSecurityPolicy/connect-src-eventsource-redirect-to-blocked.html failed unexpectedly (text diff) http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-redirect-to-blocked.html failed unexpectedly (text diff) http/tests/security/contentSecurityPolicy/1.1/child-src/worker-redirect-blocked.html failed unexpectedly (text diff) http/tests/security/isolatedWorld/bypass-main-world-csp-worker-redirect.html failed unexpectedly (text diff) http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html failed unexpectedly (text diff) http/tests/security/mixedContent/insecure-plugin-in-iframe.html failed unexpectedly (text diff) http/tests/subresource-integrity/sri-fetch-worker.html failed unexpectedly (text diff) http/tests/security/cross-origin-xsl-redirect-BLOCKED.html failed unexpectedly (text diff) http/tests/subresource-integrity/sri-fetch.html failed unexpectedly (text diff) http/tests/security/load-image-after-redirection.html failed unexpectedly (text diff) http/tests/security/shape-image-cors-redirect-error-message-logging-3.html failed unexpectedly (text diff) http/tests/security/shape-image-cors-redirect-error-message-logging-4.html failed unexpectedly (text diff) http/tests/security/worker-cross-origin.html failed unexpectedly (text diff) http/tests/xmlhttprequest/access-control-and-redirects-async-same-origin.html failed unexpectedly (text diff) http/tests/xmlhttprequest/access-control-and-redirects-async.html failed unexpectedly (text diff) http/tests/xmlhttprequest/access-control-and-redirects.html failed unexpectedly (text diff) http/tests/xmlhttprequest/access-control-basic-allow-preflight-cache-invalidation-by-method.html failed unexpectedly (text diff) http/tests/workers/worker-redirect.html failed unexpectedly (text diff) http/tests/xmlhttprequest/access-control-repeated-failed-preflight-crash.html failed unexpectedly (text diff) http/tests/xmlhttprequest/cross-origin-no-authorization.html failed unexpectedly (text diff) http/tests/xmlhttprequest/cross-origin-no-credential-prompt.html failed unexpectedly (text diff) http/tests/xmlhttprequest/cross-site-denied-response.html failed unexpectedly (text diff) http/tests/xmlhttprequest/onerror-event.html failed unexpectedly (text diff) http/tests/xmlhttprequest/origin-whitelisting-https.html failed unexpectedly (text diff) http/tests/xmlhttprequest/origin-whitelisting-ip-addresses-with-subdomains.html failed unexpectedly (text diff) http/tests/xmlhttprequest/post-blob-content-type-async.html failed unexpectedly (text diff) http/tests/workers/service/shift-reload-navigation.html failed unexpectedly (text diff) http/tests/xmlhttprequest/redirect-cross-origin-2.html failed unexpectedly (text diff) http/tests/xmlhttprequest/redirect-cross-origin-post.html failed unexpectedly (text diff) http/tests/xmlhttprequest/redirect-cross-origin-tripmine.html failed unexpectedly (text diff) http/tests/xmlhttprequest/redirect-cross-origin.html failed unexpectedly (text diff) http/tests/xmlhttprequest/redirections-and-user-headers.html failed unexpectedly (text diff) http/tests/xmlhttprequest/simple-cross-origin-denied-events.html failed unexpectedly (text diff) http/tests/xmlhttprequest/simple-cross-origin-progress-events.html failed unexpectedly (text diff) … and timeouts that I don't know if they're expected: http/tests/appcache/identifier-test.html failed unexpectedly (test timed out, test was not run) http/tests/download/anchor-load-after-download.html failed unexpectedly (test timed out, test was not run) http/tests/from-origin/script-from-origin-same-accepted.html failed unexpectedly (test timed out, test was not run) http/tests/history/replacestate-no-url.html failed unexpectedly (test timed out, test was not run) http/tests/appcache/online-whitelist.html failed unexpectedly (test timed out, test was not run) http/tests/security/canvas-remote-read-data-url-image-redirect.html failed unexpectedly (test timed out, text diff) http/tests/workers/service/cors-image-fetch.html failed unexpectedly (test timed out, text diff) > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1274 > > + encoder.encodeEnum(resourceError.type()); > > This is no longer needed as another patch fixed it already. Will remove my changes. > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2841 > > + origins.reserveCapacity(dataSize); > > I would use reserveInitialCapacity so that we ensure that origins is empty. Will fix. > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2846 > > + origins.uncheckedAppend(decodedOriginRefPtr); > > decodedOriginRefPtr.releaseNotNull() Will fix. > > Source/WebKit/Shared/WebCoreArgumentCoders.h:705 > > +}; > > If we add template<> struct ArgumentCoder<RefPtr<WebCore::SecurityOrigin>>, > we might be able to use the regular Vector encoder. > If it cannot be done in that patch, we should fix it as a follow-up when > moving to a Vector<Ref<SecurityOrigin>> Leaving for follow-up. > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:303 > > + loadParameters.isMainFrameNavigation = resourceLoader.frame() && resourceLoader.frame()->isMainFrame() && resourceLoader.options().mode == FetchOptions::Mode::Navigate; > > Maybe we should merge these two parameters. If isMainFrameNavigation is > true, shouldEnableFromOriginResponseHeader could be set to false. Will fix. > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:312 > > + loadParameters.frameAncestorOrigins = frameAncestorOrigins; > > WTFMove() Will fix.
Created attachment 338565 [details] Patch using WebKit::NetworkLoadChecker Attaching patch with Youenn's latest proposed changes. This causes test failures as described.
> > This is kind of ok but does not cover the case of cached entries. > > Instead of doing that check here, it should be done in > > NetworkLoadChecker::validateResponse to cover the cached entry case and the > > case of 304 responses that may not contain From-Origin but the cached > > response does. > > I did this change which requires that I return true in > NetworkResourceLoader's shouldUseNetworkLoadChecker() for when > parameters.shouldEnableFromOriginResponseHeader is true. This effectively > means the checker is turned on as soon as we do subresource loads because > the response may contain a From-Origin header. > > The checker being run much more often results in test failures (I only ran > http tests) … Right, goal is for the checker to be activated for all loads baed on a runtime flag. This should be done fairly quickly this week. That said, NetworkLoadChecker is activated based on a runtime flag which is different from From-Origin runtime flag. And one might become active sooner than the other. To cover all cases, I think a From-Origin check should be added to any place where validateResponse/sanitizeResponseIfPossible is called in NetworkResourceLoader.
Created attachment 338606 [details] Patch
Created attachment 338607 [details] Patch
Comment on attachment 338607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338607&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:64 > + Vector<RefPtr<WebCore::SecurityOrigin>> frameAncestorOrigins { }; No need for { }; > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:365 > +static inline String partitionName(const String& host) Not sure it brings much, maybe we can remove it? > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:375 > + if (frameOrigin.get()->isUnique() || partitionName(frameOrigin.get()->host()) != responsePartition) frameOrigin->isUnique/host would work I believe. But instead you can probably try using Vector::findMatching > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:389 > + } Ditto with Vector::findMatching > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:393 > +bool NetworkResourceLoader::shouldCancelCrossOriginLoad(const ResourceResponse& response) const You could make it a free function by passing a const Vector<RefPtr<SecurityOrigin>>& I would probably go with something like: ResourceError validateFromOriginResponse(const NetworkResourceLoadParameters&, const ResourceResponse&) Then callers would check for isNull(). Another approach might be to return an optional so that you could write: if (auto error = validateFromOriginResponse(...)) > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:446 > + else if (m_networkLoadChecker) We probably want to keep checking the response with m_networkLoadChecker. So something like: if (m_parameters.shouldEnableFromOriginResponseHeader && shouldCancelCrossOriginLoad(m_response)) error = fromOriginResourceError(m_response.url()); if (error.isNull() && m_networkLoadChecker) ... > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:609 > + didFailLoading(fromOriginResourceError(url)); Please check for "if (m_networkLoad)" as the load might be cancelled between the time we call callOnMainThread and the time we execute the lambda. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:736 > + else if (m_networkLoadChecker) Ditto, we need to call m_networkLoadChecker->validateResponse if error is not null. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:848 > + auto response = entry->response(); There is no need to construct a new ResourceResponse here. We can probably pass the const ResourceResponse& directly to shouldCancelCrossOriginLoad. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:851 > + didFailLoading(fromOriginResourceError(url)); Ditto for if (m_networkLoad)
Created attachment 338630 [details] Patch
Thanks, Youenn! I've fixed all the things you commented on.
GTK's compiler is segfaulting.
WPE compiler is running out of memory.
WPE/GTK bots have some issues these past days.
Comment on attachment 338630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338630&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:373 > + return false; Probably need UNUSED_PARAM for response and frameAncestorOrigins. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:380 > + return !item->isSameOriginAs(SecurityOrigin::create(response.url())); It is a bit sad to create a SecurityOrigin for this check but this is the way SecurityOrigin is now. We should create it only once, outside the lambda. With this current check, any unique origin will make the test fail. Note that a data URL frame being controlled by its parent service worker will be able to load resources that it would not be able if there was no service worker. We probably need to write a test for this case as well and decide whether this 'inconsistency' is fine or should be fixed in some way. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:604 > + return; Actually, I am not sure we need to do call callOnMainThread here. Can you check whether we can remove it? > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:726 > + ResourceError error { }; no need for " { };" > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:844 > + callOnMainThread([protectedThis = makeRef(*this), this, url = response.url()] { Not sure we need this callOnMainThread either. > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:311 > + break; We currently send the frame hierarchy at the start of the load. I guess that current CSP checks are using the hierarchy at the time we process the response in WebProcess. Could the hierarchy evolve during the load and have an impact on the CSP or FromOrigin checks? If so, doing this in NetworkProcess might be slightly different from doing it in WebProcess. This patch approach seems fine as is for now but it would be good to know whether ancestor chain changes can be an issue in practice.
Created attachment 338658 [details] Patch for landing
Thank you, Youenn! I made all the final changes you asked for.
Comment on attachment 338658 [details] Patch for landing Rejecting attachment 338658 [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', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 338658, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: 04bc25a7a master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 230963 = 613b51e641bbfab862ce9f540f41caa94b63ff98 r230964 = cd04bc25a7a7a884c6e11c8b8ef0322a9f063440 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Total errors found: 0 in 3 files Full output: http://webkit-queues.webkit.org/results/7425092
Committed r230968: <https://trac.webkit.org/changeset/230968>
Landed manually. We'll see how it goes.
It could use a RELEASE_ASSERT_NOT_REACHED() at the bottom here: ../../Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp: In function ‘bool WebKit::shouldCancelCrossOriginLoad(const WebCore::ResourceResponse&, const WTF::Vector<WTF::RefPtr<WebCore::SecurityOrigin> >&)’: ../../Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:398:1: warning: control reaches end of non-void function [-Wreturn-type] } ^ to teach GCC that it's unreachable.
Committed r230997: <https://trac.webkit.org/changeset/230997>
Thanks, Michael!
Comment on attachment 338658 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=338658&action=review > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:446 > + protectedThis->didFailLoading(error); From my understanding From-Origin can be used to achieve the same effect as X-Frame-Options when delivered in the HTTP response to a document. We need to take the same approach as we do for handling a X-Frame-Options violation and make it appear that the page did load successfully because failing the load the way this code does reveals the existence of the document, which is sensitive. In particular, we need to cancel the load, logging a console message, sandboxing the iframe that load was destined for and dispatching a DOM load event to the destination frame. For subresources we need to keep the current behavior and fail the load. A side benefit of taking this approach is that we can remove the use of the setTimeout() from the test sandboxed-sub-frame-from-origin-same-blocked.html and instead end the test once we get a DOM load event for the frame. > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:602 > + didFailLoading(fromOriginResourceError(redirectResponse.url())); Ditto. (It’s mildly amusing that we call shouldCancelCrossOriginLoad() and fail the load by calling didFailLoading() if it returns true. I mean, the name of that function explicitly mentions “cancel” as the action a caller should take based on its result.) > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:843 > + didFailLoading(fromOriginResourceError(response.url())); Ditto. > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:334 > + if (frame->document()) I thought you were going to remove this per comment 14 or were you looking for a confirmatory reply. This check is unnecessary as I can tell. > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:336 > + if (frame->isMainFrame()) This if statement is unncessary now that you are using a for-loop because the main frame is defined to be the frame without a parent. > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:339 > + loadParameters.frameAncestorOrigins = WTFMove(frameAncestorOrigins); I wish we could have come up with a better name for this parameter. I mean this vector includes both the origin of the initiating frame’s document and the origin of its ancestors. That is, it does not only contain the origin of the ancestor frames.
> From my understanding From-Origin can be used to achieve the same effect as > X-Frame-Options when delivered in the HTTP response to a document. We need > to take the same approach as we do for handling a X-Frame-Options violation > and make it appear that the page did load successfully because failing the > load the way this code does reveals the existence of the document, which is > sensitive. In particular, we need to cancel the load, logging a console > message, sandboxing the iframe that load was destined for and dispatching a > DOM load event to the destination frame. For subresources we need to keep > the current behavior and fail the load. We should probably be consistent no matter whether the load destination is Document or not. I guess that we could either fail or return an empty response. This seems to be a good topic to discuss in whatwg/fetch/github