Preserve non-POST methods on 301/302 requests.

BUG=56373
TEST=URLRequestTestHTTP.Redirect30*Tests

Review URL: http://codereview.chromium.org/8375002

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

8 files changed, 100 insertions, 33 deletions

diff --git a/net/data/url_request_unittest/redirect301-to-echo b/net/data/url_request_unittest/redirect301-to-echo
new file mode 100644
index 0000000..7898192
--- /dev/null
+++ b/net/data/url_request_unittest/redirect301-to-echo
@@ -0,0 +1 @@
+a
diff --git a/net/data/url_request_unittest/redirect301-to-echo.mock-http-headers b/net/data/url_request_unittest/redirect301-to-echo.mock-http-headers
new file mode 100644
index 0000000..2f97903
--- /dev/null
+++ b/net/data/url_request_unittest/redirect301-to-echo.mock-http-headers
@@ -0,0 +1,2 @@
+HTTP/1.1 301 Yo
+Location: /echo
diff --git a/net/data/url_request_unittest/redirect302-to-echo b/net/data/url_request_unittest/redirect302-to-echo
new file mode 100644
index 0000000..7898192
--- /dev/null
+++ b/net/data/url_request_unittest/redirect302-to-echo
@@ -0,0 +1 @@
+a
diff --git a/net/data/url_request_unittest/redirect302-to-echo.mock-http-headers b/net/data/url_request_unittest/redirect302-to-echo.mock-http-headers
new file mode 100644
index 0000000..819878d
--- /dev/null
+++ b/net/data/url_request_unittest/redirect302-to-echo.mock-http-headers
@@ -0,0 +1,2 @@
+HTTP/1.1 302 Yo
+Location: /echo
diff --git a/net/data/url_request_unittest/redirect303-to-echo b/net/data/url_request_unittest/redirect303-to-echo
new file mode 100644
index 0000000..7898192
--- /dev/null
+++ b/net/data/url_request_unittest/redirect303-to-echo
@@ -0,0 +1 @@
+a
diff --git a/net/data/url_request_unittest/redirect303-to-echo.mock-http-headers b/net/data/url_request_unittest/redirect303-to-echo.mock-http-headers
new file mode 100644
index 0000000..63e0884
--- /dev/null
+++ b/net/data/url_request_unittest/redirect303-to-echo.mock-http-headers
@@ -0,0 +1,2 @@
+HTTP/1.1 303 Yo
+Location: /echo
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index f6af23b..b46b6cc 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -695,17 +695,26 @@ int URLRequest::Redirect(const GURL& location, int http_status_code) {
     return ERR_UNSAFE_REDIRECT;
   }
 
-  bool strip_post_specific_headers = false;
-  if (http_status_code != 307) {
-    // NOTE: Even though RFC 2616 says to preserve the request method when
-    // following a 302 redirect, normal browsers don't do that.  Instead, they
-    // all convert a POST into a GET in response to a 302 and so shall we.  For
-    // 307 redirects, browsers preserve the method.  The RFC says to prompt the
-    // user to confirm the generation of a new POST request, but IE omits this
-    // prompt and so shall we.
-    strip_post_specific_headers = method_ == "POST";
+  // For 303 redirects, all request methods are converted to GETs, as per RFC
+  // 2616.  The latest httpbis draft also allows POST requests to be converted
+  // to GETs when following 301/302 redirects for historical reasons.  Most
+  // major browsers do this and so shall we.  The RFC says to prompt the user
+  // to confirm the generation of new requests, other than GET and HEAD
+  // requests, but IE omits these prompts and so shall we.
+  // See:  http://greenbytes.de/tech/webdav/draft-ietf-httpbis-p2-semantics-latest.html#status.3xx
+  bool was_post = method_ == "POST";
+  if (http_status_code == 303 ||
+      ((http_status_code == 301 || http_status_code == 302) && was_post)) {
     method_ = "GET";
     upload_ = NULL;
+    if (was_post) {
+      // If being switched from POST to GET, must remove headers that were
+      // specific to the POST and don't have meaning in GET. For example
+      // the inclusion of a multipart Content-Type header in GET can cause
+      // problems with some servers:
+      // http://code.google.com/p/chromium/issues/detail?id=843
+      StripPostSpecificHeaders(&extra_request_headers_);
+    }
   }
 
   // Suppress the referrer if we're redirecting out of https.
@@ -715,15 +724,6 @@ int URLRequest::Redirect(const GURL& location, int http_status_code) {
   url_chain_.push_back(location);
   --redirect_limit_;
 
-  if (strip_post_specific_headers) {
-    // If being switched from POST to GET, must remove headers that were
-    // specific to the POST and don't have meaning in GET. For example
-    // the inclusion of a multipart Content-Type header in GET can cause
-    // problems with some servers:
-    // http://code.google.com/p/chromium/issues/detail?id=843
-    StripPostSpecificHeaders(&extra_request_headers_);
-  }
-
   if (!final_upload_progress_)
     final_upload_progress_ = job_->GetUploadProgress();
 
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index 2645861..53ba466 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -283,6 +283,45 @@ class URLRequestTestHTTP : public URLRequestTest {
   }
 
  protected:
+  // 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.
+  // If |include_data| is true, data is uploaded with the request.  The
+  // response body is expected to match it exactly, if and only if
+  // |request_method| == |redirect_method|.
+  void HTTPRedirectMethodTest(const GURL& redirect_url,
+                              const std::string& request_method,
+                              const std::string& redirect_method,
+                              bool include_data) {
+    static const char kData[] = "hello world";
+    TestDelegate d;
+    TestURLRequest req(redirect_url, &d);
+    req.set_context(default_context_);
+    req.set_method(request_method);
+    if (include_data) {
+      req.set_upload(CreateSimpleUploadData(kData).get());
+      HttpRequestHeaders headers;
+      headers.SetHeader(HttpRequestHeaders::kContentLength,
+                        base::UintToString(arraysize(kData) - 1));
+      req.SetExtraRequestHeaders(headers);
+    }
+    req.Start();
+    MessageLoop::current()->Run();
+    EXPECT_EQ(redirect_method, req.method());
+    EXPECT_EQ(URLRequestStatus::SUCCESS, req.status().status());
+    EXPECT_EQ(OK, req.status().error());
+    if (include_data) {
+      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;
+  }
+
   void HTTPUploadDataOperationTest(const std::string& method) {
     const int kMsgSize = 20000;  // multiple of 10
     const int kIterations = 50;
@@ -2443,24 +2482,43 @@ TEST_F(URLRequestTestHTTP, Post302RedirectGet) {
   EXPECT_TRUE(ContainsString(data, "Accept-Charset:"));
 }
 
-TEST_F(URLRequestTestHTTP, Post307RedirectPost) {
+// The following tests check that we handle mutating the request method for
+// HTTP redirects as expected.  See http://crbug.com/56373.
+
+TEST_F(URLRequestTestHTTP, Redirect301Tests) {
   ASSERT_TRUE(test_server_.Start());
 
-  const char kData[] = "hello world";
+  const GURL url = test_server_.GetURL("files/redirect301-to-echo");
 
-  TestDelegate d;
-  TestURLRequest req(test_server_.GetURL("files/redirect307-to-echo"), &d);
-  req.set_context(default_context_);
-  req.set_method("POST");
-  req.set_upload(CreateSimpleUploadData(kData).get());
-  HttpRequestHeaders headers;
-  headers.SetHeader(HttpRequestHeaders::kContentLength,
-                    base::UintToString(arraysize(kData) - 1));
-  req.SetExtraRequestHeaders(headers);
-  req.Start();
-  MessageLoop::current()->Run();
-  EXPECT_EQ("POST", req.method());
-  EXPECT_EQ(kData, d.data_received());
+  HTTPRedirectMethodTest(url, "POST", "GET", true);
+  HTTPRedirectMethodTest(url, "PUT", "PUT", true);
+}
+
+TEST_F(URLRequestTestHTTP, Redirect302Tests) {
+  ASSERT_TRUE(test_server_.Start());
+
+  const GURL url = test_server_.GetURL("files/redirect302-to-echo");
+
+  HTTPRedirectMethodTest(url, "POST", "GET", true);
+  HTTPRedirectMethodTest(url, "PUT", "PUT", true);
+}
+
+TEST_F(URLRequestTestHTTP, Redirect303Tests) {
+  ASSERT_TRUE(test_server_.Start());
+
+  const GURL url = test_server_.GetURL("files/redirect303-to-echo");
+
+  HTTPRedirectMethodTest(url, "POST", "GET", true);
+  HTTPRedirectMethodTest(url, "PUT", "GET", true);
+}
+
+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);
 }
 
 TEST_F(URLRequestTestHTTP, InterceptPost302RedirectGet) {