Repository: tornadoweb/tornado

Issue:
tornado.httpclient.HTTPClient.fetch() executes callback on contents of redirect response
Assume the following example of using the HTTPClient to retrieve a file:

```python
>>> tornado.version
'4.2.1'
>>> try:
...     fh = open('/tmp/foo.msi', 'wb')
...     def handle_chunk(chunk):
...         fh.write(chunk)
...     tornado.httpclient.HTTPClient(max_body_size=107374182400).fetch('http://download.macromedia.com/get/flashplayer/current/licensing/win/install_flash_player_18_plugin.msi', method='GET', streaming_callback=handle_chunk)
... finally:
...     fh.close()
...
HTTPResponse(_body=None,buffer=<_io.BytesIO object at 0x7f933e4a7470>,code=200,effective_url='http://fpdownload.macromedia.com/get/flashplayer/current/licensing/win/install_flash_player_18_plugin.msi',error=None,headers={'Content-Length': '19924480', 'Accept-Ranges': 'bytes', 'Server': 'Apache', 'Last-Modified': 'Fri, 07 Aug 2015 12:22:31 GMT', 'Connection': 'close', 'Etag': '"1300600-51cb7b0998fc0"', 'Date': 'Mon, 14 Sep 2015 17:24:22 GMT', 'Content-Type': 'application/x-msi'},reason='OK',request=<tornado.httpclient.HTTPRequest object at 0x7f933e196250>,request_time=4.273449897766113,time_info={})
>>> with open('/tmp/foo.msi', 'rb') as fh:
...     first_288_bytes = fh.read(288)
...
>>> first_288_bytes
'<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">\n<html><head>\n<title>302 Found</title>\n</head><body>\n<h1>Found</h1>\n<p>The document has moved <a href="http://fpdownload.macromedia.com/get/flashplayer/current/licensing/win/install_flash_player_18_plugin.msi">here</a>.</p>\n</body></html>'
```

This results in a corrupted file, as the HTTP response from the 302 redirect is written to the destination file via the callback.

It's entirely possible that I am doing something wrong, but this looks like a bug. I can conceive of no way to reliably determine that the callback is being executed on a 302 redirect response, since the raw data from the chunk of the file is what is passed to the callback function, leaving nothing from the HTTPResponse object to be examined in order to determine whether or not to write the chunk to the destination file.

Discussion:

Patch:
diff --git a/.travis.yml b/.travis.yml
index 4b199eb8..0a184314 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -35,6 +35,7 @@ install:
     # codecov tries to install the latest.
     - if [[ $TRAVIS_PYTHON_VERSION == '3.2' ]]; then travis_retry pip install 'coverage<4.0'; fi
     - travis_retry pip install codecov
+    - curl-config --version; pip freeze
 
 script:
     # Get out of the source directory before running tests to avoid PYTHONPATH
diff --git a/tornado/simple_httpclient.py b/tornado/simple_httpclient.py
index 81ed8873..074d18b8 100644
--- a/tornado/simple_httpclient.py
+++ b/tornado/simple_httpclient.py
@@ -462,9 +462,12 @@ class _HTTPConnection(httputil.HTTPMessageDelegate):
         if self.request.expect_100_continue and first_line.code == 100:
             self._write_body(False)
             return
-        self.headers = headers
         self.code = first_line.code
         self.reason = first_line.reason
+        self.headers = headers
+
+        if self._should_follow_redirect():
+            return
 
         if self.request.header_callback is not None:
             # Reassemble the start line.
@@ -473,14 +476,17 @@ class _HTTPConnection(httputil.HTTPMessageDelegate):
                 self.request.header_callback("%s: %s\r\n" % (k, v))
             self.request.header_callback('\r\n')
 
+    def _should_follow_redirect(self):
+        return (self.request.follow_redirects and
+                self.request.max_redirects > 0 and
+                self.code in (301, 302, 303, 307))
+
     def finish(self):
         data = b''.join(self.chunks)
         self._remove_timeout()
         original_request = getattr(self.request, "original_request",
                                    self.request)
-        if (self.request.follow_redirects and
-            self.request.max_redirects > 0 and
-                self.code in (301, 302, 303, 307)):
+        if self._should_follow_redirect():
             assert isinstance(self.request, _RequestProxy)
             new_request = copy.copy(self.request.request)
             new_request.url = urlparse.urljoin(self.request.url,
@@ -527,6 +533,9 @@ class _HTTPConnection(httputil.HTTPMessageDelegate):
         self.stream.close()
 
     def data_received(self, chunk):
+        if self._should_follow_redirect():
+            # We're going to follow a redirect so just discard the body.
+            return
         if self.request.streaming_callback is not None:
             self.request.streaming_callback(chunk)
         else:
diff --git a/tornado/web.py b/tornado/web.py
index afa9ca58..aa203495 100644
--- a/tornado/web.py
+++ b/tornado/web.py
@@ -1065,33 +1065,12 @@ class RequestHandler(object):
     def current_user(self):
         """The authenticated user for this request.
 
-        This is set in one of two ways:
+        This is a cached version of `get_current_user`, which you can
+        override to set the user based on, e.g., a cookie. If that
+        method is not overridden, this method always returns None.
 
-        * A subclass may override `get_current_user()`, which will be called
-          automatically the first time ``self.current_user`` is accessed.
-          `get_current_user()` will only be called once per request,
-          and is cached for future access::
-
-              def get_current_user(self):
-                  user_cookie = self.get_secure_cookie("user")
-                      if user_cookie:
-                          return json.loads(user_cookie)
-                  return None
-
-        * It may be set as a normal variable, typically from an overridden
-          `prepare()`::
-
-              @gen.coroutine
-              def prepare(self):
-                  user_id_cookie = self.get_secure_cookie("user_id")
-                  if user_id_cookie:
-                      self.current_user = yield load_user(user_id_cookie)
-
-        Note that `prepare()` may be a coroutine while `get_current_user()`
-        may not, so the latter form is necessary if loading the user requires
-        asynchronous operations.
-
-        The user object may any type of the application's choosing.
+        We lazy-load the current user the first time this method is called
+        and cache the result after that.
         """
         if not hasattr(self, "_current_user"):
             self._current_user = self.get_current_user()
@@ -1102,10 +1081,7 @@ class RequestHandler(object):
         self._current_user = value
 
     def get_current_user(self):
-        """Override to determine the current user from, e.g., a cookie.
-
-        This method may not be a coroutine.
-        """
+        """Override to determine the current user from, e.g., a cookie."""
         return None
 
     def get_login_url(self):


Test patch:
diff --git a/tornado/test/httpclient_test.py b/tornado/test/httpclient_test.py
index ecc63e4a..6254c266 100644
--- a/tornado/test/httpclient_test.py
+++ b/tornado/test/httpclient_test.py
@@ -48,6 +48,7 @@ class PutHandler(RequestHandler):
 
 class RedirectHandler(RequestHandler):
     def prepare(self):
+        self.write('redirects can have bodies too')
         self.redirect(self.get_argument("url"),
                       status=int(self.get_argument("status", "302")))
 
diff --git a/tornado/test/simple_httpclient_test.py b/tornado/test/simple_httpclient_test.py
index dc4c865b..5214c1e4 100644
--- a/tornado/test/simple_httpclient_test.py
+++ b/tornado/test/simple_httpclient_test.py
@@ -11,14 +11,15 @@ import socket
 import ssl
 import sys
 
+from tornado.escape import to_unicode
 from tornado import gen
 from tornado.httpclient import AsyncHTTPClient
 from tornado.httputil import HTTPHeaders, ResponseStartLine
 from tornado.ioloop import IOLoop
 from tornado.log import gen_log
 from tornado.netutil import Resolver, bind_sockets
-from tornado.simple_httpclient import SimpleAsyncHTTPClient, _default_ca_certs
-from tornado.test.httpclient_test import ChunkHandler, CountdownHandler, HelloWorldHandler
+from tornado.simple_httpclient import SimpleAsyncHTTPClient
+from tornado.test.httpclient_test import ChunkHandler, CountdownHandler, HelloWorldHandler, RedirectHandler
 from tornado.test import httpclient_test
 from tornado.testing import AsyncHTTPTestCase, AsyncHTTPSTestCase, AsyncTestCase, ExpectLog
 from tornado.test.util import skipOnTravis, skipIfNoIPv6, refusing_port, unittest
@@ -145,6 +146,7 @@ class SimpleHTTPClientTestMixin(object):
             url("/no_content_length", NoContentLengthHandler),
             url("/echo_post", EchoPostHandler),
             url("/respond_in_prepare", RespondInPrepareHandler),
+            url("/redirect", RedirectHandler),
         ], gzip=True)
 
     def test_singleton(self):
@@ -416,6 +418,24 @@ class SimpleHTTPClientTestMixin(object):
                               expect_100_continue=True)
         self.assertEqual(response.code, 403)
 
+    def test_streaming_follow_redirects(self):
+        # When following redirects, header and streaming callbacks
+        # should only be called for the final result.
+        # TODO(bdarnell): this test belongs in httpclient_test instead of
+        # simple_httpclient_test, but it fails with the version of libcurl
+        # available on travis-ci. Move it when that has been upgraded
+        # or we have a better framework to skip tests based on curl version.
+        headers = []
+        chunks = []
+        self.fetch("/redirect?url=/hello",
+                   header_callback=headers.append,
+                   streaming_callback=chunks.append)
+        chunks = list(map(to_unicode, chunks))
+        self.assertEqual(chunks, ['Hello world!'])
+        # Make sure we only got one set of headers.
+        num_start_lines = len([h for h in headers if h.startswith("HTTP/")])
+        self.assertEqual(num_start_lines, 1)
+
 
 class SimpleHTTPClientTestCase(SimpleHTTPClientTestMixin, AsyncHTTPTestCase):
     def setUp(self):