Repository: Yelp/bravado

Issue:
Bravado should not silently follow redirects
In my API spec, I have an endpoint that returns a 301 redirect. This redirects to another URL on a different server that subsequently returns a 200. However, because bravado is using requests, this redirect never comes back to bravado which gets the 200 (which is not a specified response) and then throws an error.

Relevant section of logs below. Logs without a prefix for where it came from comes from setting `http.client.HTTPConnection.debuglevel = 5`. Comments are inserted for clarity and some addresses are redacted for privacy (as this is on a private network).

```
# here we send the initial request which responds with a 301
[10:11:10.432][bravado.client][DEBUG] getSecureClient({'_request_options': {'headers': {'UserID': '123456'}}, 'storeID': '3888d0f1-7896-4c8e-98c8-4934d5a37aff'})
send: b'GET /api/v2/backup/stores/3888d0f1-7896-4c8e-98c8-4934d5a37aff/secure-client HTTP/1.1\r\nHost: localhost:4000\r\nUser-Agent: python-requests/2.22.0\r\nAccept-Encoding: gzip, d                                        eflate\r\nAccept: */*\r\nConnection: keep-alive\r\nUserID: 123456\r\n\r\n'
reply: 'HTTP/1.1 301 Moved Permanently\r\n'
header: Location: http://<redirected-url>/<somefile>
header: Vary: Origin
header: Date: Mon, 17 Feb 2020 10:11:10 GMT
header: Content-Length: 0
[10:11:10.437][urllib3.connectionpool][DEBUG] http://localhost:4000 "GET /api/v2/backup/stores/3888d0f1-7896-4c8e-98c8-4934d5a37aff/secure-client HTTP/1.1" 301 0

# because bravado doesn't configure anything for redirects
# they are automatically followed by requests/urllib3
[10:11:10.440][urllib3.connectionpool][DEBUG] Starting new HTTP connection (1): web-proxy.sdc.hpecorp.net:8080
send: b'GET http://<redirected-url>/<somefile> HTTP/1.1\r\nHost: 16.26.136.45:8080\r\nUser-Agent: python-requests/2.22.0\r\nAccept-Encoding: gzip, deflate\r\nAccept: */*\r\nConnection: keep-alive\r\nUserID: 123456\r\n\r\n'
reply: 'HTTP/1.1 200 OK\r\n'
header: Date: Mon, 17 Feb 2020 10:12:29 GMT
header: X-Content-Type-Options: nosniff
header: Content-Security-Policy: sandbox; default-src 'none'; img-src 'self'; style-src 'self';
header: X-WebKit-CSP: sandbox; default-src 'none'; img-src 'self'; style-src 'self';
header: X-Content-Security-Policy: sandbox; default-src 'none'; img-src 'self'; style-src 'self';
header: Last-Modified: Mon, 17 Feb 2020 09:49:05 GMT
header: Expires: Mon, 17 Feb 2020 09:49:05 GMT
header: Accept-Ranges: bytes
header: Content-Type: application/octet-stream
header: Content-Length: 17681912
header: Server: Jetty(9.4.z-SNAPSHOT)
header: Connection: Keep-Alive
header: Age: 0
[10:11:10.474][urllib3.connectionpool][DEBUG] http://<proxy>:8080 "GET http://<redirected-url>/<somefile> HTTP/1.1" 200 17681912
Traceback (most recent call last):
  File "/home/matt/repos/baas-cli/.venv/lib/python3.7/site-packages/bravado/http_future.py", line 337, in unmarshal_response
    op=operation,
  File "/home/matt/repos/baas-cli/.venv/lib/python3.7/site-packages/bravado/http_future.py", line 370, in unmarshal_response_inner
    response_spec = get_response_spec(status_code=response.status_code, op=op)
  File "/home/matt/repos/baas-cli/.venv/lib/python3.7/site-packages/bravado_core/response.py", line 160, in get_response_spec
    "status_code or use a `default` response.".format(status_code, op),
bravado_core.exception.MatchingResponseNotFound: Response specification matching http status_code 200 not found for operation Operation(getSecureClient). Either add a response specifi                                        cation for the status_code or use a `default` response.
```

Initially I wondered if it was worth exposing out redirect configuration to the bravado client, but on reflection I don't think bravado should be doing anything in this area. If the API spec says it returns a 301, bravado should not be doing anything extra that might obscure that. I also wondered that if the responses claim they should return a 30X status then redirects should be implicitly disabled, but that creeps into the territory of being a bit too smart or non-obvious.

Discussion:

Patch:
diff --git a/bravado/fido_client.py b/bravado/fido_client.py
index 81743b9..ba3164c 100644
--- a/bravado/fido_client.py
+++ b/bravado/fido_client.py
@@ -226,6 +226,7 @@ class FidoClient(HttpClient):
                 'url': string,
                 'timeout': float,  # optional
                 'connect_timeout': float,  # optional
+                'allow_redirects': bool,  # optional
             }
         """
 
@@ -244,7 +245,7 @@ class FidoClient(HttpClient):
             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
diff --git a/bravado/requests_client.py b/bravado/requests_client.py
index 6e2db22..eb1563e 100644
--- a/bravado/requests_client.py
+++ b/bravado/requests_client.py
@@ -266,6 +266,7 @@ class RequestsFutureAdapter(FutureAdapter):
         response = self.session.send(
             prepared_request,
             timeout=self.build_timeout(timeout),
+            allow_redirects=self.misc_options['allow_redirects'],
             **settings
         )
         return response
@@ -360,6 +361,7 @@ class RequestsClient(HttpClient):
         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:


Test patch:
diff --git a/bravado/testing/integration_test.py b/bravado/testing/integration_test.py
index ed1cca6..1931cdf 100644
--- a/bravado/testing/integration_test.py
+++ b/bravado/testing/integration_test.py
@@ -16,6 +16,7 @@ from msgpack import unpackb
 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.swagger_model import Loader
@@ -159,6 +160,17 @@ SWAGGER_SPEC_DICT = {
                 },
             },
         },
+        '/redirect': {
+            'get': {
+                'operationId': 'redirect_test',
+                'produces': ['application/json'],
+                'responses': {
+                    '301': {
+                        'description': 'HTTP/301',
+                    },
+                },
+            },
+        },
     },
 }
 
@@ -234,6 +246,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)
@@ -473,6 +493,43 @@ class IntegrationTestsBaseClass(IntegrationTestingFixturesMixin):
         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):
+        try:
+            # if running the default tests, the fido dependencies aren't loaded
+            # so rather than load dependencies unnecessarily, skip the test if that's not the case
+            # the coverage test incorrectly marks the exception handling as uncovered
+            # hence the pragma usage
+            from bravado.fido_client import FidoClient
+        except ImportError:  # pragma: no cover
+            pytest.skip('Fido dependencies have not been loaded, skipping test')  # pragma: no cover
+        else:
+            # the FidoClient doesn't have a way to turn off redirects being followed
+            # so limit this test to the RequestsClient instead
+            if isinstance(self.http_client, FidoClient):
+                pytest.skip('Following redirects is not supported in the FidoClient')
+
+        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):
+        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'
+
     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))
diff --git a/tests/requests_client/RequestsClient/separate_params_test.py b/tests/requests_client/RequestsClient/separate_params_test.py
index bc7db3e..3ff41ec 100644
--- a/tests/requests_client/RequestsClient/separate_params_test.py
+++ b/tests/requests_client/RequestsClient/separate_params_test.py
@@ -14,5 +14,6 @@ def test_separate_params():
         'connect_timeout': 1,
         'ssl_cert': None,
         'ssl_verify': True,
+        'allow_redirects': False,
         'timeout': 2,
     }
diff --git a/tests/requests_client/RequestsFutureAdapter/header_type_casting_test.py b/tests/requests_client/RequestsFutureAdapter/header_type_casting_test.py
index ea9bbce..c621b94 100644
--- a/tests/requests_client/RequestsFutureAdapter/header_type_casting_test.py
+++ b/tests/requests_client/RequestsFutureAdapter/header_type_casting_test.py
@@ -5,7 +5,15 @@ from bravado.requests_client import RequestsFutureAdapter
 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)