[fix] don't use AHC cookie store

[bomgar]

The WSRequest withCookies method suggests all cookies will be
discarded. This is not the case if the underlying http client has a
cookie store. Play ws also does not know about those cookies so for
example the AhcCurlRequestLogger does not log these cookies.

[marcospereira]

LGTM.

This also highlights that not all the new configurations available in AHC are available for play-ws. We should review that.

[marcospereira]

Backport to 2.0.x: b292607

[marcospereira]

I will revert this since it has a regression. The current behavior without this patch is to add the cookies when following a redirect automatically, but we lose this behavior when applying the patch because it disables CookieStore completely.

A better approach then should be to add a configuration that enables or disables it accordingly to what the user wants. As far as I can tell, there is no way to configure the CookieStore at the request level. See discussion here: AsyncHttpClient/async-http-client#1565

[bomgar]

Please don't. The behavior with the cookie store is horrible. The documentation of the WS api implies that everything is stateless. Also things like the AhcCurlRequestLogger expects all cookies to be part of the WS request. State in the underlying client is hard to understand and predict. If it were possible to activate this on a request level it might be ok but should never be the default behavior.

[marcospereira]

The idea is to make it configurable, although I still investigating if it is possible to do that without breaking binary compatibility. But the behavior with this patch is breaking Play tests (some that haven't changed in some time). See the build here:

https://travis-ci.com/playframework/playframework/jobs/185425206#L3218-L3241

So, no having the cookies sent when following a redirect is also not good and unexpected.

If it were possible to activate this on a request level it might be ok but should never be the default behavior.

There is no way to activate it on request level right now since this is not available in AHC. The default cannot be the best for your application, but it is hard to say that it should never be the default. Anyway, by making it configurable, users can decide by themselves, but the default will be on since our tests are relying on it.

[bomgar]

I noticed this behavior with the upgrade from play 2.6 to 2.7
It did behave as expected with 2.6

[bomgar]

You should at least update the documentation of addCookie and withCookie to inform users about this.

[mriehl]

@marcospereira if this is the default it should have a security notice IMHO, if I understand it correctly then performing some authentication with play-wsclient that results in a set-cookie header will leak the cookie to every subsequent call to that domain which is like.. terrible?

We noticed this change because a test was sending auth cookies that should not even be there, just because there was a test before that got a set-cookie reply. And since it's mutable state, it could bite you anywhere. I understand getting a set-cookie in a WS library is not that common or sensible, but the implications of silently leaking that cookie to every call to that domain are... not great. Even worse that when you call discardingCookies it's silently ignored.

TBH I would never expect this from a library. I mean reusing cookies if you have some local request thingy that can fire subsequent requests is nice, but this is on a "shared" (in most play apps) client...

[marcospereira]

Hi @mriehl,

Thanks for joining the conversation.

@marcospereira if this is the default it should have a security notice IMHO, if I understand it correctly then performing some authentication with play-wsclient that results in a set-cookie header will leak the cookie to every subsequent call to that domain which is like.. terrible?

Could you better elaborate on why do you consider this to terrible? Isn't this exactly what browsers do?

We noticed this change because a test was sending auth cookies that should not even be there, just because there was a test before that got a set-cookie reply. And since it's mutable state, it could bite you anywhere. I understand getting a set-cookie in a WS library is not that common or sensible, but the implications of silently leaking that cookie to every call to that domain are... not great.

That is why my proposal here is to offer a configuration and let users decide based on their application needs. My idea to keep on as the default is to:

1. Respect the tests that we have in Play since 2.6.x (which means we are respecting the behavior since these tests exist)
2. Respect the behavior we have for 2.7.x since we usually avoid to change it in minor releases (such as 2.7.1)

Even worse that when you call discardingCookies it's silently ignored.

Which discardingCookies are you referring to here? There is no such thing in play-ws, but maybe you are talking about what is documented in withCookies. If so, I agree that no matter which configuration is set, it should consider the underlying cookie store.

[bomgar]

Could you better elaborate on why do you consider this to terrible? Isn't this exactly what browsers do?

Browsers are not used by multiple users. The play ws client is used for calls from different users. Sharing cookies between those is definitely something that should never happen.
We use the WS client to forward the user's auth token to some of our other systems. There should never be shared state between them. Disabling the cookie store using the configuration is a good solution. I still think enabling it by default is not a good way to handle this.

[mriehl]

Which discardingCookies are you referring to here? There is no such thing in play-ws, but maybe you are talking about what is documented in withCookies.

You are right, I have this confused with the play result. I mean withCookies, which is supposed to send no cookies except the ones given.

Could you better elaborate on why do you consider this to terrible? Isn't this exactly what browsers do?

You are right, but I think a WS library is about user control. In my browser it's hard to send a request manually, most of it is done transparently. Having a WS library decide when to send what cookie violates my assumption about the builder pattern that it starts "from scratch", if that makes sense?
This also allows servers to perform cookie based fingerprinting of play ws clients.

I feel like there is a solution somewhere that both keeps redirect cookie backcompat and doesn't leak cookies. Since before 2.7 there was no cookie leak but follow redirects was working. I'll dig in the ahc api a bit more.

[mriehl]

One example where this new behavior is terrible would be writing a http proxy with play. Do you expect that when you log in to facebook through awesomeproxy.dev the next user to access it will have your session cookie too?

[marcospereira]

I feel like there is a solution somewhere that both keeps redirect cookie backcompat and doesn't leak cookies. Since before 2.7 there was no cookie leak but follow redirects was working. I'll dig in the ahc api a bit more.

I appreciate that. If there is such a thing I will more than happy to accept a pull request. Are you working on this now?

One example where this new behavior is terrible would be writing a http proxy with play.

Agree with that. Thanks for better explaining your use case.

[mriehl]

I appreciate that. If there is such a thing I will more than happy to accept a pull request. Are you working on this now?

Yes and no :/ unfortunately the 30x interceptor in AHC completely relies on the cookie store to keep the cookies set during a redirect. And since the cookie store is shared across threads I don't see a good way to customize it.

I think a good possibiliry would be to honor setFollowRedirects in play-ws itself, for a redirect one just has to send the cookies from the redirect response headers, no need for a cookie store. Alternatively if there is a way to override the ahc 30x interceptor that would be even better since this removes the need to have redirect logic in play ws (especially the counting down redirects to failure part is annoying).

I don't know what the best course is. Based on the AHC issue you linked, the optimal solution would be a per-request cookie store, since this would work for redirects without sharing any state with other requests. But I don't know if they are working on it. Clearly the simplest solution is the configuration you suggested, but with the aforementioned drawbacks.

[marcospereira]

Another possible solution is to turn cookie store off and handle redirects in play-ws, following the semantics defined in async-http-client, but keeping when doing the redirect.

[marcospereira]

@mriehl & @bomgar see #313.

[fedork]

@bomgar what is the situation with play 2.5? I assume it behaves correctly (no cookie store)? Can you confirm?