Repository navigation
Conversation
…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.
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.
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:
Maintainers: reopen, remove |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two small defects in token expiry handling, each with a failing test first.
1.
calculate_token_expirycannot parse a fractional string, and truncates floatssrc/mcp/shared/auth_utils.pydeclaresexpires_in: int | str | Noneand its comment says the string case is handled defensively, but the body istime.time() + int(expires_in).int("3599.5")raisesValueError, so an authorization server that emits a fractionalexpires_inas a JSON string fails during token setup instead of rounding.int()truncates, so the client expires the token up to a second early.float()handles3600,"3600","3599.5"and3599.5without changing the result for the integer case.2.
is_token_validtreats an expiry of0.0as "no expiry"src/mcp/client/auth/oauth2.pyreadsnot self.token_expiry_time or time.time() <= self.token_expiry_time.Nonelegitimately means the authorization server declared no expiry, but a falsy test also catches0.0, which is the epoch and long expired, so the token is presented indefinitely. Comparing againstNoneexplicitly keeps the intended meaning and fixes the falsy-zero conflation.Verification
New tests in
tests/shared/test_auth_utils.pycoverNone,int, numeric string, fractional string and float inputs.tests/client/test_auth.pygains 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 threeexpires_invalues at 50 ms steps: the set was empty, because the bearer middleware compares against a flooredint(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 absentexpires_in, which makescalculate_token_expiryreturnNone, and that is a behaviour change rather than a bug fix, so it does not belong in this PR.