Skip to content

Remove cookies with a non-positive Max-Age from sessions - #1914

Open
alutoin-cloudamite wants to merge 1 commit into
httpie:masterfrom
alutoin-cloudamite:fix/negative-max-age-cookie-expiry
Open

Remove cookies with a non-positive Max-Age from sessions#1914
alutoin-cloudamite wants to merge 1 commit into
httpie:masterfrom
alutoin-cloudamite:fix/negative-max-age-cookie-expiry

Conversation

@alutoin-cloudamite

Copy link
Copy Markdown

Found this with Claude, curated the fix with it, I hope it is acceptable.

Completes #998, which reported that Max-Age=0 on its own never removed a cookie from the session
file. #1029 fixed that by translating max-age into expires, gated on max_age.isdigit() — which
is False for negatives, so Max-Age=-1 still never gets an expires and the cookie is never
removed. HTTPie keeps sending a cookie the server asked it to delete.

RFC 6265 §5.2.2: "If delta-seconds is less than or equal to zero, let expiry-time be the earliest
representable date and time."

>>> from httpie.utils import get_expired_cookies
>>> get_expired_cookies('one=two; Max-Age=0; path=/', now=1000.0)
[{'name': 'one', 'path': '/'}]
>>> get_expired_cookies('one=two; Max-Age=-1; path=/', now=1000.0)
[]                                            # should also be expired

Change

-        if max_age and max_age.isdigit():
-            cookie['expires'] = now + float(max_age)
+        try:
+            delta_seconds = int(max_age)
+        except (TypeError, ValueError):
+            continue
+        cookie['expires'] = now + delta_seconds

int() is what http.cookiejar itself uses for max-age. A narrower lstrip('-').isdigit() would
make int('--1') raise; the new test pins that case.

Tests

Parametrized next to the existing Max-Age=0 test from #10290, -1, -100, 3600, plus
''/abc/1.5/--1 for unparseable input. Fails on -1 and -100 before the change, 8 passed
after; full suite 994 passed.

CI will show red, but not from this change: test_cli_ui.py::test_naked_invocation and two
test_encoding.py big5 cases already fail on master at 5b604c3, and the 3.7 jobs can't install
Python 3.7 on current runners. code-style passes — I ran the workflows on a fork first.

Why not just delete the workaround

Its HACK/FIXME points at psf/requests#5743, which is closed as a no-op, so no upstream change is
coming. I measured whether it is still needed: with get_expired_cookies() disabled, current-format
sessions still drop the cookie — the cookiejar manages it now that cookies are domain-bound (#1312) —
but pre-3.1.0 dict-layout sessions keep it. Deleting would therefore reintroduce #998 for any
session not yet upgraded, so this fixes it in place. The docstring is rewritten to record that, since
the missing context is what let the bug survive #1029.

No CHANGELOG entry, but happy to add one in whatever form you would like.

Completes the fix for httpie#998, which reported that a cookie carrying only
`Max-Age=0` (no `Expires`) was never removed from the session file. httpie#1029
addressed that by translating `max-age` into `expires` in
`_max_age_to_expires()`, gated on `max_age.isdigit()`.

`str.isdigit()` is False for any negative value, so `Max-Age=-1` still never
receives an `expires` key, is never recognised as expired by
`get_expired_cookies()`, and is never removed -- the same symptom httpie#998
described, for the other common deletion idiom. HTTPie keeps sending a cookie
the server has told it to delete.

RFC 6265 section 5.2.2: "If delta-seconds is less than or equal to zero, let
expiry-time be the earliest representable date and time."

Parse with `int()` instead, so negative deltas are honoured while unparseable
values are still ignored rather than raising -- note that a narrower fix such as
`max_age.lstrip('-').isdigit()` would make `int('--1')` throw, which the new
test pins. This matches the standard library, which parses `max-age` with
`int()` in `http.cookiejar`.

Also rewrites the docstring. It read `HACK/FIXME: <psf/requests#5743>`, but that
issue was declined as a no-op rather than fixed, so this is not a workaround
awaiting upstream removal. The docstring now records the RFC rule, and that the
function is only load-bearing for pre-3.1.0 session layouts: those stored
cookies as a domainless dict, so `http.cookiejar`'s `clear(domain, path, name)`
could not match them, whereas since 3.1.0 each cookie is bound to a domain
(httpie#1312) and the cookiejar removes expired ones by itself. Removing this function
therefore means dropping support for the old layout first.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant