Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(49)

Unified Diff: third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp

Issue 1975373002: Clean up response handling in ResourceLoader/ResourceFetcher (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Rebase Created 4 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
index dd07563a5e11cf9cc61916fcb27957720a7dd178..6264bb2ad99d32653b4ac7e6869ff9deff729a7e 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
@@ -239,25 +239,28 @@ Resource* ResourceFetcher::cachedResource(const KURL& resourceURL) const
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 becau
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() he
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::allowStoredCredenti
hiroshige 2016/07/28 09:56:42 Memo: This inlines Resource::passesAccessControlCh
Nate Chapin 2016/07/28 22:29:35 Also, see comment about setReponse() call in Resou
Nate Chapin 2016/07/28 22:29:35 That may make sense, but some places don't use the
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;
}
@@ -945,21 +948,39 @@ void ResourceFetcher::didFailLoading(Resource* resource, const ResourceError& er
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 call
+ 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)

Powered by Google App Engine
This is Rietveld 408576698