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

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 fb12a9b95407a2d9276c7bbe9279646f22f837d8..d4c14cb0e7a84f46b939238be10b8a07b2fb5661 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -1887,6 +1887,17 @@ void ResourceDispatcherHostImpl::BeginSaveFile(const GURL& url,
       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));
 }