Do not copy reference fragments for overridden redirects.

    
If a network delegates sets a redirect URL (in BeforeRequest or HeadersReceived), then it should get full
control over the target URL.
In particular, data:-URIs should remain unchanged because Chrome does not interpret # as a delimiter for reference fragments.
    
BUG=354653
TEST=net_unittests:
URLRequestTestHTTP.RedirectWithReferenceFragment:URLRequestTestHTTP.RedirectJobWithReferenceFragment (tests new behavior)
URLRequestTestHTTP.RedirectWithReferenceFragmentAndUnrelatedUnsafeUrl:URLRequestTestHTTP.Redirect302PreserveReferenceFragment (regression test)

Review URL: https://codereview.chromium.org/212543005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@261412 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 119 insertions, 37 deletions

diff --git a/net/base/network_delegate.h b/net/base/network_delegate.h
index 4f930cf..4be320b 100644
--- a/net/base/network_delegate.h
+++ b/net/base/network_delegate.h
@@ -102,12 +102,15 @@ class NET_EXPORT NetworkDelegate : public base::NonThreadSafe {
   // member function, which will perform basic sanity checking.
 
   // Called before a request is sent. Allows the delegate to rewrite the URL
-  // being fetched by modifying |new_url|. |callback| and |new_url| are valid
-  // only until OnURLRequestDestroyed is called for this request. Returns a net
-  // status code, generally either OK to continue with the request or
-  // ERR_IO_PENDING if the result is not ready yet. A status code other than OK
-  // and ERR_IO_PENDING will cancel the request and report the status code as
-  // the reason.
+  // being fetched by modifying |new_url|. If set, the URL must be valid. The
+  // reference fragment from the original URL is not automatically appended to
+  // |new_url|; callers are responsible for copying the reference fragment if
+  // desired.
+  // |callback| and |new_url| are valid only until OnURLRequestDestroyed is
+  // called for this request. Returns a net status code, generally either OK to
+  // continue with the request or ERR_IO_PENDING if the result is not ready yet.
+  // A status code other than OK and ERR_IO_PENDING will cancel the request and
+  // report the status code as the reason.
   //
   // The default implementation returns OK (continue with request).
   virtual int OnBeforeURLRequest(URLRequest* request,
@@ -134,6 +137,11 @@ class NET_EXPORT NetworkDelegate : public base::NonThreadSafe {
   // network, these must not be modified. |override_response_headers| can be set
   // to new values, that should be considered as overriding
   // |original_response_headers|.
+  // If the response is a redirect, and the Location response header value is
+  // identical to |allowed_unsafe_redirect_url|, then the redirect is never
+  // blocked and the reference fragment is not copied from the original URL
+  // to the redirection target.
+  //
   // |callback|, |original_response_headers|, and |override_response_headers|
   // are only valid until OnURLRequestDestroyed is called for this request.
   // See OnBeforeURLRequest for return value description. Returns OK by default.
diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
index 78eca41..c5c209c 100644
--- a/net/url_request/url_request_http_job.cc
+++ b/net/url_request/url_request_http_job.cc
@@ -1034,6 +1034,16 @@ Filter* URLRequestHttpJob::SetupFilter() const {
       ? Filter::Factory(encoding_types, *filter_context_) : NULL;
 }
 
+bool URLRequestHttpJob::CopyFragmentOnRedirect(const GURL& location) const {
+  // Allow modification of reference fragments by default, unless
+  // |allowed_unsafe_redirect_url_| is set and equal to the redirect URL.
+  // When this is the case, we assume that the network delegate has set the
+  // desired redirect URL (with or without fragment), so it must not be changed
+  // any more.
+  return !allowed_unsafe_redirect_url_.is_valid() ||
+       allowed_unsafe_redirect_url_ != location;
+}
+
 bool URLRequestHttpJob::IsSafeRedirect(const GURL& location) {
   // HTTP is always safe.
   // TODO(pauljensen): Remove once crbug.com/146591 is fixed.
@@ -1041,14 +1051,10 @@ bool URLRequestHttpJob::IsSafeRedirect(const GURL& location) {
       (location.scheme() == "http" || location.scheme() == "https")) {
     return true;
   }
-  // Delegates may mark an URL as safe for redirection.
-  if (allowed_unsafe_redirect_url_.is_valid()) {
-    GURL::Replacements replacements;
-    replacements.ClearRef();
-    if (allowed_unsafe_redirect_url_.ReplaceComponents(replacements) ==
-        location.ReplaceComponents(replacements)) {
-      return true;
-    }
+  // Delegates may mark a URL as safe for redirection.
+  if (allowed_unsafe_redirect_url_.is_valid() &&
+      allowed_unsafe_redirect_url_ == location) {
+    return true;
   }
   // Query URLRequestJobFactory as to whether |location| would be safe to
   // redirect to.
diff --git a/net/url_request/url_request_http_job.h b/net/url_request/url_request_http_job.h
index 95b77c9..d5aa5ba 100644
--- a/net/url_request/url_request_http_job.h
+++ b/net/url_request/url_request_http_job.h
@@ -107,6 +107,7 @@ class NET_EXPORT_PRIVATE URLRequestHttpJob : public URLRequestJob {
   virtual bool GetResponseCookies(std::vector<std::string>* cookies) OVERRIDE;
   virtual int GetResponseCode() const OVERRIDE;
   virtual Filter* SetupFilter() const OVERRIDE;
+  virtual bool CopyFragmentOnRedirect(const GURL& location) const OVERRIDE;
   virtual bool IsSafeRedirect(const GURL& location) OVERRIDE;
   virtual bool NeedsAuth() OVERRIDE;
   virtual void GetAuthChallengeInfo(scoped_refptr<AuthChallengeInfo>*) OVERRIDE;
@@ -257,6 +258,8 @@ class NET_EXPORT_PRIVATE URLRequestHttpJob : public URLRequestJob {
   scoped_refptr<HttpResponseHeaders> override_response_headers_;
 
   // The network delegate can mark a URL as safe for redirection.
+  // The reference fragment of the original URL is not appended to the redirect
+  // URL when the redirect URL is equal to |allowed_unsafe_redirect_url_|.
   GURL allowed_unsafe_redirect_url_;
 
   // Flag used to verify that |this| is not deleted while we are awaiting
diff --git a/net/url_request/url_request_job.cc b/net/url_request/url_request_job.cc
index 35dbbb1..31a0d50 100644
--- a/net/url_request/url_request_job.cc
+++ b/net/url_request/url_request_job.cc
@@ -158,6 +158,10 @@ bool URLRequestJob::IsRedirectResponse(GURL* location,
   return true;
 }
 
+bool URLRequestJob::CopyFragmentOnRedirect(const GURL& location) const {
+  return true;
+}
+
 bool URLRequestJob::IsSafeRedirect(const GURL& location) {
   return true;
 }
@@ -335,7 +339,8 @@ void URLRequestJob::NotifyHeadersComplete() {
 
     // Move the reference fragment of the old location to the new one if the
     // new one has none. This duplicates mozilla's behavior.
-    if (url.is_valid() && url.has_ref() && !new_location.has_ref()) {
+    if (url.is_valid() && url.has_ref() && !new_location.has_ref() &&
+        CopyFragmentOnRedirect(new_location)) {
       GURL::Replacements replacements;
       // Reference the |ref| directly out of the original URL to avoid a
       // malloc.
diff --git a/net/url_request/url_request_job.h b/net/url_request/url_request_job.h
index c6f7ba4..c1fd3bb 100644
--- a/net/url_request/url_request_job.h
+++ b/net/url_request/url_request_job.h
@@ -157,6 +157,13 @@ class NET_EXPORT URLRequestJob
   // The default implementation inspects the response_info_.
   virtual bool IsRedirectResponse(GURL* location, int* http_status_code);
 
+  // Called to determine if it is okay to copy the reference fragment from the
+  // original URL (if existent) to the redirection target when the redirection
+  // target has no reference fragment.
+  //
+  // The default implementation returns true.
+  virtual bool CopyFragmentOnRedirect(const GURL& location) const;
+
   // Called to determine if it is okay to redirect this job to the specified
   // location.  This may be used to implement protocol-specific restrictions.
   // If this function returns false, then the URLRequest will fail
diff --git a/net/url_request/url_request_redirect_job.cc b/net/url_request/url_request_redirect_job.cc
index c9d30ae..818d4c3 100644
--- a/net/url_request/url_request_redirect_job.cc
+++ b/net/url_request/url_request_redirect_job.cc
@@ -44,6 +44,12 @@ bool URLRequestRedirectJob::IsRedirectResponse(GURL* location,
   return true;
 }
 
+bool URLRequestRedirectJob::CopyFragmentOnRedirect(const GURL& location) const {
+  // The instantiators have full control over the desired redirection target,
+  // including the reference fragment part of the URL.
+  return false;
+}
+
 URLRequestRedirectJob::~URLRequestRedirectJob() {}
 
 void URLRequestRedirectJob::StartAsync() {
diff --git a/net/url_request/url_request_redirect_job.h b/net/url_request/url_request_redirect_job.h
index 8321477..580fec2 100644
--- a/net/url_request/url_request_redirect_job.h
+++ b/net/url_request/url_request_redirect_job.h
@@ -40,6 +40,7 @@ class NET_EXPORT URLRequestRedirectJob : public URLRequestJob {
   virtual void Start() OVERRIDE;
   virtual bool IsRedirectResponse(GURL* location,
                                   int* http_status_code) OVERRIDE;
+  virtual bool CopyFragmentOnRedirect(const GURL& location) const OVERRIDE;
 
   virtual void GetLoadTimingInfo(
       LoadTimingInfo* load_timing_info) const OVERRIDE;
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index aab2b32..01af382 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -5331,14 +5331,14 @@ TEST_F(URLRequestTestHTTP, UnsafeRedirectToDifferentUnsafeURL) {
   }
 }
 
-// Redirects from an URL with fragment to an unsafe URL without fragment should
-// be allowed.
-TEST_F(URLRequestTestHTTP, UnsafeRedirectWithReferenceFragment) {
+// 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#fragment"));
-  GURL unsafe_url("data:,url-marked-safe-and-used-in-redirect");
-  GURL expected_url("data:,url-marked-safe-and-used-in-redirect#fragment");
+  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);
@@ -5358,16 +5358,17 @@ TEST_F(URLRequestTestHTTP, UnsafeRedirectWithReferenceFragment) {
   }
 }
 
-// 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) {
+// 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, RedirectWithReferenceFragmentAndUnrelatedUnsafeUrl) {
   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");
+  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"));
 
-  default_network_delegate_.set_redirect_on_headers_received_url(unsafe_url);
+  default_network_delegate_.set_redirect_on_headers_received_url(redirect_url);
   default_network_delegate_.set_allowed_unsafe_redirect_url(unsafe_url);
 
   TestDelegate d;
@@ -5381,22 +5382,21 @@ TEST_F(URLRequestTestHTTP, UnsafeRedirectWithDifferentReferenceFragment) {
     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());
+    EXPECT_EQ(expected_redirect_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;
   {
@@ -5409,10 +5409,35 @@ TEST_F(URLRequestTestHTTP, RedirectWithReferenceFragment) {
     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 the target 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());
+}
+
 TEST_F(URLRequestTestHTTP, NoUserPassInReferrer) {
   ASSERT_TRUE(test_server_.Start());
 
@@ -5992,6 +6017,27 @@ TEST_F(URLRequestTestHTTP, Redirect307Tests) {
   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());