Repository: tornadoweb/tornado
Pull request #2440 (2018-07-11 02:35:40)

httpclient: Validate redirect location header and method
This is a second attempt at #2409, which was reverted in #2439 because it was merged too close to 5.1's release. This should have a test added (to verify that the behavior is consistent between simple and curl clients) before it is merged. 

gizmo753: Please fix

gizmo753: Please fix

Patch:
diff --git a/tornado/http1connection.py b/tornado/http1connection.py
index 30f2a172..fd63be62 100644
--- a/tornado/http1connection.py
+++ b/tornado/http1connection.py
@@ -390,7 +390,10 @@ class HTTP1Connection(httputil.HTTPConnection):
             self._chunking_output = (
                 start_line.method in ("POST", "PUT", "PATCH")
                 and "Content-Length" not in headers
-                and "Transfer-Encoding" not in headers
+                and (
+                    "Transfer-Encoding" not in headers
+                    or headers["Transfer-Encoding"] == "chunked"
+                )
             )
         else:
             assert isinstance(start_line, httputil.ResponseStartLine)
diff --git a/tornado/queues.py b/tornado/queues.py
index 19ff6c66..d135c3cb 100644
--- a/tornado/queues.py
+++ b/tornado/queues.py
@@ -232,16 +232,6 @@ class Queue(Generic[_T]):
         scale as `tornado.ioloop.IOLoop.time`, normally `time.time`), or a
         `datetime.timedelta` object for a deadline relative to the
         current time.
-
-        .. note::
-
-           The ``timeout`` argument of this method differs from that
-           of the standard library's `queue.Queue.get`. That method
-           interprets numeric values as relative timeouts; this one
-           interprets them as absolute deadlines and requires
-           ``timedelta`` objects for relative timeouts (consistent
-           with other timeouts in Tornado).
-
         """
         future = Future()  # type: Future[_T]
         try:
diff --git a/tornado/simple_httpclient.py b/tornado/simple_httpclient.py
index 81a0fd24..6e075aa6 100644
--- a/tornado/simple_httpclient.py
+++ b/tornado/simple_httpclient.py
@@ -608,6 +608,8 @@ class _HTTPConnection(httputil.HTTPMessageDelegate):
             return (
                 self.code in (301, 302, 303, 307, 308)
                 and self.request.max_redirects > 0
+                and self.headers is not None
+                and self.headers.get("Location") is not None
             )
         return False
 
@@ -624,14 +626,17 @@ class _HTTPConnection(httputil.HTTPMessageDelegate):
             )
             new_request.max_redirects = self.request.max_redirects - 1
             del new_request.headers["Host"]
-            # http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.4
-            # Client SHOULD make a GET request after a 303.
-            # According to the spec, 302 should be followed by the same
-            # method as the original request, but in practice browsers
-            # treat 302 the same as 303, and many servers use 302 for
-            # compatibility with pre-HTTP/1.1 user agents which don't
-            # understand the 303 status.
-            if self.code in (302, 303):
+            # https://tools.ietf.org/html/rfc7231#section-6.4
+            #
+            # The original HTTP spec said that after a 301 or 302
+            # redirect, the request method should be preserved.
+            # However, browsers implemented this by changing the
+            # method to GET, and the behavior stuck. 303 redirects
+            # always specified this POST-to-GET behavior (arguably 303
+            # redirects should change *all* requests to GET, but
+            # libcurl only does this for POST so we follow their
+            # example).
+            if self.code in (301, 302, 303) and self.request.method == "POST":
                 new_request.method = "GET"
                 new_request.body = None
                 for h in [


Test patch:
diff --git a/tornado/test/httpclient_test.py b/tornado/test/httpclient_test.py
index ca7fb5a9..26a1ad7f 100644
--- a/tornado/test/httpclient_test.py
+++ b/tornado/test/httpclient_test.py
@@ -62,6 +62,13 @@ class RedirectHandler(RequestHandler):
         )
 
 
+class RedirectWithoutLocationHandler(RequestHandler):
+    def prepare(self):
+        # For testing error handling of a redirect with no location header.
+        self.set_status(301)
+        self.finish()
+
+
 class ChunkHandler(RequestHandler):
     @gen.coroutine
     def get(self):
@@ -119,7 +126,7 @@ class AllMethodsHandler(RequestHandler):
     def method(self):
         self.write(self.request.method)
 
-    get = post = put = delete = options = patch = other = method  # type: ignore
+    get = head = post = put = delete = options = patch = other = method  # type: ignore
 
 
 class SetHeaderHandler(RequestHandler):
@@ -143,6 +150,7 @@ class HTTPClientCommonTestCase(AsyncHTTPTestCase):
                 url("/post", PostHandler),
                 url("/put", PutHandler),
                 url("/redirect", RedirectHandler),
+                url("/redirect_without_location", RedirectWithoutLocationHandler),
                 url("/chunk", ChunkHandler),
                 url("/auth", AuthHandler),
                 url("/countdown/([0-9]+)", CountdownHandler, name="countdown"),
@@ -291,6 +299,55 @@ Transfer-Encoding: chunked
         self.assertTrue(response.effective_url.endswith("/countdown/0"))
         self.assertEqual(b"Zero", response.body)
 
+    def test_redirect_without_location(self):
+        response = self.fetch("/redirect_without_location", follow_redirects=True)
+        # If there is no location header, the redirect response should
+        # just be returned as-is. (This should arguably raise an
+        # error, but libcurl doesn't treat this as an error, so we
+        # don't either).
+        self.assertEqual(301, response.code)
+
+    def test_redirect_put_with_body(self):
+        response = self.fetch(
+            "/redirect?url=/put&status=307", method="PUT", body="hello"
+        )
+        self.assertEqual(response.body, b"Put body: hello")
+
+    def test_redirect_put_without_body(self):
+        # This "without body" edge case is similar to what happens with body_producer.
+        response = self.fetch(
+            "/redirect?url=/put&status=307",
+            method="PUT",
+            allow_nonstandard_methods=True,
+        )
+        self.assertEqual(response.body, b"Put body: ")
+
+    def test_method_after_redirect(self):
+        # Legacy redirect codes (301, 302) convert POST requests to GET.
+        for status in [301, 302, 303]:
+            url = "/redirect?url=/all_methods&status=%d" % status
+            resp = self.fetch(url, method="POST", body=b"")
+            self.assertEqual(b"GET", resp.body)
+
+            # Other methods are left alone.
+            for method in ["GET", "OPTIONS", "PUT", "DELETE"]:
+                resp = self.fetch(url, method=method, allow_nonstandard_methods=True)
+                self.assertEqual(utf8(method), resp.body)
+            # HEAD is different so check it separately.
+            resp = self.fetch(url, method="HEAD")
+            self.assertEqual(200, resp.code)
+            self.assertEqual(b"", resp.body)
+
+        # Newer redirects always preserve the original method.
+        for status in [307, 308]:
+            url = "/redirect?url=/all_methods&status=307"
+            for method in ["GET", "OPTIONS", "POST", "PUT", "DELETE"]:
+                resp = self.fetch(url, method=method, allow_nonstandard_methods=True)
+                self.assertEqual(method, to_unicode(resp.body))
+            resp = self.fetch(url, method="HEAD")
+            self.assertEqual(200, resp.code)
+            self.assertEqual(b"", resp.body)
+
     def test_credentials_in_url(self):
         url = self.get_url("/auth").replace("http://", "http://me:secret@")
         response = self.fetch(url)