Merge pull request #3757 from nateprewitt/2590_redirected_bodies

301/302 redirects for non-POST with body

3.0-HISTORY.rst

@@ -1,6 +1,9 @@
 3.0.0 (2016-xx-xx)
 ++++++++++++++++++
 
+- Relax how Requests strips bodies from redirects. 3.0.0 only supports body
+  removal on 301/302 POST redirects and all 303 redirects.
+
 - Remove support for non-string/bytes parameters in ``_basic_auth_str``.
 
 - Prevent ``Session.merge_environment`` from erroneously setting the

requests/sessions.py

@@ -157,11 +157,11 @@ class SessionRedirectMixin(object):
             if response.is_permanent_redirect and request.url != prepared_request.url:
                 self.redirect_cache[request.url] = prepared_request.url
 
-            self.rebuild_method(prepared_request, response)
+            method_changed = self.rebuild_method(prepared_request, response)
 
-            # https://github.com/kennethreitz/requests/issues/1084
-            if response.status_code not in (codes.temporary_redirect,
-                    codes.permanent_redirect):
+            # https://github.com/kennethreitz/requests/issues/2590
+            # If method is changed to GET we need to remove body and associated headers.
+            if method_changed and prepared_request.method == 'GET':
                 # https://github.com/kennethreitz/requests/issues/3490
                 purged_headers = ('Content-Length', 'Content-Type', 'Transfer-Encoding')
                 for header in purged_headers:
@@ -282,24 +282,26 @@ class SessionRedirectMixin(object):
     def rebuild_method(self, prepared_request, response):
         """When being redirected we may want to change the method of the request
         based on certain specs or browser behavior.
+
+        :rtype bool:
+        :return: boolean expressing if the method changed during rebuild.
         """
-        method = prepared_request.method
+        method = original_method = prepared_request.method
 
         # http://tools.ietf.org/html/rfc7231#section-6.4.4
         if response.status_code == codes.see_other and method != 'HEAD':
             method = 'GET'
 
-        # Do what the browsers do, despite standards...
-        # First, turn 302s into GETs.
-        if response.status_code == codes.found and method != 'HEAD':
-            method = 'GET'
-
-        # Second, if a POST is responded to with a 301, turn it into a GET.
-        # This bizarre behaviour is explained in Issue 1704.
-        if response.status_code == codes.moved and method == 'POST':
+        # If a POST is responded to with a 301 or 302, turn it into a GET. This has
+        # become a common pattern in browsers and was introduced into later versions
+        # of HTTP RFCs. While some browsers transform other methods to GET, little of
+        # that has been standardized. For that reason, we're using curl as a model
+        # which only supports POST->GET.
+        if response.status_code in (codes.found, codes.moved) and method == 'POST':
             method = 'GET'
 
         prepared_request.method = method
+        return method != original_method
 
 
 class Session(SessionRedirectMixin):

tests/test_requests.py

@@ -225,48 +225,92 @@ class TestRequests:
         else:
             pytest.fail('Expected custom max number of redirects to be respected but was not')
 
-    def test_http_301_changes_post_to_get(self, httpbin):
-        r = requests.post(httpbin('status', '301'))
-        assert r.status_code == 200
-        assert r.request.method == 'GET'
+    @pytest.mark.parametrize(
+        'method, body, expected', (
+            ('GET', None, 'GET'),
+            ('HEAD', None, 'HEAD'),
+            ('POST', 'test', 'GET'),
+            ('PUT', 'put test', 'PUT'),
+            ('PATCH', 'patch test', 'PATCH'),
+            ('DELETE', '', 'DELETE')
+        )
+    )
+    def test_http_301_for_redirectable_methods(self, httpbin, method, body, expected):
+        """Tests all methods except OPTIONS for expected redirect behaviour.
+
+        OPTIONS responses can behave differently depending on the server, so
+        we don't have anything uniform to test except how httpbin responds
+        to them. For that reason they aren't included here.
+        """
+        params = {'url': '/%s' % expected.lower(), 'status_code': '301'}
+        r = requests.request(method, httpbin('redirect-to'), data=body, params=params)
+
+        assert r.request.url == httpbin(expected.lower())
+        assert r.request.method == expected
         assert r.history[0].status_code == 301
         assert r.history[0].is_redirect
 
-    def test_http_301_doesnt_change_head_to_get(self, httpbin):
-        r = requests.head(httpbin('status', '301'), allow_redirects=True)
-        print(r.content)
-        assert r.status_code == 200
-        assert r.request.method == 'HEAD'
-        assert r.history[0].status_code == 301
-        assert r.history[0].is_redirect
+        if expected in ('GET', 'HEAD'):
+            assert r.request.body is None
+        else:
+            assert r.json()['data'] == body
 
-    def test_http_302_changes_post_to_get(self, httpbin):
-        r = requests.post(httpbin('status', '302'))
-        assert r.status_code == 200
-        assert r.request.method == 'GET'
+    @pytest.mark.parametrize(
+        'method, body, expected', (
+            ('GET', None, 'GET'),
+            ('HEAD', None, 'HEAD'),
+            ('POST', 'test', 'GET'),
+            ('PUT', 'put test', 'PUT'),
+            ('PATCH', 'patch test', 'PATCH'),
+            ('DELETE', '', 'DELETE')
+        )
+    )
+    def test_http_302_for_redirectable_methods(self, httpbin, method, body, expected):
+        """Tests all methods except OPTIONS for expected redirect behaviour.
+
+        OPTIONS responses can behave differently depending on the server, so
+        we don't have anything uniform to test except how httpbin responds
+        to them. For that reason they aren't included here.
+        """
+        params = {'url': '/%s' % expected.lower()}
+        r = requests.request(method, httpbin('redirect-to'), data=body, params=params)
+
+        assert r.request.url == httpbin(expected.lower())
+        assert r.request.method == expected
         assert r.history[0].status_code == 302
         assert r.history[0].is_redirect
 
-    def test_http_302_doesnt_change_head_to_get(self, httpbin):
-        r = requests.head(httpbin('status', '302'), allow_redirects=True)
-        assert r.status_code == 200
-        assert r.request.method == 'HEAD'
-        assert r.history[0].status_code == 302
-        assert r.history[0].is_redirect
+        if expected in ('GET', 'HEAD'):
+            assert r.request.body is None
+        else:
+            assert r.json()['data'] == body
 
-    def test_http_303_changes_post_to_get(self, httpbin):
-        r = requests.post(httpbin('status', '303'))
-        assert r.status_code == 200
-        assert r.request.method == 'GET'
+    @pytest.mark.parametrize(
+        'method, body, expected', (
+            ('GET', None, 'GET'),
+            ('HEAD', None, 'HEAD'),
+            ('POST', 'test', 'GET'),
+            ('PUT', 'put test', 'GET'),
+            ('PATCH', 'patch test', 'GET'),
+            ('DELETE', '', 'GET')
+        )
+    )
+    def test_http_303_for_redirectable_methods(self, httpbin, method, body, expected):
+        """Tests all methods except OPTIONS for expected redirect behaviour.
+
+        OPTIONS responses can behave differently depending on the server, so
+        we don't have anything uniform to test except how httpbin responds
+        to them. For that reason they aren't included here.
+        """
+        params = {'url': '/%s' % expected.lower(), 'status_code': '303'}
+        r = requests.request(method, httpbin('redirect-to'), data=body, params=params)
+
+        assert r.request.url == httpbin(expected.lower())
+        assert r.request.method == expected
         assert r.history[0].status_code == 303
         assert r.history[0].is_redirect
 
-    def test_http_303_doesnt_change_head_to_get(self, httpbin):
-        r = requests.head(httpbin('status', '303'), allow_redirects=True)
-        assert r.status_code == 200
-        assert r.request.method == 'HEAD'
-        assert r.history[0].status_code == 303
-        assert r.history[0].is_redirect
+        assert r.request.body is None
 
     def test_multiple_location_headers(self, httpbin):
         headers = [('Location', 'http://example.com'),