Allow file/filesystem schemes to redirect to same scheme.

Relax redirect safety checks, which exist in the network
service but were not present in URLRequestJob, to allow
the loading of a file/filesystem scheme to redirect to
the file/filesystem scheme.

One example of a redirect is where the following URL:

  file:///path/to/directory

during reload is redirected to:

  file:///path/to/directory/

This change also fixes redirects of Windows file links
(with network service) as it redirects back to the client
before following the redirect. This avoids an infinite
recursion bug triggered by circular symbolic links.

Bug: 884277,887039
Change-Id: I13923fc29397b1d3aa6679d861e9edc3af1c816b
Reviewed-on: https://chromium-review.googlesource.com/1234335Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Chris Mumford <cmumford@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594450}

content/browser/fileapi/file_system_url_loader_factory.cc

@@ -447,14 +447,6 @@ class FileSystemFileURLLoader : public FileSystemEntryURLLoader {
           original_request_.url.ReplaceComponents(replacements);
       head_.encoded_data_length = 0;
       client_->OnReceiveRedirect(redirect_info, head_);
-
-      // Restart the request with a directory loader.
-      network::ResourceRequest new_request = original_request_;
-      new_request.url = redirect_info.new_url;
-      FileSystemDirectoryURLLoader::CreateAndStart(
-          new_request, binding_.Unbind(), client_.PassInterface(),
-          std::move(params_), io_task_runner_);
-      MaybeDeleteSelf();
       return;
     }
 

content/browser/frame_host/navigation_request.cc

@@ -935,7 +935,7 @@ void NavigationRequest::OnResponseStarted(
     bool is_stream,
     PreviewsState previews_state,
     base::Optional<SubresourceLoaderParams> subresource_loader_params) {
-  DCHECK(state_ == STARTED);
+  DCHECK_EQ(state_, STARTED);
   DCHECK(response);
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                "OnResponseStarted");

content/browser/loader/navigation_url_loader_impl.cc

@@ -322,10 +322,12 @@ bool IsURLHandledByDefaultLoader(const GURL& url) {
   return IsURLHandledByNetworkService(url) || url.SchemeIs(url::kDataScheme);
 }
 
-// Determines whether it is safe to redirect to |url|.
-bool IsRedirectSafe(const GURL& url, ResourceContext* resource_context) {
-  return IsSafeRedirectTarget(url) &&
-         GetContentClient()->browser()->IsSafeRedirectTarget(url,
+// Determines whether it is safe to redirect from |from_url| to |to_url|.
+bool IsRedirectSafe(const GURL& from_url,
+                    const GURL& to_url,
+                    ResourceContext* resource_context) {
+  return IsSafeRedirectTarget(from_url, to_url) &&
+         GetContentClient()->browser()->IsSafeRedirectTarget(to_url,
                                                              resource_context);
 }
 
@@ -1186,7 +1188,7 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
                          const network::ResourceResponseHead& head) override {
     if (base::FeatureList::IsEnabled(network::features::kNetworkService) &&
         !bypass_redirect_checks_ &&
-        !IsRedirectSafe(redirect_info.new_url, resource_context_)) {
+        !IsRedirectSafe(url_, redirect_info.new_url, resource_context_)) {
       OnComplete(network::URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT));
       return;
     }

content/public/common/url_utils.cc

@@ -111,14 +111,23 @@ bool IsRendererDebugURL(const GURL& url) {
   return false;
 }
 
-bool IsSafeRedirectTarget(const GURL& url) {
+bool IsSafeRedirectTarget(const GURL& from_url, const GURL& to_url) {
   static base::NoDestructor<std::set<std::string>> kUnsafeSchemes(
       std::set<std::string>({
           url::kAboutScheme, url::kDataScheme, url::kFileScheme,
           url::kFileSystemScheme,
       }));
-  return !HasWebUIScheme(url) &&
-         kUnsafeSchemes->find(url.scheme()) == kUnsafeSchemes->end();
+  if (HasWebUIScheme(to_url))
+    return false;
+  if (kUnsafeSchemes->find(to_url.scheme()) == kUnsafeSchemes->end())
+    return true;
+  if (from_url.is_empty())
+    return false;
+  if (from_url.SchemeIsFile() && to_url.SchemeIsFile())
+    return true;
+  if (from_url.SchemeIsFileSystem() && to_url.SchemeIsFileSystem())
+    return true;
+  return false;
 }
 
 }  // namespace content

content/public/common/url_utils.h

@@ -39,8 +39,9 @@ CONTENT_EXPORT bool IsRendererDebugURL(const GURL& url);
 // is disabled.
 bool CONTENT_EXPORT IsURLHandledByNetworkService(const GURL& url);
 
-// Determines whether it is safe to redirect to |url|.
-CONTENT_EXPORT bool IsSafeRedirectTarget(const GURL& url);
+// Determines whether it is safe to redirect from |from_url| to |to_url|.
+CONTENT_EXPORT bool IsSafeRedirectTarget(const GURL& from_url,
+                                         const GURL& to_url);
 
 }  // namespace content
 

content/public/common/url_utils_unittest.cc

@@ -22,4 +22,23 @@ TEST(UrlUtilsTest, IsURLHandledByNetworkStack) {
   EXPECT_FALSE(IsURLHandledByNetworkStack(GURL()));
 }
 
+TEST(UrlUtilsTest, IsSafeRedirectTarget) {
+  EXPECT_FALSE(IsSafeRedirectTarget(GURL(), GURL("chrome://foo/bar.html")));
+  EXPECT_TRUE(IsSafeRedirectTarget(GURL(), GURL("http://foo/bar.html")));
+  EXPECT_FALSE(IsSafeRedirectTarget(GURL(), GURL("file:///foo/bar/")));
+  EXPECT_TRUE(
+      IsSafeRedirectTarget(GURL("file:///foo/bar"), GURL("file:///foo/bar/")));
+  EXPECT_TRUE(IsSafeRedirectTarget(GURL("filesystem://foo/bar"),
+                                   GURL("filesystem://foo/bar/")));
+  EXPECT_TRUE(IsSafeRedirectTarget(GURL(), GURL("unknown://foo/bar/")));
+  EXPECT_FALSE(IsSafeRedirectTarget(GURL("http://foo/bar.html"),
+                                    GURL("file:///foo/bar/")));
+  EXPECT_TRUE(IsSafeRedirectTarget(GURL("file:///foo/bar/"),
+                                   GURL("http://foo/bar.html")));
+
+  // TODO(cmumford): Capturing current behavior, but should probably prevent
+  //                 redirect to invalid URL.
+  EXPECT_TRUE(IsSafeRedirectTarget(GURL(), GURL()));
+}
+
 }  // namespace content

content/renderer/loader/resource_dispatcher.cc

@@ -760,7 +760,7 @@ int ResourceDispatcher::StartAsync(
     pending_requests_[request_id]->url_loader_client =
         std::make_unique<URLLoaderClientImpl>(
             request_id, this, loading_task_runner,
-            true /* bypass_redirect_checks */);
+            true /* bypass_redirect_checks */, request->url);
 
     if (request->resource_type == RESOURCE_TYPE_SHARED_WORKER) {
       // For shared workers, immediately post a task for continuing loading
@@ -782,9 +782,9 @@ int ResourceDispatcher::StartAsync(
     return request_id;
   }
 
-  std::unique_ptr<URLLoaderClientImpl> client(
-      new URLLoaderClientImpl(request_id, this, loading_task_runner,
-                              url_loader_factory->BypassRedirectChecks()));
+  std::unique_ptr<URLLoaderClientImpl> client(new URLLoaderClientImpl(
+      request_id, this, loading_task_runner,
+      url_loader_factory->BypassRedirectChecks(), request->url));
 
   if (pass_response_pipe_to_peer)
     client->SetPassResponsePipeToDispatcher(true);

content/renderer/loader/url_loader_client_impl.cc

@@ -19,10 +19,10 @@
 namespace content {
 namespace {
 
-// Determines whether it is safe to redirect to |url|.
-bool IsRedirectSafe(const GURL& url) {
-  return IsSafeRedirectTarget(url) &&
-         GetContentClient()->renderer()->IsSafeRedirectTarget(url);
+// Determines whether it is safe to redirect from |from_url| to |to_url|.
+bool IsRedirectSafe(const GURL& from_url, const GURL& to_url) {
+  return IsSafeRedirectTarget(from_url, to_url) &&
+         GetContentClient()->renderer()->IsSafeRedirectTarget(to_url);
 }
 
 }  // namespace
@@ -127,11 +127,13 @@ URLLoaderClientImpl::URLLoaderClientImpl(
     int request_id,
     ResourceDispatcher* resource_dispatcher,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-    bool bypass_redirect_checks)
+    bool bypass_redirect_checks,
+    const GURL& request_url)
     : request_id_(request_id),
       resource_dispatcher_(resource_dispatcher),
       task_runner_(std::move(task_runner)),
       bypass_redirect_checks_(bypass_redirect_checks),
+      last_loaded_url_(request_url),
       url_loader_client_binding_(this),
       weak_factory_(this) {}
 
@@ -247,11 +249,13 @@ void URLLoaderClientImpl::OnReceiveRedirect(
   DCHECK(!has_received_response_);
   DCHECK(!body_consumer_);
   if (base::FeatureList::IsEnabled(network::features::kNetworkService) &&
-      !bypass_redirect_checks_ && !IsRedirectSafe(redirect_info.new_url)) {
+      !bypass_redirect_checks_ &&
+      !IsRedirectSafe(last_loaded_url_, redirect_info.new_url)) {
     OnComplete(network::URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT));
     return;
   }
 
+  last_loaded_url_ = redirect_info.new_url;
   if (NeedsStoringMessage()) {
     StoreAndDispatch(std::make_unique<DeferredOnReceiveRedirect>(
         redirect_info, response_head, task_runner_));

content/renderer/loader/url_loader_client_impl.h

@@ -38,7 +38,8 @@ class CONTENT_EXPORT URLLoaderClientImpl final
   URLLoaderClientImpl(int request_id,
                       ResourceDispatcher* resource_dispatcher,
                       scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-                      bool bypass_redirect_checks);
+                      bool bypass_redirect_checks,
+                      const GURL& request_url);
   ~URLLoaderClientImpl() override;
 
   // Sets |is_deferred_|. From now, the received messages are not dispatched
@@ -105,6 +106,7 @@ class CONTENT_EXPORT URLLoaderClientImpl final
   ResourceDispatcher* const resource_dispatcher_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   bool bypass_redirect_checks_ = false;
+  GURL last_loaded_url_;
 
   network::mojom::URLLoaderPtr url_loader_;
   mojo::Binding<network::mojom::URLLoaderClient> url_loader_client_binding_;

extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc

@@ -176,7 +176,7 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::OnReceiveRedirect(
     const net::RedirectInfo& redirect_info,
     const network::ResourceResponseHead& head) {
   if (redirect_url_ != redirect_info.new_url &&
-      !IsRedirectSafe(redirect_info.new_url)) {
+      !IsRedirectSafe(request_.url, redirect_info.new_url)) {
     OnRequestError(
         network::URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT));
     return;
@@ -541,18 +541,19 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::OnRequestError(
   factory_->RemoveRequest(network_service_request_id_, request_id_);
 }
 
-// Determines whether it is safe to redirect to |url|.
+// Determines whether it is safe to redirect from |from_url| to |to_url|.
 bool WebRequestProxyingURLLoaderFactory::InProgressRequest::IsRedirectSafe(
-    const GURL& url) {
-  if (url.SchemeIs(extensions::kExtensionScheme)) {
+    const GURL& from_url,
+    const GURL& to_url) {
+  if (to_url.SchemeIs(extensions::kExtensionScheme)) {
     const Extension* extension =
-        factory_->info_map_->extensions().GetByID(url.host());
+        factory_->info_map_->extensions().GetByID(to_url.host());
     if (!extension)
       return false;
     return WebAccessibleResourcesInfo::IsResourceWebAccessible(extension,
-                                                               url.path());
+                                                               to_url.path());
   }
-  return content::IsSafeRedirectTarget(url);
+  return content::IsSafeRedirectTarget(from_url, to_url);
 }
 
 WebRequestProxyingURLLoaderFactory::WebRequestProxyingURLLoaderFactory(

extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h

@@ -108,7 +108,7 @@ class WebRequestProxyingURLLoaderFactory
     void HandleResponseOrRedirectHeaders(
         const net::CompletionCallback& continuation);
     void OnRequestError(const network::URLLoaderCompletionStatus& status);
-    bool IsRedirectSafe(const GURL& url);
+    bool IsRedirectSafe(const GURL& from_url, const GURL& to_url);
 
     WebRequestProxyingURLLoaderFactory* const factory_;
     network::ResourceRequest request_;