Skip to content

fix(auth): parse fractional expires_in and stop treating expiry 0 as no expiry - #3770

Closed
Ben-Home wants to merge 1 commit into
modelcontextprotocol:mainfrom
Ben-Home:fix/token-expiry-robustness
Closed

Ben-Home wants to merge 1 commit into
modelcontextprotocol:mainfrom
Ben-Home:fix/token-expiry-robustness

Conversation

@Ben-Home

Copy link
Copy Markdown

What

Two small defects in token expiry handling, each with a failing test first.

1. calculate_token_expiry cannot parse a fractional string, and truncates floats

src/mcp/shared/auth_utils.py declares expires_in: int | str | None and its comment says the string case is handled defensively, but the body is time.time() + int(expires_in).

  • int("3599.5") raises ValueError, so an authorization server that emits a fractional expires_in as a JSON string fails during token setup instead of rounding.
  • For a float input, int() truncates, so the client expires the token up to a second early.

float() handles 3600, "3600", "3599.5" and 3599.5 without changing the result for the integer case.

2. is_token_valid treats an expiry of 0.0 as "no expiry"

src/mcp/client/auth/oauth2.py reads not self.token_expiry_time or time.time() <= self.token_expiry_time. None legitimately means the authorization server declared no expiry, but a falsy test also catches 0.0, which is the epoch and long expired, so the token is presented indefinitely. Comparing against None explicitly keeps the intended meaning and fixes the falsy-zero conflation.

Verification

uv run pytest tests/shared tests/client -q
1804 passed, 1 xfailed

uv run ruff check src/mcp/shared/auth_utils.py src/mcp/client/auth/oauth2.py \
  tests/shared/test_auth_utils.py tests/client/test_auth.py
All checks passed!

New tests in tests/shared/test_auth_utils.py cover None, int, numeric string, fractional string and float inputs. tests/client/test_auth.py gains a zero-expiry predicate test.

What I deliberately did not change

The <= comparison against the expiry timestamp is left alone. I checked whether the client can consider a token valid at an instant the server rejects it, enumerating four issue instants against three expires_in values at 50 ms steps: the set was empty, because the bearer middleware compares against a floored int(now) and is therefore at least as lenient as the client by up to one second. The direction of that boundary is the opposite of what it first looks like, and the detailed arithmetic is in #3729. The client-lenient path worth a separate discussion is an absent expires_in, which makes calculate_token_expiry return None, and that is a behaviour change rather than a bug fix, so it does not belong in this PR.

…no expiry

calculate_token_expiry declares int|str but calls int(), so a server emitting a
fractional expires_in as a string raises ValueError during token setup, and a float
input is truncated instead of kept. Parse with float() so both work.

is_token_valid tested token_expiry_time for truthiness, so an expiry of 0.0 was read
as 'no expiry' and the token was presented forever. Compare against None explicitly.

Tests cover None, int, numeric string, fractional string and float inputs, plus the
zero-expiry predicate.
@github-actions github-actions Bot added the missing-issue-link Auto-closed: PR needs a linked issue assigned to its author (see CONTRIBUTING.md) label Oct 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and this PR doesn't link an open issue yet.

  • If you're already assigned to an issue for this, add Fixes #<n> to the description and the PR will reopen on its own.
  • If there's no issue yet, please open one instead: what you ran into, why it matters for your use case, and a minimal reproduction. That context is super important to us and is what we use to decide what to prioritise.
  • If there's an issue but you're not assigned, add Fixes #<n> anyway so they're linked, then engage on the issue itself by confirming the repro or describing the approach you'd take. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. If you are assigned, this PR reopens automatically.

You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way.

CONTRIBUTING.md has the full reasoning, but in short:

  • We're a small team with very little capacity to review community PRs right now.
  • Many recent PRs are AI-generated with little human review, and reviewing one carefully still costs a maintainer as much time as it ever did. A well-described issue is usually more useful to us than the code.

Maintainers: reopen, remove missing-issue-link, or add bypass-issue-check to override.

@github-actions github-actions Bot closed this Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

missing-issue-link Auto-closed: PR needs a linked issue assigned to its author (see CONTRIBUTING.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant