Reactive HttpComponentsClientHttpResponse ignores Expires cookie attribute

[renetrefft]

A cookie has the optional attributes Expires and Max-Age. If both are set, the latter takes precedence.

In method adaptCookies() of class org.springframework.http.client.reactive.HttpComponentsClientHttpResponse each HttpComponents Cookie is converted to a Spring ResponseCookie.

Given is an HttpComponents Cookie which has set Expires only, since adaptCookies() just reads Max-Age from Cookie, the resulting ResponseCookie has lost its expiration information.

Note: I have also checked ReactorClientHttpResponse, and it is not affected. Netty internally converts Expires to Max-Age. Thus, its Cookie class supports Max-Age only.

[sbrannen]

Good catch.

ResponseCookie#toString() actually includes Expires if Max-Age is set, but you're right: if Expires is set without Max-Age, then the Expires attribute is not included in the generated Cookie string.

So, in general, it's org.springframework.http.ResponseCookie as well as ResponseCookieBuilder that do not have support for a stand-alone Expires attribute.

[imvtsl]

@sbrannen
I reproduced this issue here.

Netty: io.netty.handler.codec.http.cookie.ClientCookieDecoder.mergeMaxAgeAndExpires() merges Max-Age and Expires into Max-Age. Netty merges these two attributes using logic defined in RFC 6265. Point 3 of section 5.3 in this RFC gives higher precedence to Max-Age over Expires. And org.springframework.http.client.reactive.ReactorClientHttpResponse.getCookies() only considers maxAge which is okay for this case as Max-Age and Expires is already merged internally by Netty. So, no issues in org.springframework.http.client.reactive.ReactorClientHttpResponse.

Apache: Unlike Netty, Apache is not merging max age and expires. Since org.springframework.http.client.reactive.HttpComponentsClientHttpResponse is an implementation of Apache HttpComponents HttpClient 5.x, I believe it's adaptCookies() method should process both Max-Age and Expires in ResponseCookie just like Apache.

For fix, we can add Expires attribute to ResponseCookie. And in case of org.springframework.http.client.reactive.HttpComponentsClientHttpResponse.adaptCookies(), we set expires attribute if present.

Can I go ahead and raise a PR with this?

[sbrannen]

Hi @imvtsl,

We discussed this within the team and decided to update the logic in org.springframework.http.client.reactive.HttpComponentsClientHttpResponse.adaptCookies(HttpClientContext) to set maxAge() in the ResponseCookie to the converted value of the Expires attribute if Expires is present and Max-Age is not present, and we would otherwise process Max-Age as we currently do, thereby giving precedence to Max-Age.

We will also need to ensure that we have Max-Age/Expires tests in place for all org.springframework.http.client.reactive.ClientHttpResponse implementations -- for example, HttpComponentsClientHttpResponse, JettyClientHttpResponse, ReactorClientHttpResponse, and ReactorNetty2ClientHttpResponse.

If any of those other implementations lack support for handling Max-Age and Expires as previously stated, we would need to update their processing logic as well.

If you would like to submit a PR for this, feel free to do so. Otherwise, I will get to it in the coming weeks.

Cheers,

Sam

[imvtsl]

Hey @sbrannen
Thanks for the response. I will raise a PR with this approach and add tests for all org.springframework.http.client.reactive.ClientHttpResponse implementations.

Out of curiosity, why do we deviate from the approach of Apache HttpComponents HttpClient 5.x where they do not merge Max-Age and Expires. Is it to maintain uniformity across all org.springframework.http.client.reactive.ClientHttpResponse implementations?

[sbrannen]

Hi @imvtsl,

Hey @sbrannen Thanks for the response.

You're very welcome.

I will raise a PR with this approach and add tests for all org.springframework.http.client.reactive.ClientHttpResponse implementations.

That would be much appreciated. 👍

For fix, we can add Expires attribute to ResponseCookie.

We do not wish to modify the API for ResponseCookie to achieve this.

Out of curiosity, why do we deviate from the approach of Apache HttpComponents HttpClient 5.x where they do not merge Max-Age and Expires. Is it to maintain uniformity across all org.springframework.http.client.reactive.ClientHttpResponse implementations?

Yes, that's correct. We want the handling to be internal and transparent to the user, without changes to the current public APIs.

Cheers,

Sam

[imvtsl]

Hi @sbrannen
Thank you for the explanation, appreciate it.
I have written tests for org.springframework.http.client.reactive.ClientHttpResponse implementations -- for example, HttpComponentsClientHttpResponse, JettyClientHttpResponse, ReactorClientHttpResponse, and ReactorNetty2ClientHttpResponse here.

It fails for Reactor Netty 2 and HttpComponents. I will raise the fix very soon.

Thanks!

[imvtsl]

I have raised the PR with fix and tests.

[imvtsl]

Hey @sbrannen

Please let me know if there is anything required from my end. I will be on vacation from Thursday to Monday. I will be available to do changes until tomorrow, otherwise I can do them after I come back on Tuesday.

Thanks!
Vatsal