Bug 676059 - Make redirect prompting depend on HTTP-safeness of method, not presence of request body. r=bz

--HG--
extra : rebase_source : 758d5f6a873e072dee96da181cbb0b1c8419fcd3

netwerk/protocol/http/HttpBaseChannel.cpp

@@ -1494,6 +1494,41 @@ CopyProperties(const nsAString& aKey, nsIVariant *aData, void *aClosure)
   return PL_DHASH_NEXT;
 }
 
+// Return whether upon a redirect code of httpStatus for method, the
+// request method should be rewritten to GET.
+//
+bool
+HttpBaseChannel::ShouldRewriteRedirectToGET(PRUint32 httpStatus,
+                                            nsHttpAtom method)
+{
+  // always rewrite for 301 and 302, but see bug 598304
+  // and  RFC 2616, Section 8.3.
+  if (httpStatus == 301 || httpStatus == 302)
+    return true;
+
+  // always rewrite for 303
+  if (httpStatus == 303)
+    return true;
+
+  // otherwise, such as for 307, do not rewrite
+  return false;
+}   
+
+// Return whether the specified method is safe as per RFC 2616, Section 9.1.1.
+bool
+HttpBaseChannel::IsSafeMethod(nsHttpAtom method)
+{
+  // This code will need to be extended for new safe methods, otherwise
+  // they'll default to "not safe".
+  return method == nsHttp::Get ||
+         method == nsHttp::Head ||
+         method == nsHttp::Options ||
+         method == nsHttp::Propfind ||
+         method == nsHttp::Report ||
+         method == nsHttp::Search ||
+         method == nsHttp::Trace;
+}
+
 nsresult
 HttpBaseChannel::SetupReplacementChannel(nsIURI       *newURI, 
                                          nsIChannel   *newChannel,

netwerk/protocol/http/HttpBaseChannel.h

@@ -216,6 +216,9 @@ public:
 
 public: /* Necko internal use only... */
 
+  bool ShouldRewriteRedirectToGET(PRUint32 httpStatus, nsHttpAtom method);
+  bool IsSafeMethod(nsHttpAtom method);
+  
 protected:
 
   // Handle notifying listener, removing from loadgroup if request failed.

netwerk/protocol/http/HttpChannelChild.cpp

@@ -757,8 +757,10 @@ HttpChannelChild::Redirect1Begin(const PRUint32& newChannelId,
   mResponseHead = new nsHttpResponseHead(responseHead);
   SetCookie(mResponseHead->PeekHeader(nsHttp::Set_Cookie));
 
-  bool preserveMethod = (mResponseHead->Status() == 307);
-  rv = SetupReplacementChannel(uri, newChannel, preserveMethod);
+  bool rewriteToGET = ShouldRewriteRedirectToGET(mResponseHead->Status(), 
+                                                 mRequestHead.Method());
+  
+  rv = SetupReplacementChannel(uri, newChannel, !rewriteToGET);
   if (NS_FAILED(rv)) {
     // Veto redirect.  nsHttpChannel decides to cancel or continue.
     OnRedirectVerifyCallback(rv);

netwerk/protocol/http/nsHttpAtomList.h

@@ -144,5 +144,7 @@ HTTP_ATOM(Post,                      "POST")
 HTTP_ATOM(Propfind,                  "PROPFIND")
 HTTP_ATOM(Proppatch,                 "PROPPATCH")
 HTTP_ATOM(Put,                       "PUT")
+HTTP_ATOM(Report,                    "REPORT")
+HTTP_ATOM(Search,                    "SEARCH")
 HTTP_ATOM(Trace,                     "TRACE")
 HTTP_ATOM(Unlock,                    "UNLOCK")

netwerk/protocol/http/nsHttpChannel.cpp

@@ -3422,9 +3422,12 @@ nsHttpChannel::ContinueProcessRedirectionAfterFallback(nsresult rv)
         }
     }
 
-    // if we need to re-send POST data then be sure to ask the user first.
-    bool preserveMethod = (mRedirectType == 307);
-    if (preserveMethod && mUploadStream) {
+    bool rewriteToGET = HttpBaseChannel::ShouldRewriteRedirectToGET(
+        mRedirectType, mRequestHead.Method());
+      
+    // prompt if the method is not safe (such as POST, PUT, DELETE, ...)
+    if (!rewriteToGET &&
+        !HttpBaseChannel::IsSafeMethod(mRequestHead.Method())) {
         rv = PromptTempRedirect();
         if (NS_FAILED(rv)) return rv;
     }
@@ -3437,7 +3440,7 @@ nsHttpChannel::ContinueProcessRedirectionAfterFallback(nsresult rv)
     rv = ioService->NewChannelFromURI(mRedirectURI, getter_AddRefs(newChannel));
     if (NS_FAILED(rv)) return rv;
 
-    rv = SetupReplacementChannel(mRedirectURI, newChannel, preserveMethod);
+    rv = SetupReplacementChannel(mRedirectURI, newChannel, !rewriteToGET);
     if (NS_FAILED(rv)) return rv;
 
     PRUint32 redirectFlags;

netwerk/test/unit/test_XHR_redirects.js

@@ -0,0 +1,127 @@
+// This file tests whether XmlHttpRequests correctly handle redirects,
+// including rewriting POSTs to GETs (on 301/302/303), as well as
+// prompting for redirects of other unsafe methods (such as PUTs, DELETEs,
+// etc--see HttpBaseChannel::IsSafeMethod).  Since no prompting is possible
+// in xpcshell, we get an error for prompts, and the request fails.
+
+do_load_httpd_js();
+
+var server;
+const BUGID = "676059";
+
+function createXHR(async, method, path)
+{
+  var xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
+            .createInstance(Ci.nsIXMLHttpRequest);
+  xhr.open(method, "http://localhost:4444" + path, async);
+  return xhr;
+}
+
+function checkResults(xhr, method, status)
+{
+  if (xhr.readyState != 4)
+    return false;
+
+  do_check_eq(xhr.status, status);
+  
+  if (status == 200) {
+    // if followed then check for echoed method name
+    do_check_eq(xhr.getResponseHeader("X-Received-Method"), method);
+  }
+  
+  return true;
+}
+
+function run_test() {
+  // start server
+  server = new nsHttpServer();
+
+  server.registerPathHandler("/bug" + BUGID + "-redirect301", bug676059redirect301);
+  server.registerPathHandler("/bug" + BUGID + "-redirect302", bug676059redirect302);
+  server.registerPathHandler("/bug" + BUGID + "-redirect303", bug676059redirect303);
+  server.registerPathHandler("/bug" + BUGID + "-redirect307", bug676059redirect307);
+  server.registerPathHandler("/bug" + BUGID + "-target", bug676059target);
+
+  server.start(4444);
+
+  // format: redirectType, methodToSend, redirectedMethod, finalStatus
+  //   redirectType sets the URI the initial request goes to
+  //   methodToSend is the HTTP method to send
+  //   redirectedMethod is the method to use for the redirect, if any
+  //   finalStatus is 200 when the redirect takes place, redirectType otherwise
+  
+  // Note that unsafe methods should not follow the redirect automatically
+  // Of the methods below, DELETE, POST and PUT are unsafe
+    
+  var tests = [
+    // 301: rewrite just POST
+    [301, "DELETE", "GET", 200], // but see bug 598304
+    [301, "GET", "GET", 200],
+    [301, "HEAD", "GET", 200], // but see bug 598304
+    [301, "POST", "GET", 200],
+    [301, "PUT", "GET", 200], // but see bug 598304
+    [301, "PROPFIND", "GET", 200], // but see bug 598304
+    // 302: see 301
+    [302, "DELETE", "GET", 200], // but see bug 598304
+    [302, "GET", "GET", 200],
+    [302, "HEAD", "GET", 200], // but see bug 598304
+    [302, "POST", "GET", 200],
+    [302, "PUT", "GET", 200], // but see bug 598304
+    [302, "PROPFIND", "GET", 200], // but see bug 598304
+    // 303: rewrite to GET except HEAD
+    [303, "DELETE", "GET", 200],
+    [303, "GET", "GET", 200],
+    [303, "HEAD", "GET", 200],
+    [303, "POST", "GET", 200],
+    [303, "PUT", "GET", 200],
+    [303, "PROPFIND", "GET", 200],
+    // 307: never rewrite
+    [307, "DELETE", null, 307],
+    [307, "GET", "GET", 200],
+    [307, "HEAD", "HEAD", 200],
+    [307, "POST", null, 307],
+    [307, "PUT", null, 307],
+    [307, "PROPFIND", "PROPFIND", 200],
+  ];
+
+  var xhr;
+  
+  for (var i = 0; i < tests.length; ++i) {
+    dump("Testing " + tests[i] + "\n");
+    xhr = createXHR(false, tests[i][1], "/bug" + BUGID + "-redirect" + tests[i][0]);
+    xhr.send(null);
+    checkResults(xhr, tests[i][2], tests[i][3]);
+  }  
+
+  server.stop(do_test_finished);
+}
+ 
+ // PATH HANDLER FOR /bug676059-redirect301
+function bug676059redirect301(metadata, response) {
+  response.setStatusLine(metadata.httpVersion, 301, "Moved Permanently");
+  response.setHeader("Location", "http://localhost:4444/bug" + BUGID + "-target");
+}
+
+ // PATH HANDLER FOR /bug676059-redirect302
+function bug676059redirect302(metadata, response) {
+  response.setStatusLine(metadata.httpVersion, 302, "Found");
+  response.setHeader("Location", "http://localhost:4444/bug" + BUGID + "-target");
+}
+
+ // PATH HANDLER FOR /bug676059-redirect303
+function bug676059redirect303(metadata, response) {
+  response.setStatusLine(metadata.httpVersion, 303, "See Other");
+  response.setHeader("Location", "http://localhost:4444/bug" + BUGID + "-target");
+}
+
+ // PATH HANDLER FOR /bug676059-redirect307
+function bug676059redirect307(metadata, response) {
+  response.setStatusLine(metadata.httpVersion, 307, "Temporary Redirect");
+  response.setHeader("Location", "http://localhost:4444/bug" + BUGID + "-target");
+}
+
+ // PATH HANDLER FOR /bug676059-target
+function bug676059target(metadata, response) {
+  response.setStatusLine(metadata.httpVersion, 200, "OK");
+  response.setHeader("X-Received-Method", metadata.method);
+}

netwerk/test/unit/xpcshell.ini

@@ -167,3 +167,4 @@ skip-if = os == "android"
 [test_traceable_channel.js]
 [test_unescapestring.js]
 [test_xmlhttprequest.js]
+[test_XHR_redirects.js]

netwerk/test/unit_ipc/test_XHR_redirects.js

@@ -0,0 +1,3 @@
+function run_test() {
+  run_test_in_child("../unit/test_XHR_redirects.js");
+}

netwerk/test/unit_ipc/xpcshell.ini

@@ -23,3 +23,4 @@ tail =
 [test_simple_wrap.js]
 [test_traceable_channel_wrap.js]
 [test_xmlhttprequest_wrap.js]
+[test_XHR_redirects.js]