Perform redirect checks before OnReceivedRedirect in //net.

net/url_request/url_request_job.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 "net/url_request/url_request_job.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 439 matching lines @@
   request_->OnHeadersComplete();
 
   GURL new_location;
   int http_status_code;
 
   if (IsRedirectResponse(&new_location, &http_status_code)) {
     // Redirect response bodies are not read. Notify the transaction
     // 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
     // code must return immediately.
     base::WeakPtr<URLRequestJob> weak_this(weak_factory_.GetWeakPtr());
 
     RedirectInfo redirect_info =
         ComputeRedirectInfo(new_location, http_status_code);
     bool defer_redirect = false;
     request_->NotifyReceivedRedirect(redirect_info, &defer_redirect);
@@ skipping 243 matching lines @@
   if (result != ERR_IO_PENDING) {
     // If the read completes synchronously, either success or failure, invoke
     // GatherRawReadStats so we can account for the completed read.
     GatherRawReadStats(result);
   } else {
     read_raw_callback_ = callback;
   }
   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) {
   DCHECK(raw_read_buffer_ || bytes_read == 0);
   DCHECK_NE(ERR_IO_PENDING, bytes_read);
 
   if (bytes_read > 0) {
     // If there is a filter, bytes will be logged after the filter is applied.
     if (source_stream_->type() != SourceStream::TYPE_NONE &&
         request()->net_log().IsCapturing()) {
@@ skipping 115 matching lines @@
   int64_t total_sent_bytes = GetTotalSentBytes();
   DCHECK_GE(total_sent_bytes, last_notified_total_sent_bytes_);
   if (total_sent_bytes > last_notified_total_sent_bytes_) {
     network_delegate_->NotifyNetworkBytesSent(
         request_, total_sent_bytes - last_notified_total_sent_bytes_);
   }
   last_notified_total_sent_bytes_ = total_sent_bytes;
 }
 
 }  // namespace net