Clean up response handling in ResourceLoader/ResourceFetcher

third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
     Copyright (C) 2009 Torch Mobile Inc. http://www.torchmobile.com/
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
     License as published by the Free Software Foundation; either
@@ skipping 221 matching lines @@
 {
 }
 
 Resource* ResourceFetcher::cachedResource(const KURL& resourceURL) const
 {
     KURL url = MemoryCache::removeFragmentIdentifierIfNeeded(resourceURL);
     const WeakMember<Resource>& resource = m_documentResources.get(url);
     return resource.get();
 }
 
-bool ResourceFetcher::canAccessResource(Resource* resource, SecurityOrigin* sourceOrigin, const KURL& url, AccessControlLoggingDecision logErrorsDecision) const

[hiroshige, 2016/07/28 09:56:42]

This CL removes AccessControlLoggingDecision because it is always
ShouldLogAccessControlErrors.
Can we split changes related to this to a separate CL?

[Nate Chapin, 2016/07/28 22:29:35]

Ok, will do.

+bool ResourceFetcher::canAccessResponse(Resource* resource, const ResourceResponse& response) const
 {
     // Redirects can change the response URL different from one of request.
     bool forPreload = resource->isUnusedPreload();
-    if (!context().canRequest(resource->getType(), resource->resourceRequest(), url, resource->options(), forPreload, FetchRequest::UseDefaultOriginRestrictionForType))
+    if (!context().canRequest(resource->getType(), resource->resourceRequest(), response.url(), resource->options(), forPreload, FetchRequest::UseDefaultOriginRestrictionForType))
         return false;
 
+    SecurityOrigin* sourceOrigin = resource->options().securityOrigin.get();
     if (!sourceOrigin)
         sourceOrigin = context().getSecurityOrigin();
 
-    if (sourceOrigin->canRequestNoSuborigin(url))
+    if (sourceOrigin->canRequestNoSuborigin(response.url()))
         return true;
 
+    // Use the original response instead of the 304 response for a successful revaldiation.
+    const ResourceResponse& responseForAccessControl = response.httpStatusCode() == 304 ? resource->response() : response;

[hiroshige, 2016/07/28 09:56:42]

We need to check m_resource->isCacheValidator() here, because a server can
return 304 for non-revalidating request (such server is not well behaving
though).

[Nate Chapin, 2016/07/28 22:29:35]

Oops, I don't remember dropping that. Done.

     String errorDescription;
-    if (!resource->passesAccessControlCheck(sourceOrigin, errorDescription)) {
+    if (!passesAccessControlCheck(responseForAccessControl, resource->options().allowCredentials, sourceOrigin, errorDescription, resource->lastResourceRequest().requestContext())) {

[hiroshige, 2016/07/28 09:56:42]

This replaces ResourceRequest::allowStoredCredentials() with
ResourceLoaderOptions::allowCredentials. I think it's good because they are
duplicated. Are we going to merge them further? (If so, let's create a tracking
crbug)

Placing DCHECK_EQ(resource->options().allowCredentials,
resource->lastResourceRequest().allowStoredCredentials() ?
AllowStoredCredentials : DoNotAllowStoredCredentials) is good for catching
potential bugs.

[hiroshige, 2016/07/28 09:56:42]

Memo: This inlines Resource::passesAccessControlCheck().

[Nate Chapin, 2016/07/28 22:29:35]

Also, see comment about setReponse() call in
ResourceLoader::didReceiveResponse(). The old variant implicitly uses
Resource::m_response, which is not yet set in the non-304 case.

[Nate Chapin, 2016/07/28 22:29:35]

That may make sense, but some places don't use the StoredCredentials enum at
all, and it's not exposed outside of blink. ResourceFetcher always holds them to
be equivalent, so it seemed correct in this specific scenario.

         resource->setCORSFailed();
-        if (!forPreload && (logErrorsDecision == ShouldLogAccessControlErrors)) {
+        if (!forPreload) {
             String resourceType = Resource::resourceTypeToString(resource->getType(), resource->options().initiatorInfo);
-            context().addConsoleMessage(resourceType + " from origin '" + SecurityOrigin::create(url)->toString() + "' has been blocked from loading by Cross-Origin Resource Sharing policy: " + errorDescription);
+            context().addConsoleMessage(resourceType + " from origin '" + SecurityOrigin::create(response.url())->toString() + "' has been blocked from loading by Cross-Origin Resource Sharing policy: " + errorDescription);
         }
         return false;
     }
     return true;
 }
 
 bool ResourceFetcher::isControlledByServiceWorker() const
 {
     return context().isControlledByServiceWorker();
 }
@@ skipping 667 matching lines @@
 {
     TRACE_EVENT_ASYNC_END0("blink.net", "Resource", resource->identifier());
     removeResourceLoader(resource->loader());
     m_resourceTimingInfoMap.take(const_cast<Resource*>(resource));
     bool isInternalRequest = resource->options().initiatorInfo.name == FetchInitiatorTypeNames::internal;
     context().dispatchDidFail(resource->identifier(), error, isInternalRequest);
     resource->error(error);
     context().didLoadResource(resource);
 }
 
-void ResourceFetcher::didReceiveResponse(Resource* resource, const ResourceResponse& response)
+bool ResourceFetcher::didReceiveResponse(Resource* resource, const ResourceResponse& response)
 {
-    // If the response is fetched via ServiceWorker, the original URL of the response could be different from the URL of the request.
-    // We check the URL not to load the resources which are forbidden by the page CSP.
-    // https://w3c.github.io/webappsec-csp/#should-block-response
+    context().dispatchDidReceiveResponse(resource->identifier(), response, resource->resourceRequest().frameType(), resource->resourceRequest().requestContext(), resource);
+
     if (response.wasFetchedViaServiceWorker()) {
+        if (resource->options().corsEnabled == IsCORSEnabled && response.wasFallbackRequiredByServiceWorker()) {

[hiroshige, 2016/07/28 09:56:42]

memo for myself: this block is moved from the callee:
ResourceLoader::didReceiveResponse().

+            ResourceRequest request = resource->lastResourceRequest();
+            DCHECK_EQ(request.skipServiceWorker(), WebURLRequest::SkipServiceWorker::None);
+            // This code handles the case when a regular controlling service worker
+            // doesn't handle a cross origin request. When this happens we still
+            // want to give foreign fetch a chance to handle the request, so
+            // only skip the controlling service worker for the fallback request.
+            // This is currently safe because of http://crbug.com/604084 the
+            // wasFallbackRequiredByServiceWorker flag is never set when foreign fetch
+            // handled a request.
+            request.setSkipServiceWorker(WebURLRequest::SkipServiceWorker::Controlling);
+            resource->loader()->restart(request);
+            return false;
+        }
+
+        // If the response is fetched via ServiceWorker, the original URL of the response could be different from the URL of the request.
+        // We check the URL not to load the resources which are forbidden by the page CSP.
+        // https://w3c.github.io/webappsec-csp/#should-block-response
         const KURL& originalURL = response.originalURLViaServiceWorker();
         if (!originalURL.isEmpty() && !context().allowResponse(resource->getType(), resource->resourceRequest(), originalURL, resource->options())) {
-            resource->loader()->cancel();
-            bool isInternalRequest = resource->options().initiatorInfo.name == FetchInitiatorTypeNames::internal;
-            context().dispatchDidFail(resource->identifier(), ResourceError(errorDomainBlinkInternal, 0, originalURL.getString(), "Unsafe attempt to load URL " + originalURL.elidedString() + " fetched by a ServiceWorker."), isInternalRequest);
-            return;
+            resource->loader()->didFail(nullptr, ResourceError::cancelledDueToAccessCheckError(response.url()));
+            return false;
         }
+    } else if (resource->options().corsEnabled == IsCORSEnabled && !canAccessResponse(resource, response)) {
+        resource->loader()->didFail(nullptr, ResourceError::cancelledDueToAccessCheckError(response.url()));
+        return false;
     }
-    context().dispatchDidReceiveResponse(resource->identifier(), response, resource->resourceRequest().frameType(), resource->resourceRequest().requestContext(), resource);
+    return true;
 }
 
 void ResourceFetcher::didReceiveData(const Resource* resource, const char* data, int dataLength, int encodedDataLength)
 {
     context().dispatchDidReceiveData(resource->identifier(), data, dataLength, encodedDataLength);
 }
 
 void ResourceFetcher::didDownloadData(const Resource* resource, int dataLength, int encodedDataLength)
 {
     context().dispatchDidDownloadData(resource->identifier(), dataLength, encodedDataLength);
@@ skipping 299 matching lines @@
     visitor->trace(m_context);
     visitor->trace(m_archive);
     visitor->trace(m_loaders);
     visitor->trace(m_nonBlockingLoaders);
     visitor->trace(m_documentResources);
     visitor->trace(m_preloads);
     visitor->trace(m_resourceTimingInfoMap);
 }
 
 } // namespace blink