dagolden / HTTP-CookieJar

A minimalist HTTP user agent cookie jar
2 stars 7 forks source link

handling of expired cookies #6

Closed jvolkening closed 4 years ago

jvolkening commented 4 years ago

I make frequent use of HTTP::Tiny with HTTP::CookieJar, and today ran into a quirk in the way it handles expired cookies that may be considered a bug.

When a site returns a Set-Cookie header with an expiration date in the past, or with Max-Age <= 0, that cookie is supposed to be deleted. However, HTTP::CookieJar does not seem to handle this correctly. Instead, it first sets the expiration date to the current time + Max-Age and then checks if the expiration date is less than the current time, deleting the cookie if true. However, since this is an integer comparison, and since it happens quickly, cookies with Max-Age == 0 are not deleted.

Then, if another HTTP::Tiny request is performed within the same epoch second, incorrect cookies can be sent.

I believe the following change would conform to specifications:

From

    # if cookie has expired, purge any old matching cookie, too
    if ( defined $parse->{expires} && $parse->{expires} < $now ) {
        delete $self->{store}{$domain}{$path}{$name};
    }

to

    # if cookie has expired, purge any old matching cookie, too
    if (
        (defined $parse->{expires} && $parse->{expires} < $now)
     || (defined $parse->{'max-age'} && $parse->{'max-age'} <= 0)
    ) {
        delete $self->{store}{$domain}{$path}{$name};
    }

If you agree with my reasoning on this, I would be happy to submit a PR.

xdg commented 4 years ago

Without checking the RFC, it sounds right. A PR would be welcome, but please include tests.

jvolkening commented 4 years ago

I think the relevant bit is from RFC 6265 section 5:

Let delta-seconds be the attribute-value converted to an integer.

If delta-seconds is less than or equal to zero (0), let expiry-time be the earliest representable date and time. Otherwise, let the expiry-time be the current date and time plus delta-seconds seconds.

Append an attribute to the cookie-attribute-list with an attribute- name of Max-Age and an attribute-value of expiry-time.

I don't quite understand the last sentence, since there doesn't seem to ever be a 'Max-Age' attribute actually saved with the cookie, but the effect on expiration time seems clear. If I can think of how to add proper tests I will submit a PR.

xdg commented 4 years ago

Stable 0.010 release to CPAN with this fix