Rest Client NewCookie Handler cannot parse all possible values of "Expires" attribute

myroch commented 1 day ago

Describe the bug

We are facing the case, where we get in Response following Set-Cookie Header: cookie1=value1; Expires=Mon, 11-Nov-24 22:59:57 GMT; Path=/; Domain=mydomain.com; Secure

Expires is defined in RFC 850. It is in between obsolete, but RFC2616 states clearly, that it should be still supported on client side. https://datatracker.ietf.org/doc/html/rfc2616#section-3.3.1

org.jboss.resteasy.reactive.common.headers.NewCookieHeaderDelegate doesn't support neither RFC 850 nor ANSI C's asctime() format. It should be nice to have this functionality on place, for example using following logic:

    private static final DateTimeFormatter RFC_822_DATE_FORMAT = DateTimeFormatter.ofPattern("EE, dd MMM uuuu HH:mm:ss z", Locale.US); // Sun, 06 Nov 1994 08:49:37 GMT
    private static final DateTimeFormatter RFC_850_DATE_FORMAT = DateTimeFormatter.ofPattern("EEE, dd-MMM-uu HH:mm:ss z", Locale.US); // Sunday, 06-Nov-94 08:49:37 GMT
    private static final DateTimeFormatter ANSI_C_DATE_FORMAT = DateTimeFormatter.ofPattern("EE MMM ppd HH:mm:ss uuuu", Locale.US).withZone(ZoneOffset.UTC); // Sun Nov  6 08:49:37 1994

    public Object fromString(String newCookie) throws IllegalArgumentException {
...
            } else if (name.equalsIgnoreCase("Expires")) {
                try {
                    if (value != null) {
                        if (StringUtils.indexOf(value, ',') > 0) { // RFC 850 or RFC 822
                            if (StringUtils.indexOf(value, '-') > 0) { // RFC 850, two digits year
                                expiry = Date.from(ZonedDateTime.parse(value, RFC_850_DATE_FORMAT).toInstant());
                            } else { // RFC 822, standard format, four digits year
                                expiry = Date.from(ZonedDateTime.parse(value, RFC_822_DATE_FORMAT).toInstant());
                            }
                        } else { // ANSI C asctime format
                            expiry = Date.from(ZonedDateTime.parse(value, ANSI_C_DATE_FORMAT).toInstant());
                        }
                    }
                } catch (DateTimeParseException e) {
                    LOGGER.debug("Cannot parse expires=[{}] of the cookie=[{}]", value, cookieName);
                }
            }
...

Expected behavior

NewCookieHeaderDelegate can parse RFC 822, RFC 850 and ANSI C's asctime() format of Expires

Actual behavior

No response

How to Reproduce?

No response

Output of `uname -a` or `ver`

No response

Output of `java -version`

No response

Quarkus version or git rev

3.15.1

Build tool (ie. output of `mvnw --version` or `gradlew --version`)

No response

Additional information

No response

quarkus-bot[bot] commented 1 day ago

/cc @cescoffier (rest-client), @geoand (rest-client)

geoand commented 1 day ago

Whenever I see RFC in the description, I summon @FroMage :)

geoand commented 1 day ago

@jamezp is probably also interested in this as the code was copied from org.jboss.resteasy.plugins.delegates.NewCookieHeaderDelegate

jamezp commented 1 day ago

Thank you @geoand. The NewCookie JavaDoc has a "see also" for RFC 2109. The date format there seems to be in the pattern of Wdy, DD-Mon-YY HH:MM:SS GMT.

That said, this is the second issue recently with Set-Cookie that's come up and I do think it makes sense to figure out a way to support RFC 6265 which has the date format defined https://datatracker.ietf.org/doc/html/rfc6265#section-5.1.1.

FWIW RFC 2616 is obsolete as is RFC 2109.

geoand commented 1 day ago

That said, this is the second issue recently with Set-Cookie that's come up and I do think it makes sense to figure out a way to support RFC 6265 which has the date format defined https://datatracker.ietf.org/doc/html/rfc6265#section-5.1.1.

Definitely +1

gsmet commented 1 day ago

I'm fine with us being more robust.

Still not sure about the Locale.US in the code you drafted? Did I miss something?

@myroch given you already give it some thoughts, would you want to give it a try and make sure we support all formats, including the one posted by @jamezp for RFC 6265?

Ideally, let's code this in a way that the RFC 6265 code path is the most efficient (if it makes any difference in how things should be implemented).

FroMage commented 15 hours ago

RFC6265 (the latest one) format is pretty interesting, because it can't be parsed by a Java Date parser, I don't think.

The idea is to parse these time members in any order:

Time: hh:mm:ss (though each member can be 1 or 2 digits)
Day of month: dd (one or two digits)
Month: jan…dec (three letters, in US short month names)
Year: yyyy (two or four digits, with auto +1900 if two digits >= 70, and auto +2000 if two digits < 70)

These can appear in any order, but they can only appear once, and they are selected by preference order (as listed above).

The spec says it is an error if there isn't at least one member, sort of implying that only one is mandatory, but then it has rules about what values each member must have, and it doesn't say that members have an initial default value, so I suppose they're all mandatory.

All times are UTC

So, for example:

01:02:03 23 feb 24 is 23rd of february 2024 at 01:02:03
23 24 feb 01:02:03 is the same date, different order

Interestingly, https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#expiresdate on the contrary pretends the format is that of an HTTP Date, which has a very different syntax: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date#syntax

quarkusio / quarkus