Do not copy reference fragments for overridden redirects.

net/url_request/url_request_unittest.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 "build/build_config.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shlobj.h>
 #endif
@@ skipping 5313 matching lines @@
                  &default_context_);
 
     r.Start();
     base::RunLoop().Run();
 
     EXPECT_EQ(URLRequestStatus::FAILED, r.status().status());
     EXPECT_EQ(ERR_UNSAFE_REDIRECT, r.status().error());
   }
 }
 
-// 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");
-
-  default_network_delegate_.set_redirect_on_headers_received_url(unsafe_url);
-  default_network_delegate_.set_allowed_unsafe_redirect_url(unsafe_url);
-
-  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(net::OK, r.status().error());
-    EXPECT_EQ(original_url, r.original_url());
-    EXPECT_EQ(expected_url, r.url());
-  }
-}
-
 // Redirects from an URL with fragment to an unsafe URL with fragment should
 // be allowed, and the reference fragment of the target URL should be preserved.
 TEST_F(URLRequestTestHTTP, UnsafeRedirectWithDifferentReferenceFragment) {
   ASSERT_TRUE(test_server_.Start());
 
   GURL original_url(test_server_.GetURL("original#fragment1"));
   GURL unsafe_url("data:,url-marked-safe-and-used-in-redirect#fragment2");
   GURL expected_url("data:,url-marked-safe-and-used-in-redirect#fragment2");
 
   default_network_delegate_.set_redirect_on_headers_received_url(unsafe_url);
   default_network_delegate_.set_allowed_unsafe_redirect_url(unsafe_url);
 
   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(net::OK, 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.
+// When a delegate has specified a safe redirect URL, assume that the redirect
+// URL should not be changed. In particular, the reference fragment should not
+// be modified.
 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"));
+  GURL original_url(test_server_.GetURL("original#should-not-be-appended"));
+  GURL redirect_url("data:text/html,expect-no-reference-fragment");
 
   default_network_delegate_.set_redirect_on_headers_received_url(redirect_url);
-  default_network_delegate_.set_allowed_unsafe_redirect_url(unsafe_url);
+  default_network_delegate_.set_allowed_unsafe_redirect_url(redirect_url);
 
   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(net::OK, r.status().error());
     EXPECT_EQ(original_url, r.original_url());
-    EXPECT_EQ(expected_redirect_url, r.url());
+    EXPECT_EQ(redirect_url, r.url());
   }
 }
 
+// When a URLRequestRedirectJob is created, the redirection must be followed and
+// the reference fragment of thetarget URL must not be modified.
+TEST_F(URLRequestTestHTTP, RedirectJobWithReferenceFragment) {
+  ASSERT_TRUE(test_server_.Start());
+
+  GURL original_url(test_server_.GetURL("original#should-not-be-appended"));
+  GURL redirect_url(test_server_.GetURL("echo"));
+
+  TestDelegate d;
+  URLRequest r(original_url, DEFAULT_PRIORITY, &d, &default_context_);
+
+  URLRequestRedirectJob* job = new URLRequestRedirectJob(
+      &r, &default_network_delegate_, redirect_url,
+      URLRequestRedirectJob::REDIRECT_302_FOUND, "Very Good Reason");
+  AddTestInterceptor()->set_main_intercept_job(job);
+
+  r.Start();
+  base::RunLoop().Run();
+
+  EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
+  EXPECT_EQ(net::OK, r.status().error());
+  EXPECT_EQ(original_url, r.original_url());
+  EXPECT_EQ(redirect_url, r.url());
+}

[mmenke, 2014/03/31 20:45:49]

Suggest we keep the old "RedirectWithReferenceFragment" test from before
(Possibly with a new name), as well as having these two tests.

[robwu, 2014/04/01 16:09:33]

Done.

+
 TEST_F(URLRequestTestHTTP, NoUserPassInReferrer) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
   URLRequest req(test_server_.GetURL("echoheader?Referer"),
                  DEFAULT_PRIORITY,
                  &d,
                  &default_context_);
   req.SetReferrer("http://user:pass@foo.com/");
   req.Start();
@@ skipping 559 matching lines @@
 TEST_F(URLRequestTestHTTP, Redirect307Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect307-to-echo");
 
   HTTPRedirectMethodTest(url, "POST", "POST", true);
   HTTPRedirectMethodTest(url, "PUT", "PUT", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
 }
 
+TEST_F(URLRequestTestHTTP, Redirect302PreserveReferenceFragment) {
+  ASSERT_TRUE(test_server_.Start());
+
+  GURL original_url(test_server_.GetURL("files/redirect302-to-echo#fragment"));
+  GURL expected_url(test_server_.GetURL("echo#fragment"));
+
+  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(net::OK, r.status().error());
+    EXPECT_EQ(original_url, r.original_url());
+    EXPECT_EQ(expected_url, r.url());
+  }
+}
+
 TEST_F(URLRequestTestHTTP, InterceptPost302RedirectGet) {
   ASSERT_TRUE(test_server_.Start());
 
   const char kData[] = "hello world";
 
   TestDelegate d;
   URLRequest req(test_server_.GetURL("empty.html"),
                  DEFAULT_PRIORITY,
                  &d,
                  &default_context_);
@@ skipping 1912 matching lines @@
 
     EXPECT_FALSE(r.is_pending());
     EXPECT_EQ(1, d->response_started_count());
     EXPECT_FALSE(d->received_data_before_response());
     EXPECT_EQ(d->bytes_received(), static_cast<int>(file_size));
   }
 }
 #endif  // !defined(DISABLE_FTP_SUPPORT)
 
 }  // namespace net