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