Perform redirect checks before OnReceivedRedirect in //net.

net/url_request/url_request_job.cc

Index: net/url_request/url_request_job.cc
diff --git a/net/url_request/url_request_job.cc b/net/url_request/url_request_job.cc
index 050ba283f66a808362612c1fa6c00756e8a841a9..78d44f50a0b9cf4a5b1d1604729e437b0ba67356 100644
--- a/net/url_request/url_request_job.cc
+++ b/net/url_request/url_request_job.cc
@@ -457,6 +457,18 @@ void URLRequestJob::NotifyHeadersComplete() {
     // so it does not treat being stopped as an error.
     DoneReadingRedirectResponse();
 
+    // Invalid redirect targets are failed early before
+    // NotifyReceivedRedirect. This means the delegate can assume that, if it
+    // accepts the redirect, future calls to OnResponseStarted correspond to
+    // |redirect_info.new_url|.
+    int redirect_valid = CanFollowRedirect(new_location);
+    if (redirect_valid != OK) {
+      has_handled_response_ = true;
+      request_->NotifyResponseStarted(
+          URLRequestStatus::FromError(redirect_valid));

[mmenke, 2017/06/05 19:25:15]

Why not use OnDone()?

[davidben, 2017/06/05 19:44:17]

Done.

+      return;
+    }
+
     // When notifying the URLRequest::Delegate, it can destroy the request,
     // which will destroy |this|.  After calling to the URLRequest::Delegate,
     // pointer must be checked to see if |this| still exists, and if not, the
@@ -720,10 +732,25 @@ int URLRequestJob::ReadRawDataHelper(IOBuffer* buf,
   return result;
 }
 
+int URLRequestJob::CanFollowRedirect(const GURL& new_url) {
+  if (request_->redirect_limit_ <= 0) {
+    DVLOG(1) << "disallowing redirect: exceeds limit";
+    return ERR_TOO_MANY_REDIRECTS;
+  }
+
+  if (!new_url.is_valid())
+    return ERR_INVALID_REDIRECT;
+
+  if (!IsSafeRedirect(new_url)) {

[mmenke, 2017/06/05 19:25:15]

Doesn't RDH/CRDHD currently allow redirects to schemes the network stack doesn't
handle?  (Like email:foo, or whatever weird third party schemes are registered
locally?)  I think this check breaks that.

[davidben, 2017/06/05 19:44:17]

Yup, this is exactly why the spec is split up in this bizarre way. :-)

Is SafeRedirect lets through anything it doesn't understand, probably to keep
this exact case working.
https://cs.chromium.org/chromium/src/net/url_request/url_request_job_factory_...

I suppose that too is sort of weird. The nuisance is if we decide to order the
other way, we actually need two signals out of URLRequestJob (so //content knows
it's okay to update the URL), which means two process/IPC hops.

[mmenke, 2017/06/05 19:52:01]

Ah, you're right.  I had thought the reason we informed the Delegate before the
check was because this check failed, but I was wrong.

+    DVLOG(1) << "disallowing redirect: unsafe protocol";
+    return ERR_UNSAFE_REDIRECT;
+  }
+
+  return OK;
+}
+
 void URLRequestJob::FollowRedirect(const RedirectInfo& redirect_info) {
-  int rv = request_->Redirect(redirect_info);
-  if (rv != OK)
-    OnDone(URLRequestStatus(URLRequestStatus::FAILED, rv), true);
+  request_->Redirect(redirect_info);
 }
 
 void URLRequestJob::GatherRawReadStats(int bytes_read) {