From 084fb05d52c9e244b395fc3f41888b1b1f00be65 Mon Sep 17 00:00:00 2001 From: Nate Prewitt Date: Wed, 7 Dec 2016 20:32:46 -0700 Subject: [PATCH] fixing redirects for non-GET/HEAD/POST methods --- requests/sessions.py | 22 +++++----- tests/test_requests.py | 99 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 12 deletions(-) diff --git a/requests/sessions.py b/requests/sessions.py index f6d39827..396e974f 100644 --- a/requests/sessions.py +++ b/requests/sessions.py @@ -157,11 +157,13 @@ class SessionRedirectMixin(object): if response.is_permanent_redirect and request.url != prepared_request.url: self.redirect_cache[request.url] = prepared_request.url + old_method = prepared_request.method self.rebuild_method(prepared_request, response) + new_method = prepared_request.method - # 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 old_method != new_method and new_method == 'GET': # https://github.com/kennethreitz/requests/issues/3490 purged_headers = ('Content-Length', 'Content-Type', 'Transfer-Encoding') for header in purged_headers: @@ -289,14 +291,12 @@ class SessionRedirectMixin(object): 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 diff --git a/tests/test_requests.py b/tests/test_requests.py index fdb0fa6a..c76c8417 100755 --- a/tests/test_requests.py +++ b/tests/test_requests.py @@ -234,12 +234,52 @@ class TestRequests: 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 + def test_http_301_doesnt_change_non_post_to_get(self, httpbin): + r = requests.patch(httpbin('redirect-to'), + data='test body', + params={'url': 'patch', 'status_code': '301'}) + assert r.status_code == 200 + assert r.request.method == 'PATCH' + assert r.history[0].status_code == 301 + assert r.history[0].is_redirect + assert r.request.body == 'test body' + assert r.json()['data'] == 'test body' + + @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 + + 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 @@ -254,6 +294,36 @@ class TestRequests: assert r.history[0].status_code == 302 assert r.history[0].is_redirect + @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 + + 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 @@ -268,6 +338,33 @@ class TestRequests: assert r.history[0].status_code == 303 assert r.history[0].is_redirect + @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 + + assert r.request.body is None + def test_multiple_location_headers(self, httpbin): headers = [('Location', 'http://example.com'), ('Location', 'https://example.com/1')]