Support redirectUrl at onHeadersReceived in WebRequest / DWR API

net/url_request/url_request_unittest.cc

Index: net/url_request/url_request_unittest.cc
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index 63e7c757244ddf6c31f28adcd369cd86c2a0ef48..1bd981e37e6713b0258f3a752ce35746b7f36769 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -338,6 +338,9 @@ class BlockingNetworkDelegate : public TestNetworkDelegate {
   void set_redirect_url(const GURL& url) {
     redirect_url_ = url;
   }
+  void set_redirect_url_on_headers_received(const GURL& url) {
+    redirect_url_on_headers_received_ = url;
+  }
 
   void set_block_on(int block_on) {
     block_on_ = block_on;
@@ -367,7 +370,8 @@ class BlockingNetworkDelegate : public TestNetworkDelegate {
       URLRequest* request,
       const CompletionCallback& callback,
       const HttpResponseHeaders* original_response_headers,
-      scoped_refptr<HttpResponseHeaders>* override_response_headers) OVERRIDE;
+      scoped_refptr<HttpResponseHeaders>* override_response_headers,
+      GURL* allowed_unsafe_redirect_url) OVERRIDE;
 
   virtual NetworkDelegate::AuthRequiredResponse OnAuthRequired(
       URLRequest* request,
@@ -390,7 +394,8 @@ class BlockingNetworkDelegate : public TestNetworkDelegate {
   int retval_;  // To be returned in non-auth stages.
   AuthRequiredResponse auth_retval_;
 
-  GURL redirect_url_;  // Used if non-empty.
+  GURL redirect_url_;  // Used if non-empty during OnBeforeURLRequest.
+  GURL redirect_url_on_headers_received_;  // Used if non-empty.
   int block_on_;  // Bit mask: in which stages to block.
 
   // |auth_credentials_| will be copied to |*target_auth_credential_| on
@@ -482,10 +487,26 @@ int BlockingNetworkDelegate::OnHeadersReceived(
     URLRequest* request,
     const CompletionCallback& callback,
     const HttpResponseHeaders* original_response_headers,
-    scoped_refptr<HttpResponseHeaders>* override_response_headers) {
+    scoped_refptr<HttpResponseHeaders>* override_response_headers,
+    GURL* allowed_unsafe_redirect_url) {
   TestNetworkDelegate::OnHeadersReceived(
       request, callback, original_response_headers,
-      override_response_headers);
+      override_response_headers, allowed_unsafe_redirect_url);
+
+  if (!redirect_url_on_headers_received_.is_empty()) {
+    if (override_response_headers->get() == NULL) {
+      *override_response_headers = new net::HttpResponseHeaders(
+          original_response_headers->raw_headers());
+    }
+    (*override_response_headers)->ReplaceStatusLine(
+        "HTTP/1.1 307 Temporary Redirect");
+    (*override_response_headers)->RemoveHeader("location");
+    (*override_response_headers)->AddHeader(
+        "Location: " + redirect_url_on_headers_received_.spec());
+    *allowed_unsafe_redirect_url = redirect_url_on_headers_received_;
+    // Redirect just once.
+    redirect_url_on_headers_received_ = GURL();
+  }
 
   return MaybeBlockStage(ON_HEADERS_RECEIVED, callback);
 }
@@ -2416,8 +2437,8 @@ class FixedDateNetworkDelegate : public TestNetworkDelegate {
       net::URLRequest* request,
       const net::CompletionCallback& callback,
       const net::HttpResponseHeaders* original_response_headers,
-      scoped_refptr<net::HttpResponseHeaders>* override_response_headers)
-      OVERRIDE;
+      scoped_refptr<net::HttpResponseHeaders>* override_response_headers,
+      GURL* allowed_unsafe_redirect_url) OVERRIDE;
 
  private:
   std::string fixed_date_;
@@ -2429,7 +2450,8 @@ int FixedDateNetworkDelegate::OnHeadersReceived(
     net::URLRequest* request,
     const net::CompletionCallback& callback,
     const net::HttpResponseHeaders* original_response_headers,
-    scoped_refptr<net::HttpResponseHeaders>* override_response_headers) {
+    scoped_refptr<net::HttpResponseHeaders>* override_response_headers,
+    GURL* allowed_unsafe_redirect_url) {
   net::HttpResponseHeaders* new_response_headers =
       new net::HttpResponseHeaders(original_response_headers->raw_headers());
 
@@ -2440,7 +2462,8 @@ int FixedDateNetworkDelegate::OnHeadersReceived(
   return TestNetworkDelegate::OnHeadersReceived(request,
                                                 callback,
                                                 original_response_headers,
-                                                override_response_headers);
+                                                override_response_headers,
+                                                allowed_unsafe_redirect_url);
 }
 
 // Test that cookie expiration times are adjusted for server/client clock
@@ -2696,6 +2719,196 @@ class URLRequestTestHTTP : public URLRequestTest {
   LocalHttpTestServer test_server_;
 };
 
+namespace {
+
+// RedirectOnHeadersReceivedNetworkDelegate redirects to |redirect_url|,
+// optionally with an URL that has been marked safe for redirection.
+class RedirectOnHeadersReceivedNetworkDelegate : public TestNetworkDelegate {
+ public:
+  explicit RedirectOnHeadersReceivedNetworkDelegate(const GURL& redirect_url)
+      : redirect_url_(redirect_url) {}
+  virtual ~RedirectOnHeadersReceivedNetworkDelegate() {}
+
+  // net::NetworkDelegate implementation
+  virtual int OnHeadersReceived(
+      net::URLRequest* request,
+      const net::CompletionCallback& callback,
+      const net::HttpResponseHeaders* original_response_headers,
+      scoped_refptr<net::HttpResponseHeaders>* override_response_headers,
+      GURL* allowed_unsafe_redirect_url) OVERRIDE;
+
+  void set_url_marked_safe_for_redirect(const GURL& safe_url) {
+    safe_url_ = safe_url;
+  }
+
+ private:
+  GURL redirect_url_;
+  GURL safe_url_;
+
+  DISALLOW_COPY_AND_ASSIGN(RedirectOnHeadersReceivedNetworkDelegate);
+};
+
+int RedirectOnHeadersReceivedNetworkDelegate::OnHeadersReceived(
+    net::URLRequest* request,
+    const net::CompletionCallback& callback,
+    const net::HttpResponseHeaders* original_response_headers,
+    scoped_refptr<net::HttpResponseHeaders>* override_response_headers,
+    GURL* allowed_unsafe_redirect_url) {
+
+  if (!redirect_url_.is_empty()) {
+    net::HttpResponseHeaders* new_response_headers =
+        new net::HttpResponseHeaders(original_response_headers->raw_headers());
+
+    new_response_headers->ReplaceStatusLine("HTTP/1.1 302 Found");
+    new_response_headers->RemoveHeader("Location");
+    new_response_headers->AddHeader("Location: " + redirect_url_.spec());
+
+    *override_response_headers = new_response_headers;
+    *allowed_unsafe_redirect_url = safe_url_;
+    redirect_url_ = GURL();
+  }
+  return TestNetworkDelegate::OnHeadersReceived(request,
+                                                callback,
+                                                original_response_headers,
+                                                override_response_headers,
+                                                allowed_unsafe_redirect_url);
+}
+
+}  // namespace
+
+// Tests that the allowed redirect URL is preserved when the Location header
+// has been overwritten.
+TEST_F(URLRequestTestHTTP, UnsafeRedirectToWhitelistedUnsafeURL) {
+  ASSERT_TRUE(test_server_.Start());
+
+  GURL unsafe_url("data:text/html,this-is-considered-an-unsafe-url");
+  RedirectOnHeadersReceivedNetworkDelegate network_delegate(unsafe_url);
+  network_delegate.set_url_marked_safe_for_redirect(unsafe_url);
+  default_context_.set_network_delegate(&network_delegate);
+  TestDelegate d;
+  {
+    URLRequest r(test_server_.GetURL("whatever"),
+                 DEFAULT_PRIORITY,
+                 &d,
+                 &default_context_);
+
+    r.Start();
+    base::RunLoop().Run();
+
+    EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
+
+    EXPECT_EQ(2U, r.url_chain().size());
+    EXPECT_EQ(0, r.status().error());
+    EXPECT_EQ(unsafe_url, r.url());
+  }
+}
+
+// Tests that a redirect to a different unsafe URL is blocked, even after adding
+// some other URL to the whitelist.
+TEST_F(URLRequestTestHTTP, UnsafeRedirectToDifferentUnsafeURL) {
+  ASSERT_TRUE(test_server_.Start());
+
+  GURL unsafe_url("data:text/html,something");
+  GURL different_unsafe_url("data:text/html,something-else");
+  RedirectOnHeadersReceivedNetworkDelegate network_delegate(unsafe_url);
+  network_delegate.set_url_marked_safe_for_redirect(different_unsafe_url);
+  default_context_.set_network_delegate(&network_delegate);
+  TestDelegate d;
+  {
+    URLRequest r(test_server_.GetURL("whatever"),
+                 DEFAULT_PRIORITY,
+                 &d,
+                 &default_context_);
+
+    r.Start();
+    base::RunLoop().Run();
+
+    EXPECT_EQ(URLRequestStatus::FAILED, r.status().status());
+    EXPECT_EQ(ERR_UNSAFE_REDIRECT, r.status().error());
+  }
+}
+
+// Tests that a redirect to a safe URL is allowed, regardless of whether an
+// unsafe URL was whitelisted.
+TEST_F(URLRequestTestHTTP, UnsafeRedirectToSafeURL) {
+  ASSERT_TRUE(test_server_.Start());
+
+  GURL safe_url(test_server_.GetURL("echoheader?"));
+  GURL unrelated_unsafe_url("data:text/html,does-not-match-safe-url");
+  RedirectOnHeadersReceivedNetworkDelegate network_delegate(safe_url);
+  network_delegate.set_url_marked_safe_for_redirect(unrelated_unsafe_url);
+  default_context_.set_network_delegate(&network_delegate);
+  TestDelegate d;
+  {
+    URLRequest r(test_server_.GetURL("whatever"),
+                 DEFAULT_PRIORITY,
+                 &d,
+                 &default_context_);
+
+    r.Start();
+    base::RunLoop().Run();
+
+    EXPECT_EQ(2U, r.url_chain().size());
+    EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
+    EXPECT_EQ(0, r.status().error());
+    EXPECT_EQ(safe_url, r.url());
+    EXPECT_EQ("", d.data_received());
+  }
+}

[mmenke, 2014/03/21 18:15:04]

This test is indeed probably overkill.

[robwu, 2014/03/25 00:04:00]

Indeed, removed; This case is already covered by the
NetworkDelegateRedirectRequestOnHeadersReceived test.

+
+// Redirects from an URL with fragment to an unsafe URL without fragment should
+// be allowed.
+TEST_F(URLRequestTestHTTP, UnsafeRedirectWithReferenceFragment) {
+  ASSERT_TRUE(test_server_.Start());
+
+  GURL original_url(test_server_.GetURL("original#fragment"));
+  GURL unsafe_url("data:,url-marked-safe-and-used-in-redirect");
+  GURL expected_url("data:,url-marked-safe-and-used-in-redirect#fragment");

[mmenke, 2014/03/21 18:15:04]

Suggest a linebreak after the GURLs in all of these, for easier reading.

[robwu, 2014/03/25 00:04:00]

Done.

+  RedirectOnHeadersReceivedNetworkDelegate network_delegate(unsafe_url);
+  network_delegate.set_url_marked_safe_for_redirect(unsafe_url);
+  default_context_.set_network_delegate(&network_delegate);
+  TestDelegate d;
+  {
+    URLRequest r(original_url, DEFAULT_PRIORITY, &d, &default_context_);
+
+    r.Start();
+    base::RunLoop().Run();
+
+    EXPECT_EQ(2U, r.url_chain().size());
+    EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
+    EXPECT_EQ(0, r.status().error());
+    EXPECT_EQ(original_url, r.original_url());
+    EXPECT_EQ(expected_url, r.url());
+  }
+}
+
+// When a delegate has specified a safe redirect URL, but it does not match the
+// redirect target, then do not prevent the reference fragment from being added.
+TEST_F(URLRequestTestHTTP, RedirectWithReferenceFragment) {
+  ASSERT_TRUE(test_server_.Start());
+
+  GURL original_url(test_server_.GetURL("original#expected-fragment"));
+  GURL unsafe_url("data:text/html,this-url-does-not-match-redirect-url");
+  GURL redirect_url(test_server_.GetURL("target"));
+  GURL expected_redirect_url(test_server_.GetURL("target#expected-fragment"));
+  RedirectOnHeadersReceivedNetworkDelegate network_delegate(redirect_url);
+  network_delegate.set_url_marked_safe_for_redirect(unsafe_url);
+  default_context_.set_network_delegate(&network_delegate);
+  TestDelegate d;
+  {
+    URLRequest r(original_url, DEFAULT_PRIORITY, &d, &default_context_);
+
+    r.Start();
+    base::RunLoop().Run();
+
+    EXPECT_EQ(2U, r.url_chain().size());
+    EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
+    EXPECT_EQ(0, r.status().error());
+    EXPECT_EQ(original_url, r.original_url());
+    EXPECT_EQ(expected_redirect_url, r.url());
+  }
+}

[mmenke, 2014/03/21 18:15:04]

I don't think we need this test.  But I do think we should have a test where the
redirect_url has a different fragment from the original url.

[robwu, 2014/03/25 00:04:00]

Currently, the test is indeed redundant (it merely confirms the current
behavior).
However, after landing this feature, I will submit another patch for
http://crbug.com/354653, right after landing this patch. Then, the expected
behavior becomes "Do not add a fragment even if the original URL has a
fragment". This is important and must be tested (then expected_redirect_url will
be removed, and the expectation is that r.url() is equal to redirect_url).

+
 // In this unit test, we're using the HTTPTestServer as a proxy server and
 // issuing a CONNECT request with the magic host name "www.redirect.com".
 // The HTTPTestServer will return a 302 response, which we should not
@@ -3007,6 +3220,83 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequestPost) {
   EXPECT_EQ(1, network_delegate.destroyed_requests());
 }
 
+// Tests that the network delegate can block and redirect a request to a new
+// URL during OnHeadersReceived.
+TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequestOnHeadersReceived) {
+  ASSERT_TRUE(test_server_.Start());
+
+  TestDelegate d;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.set_block_on(BlockingNetworkDelegate::ON_HEADERS_RECEIVED);
+  GURL redirect_url(test_server_.GetURL("simple.html"));
+  network_delegate.set_redirect_url_on_headers_received(redirect_url);
+
+  TestURLRequestContextWithProxy context(
+      test_server_.host_port_pair().ToString(), &network_delegate);
+
+  {
+    GURL original_url(test_server_.GetURL("empty.html"));
+    URLRequest r(original_url, DEFAULT_PRIORITY, &d, &context);
+
+    r.Start();
+    base::RunLoop().Run();
+
+    EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
+    EXPECT_EQ(0, r.status().error());
+    EXPECT_EQ(redirect_url, r.url());
+    EXPECT_EQ(original_url, r.original_url());
+    EXPECT_EQ(2U, r.url_chain().size());
+    EXPECT_EQ(2, network_delegate.created_requests());
+    EXPECT_EQ(0, network_delegate.destroyed_requests());
+  }
+  EXPECT_EQ(1, network_delegate.destroyed_requests());
+}
+
+// Tests that redirects caused by the network delegate during OnHeadersReceived
+// preserve POST data.

[mmenke, 2014/03/21 18:15:04]

Both these test are probably overkill as well.  More tests is generally good,
but during refactorings, there's a non-zero costs for extra tests.

If you want async tests, could just turn the other tests into async tests.

[robwu, 2014/03/25 00:04:00]

You're right. What I actually wanted to do was to test the correctness of the
webRequest API. That is already done with the new tests in the extensions/
namespace, so I've removed these two tests.

+TEST_F(URLRequestTestHTTP,
+       NetworkDelegateRedirectRequestOnHeadersReceivedPost) {
+  ASSERT_TRUE(test_server_.Start());
+
+  const char kData[] = "hello world";
+
+  TestDelegate d;
+  BlockingNetworkDelegate network_delegate(
+      BlockingNetworkDelegate::AUTO_CALLBACK);
+  network_delegate.set_block_on(BlockingNetworkDelegate::ON_HEADERS_RECEIVED);
+  GURL redirect_url(test_server_.GetURL("echo"));
+  network_delegate.set_redirect_url_on_headers_received(redirect_url);
+
+  TestURLRequestContext context(true);
+  context.set_network_delegate(&network_delegate);
+  context.Init();
+
+  {
+    GURL original_url(test_server_.GetURL("empty.html"));
+    URLRequest r(original_url, DEFAULT_PRIORITY, &d, &context);
+    r.set_method("POST");
+    r.set_upload(make_scoped_ptr(CreateSimpleUploadData(kData)));
+    HttpRequestHeaders headers;
+    headers.SetHeader(HttpRequestHeaders::kContentLength,
+                      base::UintToString(arraysize(kData) - 1));
+    r.SetExtraRequestHeaders(headers);
+    r.Start();
+    base::RunLoop().Run();
+
+    EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
+    EXPECT_EQ(0, r.status().error());
+    EXPECT_EQ(redirect_url, r.url());
+    EXPECT_EQ(original_url, r.original_url());
+    EXPECT_EQ(2U, r.url_chain().size());
+    EXPECT_EQ(2, network_delegate.created_requests());
+    EXPECT_EQ(0, network_delegate.destroyed_requests());
+    EXPECT_EQ("POST", r.method());
+    EXPECT_EQ(kData, d.data_received());
+  }
+  EXPECT_EQ(1, network_delegate.destroyed_requests());
+}
+
 // Tests that the network delegate can synchronously complete OnAuthRequired
 // by taking no action. This indicates that the NetworkDelegate does not want to
 // handle the challenge, and is passing the buck along to the
@@ -3919,10 +4209,12 @@ class AsyncLoggingNetworkDelegate : public TestNetworkDelegate {
       URLRequest* request,
       const CompletionCallback& callback,
       const HttpResponseHeaders* original_response_headers,
-      scoped_refptr<HttpResponseHeaders>* override_response_headers) OVERRIDE {
+      scoped_refptr<HttpResponseHeaders>* override_response_headers,
+      GURL* allowed_unsafe_redirect_url) OVERRIDE {
     TestNetworkDelegate::OnHeadersReceived(request, callback,
                                            original_response_headers,
-                                           override_response_headers);
+                                           override_response_headers,
+                                           allowed_unsafe_redirect_url);
     return RunCallbackAsynchronously(request, callback);
   }