SubresourceLoader is doing redirection checks in CORS mode but not in No-Cors mode. If a resource is fetched in no-cors mode, the cached resource may be reused in cors mode without revalidation each redirection. Also, reusing a cached resource for another load with different fetch options has impact on the CachedResource. Its response tainting and its origin clean in particular may not be correct.
If we plan to further align with the specs, we should probably forbid reusing resources with different fetch options. Or we should have a way to easily clone one reusable resource into a new one with correctly set states. Note that this does not apply to XHR/fetch API, since resources are not allowed to be reused in that case.
Created attachment 287404 [details] Patch
This patch is somehow going against https://trac.webkit.org/changeset/201805 since we would reload whenever the origin changes. I am not sure how important for performances it is. But it would be much simpler/less error-prone to have simpler rules for the cache. If performance is an issue and reloading is not a good enough idea, we might want to try separating resource specific data from the other cached resource data (the request, the options, the states), so that we can efficiently create a new CachedResource from one in the cache. Any insight?
Comment on attachment 287404 [details] Patch Attachment 287404 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1974146 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html http/tests/security/cross-origin-cached-resource-parallel.html imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.html http/tests/loading/cross-origin-XHR-willLoadRequest.html imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html
Created attachment 287412 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 287404 [details] Patch Attachment 287404 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1974156 New failing tests: http/tests/security/cross-origin-cached-resource-parallel.html imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html
Created attachment 287414 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 287404 [details] Patch Attachment 287404 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1974155 New failing tests: http/tests/security/cross-origin-cached-resource-parallel.html imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.html http/tests/loading/cross-origin-XHR-willLoadRequest.html imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html
Created attachment 287417 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 287404 [details] Patch Attachment 287404 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1974174 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html http/tests/security/cross-origin-cached-resource-parallel.html http/tests/navigation/post-frames-goback1.html imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.html imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html
Created attachment 287418 [details] Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 287518 [details] Rebasing failing test expectations
Comment on attachment 287518 [details] Rebasing failing test expectations Attachment 287518 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1981523 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html http/tests/navigation/post-frames-goback1.html imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.html
Created attachment 287520 [details] Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 287616 [details] Cloning CachedImage when needed
Comment on attachment 287616 [details] Cloning CachedImage when needed Attachment 287616 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1987688 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html
Created attachment 287620 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 287616 [details] Cloning CachedImage when needed Attachment 287616 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1987694 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html
Created attachment 287621 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 287616 [details] Cloning CachedImage when needed Attachment 287616 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1987696 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html
Created attachment 287623 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 287616 [details] Cloning CachedImage when needed Attachment 287616 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1987704 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html
Created attachment 287625 [details] Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 287627 [details] Fixing test
Comment on attachment 287627 [details] Fixing test Attachment 287627 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1988108 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-origin-worker.html imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html
Created attachment 287638 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 287627 [details] Fixing test Attachment 287627 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1988120 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-origin-worker.html imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html
Created attachment 287641 [details] Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 287749 [details] WIP: Adding new expectations for WK2 failing tests
Created attachment 287758 [details] Adding CachedImage cloning
Created attachment 287759 [details] Adding CachedImage cloning
Comment on attachment 287759 [details] Adding CachedImage cloning Attachment 287759 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1996035 New failing tests: http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html
Created attachment 287810 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 287759 [details] Adding CachedImage cloning View in context: https://bugs.webkit.org/attachment.cgi?id=287759&action=review > Source/WebCore/loader/SubresourceLoader.cpp:421 > + cancel(ResourceError(String(), 0, request().url(), errorMessage, ResourceError::Type::AccessControl)); I think it’s confusing factoring to have this function actually cancel the load of the resource; the name of the function is "check" and it returns a boolean. Is this following some existing pattern in this class? If the function’s job is to log an error and cancel if there is something wrong, then I think "check" is not a sufficiently clear name for it. A function named check seems like it would do a check and return the results of that check to the caller, not do a check and then, if the check fails, take action causing various side effects. > Source/WebCore/loader/SubresourceLoader.cpp:425 > + return false; > + > +} Extra blank line here. > Source/WebCore/loader/cache/CachedImage.cpp:124 > + if (m_image && m_image->isSVGImage()) Should be able to write this instead: if (is<SVGImage>(m_image)) > Source/WebCore/loader/cache/CachedImage.cpp:125 > + m_svgImageCache = std::make_unique<SVGImageCache>(static_cast<SVGImage*>(m_image.get())); This should use &downcast<SVGImage>(*m_image), and not static_cast. > Source/WebCore/loader/cache/CachedResource.cpp:244 > +void CachedResource::computeOrigin(CachedResourceLoader& cachedResourceLoader) Given the context, I think we can just call this "loader". After all, this is a CachedResource member function, and in that context loader would seem to mean a CachedResourceLoader. > Source/WebCore/loader/cache/CachedResource.cpp:246 > + if (type() != MainResource) { Now that this is in a function, I suggest using an early return rather than putting the entire function’s logic nested inside an if. > Source/WebCore/loader/cache/CachedResource.cpp:250 > + m_origin = cachedResourceLoader.document()->securityOrigin(); What guarantee do we have that loader.document() is not null? Why does it return a pointer if it can never be null? > Source/WebCore/loader/cache/CachedResource.cpp:254 > + if (!(m_resourceRequest.url().protocolIsData() && m_options.sameOriginDataURLFlag == SameOriginDataURLFlag::Set) && !m_origin->canRequest(m_resourceRequest.url())) > + setCrossOrigin(); Code you moved here and it’s not new, but: There’s an extra space here before "&&". And the special case for the data protocol is really ugly. Would be nice to find a better way to factor that. > Source/WebCore/loader/cache/CachedResource.h:213 > + void didReceiveResponse(const ResourceResponse&, Document*); Why a Document* rather than Document&? Can it be null? > Source/WebCore/loader/cache/CachedResource.h:311 > + virtual void cloneData(const CachedResource&) { } What does it mean to "clone" data? I am not sure this is a good function name.
Created attachment 287945 [details] Patch for landing
Comment on attachment 287945 [details] Patch for landing Clearing flags on attachment: 287945 Committed r205450: <http://trac.webkit.org/changeset/205450>
All reviewed patches have been landed. Closing bug.
Thanks for the review. (In reply to comment #34) > Comment on attachment 287759 [details] > Adding CachedImage cloning > > View in context: > https://bugs.webkit.org/attachment.cgi?id=287759&action=review > > > Source/WebCore/loader/SubresourceLoader.cpp:421 > > + cancel(ResourceError(String(), 0, request().url(), errorMessage, ResourceError::Type::AccessControl)); > > I think it’s confusing factoring to have this function actually cancel the > load of the resource; the name of the function is "check" and it returns a > boolean. Is this following some existing pattern in this class? Right, I updated the patch to stick to the current code organisation. I'll see whether we can make some refactoring in the future. > > Source/WebCore/loader/SubresourceLoader.cpp:425 > > + return false; > > + > > +} > > Extra blank line here. OK > > Source/WebCore/loader/cache/CachedImage.cpp:124 > > + if (m_image && m_image->isSVGImage()) > > Should be able to write this instead: OK > if (is<SVGImage>(m_image)) > > > Source/WebCore/loader/cache/CachedImage.cpp:125 > > + m_svgImageCache = std::make_unique<SVGImageCache>(static_cast<SVGImage*>(m_image.get())); > > This should use &downcast<SVGImage>(*m_image), and not static_cast. OK > > Source/WebCore/loader/cache/CachedResource.cpp:244 > > +void CachedResource::computeOrigin(CachedResourceLoader& cachedResourceLoader) > > Given the context, I think we can just call this "loader". After all, this > is a CachedResource member function, and in that context loader would seem > to mean a CachedResourceLoader. OK > > Source/WebCore/loader/cache/CachedResource.cpp:246 > > + if (type() != MainResource) { > > Now that this is in a function, I suggest using an early return rather than > putting the entire function’s logic nested inside an if. OK > > Source/WebCore/loader/cache/CachedResource.cpp:250 > > + m_origin = cachedResourceLoader.document()->securityOrigin(); > > What guarantee do we have that loader.document() is not null? Why does it > return a pointer if it can never be null? Since the resource is not a main resource, there should be a document. I added an ASSERT > > Source/WebCore/loader/cache/CachedResource.cpp:254 > > + if (!(m_resourceRequest.url().protocolIsData() && m_options.sameOriginDataURLFlag == SameOriginDataURLFlag::Set) && !m_origin->canRequest(m_resourceRequest.url())) > > + setCrossOrigin(); > > Code you moved here and it’s not new, but: There’s an extra space here > before "&&". And the special case for the data protocol is really ugly. > Would be nice to find a better way to factor that. OK As for the refactoring, I agree. This check should be done early in the CachedResourceLoader loading code. Further refactoring should help here. > > Source/WebCore/loader/cache/CachedResource.h:213 > > + void didReceiveResponse(const ResourceResponse&, Document*); > > Why a Document* rather than Document&? Can it be null? Removed. > > Source/WebCore/loader/cache/CachedResource.h:311 > > + virtual void cloneData(const CachedResource&) { } > > What does it mean to "clone" data? I am not sure this is a good function > name. Renamed to setBodyDataFrom.
This change made http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html very flaky, so much that is causes false positives on EWS. I'm going to roll out because of EWS impact.
Diff: @@ -1,3 +1,4 @@ +CONSOLE MESSAGE: Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin. CONSOLE MESSAGE: XMLHttpRequest cannot load http://127.0.0.1:8000/security/contentSecurityPolicy/resources/redir.php?url=http://localhost:8000/xmlhttprequest/resources/access-control-basic-allow.cgi. Cross-origin redirection denied by Content Security Policy. This tests an XHR request made from a worker is blocked if it redirects to a cross-origin resource that is not listed as a connect-src in the CSP of the worker. imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html is flaky too: @@ -1,4 +1,3 @@ -CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by Access-Control-Allow-Origin. CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by Access-Control-Allow-Origin. CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by Access-Control-Allow-Origin. CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by Access-Control-Allow-Origin.
Re-opened since this is blocked by bug 161614
Thanks for spotting these issues. > @@ -1,3 +1,4 @@ > +CONSOLE MESSAGE: Origin http://127.0.0.1:8000 is not allowed by > Access-Control-Allow-Origin. > CONSOLE MESSAGE: XMLHttpRequest cannot load > http://127.0.0.1:8000/security/contentSecurityPolicy/resources/redir. > php?url=http://localhost:8000/xmlhttprequest/resources/access-control-basic- > allow.cgi. Cross-origin redirection denied by Content Security Policy. > This tests an XHR request made from a worker is blocked if it redirects to > a cross-origin resource that is not listed as a connect-src in the CSP of > the worker. It seems that DocumentThreadableLoader sets the ResourceRequest to null when being notified of a redirection. Then SubresourceLoader gets notified of the same redirection response in didReceiveResponse. This is messing up with CORS checks as SubresourceLoader expects to check the response following the redirectionResponse, not the redirection respponse itself. I'll mark the test as flaky and fix it in a follow-up patch. > imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html is > flaky too: > > @@ -1,4 +1,3 @@ > -CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by > Access-Control-Allow-Origin. > CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by > Access-Control-Allow-Origin. > CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by > Access-Control-Allow-Origin. > CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by > Access-Control-Allow-Origin. Test should be fixed: one of the sub-test is a promise test but promise is not returned so test is not waiting for the end of the test. I'll fix it in this landing patch.
Created attachment 288003 [details] Patch for landing
Comment on attachment 288003 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=288003&action=review > LayoutTests/imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.js:23 > + return fetch(url + RESOURCES_DIR + "top.txt", {"mode": "cors"} ).then(function(resp) { Here is the fix for WPT flaky test.
Created attachment 288006 [details] Patch for landing
Comment on attachment 288006 [details] Patch for landing Rejecting attachment 288006 [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', 288006, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: stage entries for merged file 'LayoutTests/imported/w3c' Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 128 cwd: /Volumes/Data/EWS/WebKit You have both LayoutTests/imported/w3c and LayoutTests/imported/w3c/ChangeLog fatal: multiple stage entries for merged file 'LayoutTests/imported/w3c' Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 128 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Total errors found: 0 in 1 files Full output: http://webkit-queues.webkit.org/results/2016876
Comment on attachment 288006 [details] Patch for landing Clearing flags on attachment: 288006 Committed r205473: <http://trac.webkit.org/changeset/205473>