Repository navigation
Conversation
OAuth.initialize() discarded an expired token and returned None even when the stored token carried a refresh_token, so HttpConnector.connect() fell through to the interactive browser flow although OAuth.refresh_token() was available. Attempt the refresh after metadata discovery and reuse the refreshed token.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="libraries/python/mcp_use/client/auth/oauth.py">
<violation number="1" location="libraries/python/mcp_use/client/auth/oauth.py:275">
P1: When a token was obtained through dynamic registration and the client restarts without a configured `client_id`, this call still returns `None` without contacting the token endpoint. `refresh_token()` refuses to construct its temporary client when `self.client_id` is missing, while `authenticate()` is the only path that loads `_load_client_registration()`, so expired DCR tokens always fall back to interactive login despite a valid refresh token. Load the stored registration before calling `refresh_token()`.</violation>
<violation number="2" location="libraries/python/mcp_use/client/auth/oauth.py:275">
P2: When the token endpoint is temporarily unreachable, `refresh_token()` re-raises `httpx.RequestError` (only `OAuth2Error` is caught there), and this new call site lets it escape `initialize()` and `connect()`. So a transient network failure now breaks the session instead of falling back to interactive auth. Catch `httpx.HTTPError` around the call and treat it as a failed refresh.</violation>
</file>
<file name="libraries/python/tests/unit/test_oauth_initialize.py">
<violation number="1" location="libraries/python/tests/unit/test_oauth_initialize.py:131">
P3: The PR advertises "if refresh_token() returns None (refresh rejected or unusable), preserve existing behavior and return None", but no test covers the non-raising None path. test_failed_refresh_returns_none only exercises the OAuth2Error-raise branch. A regression that removes the client_id guard or breaks the silent-None fallback in refresh_token() would pass the current suite. Add a test that builds OAuth with expired stored tokens plus a refresh_token but no client_id (client_id=None), verifies refresh_token() is called, initialize() returns None, and the interactive flow can proceed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # Refresh needs the token endpoint, hence after metadata discovery | ||
| if token_data and token_data.refresh_token: | ||
| logger.debug("Existing token is expired but has a refresh token, attempting refresh") | ||
| refreshed_auth = await self.refresh_token() |
There was a problem hiding this comment.
P1: When a token was obtained through dynamic registration and the client restarts without a configured client_id, this call still returns None without contacting the token endpoint. refresh_token() refuses to construct its temporary client when self.client_id is missing, while authenticate() is the only path that loads _load_client_registration(), so expired DCR tokens always fall back to interactive login despite a valid refresh token. Load the stored registration before calling refresh_token().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libraries/python/mcp_use/client/auth/oauth.py, line 275:
<comment>When a token was obtained through dynamic registration and the client restarts without a configured `client_id`, this call still returns `None` without contacting the token endpoint. `refresh_token()` refuses to construct its temporary client when `self.client_id` is missing, while `authenticate()` is the only path that loads `_load_client_registration()`, so expired DCR tokens always fall back to interactive login despite a valid refresh token. Load the stored registration before calling `refresh_token()`.</comment>
<file context>
@@ -269,6 +269,14 @@ async def initialize(self, client: httpx.AsyncClient) -> BearerAuth | None:
+ # Refresh needs the token endpoint, hence after metadata discovery
+ if token_data and token_data.refresh_token:
+ logger.debug("Existing token is expired but has a refresh token, attempting refresh")
+ refreshed_auth = await self.refresh_token()
+ if refreshed_auth:
+ logger.debug("OAuth.initialize returning refreshed BearerAuth")
</file context>
| refreshed_auth = await self.refresh_token() | |
| if not self.client_id: | |
| await self._load_client_registration() | |
| refreshed_auth = await self.refresh_token() |
| # Refresh needs the token endpoint, hence after metadata discovery | ||
| if token_data and token_data.refresh_token: | ||
| logger.debug("Existing token is expired but has a refresh token, attempting refresh") | ||
| refreshed_auth = await self.refresh_token() |
There was a problem hiding this comment.
P2: When the token endpoint is temporarily unreachable, refresh_token() re-raises httpx.RequestError (only OAuth2Error is caught there), and this new call site lets it escape initialize() and connect(). So a transient network failure now breaks the session instead of falling back to interactive auth. Catch httpx.HTTPError around the call and treat it as a failed refresh.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libraries/python/mcp_use/client/auth/oauth.py, line 275:
<comment>When the token endpoint is temporarily unreachable, `refresh_token()` re-raises `httpx.RequestError` (only `OAuth2Error` is caught there), and this new call site lets it escape `initialize()` and `connect()`. So a transient network failure now breaks the session instead of falling back to interactive auth. Catch `httpx.HTTPError` around the call and treat it as a failed refresh.</comment>
<file context>
@@ -269,6 +269,14 @@ async def initialize(self, client: httpx.AsyncClient) -> BearerAuth | None:
+ # Refresh needs the token endpoint, hence after metadata discovery
+ if token_data and token_data.refresh_token:
+ logger.debug("Existing token is expired but has a refresh token, attempting refresh")
+ refreshed_auth = await self.refresh_token()
+ if refreshed_auth:
+ logger.debug("OAuth.initialize returning refreshed BearerAuth")
</file context>
| refreshed_auth = await self.refresh_token() | |
| try: | |
| refreshed_auth = await self.refresh_token() | |
| except httpx.HTTPError: | |
| logger.debug("Token refresh failed, falling back to interactive flow") | |
| refreshed_auth = None |
| mock_client_class.assert_not_called() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_failed_refresh_returns_none(self, tmp_path): |
There was a problem hiding this comment.
P3: The PR advertises "if refresh_token() returns None (refresh rejected or unusable), preserve existing behavior and return None", but no test covers the non-raising None path. test_failed_refresh_returns_none only exercises the OAuth2Error-raise branch. A regression that removes the client_id guard or breaks the silent-None fallback in refresh_token() would pass the current suite. Add a test that builds OAuth with expired stored tokens plus a refresh_token but no client_id (client_id=None), verifies refresh_token() is called, initialize() returns None, and the interactive flow can proceed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libraries/python/tests/unit/test_oauth_initialize.py, line 131:
<comment>The PR advertises "if refresh_token() returns None (refresh rejected or unusable), preserve existing behavior and return None", but no test covers the non-raising None path. test_failed_refresh_returns_none only exercises the OAuth2Error-raise branch. A regression that removes the client_id guard or breaks the silent-None fallback in refresh_token() would pass the current suite. Add a test that builds OAuth with expired stored tokens plus a refresh_token but no client_id (client_id=None), verifies refresh_token() is called, initialize() returns None, and the interactive flow can proceed.</comment>
<file context>
@@ -0,0 +1,168 @@
+ mock_client_class.assert_not_called()
+
+ @pytest.mark.asyncio
+ async def test_failed_refresh_returns_none(self, tmp_path):
+ """A rejected refresh falls back to the interactive flow instead of raising."""
+ storage = FileTokenStorage(base_dir=tmp_path)
</file context>
|
Thanks for taking the time to contribute. Our focus right now is on the TypeScript SDK, so we're limiting Python PRs to fixes for concrete problems that contributors have run into firsthand. This PR doesn't appear to address that kind of issue, so we're closing it for now, along with the linked issue. If this was prompted by something you hit in practice, please reply on the issue with what you were doing when it came up and how to reproduce it, and we'll reopen the issue and take another look. |
Closes #2560
Language / Project Scope
Check all that apply:
Changes
OAuth.initialize()now attempts a refresh when the stored token is expired and carries arefresh_token, returning the refreshedBearerAuthinstead ofNone. Previously it logged"Existing token is expired", skipped refresh entirely, and returnedNone, soHttpConnector.connect()fell through toOAuth.authenticate()— the interactive browser flow.Review Readiness
Implementation Details
libraries/python/mcp_use/client/auth/oauth.py— 8 added lines inOAuth.initialize(), placedafter metadata discovery and before the final
return None. The refresh is guarded bytoken_data and token_data.refresh_token; ifrefresh_token()returnsNone(rejected orunusable refresh token) the method keeps its previous behaviour and returns
None, so thecaller still falls back to
authenticate().Root cause.
libraries/python/mcp_use/client/auth/oauth.py:260-261— the expired branch onlylogs and falls through;
token_data.refresh_tokenis never consulted. The refresh implementationalready existed at
libraries/python/mcp_use/client/auth/oauth.py:765(refresh_token(),decorated
@telemetry("oauth_refresh_token")) but had no call site anywhere inmcp_use/.Ordering. The refresh must run after the discovery block at
libraries/python/mcp_use/client/auth/oauth.py:266-270:refresh_token()returns early whenself._metadatais unset, and it needsmetadata.token_endpointto reach the token service.Python Checklist
ruff formatruff checkExample Usage (Before)
Example Usage (After)
Documentation Updates
None.
Testing
Unit tests added in
libraries/python/tests/unit/test_oauth_initialize.py(5 tests,pytest-asyncio,matching
pytest.ini'sasyncio_mode = autoand the existingtests/unitlayout).AsyncOAuth2Clientis patched, so no network is involved.
Covered: refresh on an expired token (asserts the call reaches
metadata.token_endpointwith thestored refresh token, that the returned
BearerAuthcarries the new access token, and that the newand rotated tokens are persisted); refresh when metadata comes from discovery rather than
oauth_provider;Nonewhen there is no refresh token;Nonewhen the refresh is rejected withOAuth2Error(asserting the refresh was actually attempted); and an unexpired token still short-circuiting without any client being constructed.
Commands and results (Python 3.12,
libraries/python):The one failure,
tests/unit/test_config.py::TestConnectorCreation::test_create_sandboxed_stdio_connector,is pre-existing and unrelated: it reproduces identically on a stashed (unpatched) tree, with
ImportError: E2B SDK (e2b-code-interpreter) not found ... (mcp_use/client/connectors/sandbox.py:110)— an optional extra that isn't installed in the verification environment.
tyreports the same 203 diagnostics before and after the change (all pre-existing, inmcp_use/telemetry/); none are inmcp_use/client/auth/oauth.py.End-to-end check of the issue's repro — expired token plus
refresh_tokeninFileTokenStorage,with a real HTTP token endpoint on
127.0.0.1:8199and onlyauthenticate()stubbed:Backwards Compatibility
Backwards compatible. The only behavioural change is on the path that previously returned
Noneand forced the interactive flow: that path now first tries the already-implemented
refresh_token().Every other branch of
initialize()is untouched. A server that rejects the refresh still ends upin
authenticate()exactly as before.Related Issues
Closes #2560
Summary by cubic
Fixes #2560:
OAuth.initialize()now refreshes an expired stored OAuth token when a refresh token is available, instead of returningNoneand triggering the interactive browser flow. Successful refreshes return the new auth and persist updated tokens; missing or rejected refresh tokens retain the existing fallback.Written for commit a3b3f23. Summary will update on new commits.