Set Origin header to "null" for cross origin 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 2835 matching lines @@
       if (request_method == redirect_method) {
         EXPECT_EQ(kData, d.data_received());
       } else {
         EXPECT_NE(kData, d.data_received());
       }
     }
     if (HasFailure())
       LOG(WARNING) << "Request method was: " << request_method;
   }
 
+  // Requests |redirect_url|, which must return a HTTP 3xx redirect.
+  // |request_method| is the method to use for the initial request.
+  // |redirect_method| is the method that is expected to be used for the second
+  // request, after redirection.
+  // |origin_value| is the expected value for the Origin header after
+  // redirection. If empty, expects that there will be no Origin header.
+  void HTTPRedirectOriginHeaderTest(const GURL& redirect_url,
+                                    const std::string& request_method,
+                                    const std::string& redirect_method,
+                                    const std::string& origin_value) {
+    TestDelegate d;
+    scoped_ptr<URLRequest> req(default_context_.CreateRequest(
+        redirect_url, DEFAULT_PRIORITY, &d, NULL));

[davidben, 2015/03/24 23:47:38]

Note: you'll need to remove that trailing NULL on a rebase.

[jww, 2015/03/27 22:16:15]

Will do.

+    req->set_method(request_method);
+    req->SetExtraRequestHeaderByName(HttpRequestHeaders::kOrigin,
+                                     redirect_url.GetOrigin().spec(), false);
+    req->Start();
+
+    base::RunLoop().Run();
+
+    EXPECT_EQ(redirect_method, req->method());
+    // Note that there is no check for request success here because, for
+    // purposes of testing, the request very well may fail. For example, if the
+    // test redirects to an FTP server so it is cross origin, there is no such
+    // FTP server, so the request would fail. However, that's fine, as long as
+    // the request headers are in order and pass the checks below.

[davidben, 2015/03/24 23:47:38]

It's sort of confusing to have these tests use FTP as the cross-origin target;
FTP doesn't really have headers to begin with.

[jww, 2015/03/27 22:16:15]

Agreed. I've rejiggered all of these new tests to be HTTPS (and since
test_server_ is HTTP, this has the same cross-origin effect).

+    if (origin_value.empty()) {
+      EXPECT_FALSE(
+          req->extra_request_headers().HasHeader(HttpRequestHeaders::kOrigin));
+    } else {
+      std::string origin_header;
+      EXPECT_TRUE(req->extra_request_headers().GetHeader(
+          HttpRequestHeaders::kOrigin, &origin_header));
+      EXPECT_EQ(origin_value, origin_header);
+    }
+  }
+
   void HTTPUploadDataOperationTest(const std::string& method) {
     const int kMsgSize = 20000;  // multiple of 10
     const int kIterations = 50;
     char* uploadBytes = new char[kMsgSize+1];
     char* ptr = uploadBytes;
     char marker = 'a';
     for (int idx = 0; idx < kMsgSize/10; idx++) {
       memcpy(ptr, "----------", 10);
       ptr += 10;
       if (idx % 100 == 0) {
@@ skipping 3363 matching lines @@
   EXPECT_FALSE(ContainsString(data, "Content-Length:"));
   EXPECT_FALSE(ContainsString(data, "Content-Type:"));
   EXPECT_FALSE(ContainsString(data, "Origin:"));
 
   // These extra request headers should not have been stripped.
   EXPECT_TRUE(ContainsString(data, "Accept:"));
   EXPECT_TRUE(ContainsString(data, "Accept-Language:"));
   EXPECT_TRUE(ContainsString(data, "Accept-Charset:"));
 }
 
 // The following tests check that we handle mutating the request method for

[davidben, 2015/03/24 23:47:38]

Nit: "request method" -> "request", since you've made them do more.

[jww, 2015/03/27 22:16:15]

Done.

 // HTTP redirects as expected.
 // See http://crbug.com/56373 and http://crbug.com/102130.

[davidben, 2015/03/24 23:47:38]

Nit: I guess cite your bug too?

[jww, 2015/03/27 22:16:15]

Done.

 
 TEST_F(URLRequestTestHTTP, Redirect301Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect301-to-echo");
+  const GURL ftp_redirect_url = test_server_.GetURL("files/redirect301-to-ftp");
 
   HTTPRedirectMethodTest(url, "POST", "GET", true);
   HTTPRedirectMethodTest(url, "PUT", "PUT", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
+
+  HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(ftp_redirect_url, "GET", "GET", "null");
+  HTTPRedirectOriginHeaderTest(url, "POST", "GET", std::string());

[davidben, 2015/03/24 23:47:38]

Aside: does the spec say to drop the Origin header going from POST to GET? This
behavior was there before though, so we can leave it alone for now, but I
suspect it's wrong.

[jww, 2015/03/27 22:16:15]

No, RFC 6454 does not say to do so, nor can I find it in any of the other specs.
We definitely were doing it already, though, and I'm also not sure what other
user agents do. Might be worth circling back with Brad Hill (who filed the
initial bug) to see if he knows.

+  HTTPRedirectOriginHeaderTest(ftp_redirect_url, "POST", "GET", std::string());
 }
 
 TEST_F(URLRequestTestHTTP, Redirect302Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect302-to-echo");
+  const GURL ftp_redirect_url = test_server_.GetURL("files/redirect302-to-ftp");
 
   HTTPRedirectMethodTest(url, "POST", "GET", true);
   HTTPRedirectMethodTest(url, "PUT", "PUT", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
+
+  HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(ftp_redirect_url, "GET", "GET", "null");
+  HTTPRedirectOriginHeaderTest(url, "POST", "GET", std::string());
+  HTTPRedirectOriginHeaderTest(ftp_redirect_url, "POST", "GET", std::string());
 }
 
 TEST_F(URLRequestTestHTTP, Redirect303Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect303-to-echo");
+  const GURL ftp_redirect_url = test_server_.GetURL("files/redirect303-to-ftp");
 
   HTTPRedirectMethodTest(url, "POST", "GET", true);
   HTTPRedirectMethodTest(url, "PUT", "GET", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
+
+  HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(ftp_redirect_url, "GET", "GET", "null");
+  HTTPRedirectOriginHeaderTest(url, "POST", "GET", std::string());
+  HTTPRedirectOriginHeaderTest(ftp_redirect_url, "POST", "GET", std::string());
 }
 
 TEST_F(URLRequestTestHTTP, Redirect307Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect307-to-echo");
+  const GURL ftp_redirect_url = test_server_.GetURL("files/redirect307-to-ftp");
 
   HTTPRedirectMethodTest(url, "POST", "POST", true);
   HTTPRedirectMethodTest(url, "PUT", "PUT", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
+
+  HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(ftp_redirect_url, "GET", "GET", "null");
+  HTTPRedirectOriginHeaderTest(url, "POST", "POST", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(ftp_redirect_url, "POST", "POST", "null");
 }
 
 TEST_F(URLRequestTestHTTP, Redirect308Tests) {
   ASSERT_TRUE(test_server_.Start());
 
   const GURL url = test_server_.GetURL("files/redirect308-to-echo");
+  const GURL ftp_redirect_url = test_server_.GetURL("files/redirect308-to-ftp");
 
   HTTPRedirectMethodTest(url, "POST", "POST", true);
   HTTPRedirectMethodTest(url, "PUT", "PUT", true);
   HTTPRedirectMethodTest(url, "HEAD", "HEAD", false);
+
+  HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(ftp_redirect_url, "GET", "GET", "null");
+  HTTPRedirectOriginHeaderTest(url, "POST", "POST", url.GetOrigin().spec());
+  HTTPRedirectOriginHeaderTest(ftp_redirect_url, "POST", "POST", "null");
 }
 
 // Make sure that 308 responses without bodies are not treated as redirects.
 // Certain legacy apis that pre-date the response code expect this behavior
 // (Like Google Drive).
 TEST_F(URLRequestTestHTTP, NoRedirectOn308WithoutLocationHeader) {
   ASSERT_TRUE(test_server_.Start());
 
   TestDelegate d;
   const GURL url = test_server_.GetURL("files/308-without-location-header");
@@ skipping 2654 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