HttpRequestHandlingEndpointSupport throws IllegalStateException when duplicate cookie names are in a request

[Anthonyas007]

In what version(s) of Spring Integration are you seeing this issue?

5.2.0.RELEASE and later, NOT in versions 4.x

Describe the bug

When a duplicate cookie name appears in a request, an IllegalStateException is thrown. Previously the last cookie was put in the "evaluationContext" cookies map, and duplicates were ignored. There was an update made to use Java steams instead of a for loop in the HttpRequestHandlingEndpointSupport#prepareEvaluationContext() method, however Collectors.toMap(...) was not used with a merge function, therefore it throws the IllegalStateException when you try to map two keys.

To Reproduce

Send an HTTP request with duplicate cookies

Expected behavior

I expect the request to ignore the duplicates or to throw an exception with a clear and concise message explaining that duplicate cookies are not allowed. Preferably the request would be handled gracefully, like it was in previous versions, and simply use the last cookie in the request.getCookies() array.

Sample

[artembilan]

Thank you for bringing this and for the fix. I agree that is kinda regression, but I wonder what HTTP spec says on the matter instead of that our previous blind “duplicates were ignored" and current illegal state…
The fix looks good so far. I’ll come to it on Monday!
thank you again !
Gone to read cookie RFC…

[artembilan]

See here: https://datatracker.ietf.org/doc/html/rfc6265#section-4.2

In particular,
if the Cookie header contains two cookies with the same name (e.g.,
that were set with different Path or Domain attributes), servers
SHOULD NOT rely upon the order in which these cookies appear in the
header.

So, sounds like we have to accept all of them. In other words it must be a MultiValueMap for the proper fix.
Might be treated as a breaking change for the current point release , but would be great to revise for the next 6.0 one. Although MultiValueMap is still a Map.
Let’s take a look closer on Monday!

[artembilan]

Just to confirm my original assumption.
See org.springframework.http.server.reactive.ServerHttpRequest:

/**
 * Return a read-only map of cookies sent by the client.
 */
MultiValueMap<String, HttpCookie> getCookies();

I see also there is a org.springframework.data.util.MultiValueMapCollector for our consideration, but I would say let's just iterate over cookies array and build the map manually!
For the current version (and all for back-porting) we have to do a toSingleValueMap() in the end to keep end-users from the breaking changes.
In the next 6.0 version we need to expose it as a MultiValueMap for consistency with HTTP spec.

NOTE: 5.2.x is EOL already. We are not going to be back-port the fix down there.

[Anthonyas007]

@artembilan I like the MultiValueMap approach - This would allow all cookies in the request to be available to the evaluation context and it does seem more in line with the HTTP spec as you mentioned.

[artembilan]

Right. But as you see we cannot do that in the current point version: it would be too breaking change for end-users who rely on a single value map at the moment.
So, something like toSingleValueMap() in the end before setting the variable to context should be done for time being until we come to 6.0 development phase.

[Anthonyas007]

Yes, that makes sense - The value would be a list of cookies and not a single cookie. toSingleValueMap() sounds like a good approach for now

[artembilan]

And the fix has been cherry-picked to 5.4.x & 5.3.x

[shark300]

Hi @artembilan
Thank you for this fix. Unfortunately, I did not find any infos about the milestone of 5.4.x. Could you provide this? Thank you.

[artembilan]

@shark300 ,

Have just scheduled it for the next week: https://github.com/spring-projects/spring-integration/milestone/79

[shark300]

@shark300 ,

Have just scheduled it for the next week: https://github.com/spring-projects/spring-integration/milestone/79

Thank you so much!