Missing cookies on redirect

[lukasschlueter]

Hi,

while doing some initial tests, I noticed that cookies being sent in a 302 response are not getting set.
This does not seem to be correct and was quite confusing for me.

Here's a sample test case:

fn main() {
	let mut session = user_agent::ReqwestSession::new(reqwest::Client::new());
	let mut response = session.get("https://httpbin.org/cookies/set/test/123").unwrap();
	assert_eq!(response.url().as_str(), "https://httpbin.org/cookies");
	assert_eq!(response.text().unwrap(), "{\n  \"cookies\": {\n    \"test\": \"123\"\n  }\n}\n")
}

[pfernie]

Redirects are handled in reqwest itself; from a brief look at the handling here, there is no effort to parse/handle the Set-Cookie header from the 302 response, which makes sense as reqwest is relatively cookie-agnostic. By the time user_agent sees the response, it is the final response after the redirect has occurred, so will have missed the Set-Cookie header.

I don't believe the current reqwest flow allows for us to handle externally. The custom RequestPolicy mechanism only exposes the original and redirect Urls, and even if it gave us Request/Response objects probably would not play well with users trying to define their own RedirectPolicy simultaneously (would need some policy chaining?).

So I believe this would require a change in reqwest, but simply implementing direct handling of Set-Cookie in reqwests redirect mechanism probably does not suffice. In the provided example, the Set-Cookie received in the 302 Response should be applied, and the outgoing redirected Request then (should) contain the appropriate Cookie header. However, that intermediate Set-Cookie would not be seen/stored by the user_agent (or any external) cookie store, so would effectively be lost for the session, just present in the particular redirected request. So this specific test case w/ httpbin would be fixed (the assert would pass), but the general user_agent session behavior would not; a subsequent GET in the same session of https://httpbin.org/cookies would be missing the cookie.

@seanmonstar has an open issue in reqwest of event hooks, which might be appropriate here. But that is a more general mechanism, this particular case might be more easily addressed via either:

1. A hook mechanism specific to redirect actions
2. Implementing the reqwest redirect functionality to parse Set-Cookie and set Cookie headers in the redirect logic, but to also accumulate such Set-Cookie headers through the lifecycle of the redirect and including them in the final Response object so they are visible to external cookie stores.

The second option seems more straightforward, but I am uncertain if such behavior (passing on all seen Set-Cookies) would have negative security consequences. Or at the least if maintaining ordering of the Set-Cookies encountered (if multi-hop redirect) would be a requirement. Existing reqwest redirect handling does at least try to mitigate CSRF, but currently would only concern itself with protecting Cookies set in the original request. I am not sure what the correct semantics should be w.r.t. Cookies received during a redirect lifecycle.

Closing this as a user_agent issue for the time being.

[pfernie]

Actually, I will leave this open to keep the problem visible as I agree it is surprising behavior for the crate.

[pfernie]

I updated a relevant issue on reqwest to link here as well.

[pfernie]

reqwest has introduced support for using a cookie_store in v0.9.14. A recent PR seanmonstar/reqwest#514 addresses the redirect issue now reqwest is cookie_store aware.

[lukasschlueter]

Thanks a lot for the updates!