Disable following of redirects by default #454

mattdowdell · 2020-03-19T12:52:48Z

Per #449, redirects should not be silently followed when making requests. This disables that behaviour by default in the synchronous client, and also modifies the responses to no longer consider 3XX HTTP status errors. There's perhaps some nuance to whether all of them are applicable in API responses, but RFC 7231 does not appear to consider these to be errors anyway.

I've left in the exception classes for redirects and make_http_exception is untouched, largely because I wasn't sure if that was considered part of the public API or not. I've also added an integration test and bench tested using my code that originally found the issue and confirmed that it works as expected for my use case.

Fixes #449.

coveralls · 2020-03-19T12:57:02Z

Coverage increased (+0.05%) to 96.344% when pulling 0c0d475 on mattdowdell:nofollow-redirects into 2e4c1f9 on Yelp:master.

mattdowdell · 2020-03-19T13:14:47Z

@macisamuele Are you able to review?

Also, do you understand the failure in CI for mypy? I don't understand why attributes have gone missing.

macisamuele

@mattdowdell thanks a lot for contributing to the project.

I would suggest to add an additional test to ensure that redirects are followed as well as ensuring that requests and fido clients will behave consistently.

macisamuele · 2020-03-19T15:28:25Z

bravado/http_future.py

@@ -199,7 +199,7 @@ def response(

            swagger_result = self._get_swagger_result(incoming_response)

-            if self.operation is None and incoming_response.status_code >= 300:
+            if self.operation is None and incoming_response.status_code >= 400:


I would not change this behavior (at least on this PR) as users might rely on this.

You're right to mention that RFC7231 does not consider them errors.
As this is independent from the specific issue you're trying to solve (following redirects) I would suggest to create an issue for this and eventually publish a new PR.
NOTE: changing this (so 3xx do not raise exception) requires a major version bump

Done. Instead I check the response stored in the exception for the relevant information.

Raised #455 to track not raising exceptions when redirects are returned.

macisamuele · 2020-03-19T15:40:23Z

bravado/testing/integration_test.py

@@ -473,6 +492,16 @@ 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):


I would expect a test that ensures that redirects are followed as well.

Done, but only for the RequestsClient as it doesn't look like the FidoClient supports the ability to follow redirects.

~~I think that we should be doing a small effort here to ensure that the two clients behave the same here.~~
~~I'll try to look for pointers to help you out here~~

Fido does not really support following redirects. I would say that at this point is ok to allow following redirects as best-effort. Eventually we might just want to remove the redirect support completely.

That was the conclusion I came to. The logigcal place to add support for following redirects in Fido, would be in prepare_request_for_twisted. The retuened value from that in turn goes into fido.fetch which attempts to look like the interface for requests, but isn't fully implemented (as demonstrated by the lack of allow_redirects parameter. For the record, I have no clue how the call to fido.fetch works in fido_client.py as it seems to be missing an import.

In summary, the effort required to make fido work with redirects is much more significant that getting requests to disable redirects by default. Given the bug I initially found where following redirects causes bravado to complain about unexpected HTTP statuses, I'd say that support for following redirects at all should probably be removed in the future or it should handle redirects at a higher level, e.g. response returns redirect, so now you should expect the response(s) from the target of the redirect. As not all redirects have a single target according to the spec, that gets even more complicated to handle at a library level, so I'd argue that's an implementation decision for the user of bravado to deal with.

bravado/testing/integration_test.py

…n a normal test run

mattdowdell · 2020-03-23T08:06:10Z

@macisamuele Is there anything else you'd like me to address before this gets merged in?

macisamuele · 2020-03-21T17:36:16Z

bravado/requests_client.py

@@ -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


Is unclear to me why this parameter has been removed.

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

macisamuele · 2020-03-21T18:17:16Z

bravado/testing/integration_test.py

+        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")


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'

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

Changed to that method instead and removed the coverage ignore.

macisamuele

Changes looks good to me.

Pinging @sjaensch to double check the PR and if all looks good to merge this.

sjaensch

Looks good to me! I'm requesting one minor change, as we do use (and run) those integration tests for bravado-asyncio too.

sjaensch · 2020-03-25T13:19:22Z

bravado/testing/integration_test.py

+        # 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')


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.

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.

sjaensch · 2020-03-27T20:53:02Z

Thanks for your efforts @mattdowdell!

sjaensch · 2020-03-30T14:56:34Z

This has has been released (with a slightly different name for the option, see #457) in bravado 10.6.0.

matthew-dowdell added 3 commits March 19, 2020 09:19

Add test to demonstrate issue

55406ad

disable following of redirects by default

51cb83c

fix in another spot

85a19eb

macisamuele reviewed Mar 19, 2020

View reviewed changes

matthew-dowdell added 6 commits March 19, 2020 18:13

fixes for PR comments

15b8013

correct docs

83b1ac5

rework to allow old redirect behaviour in RequestsClient

1b8a92b

rm unused variable assignment

ee3582b

tidying

28a8061

add pytest.fail to coverage exclusions as it should never be called o…

6fb8ead

…n a normal test run

mattdowdell mentioned this pull request Mar 20, 2020

Redirects should not be considered errors #455

Open

macisamuele reviewed Mar 23, 2020

View reviewed changes

get the exception out of pytest.raises instead

6f424af

macisamuele approved these changes Mar 24, 2020

View reviewed changes

sjaensch suggested changes Mar 25, 2020

View reviewed changes

blacklist fidoclient instead

0c0d475

sjaensch approved these changes Mar 27, 2020

View reviewed changes

sjaensch merged commit 41caefc into Yelp:master Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable following of redirects by default #454

Disable following of redirects by default #454

mattdowdell commented Mar 19, 2020 •

edited

Loading

coveralls commented Mar 19, 2020 •

edited

Loading

mattdowdell commented Mar 19, 2020

macisamuele left a comment

macisamuele Mar 19, 2020

mattdowdell Mar 19, 2020

mattdowdell Mar 20, 2020

macisamuele Mar 19, 2020

mattdowdell Mar 19, 2020 •

edited

Loading

macisamuele Mar 21, 2020 •

edited

Loading

mattdowdell Mar 23, 2020

mattdowdell commented Mar 23, 2020

macisamuele Mar 21, 2020

mattdowdell Mar 24, 2020

macisamuele Mar 21, 2020

mattdowdell Mar 24, 2020

macisamuele left a comment

sjaensch left a comment

sjaensch Mar 25, 2020

mattdowdell Mar 27, 2020

sjaensch commented Mar 27, 2020

sjaensch commented Mar 30, 2020

Disable following of redirects by default #454

Disable following of redirects by default #454

Conversation

mattdowdell commented Mar 19, 2020 • edited Loading

coveralls commented Mar 19, 2020 • edited Loading

mattdowdell commented Mar 19, 2020

macisamuele left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdowdell Mar 19, 2020 • edited Loading

Choose a reason for hiding this comment

macisamuele Mar 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdowdell commented Mar 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macisamuele left a comment

Choose a reason for hiding this comment

sjaensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjaensch commented Mar 27, 2020

sjaensch commented Mar 30, 2020

mattdowdell commented Mar 19, 2020 •

edited

Loading

coveralls commented Mar 19, 2020 •

edited

Loading

mattdowdell Mar 19, 2020 •

edited

Loading

macisamuele Mar 21, 2020 •

edited

Loading