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
2 changes: 2 additions & 0 deletions bravado/requests_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ def result(self, timeout=None):
response = self.session.send(
prepared_request,
timeout=self.build_timeout(timeout),
allow_redirects=self.misc_options['allow_redirects'],
**settings
)
return response
Expand Down Expand Up @@ -360,6 +361,7 @@ def separate_params(
misc_options = {
'ssl_verify': self.ssl_verify,
'ssl_cert': self.ssl_cert,
'allow_redirects': sanitized_params.pop('allow_redirects', False),
}

if 'connect_timeout' in sanitized_params:
Expand Down
49 changes: 49 additions & 0 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 @@ -159,6 +161,17 @@ def _class_fqn(clz):
},
},
},
'/redirect': {
'get': {
'operationId': 'redirect_test',
'produces': ['application/json'],
'responses': {
'301': {
'description': 'HTTP/301',
},
},
},
},
},
}

Expand Down Expand Up @@ -234,6 +247,14 @@ def sleep_api():
return sec_to_sleep


@bottle.get('/redirect')
def redirect_test():
return bottle.HTTPResponse(
status=301,
headers={'Location': '/json'},
)


def run_bottle_server(port):
"""Wrapper function for bottle.run so that the child Python interpreter finds the bottle routes on Windows."""
bottle.run(quiet=True, host='localhost', port=port)
Expand Down Expand Up @@ -473,6 +494,34 @@ 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_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 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:
pytest.skip('{} does NOT defines timeout_errors'.format(self.http_future_adapter_type))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ def test_separate_params():
'connect_timeout': 1,
'ssl_cert': None,
'ssl_verify': True,
'allow_redirects': False,
'timeout': 2,
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@
def test_result_header_values_are_bytes(session_mock, request_mock):
session_mock.merge_environment_settings.return_value = {}
request_mock.headers = {b'X-Foo': b'hi'}
RequestsFutureAdapter(session_mock, request_mock, misc_options={'ssl_verify': True, 'ssl_cert': None}).result()
RequestsFutureAdapter(
session_mock,
request_mock,
misc_options={
'ssl_verify': True,
'ssl_cert': None,
'allow_redirects': False
},
).result()

# prepare_request should be called with a request containing correctly
# casted headers (bytestrings should be preserved)
Expand Down