Resource requests from Save-Page-As should go through CanRequestURL checks.

content/browser/loader/resource_dispatcher_host_impl.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.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <stddef.h>
 
@@ skipping 1869 matching lines @@
   // Since we're just saving some resources we need, disallow downloading.
   ResourceRequestInfoImpl* extra_info =
       CreateRequestInfo(child_id, render_view_route_id,
                         render_frame_route_id, false, context);
   extra_info->AssociateWithRequest(request.get());  // Request takes ownership.
 
   std::unique_ptr<ResourceHandler> handler(new SaveFileResourceHandler(
       request.get(), save_item_id, save_package_id, child_id,
       render_frame_route_id, url, save_file_manager_.get()));
 
+  // Check if the renderer is permitted to request the requested URL.
+  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(child_id,
+                                                                    url)) {
+    VLOG(1) << "Denied unauthorized save of " << url.possibly_invalid_spec();

[Randy Smith (Not in Mondays), 2016/07/21 19:29:54]

Reasonable to make this a DVLOG?  I don't think there are a lot of cases where
we want to do this type of printing in the field.

[Łukasz Anforowicz, 2016/07/21 23:44:30]

Done.

Note that we have a VLOG for other similar messages:
- in ShouldServiceRequest (line 304)
- in BeginRequest (line 1460)

OTOH, we have a DVLOG here:
- in BeginDownload (line 718)

So my change makes it 50-50 :-P.  Still, I agree that this is not that useful in
the field + making it a DVLOG should make the release version of the browser a
tiny bit leaner.

+    handler->OnResponseCompleted(
+        net::URLRequestStatus::FromError(net::ERR_ACCESS_DENIED),
+        "",        // |security_info|;  ignored by SaveFileResourceHandler.
+        nullptr);  // |defer|;  ignored by SaveFileResourceHandler.

[mmenke, 2016/07/21 19:38:43]

This seems kind of hideous.  ResourceDispatcherHostImpl shouldn't depend on the
implementation details of SaveFileResourceHandler (Not accessing |defer|, let
alone setting it to |true|, or that this just magically works without setting up
a ResourceLoader).

[Randy Smith (Not in Mondays), 2016/07/21 19:52:53]

You're right.  Mea culpa.

(Mind you, this is a string the other end of which is getting BeginSaveFile out
of ResourceDispatcherHostImpl, but that's clearly not in scope for this CL, and
I would agree with you that calling OnResponseCompleted() in a reasonably
handler agnostic way is.)

[Łukasz Anforowicz, 2016/07/21 23:44:30]

Thanks for raising this up.  I thought that it might be okay to depend on the
type, because SaveFileResourceHandler is created right above on line 1886.

I am not sure what "calling OnResponseCompleted in a reasonable handler agnostic
way" means :-(  In particular, there is no documentation of |defer| parameter
and I am not sure if it is appropriate to duplicate what other callers of
OnResponseCompleted do (|if (!defer) ResumeIfDeferred();|) since there is
nothing save-page-related to resume and resuming non-save-page-related things
seems out of place here.

FWIW, in the latest patchset I've tried to duplicate what another piece of code
is doing at
https://cs-staging.chromium.org/chromium/src/content/browser/loader/resource_...
(including darin's TODO... :-o).

[mmenke, 2016/07/22 00:12:16]

The method is inherited from content::ResourceHandler, so that's where they're
documented.  Beyond the NULL defer parameter, it also doesn't have its
ResourceController field set, either.
Yea....Unfortunately for you, I wasn't an owner when that was signed off on, and
I don't think we need any more instances of that that's going to be hanging
around for 5 more years.  Code should be able to assume APIs are consumed
correctly.

There are a few options, as I see it:

1)  Create a ResourceLoader as normal and cancel it (Need to make sure the
URLRequest never starts).
2)  Never create a URLRequest, just cancel the request directly. 
AbortRequestBeforeItStarts does this, and if that works for you, let's use it. 
However, I assume SaveFileResourceHandler has some sort of UI hooks to tell the
user something went wrong?  Maybe a magic
InformWhateverAPIWe'reTalkingToOfFailure method, that combines what starting +
failing the request does?

+    return;
+  }
+
   BeginRequestInternal(std::move(request), std::move(handler));
 }
 
 void ResourceDispatcherHostImpl::MarkAsTransferredNavigation(
     const GlobalRequestID& id,
     const scoped_refptr<ResourceResponse>& response) {
   GetLoader(id)->MarkAsTransferring(response);
 }
 
 void ResourceDispatcherHostImpl::CancelTransferringNavigation(
@@ skipping 696 matching lines @@
   ssl.cert_id = GetCertStore()->StoreCert(ssl_info.cert.get(), child_id);
   response->head.security_info = SerializeSecurityInfo(ssl);
 }
 
 CertStore* ResourceDispatcherHostImpl::GetCertStore() {
   return cert_store_for_testing_ ? cert_store_for_testing_
                                  : CertStore::GetInstance();
 }
 
 }  // namespace content