Cookies are not sent after redirect (even on same origin)

Checklist

The bug is reproducible against the latest release and/or master.
There are no similar issues or pull requests to fix it yet.

Describe the bug

When a GET request with cookies specified is issued, and the server returns a redirect, the subsequent request is not supplied with the cookies.

To reproduce

To observe the behavior, it's possible to use https://webhook.site/ to dump the request and https://bit.ly to provide a redirect. As the https://webhook.site/ URL is per session, you'll have to create both URLs yourself, but it only takes a minute. This is also reproducible on same-origin redirects, not only different-origin as demonstrated here. You can use a local server to check.

DIRECT_URL="" # your webhook.site URL
REDIR_URL="" # your bit.ly URL

import httpx
import asyncio

async def main():
    async with httpx.AsyncClient() as client:
        await client.get(DIRECT_URL, cookies={ 'cookie': 'direct' })
        await client.get(REDIR_URL, cookies={ 'cookie': 'redir' })

asyncio.get_event_loop().run_until_complete(main())

Expected behavior

One request should arrive with cookie=direct and the other with cookie=redir.

The correct behavior can be observed by using requests:

DIRECT_URL="" # your webhook.site URL
REDIR_URL="" # your bit.ly URL

import requests

requests.get(DIRECT_URL, cookies={ 'cookie': 'direct' })
requests.get(REDIR_URL, cookies={ 'cookie': 'redir' })

Actual behavior

One request arrives with cookie=direct and the other with no such cookie.

Environment

OS: Windows 10 (2004 update)
Python version: 3.9.0
HTTPX version: 0.16.1 (reproducible on both the lastest release and master branch)
Async environment: reproducible on both asyncio / trio
HTTP proxy: no
Custom certificates: no

In my tests, this can be worked around by passing the cookies to the client object itself (instead of the individual get calls).

Indeed yup, as we can see here when building redirect requests we only take into account client-level cookies, rather than also adding any cookies set on the request:

httpx/httpx/_client.py

Lines 402 to 414 in 9005bd5

    
               def _build_redirect_request(self, request: Request, response: Response) -> Request: 
        
                   """ 
        
                   Given a request and a redirect response, return a new request that 
        
                   should be used to effect the redirect. 
        
                   """ 
        
                   method = self._redirect_method(request, response) 
        
                   url = self._redirect_url(request, response) 
        
                   headers = self._redirect_headers(request, url, method) 
        
                   stream = self._redirect_stream(request, method) 
        
                   cookies = Cookies(self.cookies) 
        
                   return Request( 
        
                       method=method, url=url, headers=headers, cookies=cookies, stream=stream 
        
                   )

I guess we just ought to add cookies.update(request.cookies)…? PRs welcome. :-)

@florimondmanca

@florimondmanca I see that session cookie is passed in request cookie even after a logout scenario. Should we ignore that cookie specifically when re-setting cookies?

httpx/tests/client/test_redirects.py

Lines 384 to 387 in 8696405

    
           # Logout redirects to the homepage, expiring the session cookie. 
        
           response = client.post("https://example.com/logout") 
        
           assert response.url == "https://example.com/" 
        
           assert response.text == "Not logged in"

Here are some prints from _build_redirect_request for that test:

https://example.com/logout
SELF COOKIES:  <Cookies[]>
REQUEST COOKIES:  <Cookies[<Cookie session=eyJ1c2VybmFtZSI6ICJ0b21 for example.com />]>

So... I don't think the expected behaviour here is necessarily all that clear cut.

Session cookies are persisted. Per-call cookies=... won't be persisted.

Once you start digging into working with the client at different levels it's not really obvious how we'd make things work consistently, if we expected client.get("https://www.example.com", cookies=...) to persist the value of the cookies argument between redirects.

It's a bit awkward to explain why, but if we break it down to the two sub-calls...

request = client.build_request("GET", "https://www.example.com", cookies=...)
client.send(request)  # Any redirects will use whatever session cookies are appropriate for subsequent requests.
                      # We don't want to blindly continue to include whatever `Cookies` header was on the initial request,
                      # because it may have been expired by the response.

Or in other words, given that our existing redirect request building behaviour is this...

httpx/httpx/_client.py

Lines 489 to 491 in 9c7c2ac

    
           # We should use the client cookie store to determine any cookie header, 
        
           # rather than whatever was on the original outgoing request. 
        
           headers.pop("Cookie", None)

What would we expect to do instead of this?

Maybe there is a sensible behaviour here that works consistently at all layers, but it's not obvious to me.

One way to avoid this ambiguity could be to outright drop per-call cookies=..., and only ever allow session cookies?

Eg...

# Fine. Equivalent to `client = httpx.Client(cookies=...); client.get(...)`
httpx.get(..., cookies=...)

# Fine.
client = httpx.Client(cookies=...)
client.get(...)

# Fine. Cookie set everywhere.
client = httpx.Client()
client.cookies.set(name, value)
client.get(...)

# Fine. Cookie set for a single domain.
client = httpx.Client()
client.cookies.set(name, value, domain="...")
client.get(...)

# Drop this usage completely, because expected `Cookies` behaviour on redirects is ambiguous.
client = httpx.Client()
client.get(cookies=...)

(Note that we'd need a minor tweak in _api.py so that cookies argument in a single call is set on the one-shot client instance, rather than on the request call)

@tomchristie

@tomchristie Wouldn't the path of least resistance be to do what requests do, since the API looks so similar that any behavioral divergence from this (very popular) library would be inherently confusing?

@JanPokorny

@JanPokorny Astute remark. That's a baseline that we certainly aim to follow yup.

There are some differences once you drop into lower levels that sometimes have necessary implications further up the stack. I think we've got a clearer more consistent API once you start getting into the fine details. As a simple example, requests has separate Request and PreparedRequest models, that are used at different times, whereas our design ends up with just a single Request model.

Also in their design cookies ends up as an extra bit of state on the request, so requests with a COOKIES header, and requests with cookies=... set have different states, and possibly? behave differently in some circumstances. In our design there's no extra state - a request with cookies=... sets the COOKIES header, and that's that.

Coming back to the specifics of per-request cookies behaviour, here's a list of behaviours where I'd want to know exactly what requests does in order to help figure out if we think there's any valid UX changes to make here...

Are cookies persisted when a per-request cookies argument is used, for a same-domain redirect?
Are cookies persisted when a per-request cookies argument is used, for a cross-domain redirect?
Are cookies persisted when a per-request cookies argument is used, for a same-domain redirect with a response that expires the cookie?
Are cookies persisted when a per-request COOKIES header is used, for a same-domain redirect?
Are cookies persisted when a per-request COOKIES header is used, for a cross-domain redirect?
Are cookies persisted when a per-request COOKIES header is used, for a same-domain redirect with a response that expires the cookie?

I know the answer for all of these with httpx, which is always simply, "no". We persist client cookies only. Knowing the equivalent set of answers for requests would help identify exactly what behaviour they have, and help us determine what we think the most desired behaviour would be.

@tomchristie

@tomchristie The answers for your questions are, in order: yes, yes, yes, no, no no.

The answer actually surprised me. requests indeed specifically drops the Cookie header from subsequent requests, although it leaves other headers intact.

Here's code to test it.

Server:

import flask
app = flask.Flask(__name__)

@app.route('/r')
def r():
    return "yes" if "test" in flask.request.cookies else "no"

@app.route('/same_domain_redirect')
def same_domain_redirect():
    return flask.redirect("/r", code=302)

@app.route('/cross_domain_redirect')
def cross_domain_redirect():
    return flask.redirect("http://localhost:5000/r", code=302)

@app.route('/same_domain_redirect_expire')
def same_domain_redirect_expire():
    resp = flask.redirect("/r", code=302)
    resp.set_cookie('test', '', expires=0)
    return resp

if __name__ == '__main__':
    app.run()

Client:

import requests

print(requests.get("http://127.0.0.1:5000/same_domain_redirect", cookies={ "test": "test" }).content)
print(requests.get("http://127.0.0.1:5000/cross_domain_redirect", cookies={ "test": "test" }).content)
print(requests.get("http://127.0.0.1:5000/same_domain_redirect_expire", cookies={ "test": "test" }).content)
print(requests.get("http://127.0.0.1:5000/same_domain_redirect", headers={ "Cookie": "test=test" }).content)
print(requests.get("http://127.0.0.1:5000/cross_domain_redirect", headers={ "Cookie": "test=test" }).content)
print(requests.get("http://127.0.0.1:5000/same_domain_redirect_expire", headers={ "Cookie": "test=test" }).content)

I have created an issue in requests to ask about this behavior. (psf/requests#5714)

As for the behavior of requests, I have learned that "sensitive" HTTP headers passed directly are scraped from subsequent requests, but direct arguments are not (cookies, auth, ...). Also the contributor who replied to me believes that having the cookies argument was a bad choice. I believe that httpx should either follow this behavior exactly, or drop the cookies argument / make some other obvious breaking change (like throwing an exception when cookies argument is passed and a redirect occurs). Just don't have it like it is now, eg. potentially hard-to-notice behavioral difference.

To be clear, I think having the cookies argument be a plain dictionary without information about each cookie as would have been received from Set-Cookie is the biggest security nightmare awaiting an unsuspecting user who's not familiar with the security implications.

@sigmavirus24

Okay, so the narrowest tack onto this for us would be:

Drop client.get(..., cookies=...), instead enforcing setting cookies at the client level.
Switch the implementation of single-call API httpx.get(..., cookies=...) so that it's setting cookies on the client, not on the call.

The benefit there is that the behaviour is less ambiguous wrt redirects. If you're setting cookies on the client, then it's much more clear if you're setting them on a particular domain, or if you're setting them globally. (We'd still have the simple, lazy style single-call functions, which would persist cookies on redirects after this change, since they'd be set on the client.)

This doesn't deal with @sigmavirus24's point about global cookies being a very blunt approach, since we still support simple ol' formulations like httpx.Client(cookies={...}), in addition to the more careful cookies.set(name, value, domain="..."). I'd be perfectly willing for us to consider that if it has advocates? I think we'd want to treat any discussion on that independently, since it has a much wider scope.

I'm going to tentatively milestone this for 0.17, which I assume will be released next week. I don't think we necessarily need to act on it, and besides it'd be good to have a little more consensus on it first, but?...

@tomchristie

@tomchristie Do I understand correctly that the variant "so that it's setting cookies on the client, not on the call" means that httpx.get(..., cookies=...) would mutate the client and save the cookies there for all subsequent calls? (ie. calling httpx.get(..., cookies=...) and then httpx.get(...) would pass the same cookies both times?)

@JanPokorny

@JanPokorny No I think that's just an internal implementation note, the behavior stays the same: httpx.get(...) doesn't maintain cookies across calls. The throwaway client used inside that call does, but it's thrown away when httpx.get() returns. :)

Navigation Menu

Repository navigation

Cookies are not sent after redirect (even on same origin) #1404

Checklist

Describe the bug

To reproduce

Expected behavior

Actual behavior

Environment

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	def _build_redirect_request(self, request: Request, response: Response) -> Request:
	"""
	Given a request and a redirect response, return a new request that
	should be used to effect the redirect.
	"""
	method = self._redirect_method(request, response)
	url = self._redirect_url(request, response)
	headers = self._redirect_headers(request, url, method)
	stream = self._redirect_stream(request, method)
	cookies = Cookies(self.cookies)
	return Request(
	method=method, url=url, headers=headers, cookies=cookies, stream=stream
	)

	# Logout redirects to the homepage, expiring the session cookie.
	response = client.post("https://example.com/logout")
	assert response.url == "https://example.com/"
	assert response.text == "Not logged in"

	# We should use the client cookie store to determine any cookie header,
	# rather than whatever was on the original outgoing request.
	headers.pop("Cookie", None)

Cookies are not sent after redirect (even on same origin) #1404

Description

Checklist

Describe the bug

To reproduce

Expected behavior

Actual behavior

Environment

Activity

craigholm commented on Dec 5, 2020

florimondmanca commented on Dec 6, 2020

aalvrz commented on Dec 15, 2020

lovelydinosaur commented on Jan 6, 2021

JanPokorny commented on Jan 6, 2021

lovelydinosaur commented on Jan 7, 2021

JanPokorny commented on Jan 7, 2021

JanPokorny commented on Jan 7, 2021

JanPokorny commented on Jan 7, 2021

sigmavirus24 commented on Jan 7, 2021

lovelydinosaur commented on Jan 8, 2021

lovelydinosaur commented on Jan 8, 2021

JanPokorny commented on Jan 8, 2021

florimondmanca commented on Jan 8, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions