Skip to content

Treat an access token as expired at its expiry time - #3761

Open
Kludex wants to merge 2 commits into
mainfrom
token-expiry-boundary
Open

Kludex wants to merge 2 commits into
mainfrom
token-expiry-boundary

Conversation

@Kludex

@Kludex Kludex commented Oct 25, 2026

Copy link
Copy Markdown
Member

Fixes #3729

The OAuth client now treats an access token as expired at its expiry time, not only after it.

Motivation and Context

OAuthContext.is_token_valid() and the cached-token check in IdentityAssertionOAuthProvider compared time.time() <= expiry, so a token was still sent at the exact instant it expired. With expires_in=0, the expiry equals the issuance time, so the client could reuse a token that should never be used, which showed up as intermittent failures on platforms with a coarse clock. RFC 7519 §4.1.4 defines exp as the time "on or after which" the token must not be accepted, so both checks now use <.

How Has This Been Tested?

  • test_auth.py: the token is invalid when the clock equals token_expiry_time.
  • test_identity_assertion.py: a stored token with expires_in=0 is not attached when the clock has not moved.
  • Both fail without the change. Full suite passes; coverage 100%, strict-no-cover and pyright clean.

Breaking Changes

None: a token is refreshed or reacquired one clock tick earlier, at its expiry instant.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I am assigned to the linked issue (or it is labeled help wanted, or I'm a maintainer)
  • I have disclosed any AI assistance and can explain the change in my own words
  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

The source change is the same as the one proposed by @pei711 in #3730, which was closed by the linked-issue bot; this adds a test for the identity-assertion path as well.

AI Disclaimer

This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-25T11:35:37.806206Z 527864f PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Turn on auto-fix | Re-trigger cubic

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked three things and ruled them out:

  • No other client-side expiry comparison still uses the inclusive boundary: the two changed lines are the only ones under src/mcp/client/auth.
  • The new identity-assertion test does hit the equality case: calculate_token_expiry only special-cases None, so expires_in=0 under the patched clock yields an expiry equal to "now".
  • mock is already imported in tests/client/test_auth.py.
Extended reasoning...

The change flips two access-token expiry comparisons from <= to < in the OAuth client (src/mcp/client/auth/oauth2.py and src/mcp/client/auth/extensions/identity_assertion.py) and adds two tests, which is client auth code, so it is not approved automatically. The note records only the checks that came up clean; the inline comments cover the rest.

Comment thread tests/client/auth/extensions/test_identity_assertion.py Outdated
Comment thread tests/client/test_auth.py Outdated
Comment thread src/mcp/client/auth/extensions/identity_assertion.py
Comment thread src/mcp/client/auth/extensions/identity_assertion.py

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread tests/client/test_auth.py
pass


def test_a_token_is_invalid_at_its_expiry_instant(oauth_provider: OAuthClientProvider):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): The new top-level test test_a_token_is_invalid_at_its_expiry_instant has no return type hint, which AGENTS.md asks for on all code. Its sibling added in the same change, at tests/client/auth/extensions/test_identity_assertion.py:329, is annotated -> None. Fix: add -> None to the signature at tests/client/test_auth.py:3672.

Why this was flagged

No runtime trigger applies; this is a convention check on the test code added since the last review. The signature at tests/client/test_auth.py:3672 reads def test_a_token_is_invalid_at_its_expiry_instant(oauth_provider: OAuthClientProvider): with a typed parameter but no return annotation. AGENTS.md line 50 says "Type hints required for all code". The other test this change adds, test_stored_token_is_not_sent_at_its_expiry_instant at tests/client/auth/extensions/test_identity_assertion.py:329, does carry -> None, so the two new tests are inconsistent with each other. The base branch has no such function; older tests in the same file (for example tests/client/test_auth.py:1170) also omit the annotation, which is legacy precedent rather than the rule. Nothing enforces it: pyright infers the return type and the test passes unchanged.

Verification: AGENTS.md line 50 reads "- Type hints required for all code". The diff adds tests/client/test_auth.py:3672 def test_a_token_is_invalid_at_its_expiry_instant(oauth_provider: OAuthClientProvider): with an annotated parameter but no return annotation, so the function is only partially type-hinted. Nothing breaks.

Finding ID: f-570feb36

This branch has not been deployed

No deployments
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.

OAuth token expiry equality boundary causes intermittent downstream test failures

1 participant