fix core: storing all cookies without possibly wrong selection, keeping session cookies semantics

[alex-aparin]

As mentioned previously in issue #1182. The current implementation of cookie storage in clients::http::response does not conform to RFC 6265 (it can incorrectly write cookies). Additionally, the accessor methods for Max-Age/Expires have default values, which is also incorrect (breaking the semantics for Session Cookies). The goal of this PR is primarily to fix the incorrect cookie writing, but without a full-fledged implementation of CookiesJar.

closes: #1182

---

Note: by creating a PR or an issue you automatically agree to the CLA. See CONTRIBUTING.md. Feel free to remove this note, the agreement holds.

[alex-aparin]

UPD: I have noticed some updates in master branch like this. Please note that just overwriting cookie with the same name is not enough, you should take into account expire/max age and domains/urls, because in some cases we should delete cookie at all or just keep additional cookie with the same name but for another domain/url. That's why I am just recording all cookies without any actions (without implementing full cookiejar)

[Malevrovich]

Hi, and thanks for the PR!

I’m not fully comfortable with the idea of putting all cookies into a vector. At the very least, this changes the public API, and I’m pretty sure many users rely on the current way cookies are exposed and read.

Would you be interested in properly supporting the cases that don’t fully comply with the RFC instead? I’d be happy to review such a change and help get it merged.

I’d prefer to avoid moving everyone from a map to a vector now, only to potentially move them back to a map later.

[alex-aparin]

Hi all. Thank you for response. There was hot discussion in userver's telegram group about breaking changes, I'll make this pr as draft, additional tests for tricky cookies will be added. I think the better way is to add additional method to allow user to specify - use old but wrong api with the same semantics (map of cookies) or use new method which will return CookieJar with needed getters (some kind of map of map (or map of list) can be used internally)

[alex-aparin]

This version can be viewed as starting point for API and tests for CookieJar compliable with RFC 6265.
Resume of commits:

1. Reverted changes. New api is compatible with current one
2. Implemented CookieJar, it now primarly used for receiving cookies from server. It has also simple api (but not so effective in terms of parsing urls) in order to SEND cookies, we need to think about API to set this to http client.
3. Added tests to covert tricky cookies.

This was not so easy as I thought and a little bit messy from my implementation, but I hope storage covers many issues, any help will be greatly appreciated. Thank you!

UPD: I have updated code due paragraph 2. Now client partially supports sending cookies from cookie jar, but we need to think where to store cookiejar, response object can be null before sending request. HttpClient.CookiesFromServerCookieJar test demonstrates the problem (it fails at runtime)

UPD2: Now client fully supports sending cookies from cookie jar. HttpClient.CookiesFromServerCookieJar test is working

[alex-aparin]

Yet another improvement, now http client fully supports sending cookies from cookies jar and also receiving. Some bugs were also fixed. At the moment APIs of cookie jar and http client are stabilized. I recommend the latest code snapshot to get familiar with. Maybe tests will be polished a little bit later

[Malevrovich]

I have reviewed the PR, and this topic is even more complex than I expected when we discussed it in the chat.

At this point, I am concerned about the RFC deviations and the additional shared_ptr allocation on every request. I would also like to study the redirect behavior in more detail: what happens if an intermediate redirect hop returns a Set-Cookie header? Is that cookie applied to the next hop? Another question is whether cookies whose Expires time has already passed can still be sent in a request.

I am not asking you to implement any changes right now. These are the areas I would like us to keep in mind for the next review iteration. I would appreciate some more time to investigate the topic, and I will come back with implementation ideas.

[Malevrovich]

Could we avoid locking these deviations in as intended behavior and fix them before merging?

We should not knowingly violate RFC 6265 here, especially for a new CookieJar API that is presented as RFC-compliant.

[alex-aparin]

Thank you for review!

Now about points:

1. About path matching and test - I have already updated path-matching algorithm with improved test to highlight the case strict rfc accordance
2. About expired cookies, actually that's good question. At the moment expired cookies are filtered only at Request's setter method (at the same time we get currently set url) , in the case of delay request or updated url there is oportunity we get obsoleted cookies. I think we should set cookies immediately before transfer, not in setter methods
3. About redirect and hops,good catch. At the moment I cannot confidently say (As I remembder RFC did not mention this explicitly). I think in the case of any non tunnelling proxy cookies will be transfered as is. The case of any redirection or tunneling proxy was totally missed by me. I need time to work through/learn such cases and cover it by tests. I suspect cookie jar will not work properly for them at all.
4. About std::shared_ptr, There is code related for reusing request_state, and to exclude repetetive copying cookie har to response:

    SetBaggageHeader(easy());

    response_ = std::make_shared<Response>();
    response_->set_cookie_engine(cookies_engine_);
    response_->SetStatusCode(Status::kInvalid);

Not sure whether it's good design to store copy of cookie jar within request state, Many implementations (like go) allow to inject reference to cookie jar (thread safe btw) to be allowed shared between many parallel requests. I will be glad to hear design wishes about it. To make more concrete code correction.