Skip to content

fix: preserve existing refresh_token when server omits it in refresh response #2274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Jah-yee wants to merge 1 commit into from
+11 −0
Closed

fix: preserve existing refresh_token when server omits it in refresh response #2274

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/mcp/client/auth/oauth2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,17 @@ async def _handle_refresh_response(self, response: httpx.Response) -> bool: # p
content = await response.aread()
token_response = OAuthToken.model_validate_json(content)

# Per RFC 6749 Section 6, the server MAY issue a new refresh token.
# If the response omits it, preserve the existing one.
if (
not token_response.refresh_token
and self.context.current_tokens
and self.context.current_tokens.refresh_token
):
token_response = token_response.model_copy(
update={"refresh_token": self.context.current_tokens.refresh_token}
)
Comment on lines +461 to +470
Copy link

@claude claude bot Mar 11, 2026

Choose a reason for hiding this comment

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

🟡 Per CLAUDE.md:28, bug fixes require regression tests, but this PR only modifies the source file. Consider adding a test in tests/client/test_auth.py that verifies _handle_refresh_response preserves the existing refresh_token when the server omits it — otherwise a future refactor could silently reintroduce #2270.

Extended reasoning...

Summary

The repository's CLAUDE.md explicitly states at line 28: "Bug fixes require regression tests". This PR fixes a real bug (#2270 — refresh tokens being lost when the OAuth server omits them in the refresh response), but only modifies src/mcp/client/auth/oauth2.py. No test file is touched, and a grep for _handle_refresh_response across tests/ returns zero matches.

Why this matters

The only existing refresh-related test is test_refresh_token_request (test_auth.py:610), which validates request building — it verifies the outgoing POST body contains grant_type=refresh_token and the correct refresh_token value. However, nothing tests the response handling path. The specific behavior this PR introduces (preserving the old refresh token when the new response omits it) is completely unverified by the test suite.

While the method carries a # pragma: no cover annotation, that directive only excludes it from coverage metrics. It does not exempt the fix from the project's documented regression-test requirement. The annotation predates this PR and was likely added because the method is exercised indirectly through the httpx auth-flow generator, which is awkward to drive in unit tests — but the method itself takes a plain httpx.Response and is trivial to test directly.

Step-by-step proof

  1. CLAUDE.md:28 reads: Bug fixes require regression tests.
  2. PR metadata shows changed-files count="1" → only src/mcp/client/auth/oauth2.py.
  3. Grep for _handle_refresh_response in tests/ → no matches.
  4. Grep for test_refresh_token → only finds test_refresh_token_request (line 610), which calls _refresh_token() (request builder), not _handle_refresh_response() (response handler).
  5. Therefore: the new preserve-on-omit logic has zero test coverage, in violation of the project's stated policy.

Suggested fix

A regression test can follow the existing patterns in test_auth.py (which already constructs httpx.Response objects directly — see line 108). For example:

@pytest.mark.anyio
async def test_refresh_response_preserves_refresh_token(self, oauth_provider, valid_tokens):
    """Per RFC 6749 §6, preserve existing refresh_token if server omits it."""
    oauth_provider.context.current_tokens = valid_tokens  # has refresh_token="test_refresh_token"

    # Server response WITHOUT a refresh_token field
    response = httpx.Response(
        200,
        content=b'{"access_token": "new_access", "token_type": "Bearer", "expires_in": 3600}',
        request=httpx.Request("POST", "https://auth.example.com/token"),
    )

    result = await oauth_provider._handle_refresh_response(response)

    assert result is True
    assert oauth_provider.context.current_tokens.access_token == "new_access"
    assert oauth_provider.context.current_tokens.refresh_token == "test_refresh_token"  # preserved!

A complementary test verifying that a new refresh_token in the response correctly replaces the old one would also be valuable.

Severity rationale

This is a nit rather than a blocker: the code change itself is correct and RFC-compliant. The concern is purely about test coverage per the project's own documented conventions. However, given that this bug (lost refresh tokens with Google/Auth0/Okta) was painful enough to warrant a fix, a regression test is worthwhile insurance against it recurring.


self.context.current_tokens = token_response
self.context.update_token_expiry(token_response)
await self.context.storage.set_tokens(token_response)
Expand Down
Loading