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

httpbin_secure: fix redirect Location to have "https://" scheme #62

Conversation

immerrr
Copy link
Contributor

@immerrr immerrr commented Nov 14, 2021

This PR fixes the problem with secure server that had a missing feature in the part where HTTPS connection was terminated: it was not setting HTTPS WSGI environment variable as required by wsgiref.util.guess_scheme. As a result, the server when trying to autogenerate an absolute URL for Location header would end up with something like:

Location: http://127.0.0.1:${HTTPS_PORT}/${RELATIVE_PATH}

That would result in an error down the road with some HTTP clients, e.g. requests, where it would try to follow a redirect opening a plaintext HTTP connection to a port that expected HTTPS and it resulted in the following error:

pytest-httpbin server hit an exception serving request: [SSL: HTTP_REQUEST] http request (_ssl.c:1125)

Ultimately, it started breaking CI builds for vcrpy

@immerrr
Copy link
Contributor Author

immerrr commented Dec 22, 2021

@kevin1024 please let me know if there's anything i can do to help get this in

@kevin1024
Copy link
Owner

Thanks for the ping! Hopefully I’ll have some time over the holidays to figure out how to do a release again

@kevin1024 kevin1024 merged commit 4fb88af into kevin1024:master Dec 25, 2021
@AdamWill
Copy link

The test from this PR is failing when we try to build pytest-httpbin 1.0.2 for Fedora, and I'm not sure why. It fails like this:

=================================== FAILURES ===================================
______________ test_redirect_location_is_https_for_secure_server _______________
httpbin_secure = <pytest_httpbin.serve.SecureServer object at 0x7fff9625e2f0>
    def test_redirect_location_is_https_for_secure_server(httpbin_secure):
        assert httpbin_secure.url.startswith('https://')
        response = requests.get(
            httpbin_secure + "/redirect-to?url=/html",
            allow_redirects=False
        )
        assert response.status_code == 302
        assert response.headers.get('Location')
>       assert response.headers['Location'].startswith('https://')
E       AssertionError: assert False
E        +  where False = <built-in method startswith of str object at 0x7fff96116eb0>('https://')
E        +    where <built-in method startswith of str object at 0x7fff96116eb0> = '/html'.startswith
tests/test_server.py:105: AssertionError

which I think is telling us that the string it expects to start with "https://" is actually just "/html". But I'm not sure why that would be. We don't patch pytest-httpbin at all, our build is straight from the tarball. We have backported a couple of patches for httpbin itself - postmanlabs/httpbin#649 and postmanlabs/httpbin#555 - but I don't think either of those should affect this. Any ideas?

@AdamWill
Copy link

OK, so now I think I know why. This PR is actually written against broken httpbin behaviour, see postmanlabs/httpbin#647 and postmanlabs/httpbin#673 . httpbin intends to tell werkzeug not to rewrite the Location header at all, by setting autocorrect_location_header = False. But the way it does this stopped working with werkzeug 2.x, and so with current stock httpbin and earlier werkzeug 2.x releases, we started getting Location headers that were rewritten by werkzeug to be absolute. The test case in this PR is actually relying on that (unintended) behaviour, when what httpbin intends is for the header to be relative. With newer versions of werkzeug, BaseResponse has gone away entirely and httpbin just doesn't work any more, that's what postmanlabs/httpbin#673 is about.

I am patching httpbin in Fedora downstream to fix this problem, so our httpbin behaves as it's intended to, which results in this test failing.

I guess the fix might just be to set autocorrect_location_header back to True just for this test, if we can do that. That should result in the behaviour the test expects. The open question is whether the "fix" this PR implemented is actually necessary at all if httpbin behaves as it intends to...

@AdamWill
Copy link

ugh, doesn't look like we can just import Response inline in the test. Well, we can, but it doesn't change the behaviour. Not sure if there's any other way to do it. :|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants