Bug 742174 - Allow empty Location header. r=mcmanus, a=akeybl

mozilla · Apr 9, 2012 · c7d5f4e · c7d5f4e
1 parent e58ab00
commit c7d5f4e
Show file tree

Hide file tree

Showing 5 changed files with 125 additions and 16 deletions.
diff --git a/netwerk/protocol/http/nsHttpHeaderArray.cpp b/netwerk/protocol/http/nsHttpHeaderArray.cpp
@@ -89,9 +89,6 @@ nsHttpHeaderArray::SetHeaderFromNet(nsHttpAtom header, const nsACString &value)
 
     if (!entry) {
         if (value.IsEmpty()) {
-            if (HeaderMustHaveValue(header)) {
-                return NS_ERROR_CORRUPTED_CONTENT;
-            }
             if (!TrackEmptyHeader(header)) {
                 LOG(("Ignoring Empty Header: %s\n", header.get()));
                 return NS_OK; // ignore empty headers by default

diff --git a/netwerk/protocol/http/nsHttpHeaderArray.h b/netwerk/protocol/http/nsHttpHeaderArray.h
@@ -113,8 +113,6 @@ class nsHttpHeaderArray
 
     // Header cannot be merged: only one value possible
     bool    IsSingletonHeader(nsHttpAtom header);
-    // For some headers, we treat no value as possible CRLF attack
-    bool    HeaderMustHaveValue(nsHttpAtom header);
     // For some headers we want to track empty values to prevent them being
     // combined with non-empty ones as a CRLF attack vector
     bool    TrackEmptyHeader(nsHttpAtom header);
@@ -161,16 +159,11 @@ nsHttpHeaderArray::IsSingletonHeader(nsHttpAtom header)
            header == nsHttp::Max_Forwards;
 }
 
-inline bool
-nsHttpHeaderArray::HeaderMustHaveValue(nsHttpAtom header)
-{
-    return header == nsHttp::Location;
-}
-
 inline bool
 nsHttpHeaderArray::TrackEmptyHeader(nsHttpAtom header)
 {
-    return header == nsHttp::Content_Length;
+    return header == nsHttp::Content_Length ||
+           header == nsHttp::Location;
 }
 
 inline void

diff --git a/netwerk/test/unit/test_duplicate_headers.js b/netwerk/test/unit/test_duplicate_headers.js
@@ -361,14 +361,13 @@ function completeTest11(request, data, ctx)
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-// Bug 716801 FAIL if any/only Location: header is blank
-test_flags[12] = CL_EXPECT_FAILURE;
+// Bug 716801 OK for Location: header to be blank
 
 function handler12(metadata, response)
 {
   var body = "012345678901234567890123456789";
   response.seizePower();
-  response.write("HTTP/1.0 301 Moved\r\n");
+  response.write("HTTP/1.0 200 OK\r\n");
   response.write("Content-Type: text/plain\r\n");
   response.write("Content-Length: 30\r\n");
   response.write("Location:\r\n");
@@ -380,7 +379,8 @@ function handler12(metadata, response)
 
 function completeTest12(request, data, ctx)
 {
-  do_check_eq(request.status, Components.results.NS_ERROR_CORRUPTED_CONTENT);
+  do_check_eq(request.status, Components.results.NS_OK);
+  do_check_eq(30, data.length);
 
   run_test_number(13);
 }
@@ -567,5 +567,33 @@ function completeTest19(request, data, ctx)
   do_check_eq(request.status, Components.results.NS_OK);
   do_check_eq(30, data.length);
 
+  run_test_number(20);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// FAIL if 1st Location: header is blank, followed by non-blank
+test_flags[20] = CL_EXPECT_FAILURE;
+
+function handler20(metadata, response)
+{
+  var body = "012345678901234567890123456789";
+  response.seizePower();
+  response.write("HTTP/1.0 301 Moved\r\n");
+  response.write("Content-Type: text/plain\r\n");
+  response.write("Content-Length: 30\r\n");
+  // redirect to previous test handler that completes OK: test 4
+  response.write("Location:\r\n");
+  response.write("Location: http://localhost:4444" + testPathBase + "4\r\n");
+  response.write("Connection: close\r\n");
+  response.write("\r\n");
+  response.write(body);
+  response.finish();
+}
+
+function completeTest20(request, data, ctx)
+{
+  do_check_eq(request.status, Components.results.NS_ERROR_CORRUPTED_CONTENT);
+
   endTests();
 }
+
diff --git a/netwerk/test/unit/test_redirect_loop.js b/netwerk/test/unit/test_redirect_loop.js
@@ -0,0 +1,90 @@
+do_load_httpd_js();
+
+/*
+ * This xpcshell test checks whether we detect infinite HTTP redirect loops.
+ * We check loops with "Location:" set to 1) full URI, 2) relative URI, and 3)
+ * empty Location header (which resolves to a relative link to the original
+ * URI when the original URI ends in a slash).
+ */
+
+var httpServer = null;
+
+var fullLoopPath = "/fullLoop"; 
+var fullLoopURI = "http://localhost:4444" + fullLoopPath;
+
+var relativeLoopPath = "/relativeLoop"; 
+var relativeLoopURI = "http://localhost:4444" + relativeLoopPath;
+
+// must use directory-style URI, so empty Location redirects back to self
+var emptyLoopPath = "/empty/";
+var emptyLoopURI = "http://localhost:4444" + emptyLoopPath;
+
+function make_channel(url, callback, ctx) {
+  var ios = Cc["@mozilla.org/network/io-service;1"].
+            getService(Ci.nsIIOService);
+  return ios.newChannel(url, "", null);
+}
+
+function fullLoopHandler(metadata, response)
+{
+  response.setStatusLine(metadata.httpVersion, 301, "Moved");
+  response.setHeader("Location", "http://localhost:4444/fullLoop", false);
+}
+
+function relativeLoopHandler(metadata, response)
+{
+  response.setStatusLine(metadata.httpVersion, 301, "Moved");
+  response.setHeader("Location", "relativeLoop", false);
+}
+
+function emptyLoopHandler(metadata, response)
+{
+  // Comrades!  We must seize power from the petty-bourgeois running dogs of
+  // httpd.js in order to reply with a blank Location header!
+  response.seizePower();
+  response.write("HTTP/1.0 301 Moved\r\n");
+  response.write("Location: \r\n");
+  response.write("Content-Length: 4\r\n");
+  response.write("\r\n");
+  response.write("oops");
+  response.finish();
+}
+
+function testFullLoop(request, buffer)
+{
+  do_check_eq(request.status, Components.results.NS_ERROR_REDIRECT_LOOP);
+
+  var chan = make_channel(relativeLoopURI);
+  chan.asyncOpen(new ChannelListener(testRelativeLoop, null, CL_EXPECT_FAILURE), 
+                 null);
+}
+
+function testRelativeLoop(request, buffer)
+{
+  do_check_eq(request.status, Components.results.NS_ERROR_REDIRECT_LOOP);
+
+  var chan = make_channel(emptyLoopURI);
+  chan.asyncOpen(new ChannelListener(testEmptyLoop, null, CL_EXPECT_FAILURE), 
+                 null);
+}
+
+function testEmptyLoop(request, buffer)
+{
+  do_check_eq(request.status, Components.results.NS_ERROR_REDIRECT_LOOP);
+
+  httpServer.stop(do_test_finished);
+}
+
+function run_test()
+{
+  httpServer = new nsHttpServer();
+  httpServer.registerPathHandler(fullLoopPath, fullLoopHandler);
+  httpServer.registerPathHandler(relativeLoopPath, relativeLoopHandler);
+  httpServer.registerPathHandler(emptyLoopPath, emptyLoopHandler);
+  httpServer.start(4444);
+
+  var chan = make_channel(fullLoopURI);
+  chan.asyncOpen(new ChannelListener(testFullLoop, null, CL_EXPECT_FAILURE), 
+                 null);
+  do_test_pending();
+}
diff --git a/netwerk/test/unit/xpcshell.ini b/netwerk/test/unit/xpcshell.ini
@@ -154,6 +154,7 @@ skip-if = os == "android"
 [test_redirect_canceled.js]
 [test_redirect_failure.js]
 [test_redirect_passing.js]
+[test_redirect_loop.js]
 [test_reentrancy.js]
 [test_reopen.js]
 [test_resumable_channel.js]