Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable following of redirects by default #454

Merged
merged 11 commits into from
Mar 27, 2020
1 change: 1 addition & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ exclude_lines =
if getattr(typing, 'TYPE_CHECKING', False):
pragma: no cover
@bottle.(get|post|route)
pytest.fail
3 changes: 2 additions & 1 deletion bravado/fido_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ def prepare_request_for_twisted(request_params):
'url': string,
'timeout': float, # optional
'connect_timeout': float, # optional
'allow_redirects': bool, # optional
}
"""

Expand All @@ -244,7 +245,7 @@ def prepare_request_for_twisted(request_params):
params=request_params.get('params'),
files=request_params.get('files'),
url=request_params.get('url'),
method=request_params.get('method')
method=request_params.get('method'),
)

# content-length was computed by 'requests' based on the current body
Expand Down
10 changes: 5 additions & 5 deletions bravado/http_future.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ def response(

swagger_result = self._get_swagger_result(incoming_response)

if self.operation is None and incoming_response.status_code >= 400:
if self.operation is None and incoming_response.status_code >= 300:
raise make_http_exception(response=incoming_response)

# Trigger fallback_result if the option is set
Expand Down Expand Up @@ -276,7 +276,7 @@ def result(
return swagger_result, incoming_response
return swagger_result

if 200 <= incoming_response.status_code < 400:
if 200 <= incoming_response.status_code < 300:
return incoming_response

raise make_http_exception(response=incoming_response)
Expand Down Expand Up @@ -410,13 +410,13 @@ def raise_on_unexpected(http_response):

def raise_on_expected(http_response):
# type: (IncomingResponse) -> None
"""Raise an HTTPError if the response is non-2XX and non-3XX and matches
a response in the swagger spec.
"""Raise an HTTPError if the response is non-2XX and matches a response
in the swagger spec.

:param http_response: :class:`bravado_core.response.IncomingResponse`
:raises: HTTPError
"""
if not 200 <= http_response.status_code < 400:
if not 200 <= http_response.status_code < 300:
raise make_http_exception(
response=http_response,
swagger_result=http_response.swagger_result)
4 changes: 1 addition & 3 deletions bravado/requests_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,6 @@ def __init__(
ssl_cert=None, # type: typing.Any
future_adapter_class=RequestsFutureAdapter, # type: typing.Type[RequestsFutureAdapter]
response_adapter_class=RequestsResponseAdapter, # type: typing.Type[RequestsResponseAdapter]
allow_redirects=False # type: bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is unclear to me why this parameter has been removed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're looking at an old changeset. It was added and then later moved to be a parameter in the request method.

):
# type: (...) -> None
"""
Expand All @@ -306,7 +305,6 @@ def __init__(
self.ssl_cert = ssl_cert
self.future_adapter_class = future_adapter_class
self.response_adapter_class = response_adapter_class
self.allow_redirects = allow_redirects

def __hash__(self):
# type: () -> int
Expand Down Expand Up @@ -363,7 +361,7 @@ def separate_params(
misc_options = {
'ssl_verify': self.ssl_verify,
'ssl_cert': self.ssl_cert,
'allow_redirects': self.allow_redirects,
'allow_redirects': sanitized_params.pop('allow_redirects', False),
}

if 'connect_timeout' in sanitized_params:
Expand Down
26 changes: 23 additions & 3 deletions bravado/testing/integration_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
from bravado.client import SwaggerClient
from bravado.exception import BravadoConnectionError
from bravado.exception import BravadoTimeoutError
from bravado.exception import HTTPMovedPermanently
from bravado.http_client import HttpClient
from bravado.http_future import FutureAdapter
from bravado.requests_client import RequestsClient
from bravado.swagger_model import Loader


Expand Down Expand Up @@ -492,15 +494,33 @@ def test_msgpack_support(self, swagger_http_server):
assert response.headers['Content-Type'] == APP_MSGPACK
assert unpackb(response.raw_bytes, encoding='utf-8') == API_RESPONSE

def test_redirects_are_not_followed(self, swagger_http_server):
def test_following_redirects(self, swagger_http_server):
# the FidoClient doesn't have a way to turn off redirects being followed
# so limit this test to the RequestsClient instead
if not isinstance(self.http_client, RequestsClient):
pytest.skip('Following redirects is only supported in the RequestsClient')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this a blacklist instead of a whitelist? bravado-asyncio does have a way to turn off following redirects, and I'd like to be able to run the test there too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, although it's somewhat clunky due to how coverage and test dependencies are loaded. If there's better suggestions on how to implement blacklisting without having to load the thing being blacklisted, I'm happy to change it.


response = self.http_client.request({
'method': 'GET',
'url': '{server_address}/redirect'.format(server_address=swagger_http_server),
'params': {},
'allow_redirects': True,
}).result(timeout=1)

assert response.status_code == 301
assert response.headers['Location'] == '/json'
assert isinstance(response, IncomingResponse) and response.status_code == 200

def test_redirects_are_not_followed(self, swagger_http_server):
try:
self.http_client.request({
'method': 'GET',
'url': '{server_address}/redirect'.format(server_address=swagger_http_server),
'params': {},
}).result(timeout=1)
except HTTPMovedPermanently as exc:
assert isinstance(exc.response, IncomingResponse) and exc.response.status_code == 301
assert isinstance(exc.response, IncomingResponse) and exc.response.headers['Location'] == '/json'
else:
pytest.fail("Expected exception was not raised")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend using pytest.raises such that we don't need manual handling of different raised exception

with pytest.raises(HTTPMovedPermanently) as excinfo:
    self.http_client.request({
        'method': 'GET',
        'url': '{server_address}/redirect'.format(server_address=swagger_http_server),
        'params': {},
    }).result(timeout=1)
exc = excinfo.value
assert isinstance(exc.response, IncomingResponse) and exc.response.status_code == 301
assert isinstance(exc.response, IncomingResponse) and exc.response.headers['Location'] == '/json'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't realise that was support by pytest.raises.

Changed to that method instead and removed the coverage ignore.


def test_timeout_errors_are_thrown_as_BravadoTimeoutError(self, swagger_http_server):
if not self.http_future_adapter_type.timeout_errors:
Expand Down