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

Cookie changes are not persisted when test client follows redirects #1491

Closed
javivdm opened this issue Mar 27, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@javivdm
Copy link

commented Mar 27, 2019

I have a pytest test from a flask application that looks like:

def test_logout_redirect(client, login, credentials_admin):
    with client as c:
        login(c, credentials_admin)
        r = c.get(url_for('main.logout'), follow_redirects=True)
        assert r.status_code == 200
        assert request.path == url_for('main.login')
        assert b'Log In' in r.data

When the user logs out, the session is cleared and it redirects them to the login page. The log in route checks if the session has a user id, and if so it redirects them to the homepage.

Therefore the test is trying to assert that the session is cleared after logging out and the user is redirected to the login page. This test was passing in 0.14.1.

However after upgrading the test fails. It seems like the user ID is back in the session after it has been cleared and therefore the user ends up in the homepage.

I am encountering this behaviour only when using the test client, which I am invoking with flask's app.test_client().

@davidism davidism changed the title [0.15] Different behaviour in the test client redirection - session not cleared Cookie changes are not persisted when test client follows redirects Mar 28, 2019

@davidism

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

This is due to #1402, which was improving the way redirects are handled by the test client.

builder = EnvironBuilder.from_environ(environ, query_string=qs)

It now copies the original environ in order to preserve the headers passed with the original request (which is what browsers do), but it doesn't take into account that Set-Cookie needs to be handled too, so instead of the current cookies, it uses the ones from the original request.

@davidism davidism added the bug label Mar 28, 2019

@davidism davidism added this to the 0.15.2 milestone Mar 28, 2019

@davidism

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

def inject_wsgi(self, environ):
"""Inject the cookies as client headers into the server's wsgi
environment.
"""
cvals = []
for cookie in self:
cvals.append("%s=%s" % (cookie.name, cookie.value))
if cvals:
environ["HTTP_COOKIE"] = "; ".join(cvals)

The cookie jar is only modifying the environ if there are cookies. So if the jar is empty, it doesn't ensure that the environ's cookies are cleared. It looks like that if cvals line was there from the beginning, and there was no test for deleting cookies across requests. It was working by accident because the redirect request started empty until #1402.

Here's a Werkzeug test:

def test_cookie_across_redirect():
    @Request.application
    def app(request):
        if request.path == "/":
            return Response(request.cookies.get("auth", "out"))

        if request.path == "/in":
            rv = redirect("/")
            rv.set_cookie("auth", "in")
            return rv

        if request.path == "/out":
            rv = redirect("/")
            rv.delete_cookie("auth")
            return rv

    c = Client(app, Response)
    assert c.get("/").data == b"out"
    assert c.get("/in", follow_redirects=True).data == b"in"
    assert c.get("/").data == b"in"
    assert c.get("/out", follow_redirects=True).data == b"out"
    assert c.get("/").data == b"out"

If you add an extra cookie during login, and don't delete it during logout so the jar isn't empty, you can see that logout works again.

I think the correct solution here is to add:

else:
    environ.pop("HTTP_COOKIE", None)
@javivdm

This comment has been minimized.

Copy link
Author

commented Mar 28, 2019

I tested it with a non empty cookie jar and then, indeed, the cookie changes persisted and the test passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.