From 0e498d38e8a442ca33646d093dd9e84c3a0cdc5d Mon Sep 17 00:00:00 2001 From: Ben-Home <238533482+Ben-Home@users.noreply.github.com> Date: Mon, 26 Oct 2026 16:06:52 -0700 Subject: [PATCH] fix(auth): parse fractional expires_in and stop treating expiry 0 as 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. --- src/mcp/client/auth/oauth2.py | 5 +++- src/mcp/shared/auth_utils.py | 10 ++++--- tests/client/test_auth.py | 13 ++++++++ tests/shared/test_auth_utils.py | 53 ++++++++++++++++++++++++++++++++- 4 files changed, 75 insertions(+), 6 deletions(-) diff --git a/src/mcp/client/auth/oauth2.py b/src/mcp/client/auth/oauth2.py index c513a99b46..594760d6fe 100644 --- a/src/mcp/client/auth/oauth2.py +++ b/src/mcp/client/auth/oauth2.py @@ -188,7 +188,10 @@ def is_token_valid(self) -> bool: return bool( self.current_tokens and self.current_tokens.access_token - and (not self.token_expiry_time or time.time() <= self.token_expiry_time) + # None means the authorization server declared no expiry. Compare against that + # explicitly: a falsy test would also treat an expiry of 0.0 (the epoch, long + # expired) as "no expiry" and keep presenting a dead token. + and (self.token_expiry_time is None or time.time() <= self.token_expiry_time) ) def can_refresh_token(self) -> bool: diff --git a/src/mcp/shared/auth_utils.py b/src/mcp/shared/auth_utils.py index 3ba880f40d..8665b4fa60 100644 --- a/src/mcp/shared/auth_utils.py +++ b/src/mcp/shared/auth_utils.py @@ -65,16 +65,18 @@ def check_resource_allowed(requested_resource: str, configured_resource: str) -> return requested_path.startswith(configured_path) -def calculate_token_expiry(expires_in: int | str | None) -> float | None: +def calculate_token_expiry(expires_in: int | float | str | None) -> float | None: """Calculate token expiry timestamp from expires_in seconds. Args: - expires_in: Seconds until token expiration (may be string from some servers) + expires_in: Seconds until token expiration (may be a string or a float from some servers) Returns: Unix timestamp when token expires, or None if no expiry specified """ if expires_in is None: return None # pragma: no cover - # Defensive: handle servers that return expires_in as string - return time.time() + int(expires_in) + # Defensive: handle servers that return expires_in as a string, including ones that + # send a fractional value such as "3599.5". int() would raise ValueError on those and + # silently truncate the float case, expiring the token up to a second early. + return time.time() + float(expires_in) diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index 18a1566705..909936c98e 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -253,6 +253,19 @@ async def test_token_validity_checking(self, oauth_provider: OAuthClientProvider context.client_info = None assert not context.can_refresh_token() + @pytest.mark.anyio + async def test_token_expiry_of_zero_is_expired(self, oauth_provider: OAuthClientProvider, valid_tokens: OAuthToken): + """An expiry timestamp of 0 means expired, not 'no expiry'. + + `token_expiry_time` is a float|None where None means the server declared no + expiry. Testing it for truthiness conflates 0.0 (the epoch, long expired) with + absence, so a token whose expiry resolves to 0 is treated as never expiring. + """ + context = oauth_provider.context + context.current_tokens = valid_tokens + context.token_expiry_time = 0.0 + assert not context.is_token_valid() + def test_clear_tokens(self, oauth_provider: OAuthClientProvider, valid_tokens: OAuthToken): """Test clear_tokens() removes token data.""" context = oauth_provider.context diff --git a/tests/shared/test_auth_utils.py b/tests/shared/test_auth_utils.py index 5ae0e22b0c..bcbd3703d0 100644 --- a/tests/shared/test_auth_utils.py +++ b/tests/shared/test_auth_utils.py @@ -1,8 +1,10 @@ """Tests for OAuth 2.0 Resource Indicators utilities.""" +import time + from pydantic import HttpUrl -from mcp.shared.auth_utils import check_resource_allowed, resource_url_from_server_url +from mcp.shared.auth_utils import calculate_token_expiry, check_resource_allowed, resource_url_from_server_url # Tests for resource_url_from_server_url function @@ -121,3 +123,52 @@ def test_check_resource_allowed_empty_paths(): assert check_resource_allowed("https://example.com", "https://example.com") is True assert check_resource_allowed("https://example.com/", "https://example.com") is True assert check_resource_allowed("https://example.com/api", "https://example.com") is True + + +# Tests for calculate_token_expiry function + + +def test_calculate_token_expiry_none_returns_none(): + """A server that declares no expiry yields None, not a timestamp.""" + assert calculate_token_expiry(None) is None + + +def test_calculate_token_expiry_integer_seconds(): + """The common case: expires_in arrives as an integer.""" + before = time.time() + expiry = calculate_token_expiry(3600) + after = time.time() + assert expiry is not None + assert before + 3600 <= expiry <= after + 3600 + + +def test_calculate_token_expiry_numeric_string(): + """Some servers emit expires_in as a JSON string.""" + before = time.time() + expiry = calculate_token_expiry("3600") + after = time.time() + assert expiry is not None + assert before + 3600 <= expiry <= after + 3600 + + +def test_calculate_token_expiry_fractional_string(): + """A fractional string must round, not raise. + + The declared signature accepts str, and the docstring promises the string case is + handled defensively, but int("3599.5") raises ValueError. Servers that send a + fractional expires_in as a string fail token setup instead of expiring 0.5s later. + """ + before = time.time() + expiry = calculate_token_expiry("3599.5") + after = time.time() + assert expiry is not None + assert before + 3599.5 <= expiry <= after + 3599.5 + + +def test_calculate_token_expiry_float(): + """A float expires_in keeps its fraction instead of being truncated.""" + before = time.time() + expiry = calculate_token_expiry(3599.5) + after = time.time() + assert expiry is not None + assert before + 3599.5 <= expiry <= after + 3599.5