Show service worker navigation preload requests in DevTools Network tab

content/browser/loader/resource_dispatcher_host_impl.cc

Index: content/browser/loader/resource_dispatcher_host_impl.cc
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc
index fa98d50d4485c8b9138174bf293a523d65aaecdd..cabd950973c9ed714d3c1b9050b78ef861c6e695 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -1501,7 +1501,10 @@ void ResourceDispatcherHostImpl::ContinuePendingBeginRequest(
   ChildProcessSecurityPolicyImpl* policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
   bool report_raw_headers = request_data.report_raw_headers;
-  if (report_raw_headers && !policy->CanReadRawCookies(child_id)) {
+  if (report_raw_headers && !policy->CanReadRawCookies(child_id) &&
+      !requester_info->IsNavigationPreload()) {
+    // |report_raw_headers| of navigation preload request was copied from the
+    // the original request. So this check has already been carried out.

[falken, 2017/01/18 14:50:07]

I don't really understand this. It's possible that
policy->CanReadRawCookies(child_id) is true for the original request but false
for the navigation preload request? Could you explain more why it's still OK to
bypass the check?

This looks a bit scary, so I want to understand.

[horo, 2017/01/19 09:57:09]

If my understanding is correct, this check is protecting the sensitive
information from malicious renderer processes. The browser process reports the
raw headers only when the DevTools is attached to the renderer process.

When a malicious renderer process without a DevTools sends a request with
report_raw_headers flag, CanReadRawCookies() returns true, so the flag is
changed to false.

Even if a new NavigationPreload request is created for the request, the flag of 
the new request is copied from the original request in 
ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). So it must be
false.

[falken, 2017/01/19 13:40:58]

I understand the request is copied, but don't understand why we can't just do
the check again. If in the original request, report_raw_headers was true, then
CanReadRawCookies was true when the check happened. The only reason to avoid the
check again for nav preload, is if CanReadRawCookies could now be false for nav
preload. Why would CanReadRawCookies have changed... and is it ok to still
bypass the check even if it changed? I guess it happens if the user closed
DevTools between when the request was created and nav preload was created?

[horo, 2017/01/19 14:33:04]

CanReadRawCookies checks the child_id of ResourceRequesterInfo.
But we are using -1 for child_id of NavigationPreload.
https://chromium.googlesource.com/chromium/src/+/7aac46c/content/browser/load...
So CanReadRawCookies returns false.

[falken, 2017/01/20 01:38:54]

That's the important detail I missed. Let's make that clear in the comment: "For
navigation preload, the child_id is -1 so CanReadRawCookies would return false.
But |report_raw_headers| of the navigation preload request was copied from the
original request, so this check has already been carried out."

BTW doesn't PlzNavigate also have child_id == -1? Does it not need DevTools
support? Might it make sense for CanReadRawCookies to return true for child_id
-1?

[horo, 2017/01/20 13:18:36]

Done.
ContinuePendingBeginRequest() is not called for BrowserSideNavigation request.
BrowserSideNavigation type ResourceRequesterInfo is created in
BeginNavigationRequest() and it directly calls BeginRequestInternal().

     // TODO: crbug.com/523063 can we call bad_message::ReceivedBadMessage here?
     VLOG(1) << "Denied unauthorized request for raw headers";
     report_raw_headers = false;