Skip to content

fix(server): reject timezone-naive expires instead of raising - #209

Open
mehmetkr-31 wants to merge 1 commit into
tempoxyz:mainfrom
mehmetkr-31:fix/verify-naive-expires
Open

fix(server): reject timezone-naive expires instead of raising#209
mehmetkr-31 wants to merge 1 commit into
tempoxyz:mainfrom
mehmetkr-31:fix/verify-naive-expires

Conversation

@mehmetkr-31

Copy link
Copy Markdown

Problem

verify_or_challenge parses the echoed expires and compares it to datetime.now(UTC):

try:
    expires_dt = datetime.fromisoformat(echo.expires.replace("Z", "+00:00"))
except ValueError:
    return await fail(InvalidChallengeError(echo.id, "invalid expires"), credential)
if expires_dt < datetime.now(UTC):

A timestamp without an offset parses successfully, so it never reaches the except, and the comparison then raises:

TypeError: can't compare offset-naive and offset-aware datetimes

The comment directly above this block states the intent — "Credentials without an expires field or with an unparseable value are rejected outright" — but a naive value is parseable, so it escapes the fail-closed path. An expired credential surfaces as a server error instead of a rejection, and because the value is fixed for the route, every subsequent request fails the same way.

Reachability

expires is covered by the challenge-id HMAC, so a client cannot inject a naive value into someone else's challenge; forging one fails the id check earlier. It is reachable when an application supplies its own expires to verify_or_challenge, for example datetime.now().isoformat() instead of datetime.now(UTC). The existing runtime guard in _create_challenge only checks that expires is a str, not that it names an instant.

So this is a robustness / fail-closed-contract bug rather than a way for a payer to bypass expiry — I did not want to overstate it.

Reproduced against main with make_bound_credential(..., expires="2000-01-01T00:00:00"): TypeError at verify.py:209, where a Challenge rejection is expected.

Fix

Reject a naive expires the same way an unparseable one is rejected, so the route still fails closed instead of erroring.

Tests

Parametrized over a future and a past naive timestamp — the past case is the one that should have been a clean expiry rejection. Both fail on main with the TypeError above and pass here.

uv run pytest → 802 passed, 41 skipped. ruff check and ruff format --check clean.

Open question

Should _create_challenge also validate an application-supplied expires at issue time, so a naive value is caught when the challenge is minted rather than when a credential comes back? That would fail earlier and more visibly, but it changes the challenge-creation contract, so I kept this PR to the crash itself. Happy to follow up if you want that guard.

Disclosure: I used an AI assistant while investigating and preparing this change; the analysis and conclusions are my own to defend.

🤖 Generated with Claude Code

`verify_or_challenge` parses the echoed `expires` and compares it to
`datetime.now(UTC)`. A value without an offset parses successfully but is
naive, so the comparison raises `TypeError: can't compare offset-naive and
offset-aware datetimes`.

The surrounding code is documented to fail closed — "credentials without an
expires field or with an unparseable value are rejected outright" — but a
naive value is parseable, so it escaped that path. An expired credential
then surfaced as a server error instead of a rejection, and every request
to the route failed the same way.

`expires` is covered by the challenge-id HMAC, so a client cannot inject
this; it is reachable when an application passes its own `expires` string,
for example `datetime.now().isoformat()` without a timezone. Reject a naive
value like any other unusable `expires`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mehmetkr-31
mehmetkr-31 force-pushed the fix/verify-naive-expires branch from c1a044c to 7385ab6 Compare August 9, 2026 09:15
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