RFR: 8351983: HttpCookie Parser Incorrectly Handles Cookies with Expires Attribute [v5]
Michael McMahon
michaelm at openjdk.org
Wed Jun 18 10:04:10 UTC 2025
On Tue, 17 Jun 2025 18:43:56 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:
>>> @Michael-Mc-Mahon, instead of making an exception for `max-age` and `expires`, and removing them from `assignors`, can't we convert the type of `assignors` from `Map` to `List` and add `max-age` & `expires` entries at the end?
>>
>> Just converting from Map to List wouldn't be enough. The problem is that both attribute types need to be handled together. You could change the attribute name recognition to some kind of pattern match to recognise either of them. Then you need to know which of them was set and what their values were.
>>
>> Maybe, I could at least use the assignor pattern to recognise the two attributes and limit the special code to just actioning the values. I'll take a look at that now.
>
> I think the last commit (b22113118) just worsened things – now the logic is spread across `assignMaxAgeAttribute`, `assignors`, and instance variables, whereas earlier it was only in `assignMaxAgeAttribute`. :face_with_diagonal_mouth: I suggest simply reverting it, that is, switching the state back to 9a495d7f9a5e.
>
> I agree that introducing a smarter data structure and iteration scheme to `assignors` would simplify things, though that is probably out of the scope of this work.
>
> Apologies for the inconvenience and thanks so much for your patient cooperation. 🙇
> I think the last commit ([b221131](https://github.com/openjdk/jdk/commit/b22113118ce68a5ba710be9072b208e9a36ae533)) just worsened things – now the logic is spread across `assignMaxAgeAttribute`, `assignors`, and instance variables, whereas earlier it was only in `assignMaxAgeAttribute`. 🫤 I suggest simply reverting it, that is, switching the state back to [9a495d7](https://github.com/openjdk/jdk/commit/9a495d7f9a5e9ae86ce71010950f11ce23ee1c2c).
>
> I agree that introducing a smarter data structure and iteration scheme to `assignors` would simplify things, though that is probably out of the scope of this work.
>
> Apologies for the inconvenience and thanks so much for your patient cooperation. 🙇
Yeah, I agree. I will revert it. The old version was clearer.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25636#discussion_r2154196899
More information about the net-dev
mailing list