Title: [206717] trunk
Revision
206717
Author
[email protected]
Date
2016-10-02 07:25:08 -0700 (Sun, 02 Oct 2016)

Log Message

Unreviewed, rolling out r206716.
https://bugs.webkit.org/show_bug.cgi?id=162858

It is breaking Mac CMake Debug build (Requested by youenn on
#webkit).

Reverted changeset:

"[Fetch API] Forbid redirection to non-HTTP(s) URL in non-
navigation mode."
https://bugs.webkit.org/show_bug.cgi?id=162785
http://trac.webkit.org/changeset/206716

Modified Paths

Diff

Modified: trunk/LayoutTests/imported/w3c/ChangeLog (206716 => 206717)


--- trunk/LayoutTests/imported/w3c/ChangeLog	2016-10-02 13:59:18 UTC (rev 206716)
+++ trunk/LayoutTests/imported/w3c/ChangeLog	2016-10-02 14:25:08 UTC (rev 206717)
@@ -1,3 +1,18 @@
+2016-10-02  Commit Queue  <[email protected]>
+
+        Unreviewed, rolling out r206716.
+        https://bugs.webkit.org/show_bug.cgi?id=162858
+
+        It is breaking Mac CMake Debug build (Requested by youenn on
+        #webkit).
+
+        Reverted changeset:
+
+        "[Fetch API] Forbid redirection to non-HTTP(s) URL in non-
+        navigation mode."
+        https://bugs.webkit.org/show_bug.cgi?id=162785
+        http://trac.webkit.org/changeset/206716
+
 2016-10-02  Youenn Fablet  <[email protected]>
 
         [Fetch API] Forbid redirection to non-HTTP(s) URL in non-navigation mode.

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-expected.txt (206716 => 206717)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-expected.txt	2016-10-02 13:59:18 UTC (rev 206716)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-expected.txt	2016-10-02 14:25:08 UTC (rev 206717)
@@ -4,8 +4,8 @@
 CONSOLE MESSAGE: Cross-origin redirection to data:text/plain;base64,cmVzcG9uc2UncyBib2R5 denied by Cross-Origin Resource Sharing policy: URL is either a non-HTTP URL or contains credentials.
 
 PASS Testing data URL loading after same-origin redirection (cors mode) 
-PASS Testing data URL loading after same-origin redirection (no-cors mode) 
+FAIL Testing data URL loading after same-origin redirection (no-cors mode) assert_unreached: Should have rejected. Reached unreachable code
 PASS Testing data URL loading after same-origin redirection (same-origin mode) 
 PASS Testing data URL loading after cross-origin redirection (cors mode) 
-PASS Testing data URL loading after cross-origin redirection (no-cors mode) 
+FAIL Testing data URL loading after cross-origin redirection (no-cors mode) assert_unreached: Should have rejected. Reached unreachable code
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-worker-expected.txt (206716 => 206717)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-worker-expected.txt	2016-10-02 13:59:18 UTC (rev 206716)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-to-dataurl-worker-expected.txt	2016-10-02 14:25:08 UTC (rev 206717)
@@ -4,8 +4,8 @@
 CONSOLE MESSAGE: Cross-origin redirection to data:text/plain;base64,cmVzcG9uc2UncyBib2R5 denied by Cross-Origin Resource Sharing policy: URL is either a non-HTTP URL or contains credentials.
 
 PASS Testing data URL loading after same-origin redirection (cors mode) 
-PASS Testing data URL loading after same-origin redirection (no-cors mode) 
+FAIL Testing data URL loading after same-origin redirection (no-cors mode) assert_unreached: Should have rejected. Reached unreachable code
 PASS Testing data URL loading after same-origin redirection (same-origin mode) 
 PASS Testing data URL loading after cross-origin redirection (cors mode) 
-PASS Testing data URL loading after cross-origin redirection (no-cors mode) 
+FAIL Testing data URL loading after cross-origin redirection (no-cors mode) assert_unreached: Should have rejected. Reached unreachable code
 

Modified: trunk/Source/WebCore/ChangeLog (206716 => 206717)


--- trunk/Source/WebCore/ChangeLog	2016-10-02 13:59:18 UTC (rev 206716)
+++ trunk/Source/WebCore/ChangeLog	2016-10-02 14:25:08 UTC (rev 206717)
@@ -1,3 +1,18 @@
+2016-10-02  Commit Queue  <[email protected]>
+
+        Unreviewed, rolling out r206716.
+        https://bugs.webkit.org/show_bug.cgi?id=162858
+
+        It is breaking Mac CMake Debug build (Requested by youenn on
+        #webkit).
+
+        Reverted changeset:
+
+        "[Fetch API] Forbid redirection to non-HTTP(s) URL in non-
+        navigation mode."
+        https://bugs.webkit.org/show_bug.cgi?id=162785
+        http://trac.webkit.org/changeset/206716
+
 2016-10-02  Youenn Fablet  <[email protected]>
 
         [Fetch API] Forbid redirection to non-HTTP(s) URL in non-navigation mode.

Modified: trunk/Source/WebCore/WebCore.xcodeproj/project.pbxproj (206716 => 206717)


--- trunk/Source/WebCore/WebCore.xcodeproj/project.pbxproj	2016-10-02 13:59:18 UTC (rev 206716)
+++ trunk/Source/WebCore/WebCore.xcodeproj/project.pbxproj	2016-10-02 14:25:08 UTC (rev 206717)
@@ -5791,7 +5791,7 @@
 		CE2849871CA360DF00B4A57F /* ContentSecurityPolicyDirectiveNames.h in Headers */ = {isa = PBXBuildFile; fileRef = CE2849861CA360DF00B4A57F /* ContentSecurityPolicyDirectiveNames.h */; };
 		CE2849891CA3614600B4A57F /* ContentSecurityPolicyDirectiveNames.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CE2849881CA3614600B4A57F /* ContentSecurityPolicyDirectiveNames.cpp */; };
 		CE6DADF91C591E6A003F6A88 /* ContentSecurityPolicyResponseHeaders.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CE6DADF71C591E6A003F6A88 /* ContentSecurityPolicyResponseHeaders.cpp */; };
-		CE6DADFA1C591E6A003F6A88 /* ContentSecurityPolicyResponseHeaders.h in Headers */ = {isa = PBXBuildFile; fileRef = CE6DADF81C591E6A003F6A88 /* ContentSecurityPolicyResponseHeaders.h */; settings = {ATTRIBUTES = (Private, ); }; };
+		CE6DADFA1C591E6A003F6A88 /* ContentSecurityPolicyResponseHeaders.h in Headers */ = {isa = PBXBuildFile; fileRef = CE6DADF81C591E6A003F6A88 /* ContentSecurityPolicyResponseHeaders.h */; };
 		CE799F971C6A46BC0097B518 /* ContentSecurityPolicySourceList.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CE799F951C6A46BC0097B518 /* ContentSecurityPolicySourceList.cpp */; };
 		CE799F981C6A46BC0097B518 /* ContentSecurityPolicySourceList.h in Headers */ = {isa = PBXBuildFile; fileRef = CE799F961C6A46BC0097B518 /* ContentSecurityPolicySourceList.h */; };
 		CE799F9B1C6A4BCD0097B518 /* ContentSecurityPolicyDirectiveList.cpp in Sources */ = {isa = PBXBuildFile; fileRef = CE799F991C6A4BCD0097B518 /* ContentSecurityPolicyDirectiveList.cpp */; };

Modified: trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp (206716 => 206717)


--- trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp	2016-10-02 13:59:18 UTC (rev 206716)
+++ trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp	2016-10-02 14:25:08 UTC (rev 206717)
@@ -216,11 +216,6 @@
     client.didFail(ResourceError(errorDomainWebKitInternal, 0, url, "Cross-origin redirection denied by Cross-Origin Resource Sharing policy.", ResourceError::Type::AccessControl));
 }
 
-static inline void reportRedirectionWithBadScheme(ThreadableLoaderClient& client, const URL& url)
-{
-    client.didFail(ResourceError(errorDomainWebKitInternal, 0, url, "Redirection to URL with a scheme that is not HTTP(S).", ResourceError::Type::AccessControl));
-}
-
 void DocumentThreadableLoader::redirectReceived(CachedResource* resource, ResourceRequest& request, const ResourceResponse& redirectResponse)
 {
     ASSERT(m_client);
@@ -227,16 +222,6 @@
     ASSERT_UNUSED(resource, resource == m_resource);
 
     Ref<DocumentThreadableLoader> protectedThis(*this);
-
-    // FIXME: We restrict this check to Fetch API for the moment, as this might disrupt WorkerScriptLoader.
-    // Reassess this check based on https://github.com/whatwg/fetch/issues/393 discussions.
-    // We should also disable that check in navigation mode.
-    if (!request.url().protocolIsInHTTPFamily() && m_options.initiator == cachedResourceRequestInitiators().fetch) {
-        reportRedirectionWithBadScheme(*m_client, request.url());
-        clearResource();
-        return;
-    }
-
     if (!isAllowedByContentSecurityPolicy(request.url(), redirectResponse.isNull() ? ContentSecurityPolicy::RedirectResponseReceived::No : ContentSecurityPolicy::RedirectResponseReceived::Yes)) {
         reportContentSecurityPolicyError(*m_client, redirectResponse.url());
         clearResource();

Modified: trunk/Source/WebCore/loader/SubresourceLoader.cpp (206716 => 206717)


--- trunk/Source/WebCore/loader/SubresourceLoader.cpp	2016-10-02 13:59:18 UTC (rev 206716)
+++ trunk/Source/WebCore/loader/SubresourceLoader.cpp	2016-10-02 14:25:08 UTC (rev 206717)
@@ -200,7 +200,7 @@
                 m_frame->page()->diagnosticLoggingClient().logDiagnosticMessageWithResult(DiagnosticLoggingKeys::cachedResourceRevalidationKey(), emptyString(), DiagnosticLoggingResultFail, ShouldSample::Yes);
         }
 
-        if (!m_documentLoader->cachedResourceLoader().canRequestAfterRedirection(m_resource->type(), newRequest.url(), options())) {
+        if (!m_documentLoader->cachedResourceLoader().canRequest(m_resource->type(), newRequest.url(), options(), false /* forPreload */, true /* didReceiveRedirectResponse */)) {
             cancel();
             return;
         }

Modified: trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp (206716 => 206717)


--- trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp	2016-10-02 13:59:18 UTC (rev 206716)
+++ trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp	2016-10-02 14:25:08 UTC (rev 206717)
@@ -183,7 +183,7 @@
             if (Document* document = frame->document())
                 document->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request.mutableResourceRequest(), ContentSecurityPolicy::InsecureRequestType::Load);
             URL requestURL = request.resourceRequest().url();
-            if (requestURL.isValid() && canRequest(CachedResource::ImageResource, requestURL, request))
+            if (requestURL.isValid() && canRequest(CachedResource::ImageResource, requestURL, request.options(), request.forPreload()))
                 PingLoader::loadImage(*frame, requestURL);
             return nullptr;
         }
@@ -333,10 +333,6 @@
 
 bool CachedResourceLoader::checkInsecureContent(CachedResource::Type type, const URL& url) const
 {
-
-    if (!canRequestInContentDispositionAttachmentSandbox(type, url))
-        return false;
-
     switch (type) {
     case CachedResource::Script:
 #if ENABLE(XSLT)
@@ -383,8 +379,13 @@
     return true;
 }
 
-bool CachedResourceLoader::allowedByContentSecurityPolicy(CachedResource::Type type, const URL& url, const ResourceLoaderOptions& options, ContentSecurityPolicy::RedirectResponseReceived redirectResponseReceived)
+static inline bool isSameOriginDataURL(const URL& url, const ResourceLoaderOptions& options, bool didReceiveRedirectResponse)
 {
+    return !didReceiveRedirectResponse && url.protocolIsData() && options.sameOriginDataURLFlag == SameOriginDataURLFlag::Set;
+}
+
+bool CachedResourceLoader::allowedByContentSecurityPolicy(CachedResource::Type type, const URL& url, const ResourceLoaderOptions& options, bool didReceiveRedirectResponse)
+{
     if (options.contentSecurityPolicyImposition == ContentSecurityPolicyImposition::SkipPolicyCheck)
         return true;
 
@@ -391,6 +392,8 @@
     ASSERT(m_document);
     ASSERT(m_document->contentSecurityPolicy());
 
+    auto redirectResponseReceived = didReceiveRedirectResponse ? ContentSecurityPolicy::RedirectResponseReceived::Yes : ContentSecurityPolicy::RedirectResponseReceived::No;
+
     switch (type) {
 #if ENABLE(XSLT)
     case CachedResource::XSLStyleSheet:
@@ -427,33 +430,25 @@
     default:
         ASSERT_NOT_REACHED();
     }
-
     return true;
 }
 
-static inline bool isSameOriginDataURL(const URL& url, const ResourceLoaderOptions& options)
+bool CachedResourceLoader::canRequest(CachedResource::Type type, const URL& url, const ResourceLoaderOptions& options, bool forPreload, bool didReceiveRedirectResponse)
 {
-    // FIXME: Remove same-origin data URL flag since it was removed from fetch spec (https://github.com/whatwg/fetch/issues/381).
-    return url.protocolIsData() && options.sameOriginDataURLFlag == SameOriginDataURLFlag::Set;
-}
-
-bool CachedResourceLoader::canRequest(CachedResource::Type type, const URL& url, const CachedResourceRequest& request)
-{
-    auto& options = request.options();
-
     if (document() && !document()->securityOrigin()->canDisplay(url)) {
-        if (!request.forPreload())
+        if (!forPreload)
             FrameLoader::reportLocalLoadFailed(frame(), url.stringCenterEllipsizedToLength());
         LOG(ResourceLoading, "CachedResourceLoader::requestResource URL was not allowed by SecurityOrigin::canDisplay");
         return false;
     }
 
-    if (options.mode == FetchOptions::Mode::SameOrigin && !m_document->securityOrigin()->canRequest(url) && !isSameOriginDataURL(url, options)) {
+    // FIXME: Remove same-origin data URL flag since it was removed from fetch spec (see https://github.com/whatwg/fetch/issues/381).
+    if (options.mode == FetchOptions::Mode::SameOrigin && !isSameOriginDataURL(url, options, didReceiveRedirectResponse) && !m_document->securityOrigin()->canRequest(url)) {
         printAccessDeniedMessage(url);
         return false;
     }
 
-    if (!allowedByContentSecurityPolicy(type, url, options, ContentSecurityPolicy::RedirectResponseReceived::No))
+    if (!allowedByContentSecurityPolicy(type, url, options, didReceiveRedirectResponse))
         return false;
 
     // SVG Images have unique security rules that prevent all subresource requests except for data urls.
@@ -462,38 +457,14 @@
             return false;
     }
 
-    // Last of all, check for insecure content. We do this last so that when folks block insecure content with a CSP policy, they don't get a warning.
-    // They'll still get a warning in the console about CSP blocking the load.
-
-    // FIXME: Should we consider whether the request is for preload here?
-    if (!checkInsecureContent(type, url))
+    if (!canRequestInContentDispositionAttachmentSandbox(type, url))
         return false;
 
-    return true;
-}
+    // Last of all, check for insecure content. We do this last so that when
+    // folks block insecure content with a CSP policy, they don't get a warning.
+    // They'll still get a warning in the console about CSP blocking the load.
 
-// FIXME: Should we find a way to know whether the redirection is for a preload request like we do for CachedResourceLoader::canRequest?
-bool CachedResourceLoader::canRequestAfterRedirection(CachedResource::Type type, const URL& url, const ResourceLoaderOptions& options)
-{
-    if (document() && !document()->securityOrigin()->canDisplay(url)) {
-        FrameLoader::reportLocalLoadFailed(frame(), url.stringCenterEllipsizedToLength());
-        LOG(ResourceLoading, "CachedResourceLoader::requestResource URL was not allowed by SecurityOrigin::canDisplay");
-        return false;
-    }
-
-    // FIXME: According to https://fetch.spec.whatwg.org/#http-redirect-fetch, we should check that the URL is HTTP(s) except if in navigation mode.
-    // But we currently allow at least data URLs to be loaded.
-
-    if (options.mode == FetchOptions::Mode::SameOrigin && !m_document->securityOrigin()->canRequest(url)) {
-        printAccessDeniedMessage(url);
-        return false;
-    }
-
-    if (!allowedByContentSecurityPolicy(type, url, options, ContentSecurityPolicy::RedirectResponseReceived::Yes))
-        return false;
-
-    // Last of all, check for insecure content. We do this last so that when folks block insecure content with a CSP policy, they don't get a warning.
-    // They'll still get a warning in the console about CSP blocking the load.
+    // FIXME: Should we consider forPreload here?
     if (!checkInsecureContent(type, url))
         return false;
 
@@ -503,7 +474,7 @@
 bool CachedResourceLoader::canRequestInContentDispositionAttachmentSandbox(CachedResource::Type type, const URL& url) const
 {
     Document* document;
-
+    
     // FIXME: Do we want to expand this to all resource types that the mixed content checker would consider active content?
     switch (type) {
     case CachedResource::MainResource:
@@ -653,8 +624,7 @@
 
     prepareFetch(type, request);
 
-    // We are passing url as well as request, as request url may contain a fragment identifier.
-    if (!canRequest(type, url, request)) {
+    if (!canRequest(type, url, request.options(), request.forPreload())) {
         RELEASE_LOG_IF_ALLOWED("requestResource: Not allowed to request resource (frame = %p)", frame());
         return nullptr;
     }

Modified: trunk/Source/WebCore/loader/cache/CachedResourceLoader.h (206716 => 206717)


--- trunk/Source/WebCore/loader/cache/CachedResourceLoader.h	2016-10-02 13:59:18 UTC (rev 206716)
+++ trunk/Source/WebCore/loader/cache/CachedResourceLoader.h	2016-10-02 14:25:08 UTC (rev 206717)
@@ -30,7 +30,6 @@
 #include "CachedResource.h"
 #include "CachedResourceHandle.h"
 #include "CachedResourceRequest.h"
-#include "ContentSecurityPolicy.h"
 #include "ResourceLoadPriority.h"
 #include "ResourceTimingInformation.h"
 #include "Timer.h"
@@ -123,7 +122,7 @@
     void loadDone(CachedResource*, bool shouldPerformPostLoadActions = true);
 
     WEBCORE_EXPORT void garbageCollectDocumentResources();
-
+    
     void incrementRequestCount(const CachedResource&);
     void decrementRequestCount(const CachedResource&);
     int requestCount() const { return m_requestCount; }
@@ -136,8 +135,7 @@
     void checkForPendingPreloads();
     void printPreloadStats();
 
-    bool canRequest(CachedResource::Type, const URL&, const CachedResourceRequest&);
-    bool canRequestAfterRedirection(CachedResource::Type, const URL&, const ResourceLoaderOptions&);
+    bool canRequest(CachedResource::Type, const URL&, const ResourceLoaderOptions&, bool forPreload = false, bool didReceiveRedirectResponse = false);
 
     static const ResourceLoaderOptions& defaultCachedResourceOptions();
 
@@ -166,7 +164,7 @@
 
     bool shouldContinueAfterNotifyingLoadedFromMemoryCache(const CachedResourceRequest&, CachedResource*);
     bool checkInsecureContent(CachedResource::Type, const URL&) const;
-    bool allowedByContentSecurityPolicy(CachedResource::Type, const URL&, const ResourceLoaderOptions&, ContentSecurityPolicy::RedirectResponseReceived);
+    bool allowedByContentSecurityPolicy(CachedResource::Type, const URL&, const ResourceLoaderOptions&, bool);
 
     void performPostLoadActions();
 
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to