PlzNavigate: Fix the FindInPageControllerTest.SearchWithinSpecialURL browser test.

content/browser/loader/resource_loader.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/loader/resource_loader.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "content/browser/appcache/appcache_interceptor.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/loader/detachable_resource_handler.h"
 #include "content/browser/loader/resource_loader_delegate.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "content/browser/service_worker/service_worker_request_handler.h"
 #include "content/browser/service_worker/service_worker_response_info.h"
 #include "content/browser/ssl/ssl_client_auth_handler.h"
 #include "content/browser/ssl/ssl_manager.h"
 #include "content/public/browser/resource_dispatcher_host_login_delegate.h"
+#include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/resource_response.h"
 #include "content/public/common/resource_type.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_response_headers.h"
 #include "net/nqe/effective_connection_type.h"
 #include "net/nqe/network_quality_estimator.h"
@@ skipping 211 matching lines @@
                                         bool* defer) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("loading"),
                "ResourceLoader::OnReceivedRedirect");
   DCHECK_EQ(request_.get(), unused);
 
   DVLOG(1) << "OnReceivedRedirect: " << request_->url().spec();
   DCHECK(request_->status().is_success());
 
   ResourceRequestInfoImpl* info = GetRequestInfo();
 
-  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(
-          info->GetChildID(), redirect_info.new_url)) {
-    DVLOG(1) << "Denied unauthorized request for "
-             << redirect_info.new_url.possibly_invalid_spec();
+  // With PlzNavigate for frame navigations this check is done in the
+  // NavigationRequest::OnRedirectChecksComplete() function.
+  if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 ||

[nasko, 2016/10/21 21:21:07]

Why check the process ID here? The check should be PlzNav and Frame navigation,
right?

[ananta, 2016/10/21 21:53:01]

For non plznavigate we want the check to happen 

[nasko, 2016/10/21 22:47:49]

Yes, that is already decided by "!IsBrowserSideNavigationEnabled()". What
happens if we have a bug in the non-PlzNavigate case where the child process id
is indeed -1? We will skip the check and default to success. This is bad from
security perspective, isn't it?

[Charlie Reis, 2016/10/21 23:01:43]

Agreed.  We should not have the child ID check here, allowing us to skip the
CanRequestURL check.

I also don't understand why the !IsResourceTypeFrame exception was added. 
CanRequestURL is not solely about navigation requests.  It's about all requests.

[ananta, 2016/10/22 00:20:42]

I removed the GetChildID check. The !IsBrowserSideNavigation() enabled check
will ensure that this works correctly for the non plznavigate case. The frame
checks have been added here with the expectation that this will get handled
eventually in NavigationRequest::OnRedirectChecksComplete() function. I verified
that the function does get called for redirects on the main and sub frames.

[Charlie Reis, 2016/10/24 22:04:36]

I see.  I was misreading it before-- the non-PlzNavigate case won't care about
the extra checks.

(I wonder if there's a way to make this code easier to read, like having a
"check_handled_elsewhere" bool set to IsBrowserSideNavigationEnabled() &&
IsResourceTypeFrame(info->GetResourceType()).  I suppose it's not a big deal,
though.)

[ananta, 2016/10/24 23:01:41]

Thanks. Done

+      !IsResourceTypeFrame(info->GetResourceType())) {
+    if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(
+            info->GetChildID(), redirect_info.new_url)) {
+      DVLOG(1) << "Denied unauthorized request for "
+               << redirect_info.new_url.possibly_invalid_spec();
 
-    // Tell the renderer that this request was disallowed.
-    Cancel();
-    return;
+      // Tell the renderer that this request was disallowed.
+      Cancel();
+      return;
+    }
   }
 
   if (delegate_->HandleExternalProtocol(this, redirect_info.new_url)) {
     // The request is complete so we can remove it.
     CancelAndIgnore();
     return;
   }
 
   scoped_refptr<ResourceResponse> response = new ResourceResponse();
   PopulateResourceResponse(info, request_.get(), response.get());
@@ skipping 436 matching lines @@
     }
 
     UMA_HISTOGRAM_ENUMERATION("Net.Prefetch.Pattern", status, STATUS_MAX);
   } else if (request_->response_info().unused_since_prefetch) {
     TimeDelta total_time = base::TimeTicks::Now() - request_->creation_time();
     UMA_HISTOGRAM_TIMES("Net.Prefetch.TimeSpentOnPrefetchHit", total_time);
   }
 }
 
 }  // namespace content