Repository navigation
fix(mcp): open MCP OAuth in same browser as gateway - #951
Conversation
…owser When MCP OAuth is triggered from the web gateway, the auth URL was being opened via `open::that()` which launches the OS default browser instead of the browser already running the gateway UI. This changes the MCP OAuth flow to use the same gateway callback pattern as WASM extensions: in gateway mode, the auth URL is returned to the frontend via SSE and opened with `window.open()`, keeping the user in the same browser. Also adds RFC 8707 `resource` parameter support to the gateway token exchange path, scoping issued tokens to the correct MCP server. Closes #299 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
scope: channel/cli
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the MCP OAuth authentication process, particularly for users interacting via the web gateway. The primary goal is to provide a more seamless and secure authentication experience by ensuring that OAuth flows remain within the user's current browser session and that tokens are precisely scoped to their intended resources. This change improves both usability and security for MCP integrations. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
Bot
added
risk: medium
There was a problem hiding this comment.
Pull request overview
This PR updates MCP OAuth when Ironclaw is running behind the web gateway so authorization opens in the same browser session as the gateway UI, and adds RFC 8707 resource scoping support to MCP token exchange.
Changes:
- In gateway mode, MCP OAuth now returns an auth URL to the frontend (via SSE) instead of launching the OS default browser.
- Adds
resourcetoPendingOAuthFlowand introducesexchange_oauth_code_with_resource()to support RFC 8707 during token exchange. - Updates the web gateway OAuth callback handler to pass
resourceinto the non-proxy token exchange path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/extensions/manager.rs |
Adds gateway-mode MCP OAuth URL construction and stores a pending gateway OAuth flow (incl. resource). |
src/cli/oauth_defaults.rs |
Adds resource to pending flow state and introduces a token exchange helper that can include RFC 8707 resource. |
src/channels/web/server.rs |
Passes flow.resource into the direct token exchange function from the gateway callback handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Err(_) => Ok(AuthResult::awaiting_token( | ||
| name, | ||
| ExtensionKind::McpServer, | ||
| format!( | ||
| "Server '{}' does not support OAuth. \ | ||
| Please provide an API token/key for this server.", | ||
| name | ||
| ), | ||
| None, | ||
| )), |
There was a problem hiding this comment.
In gateway mode this treats any error from auth_mcp_build_url() as “server does not support OAuth” and falls back to manual token entry. That will mask real failures (e.g., discovery/network/DCR errors) and makes debugging much harder. Consider only falling back on a specific “not supported” condition and otherwise returning ExtensionError::AuthFailed (or include the underlying error in the instructions).
| Err(_) => Ok(AuthResult::awaiting_token( | |
| name, | |
| ExtensionKind::McpServer, | |
| format!( | |
| "Server '{}' does not support OAuth. \ | |
| Please provide an API token/key for this server.", | |
| name | |
| ), | |
| None, | |
| )), | |
| Err(crate::tools::mcp::auth::AuthError::NotSupported) => { | |
| Ok(AuthResult::awaiting_token( | |
| name, | |
| ExtensionKind::McpServer, | |
| format!( | |
| "Server '{}' does not support OAuth. \ | |
| Please provide an API token/key for this server.", | |
| name | |
| ), | |
| None, | |
| )) | |
| } | |
| Err(e) => Err(ExtensionError::AuthFailed(e.to_string())), |
There was a problem hiding this comment.
Fixed in b7376b7. Now only falls back to manual token entry on AuthNotSupported; real errors (discovery/network/DCR) are propagated as ExtensionError::AuthFailed.
| @@ -1766,58 +1786,141 @@ impl ExtensionManager { | |||
| .await | |||
| .map_err(|e| ExtensionError::AuthFailed(e.to_string()))?; | |||
There was a problem hiding this comment.
discover_full_oauth_metadata(&server.url) is called unconditionally, which ignores any explicitly configured OAuth endpoints on server.oauth (authorization_url/token_url). For servers that provide endpoints but don’t support discovery (or return 404 for .well-known), this will fail even though config is sufficient. Consider using discover_oauth_endpoints(server) when server.oauth.is_some() (and only using full metadata discovery for the DCR/no-config path), and source authorization_endpoint/token_endpoint from those resolved endpoints.
There was a problem hiding this comment.
Good point. The current implementation already uses discover_oauth_endpoints(server) when server.oauth.is_some() in the CLI path. The gateway auth_mcp_build_url always uses full metadata discovery because it needs DCR registration_endpoint. Servers with pre-configured OAuth that don't support discovery will now get a proper error propagated (via the AuthNotSupported fix) rather than a misleading fallback.
There was a problem hiding this comment.
Acknowledged. The gateway path uses full metadata discovery because it needs the DCR registration_endpoint. Servers with pre-configured OAuth that don't support discovery now get a proper error propagated (via the AuthNotSupported fix in b7376b7) rather than a misleading fallback.
| ) | ||
| .await | ||
| .map_err(|e| e.to_string())? | ||
| } else { | ||
| oauth_defaults::exchange_oauth_code( | ||
| oauth_defaults::exchange_oauth_code_with_resource( |
There was a problem hiding this comment.
When IRONCLAW_OAUTH_EXCHANGE_URL is set, the token exchange goes through exchange_via_proxy(...), but the flow’s RFC 8707 resource is not forwarded (only the direct exchange path passes it). In hosted/gateway deployments that use the proxy, MCP tokens won’t be scoped as intended. Consider extending exchange_via_proxy (and the proxy endpoint) to accept/send resource, or bypass the proxy for flows that require resource.
There was a problem hiding this comment.
Fixed in b7376b7. When flow.resource.is_some(), we now bypass the exchange proxy and use direct token exchange with the resource parameter.
There was a problem hiding this comment.
Fixed in b7376b7. When flow.resource.is_some(), we now bypass the exchange proxy and use direct token exchange with the resource parameter.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the RFC 8707 resource parameter in OAuth flows for Multi-Cloud Platform (MCP) servers. Key changes include refactoring the exchange_oauth_code function to exchange_oauth_code_with_resource to accept an optional resource parameter, adding this parameter to the PendingOAuthFlow struct, and updating the ExtensionManager to handle OAuth authorization differently for "gateway mode" and "local/CLI mode", ensuring the resource is included in authorization URLs and stored for callback processing in gateway mode. Review comments suggest adding further explanations to improve code readability and maintainability, specifically regarding the usage of exchange_oauth_code_with_resource in an else block, the delegation pattern in exchange_oauth_code, and the purpose of the resource parameter in token parameters.
Note: Security Review did not run due to the size of the PR.
| oauth_defaults::exchange_oauth_code_with_resource( | ||
| &flow.token_url, | ||
| &flow.client_id, | ||
| flow.client_secret.as_deref(), | ||
| &code, | ||
| &flow.redirect_uri, | ||
| flow.code_verifier.as_deref(), | ||
| &flow.access_token_field, | ||
| flow.resource.as_deref(), |
There was a problem hiding this comment.
medium: Consider adding a comment explaining why exchange_oauth_code_with_resource is being called instead of exchange_oauth_code in this else block. This will improve code readability and maintainability.
| ) -> Result<OAuthTokenResponse, OAuthCallbackError> { | ||
| exchange_oauth_code_with_resource( | ||
| token_url, | ||
| client_id, | ||
| client_secret, | ||
| code, | ||
| redirect_uri, | ||
| code_verifier, | ||
| access_token_field, | ||
| None, | ||
| ) | ||
| .await |
There was a problem hiding this comment.
medium: This function is delegating to exchange_oauth_code_with_resource with None as the resource. It might be clearer to inline the exchange_oauth_code_with_resource call directly here, or add a comment explaining why this delegation is preferred.
| if let Some(resource) = resource { | ||
| token_params.push(("resource", resource.to_string())); | ||
| } |
There was a problem hiding this comment.
medium: Consider adding a comment explaining the purpose of adding the resource parameter to the token parameters. This will improve code clarity and maintainability.
There was a problem hiding this comment.
Added RFC 8707 resource parameter comment in b7376b7.
Code reviewFound 7 issues:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code reviewFound 4 issues:
ironclaw/src/extensions/manager.rs Lines 2276 to 2293 in 1a9b2d0
ironclaw/src/extensions/manager.rs Lines 2287 to 2290 in 1a9b2d0
ironclaw/src/extensions/manager.rs Lines 2287 to 2290 in 1a9b2d0
ironclaw/src/extensions/manager.rs Lines 2280 to 2331 in 1a9b2d0 |
Code reviewFound 2 issues:
|
…efresh The gateway callback handler stored access and refresh tokens but not the DCR client_id. When the token expired, refresh failed with "No client ID found" because get_client_id() could not find it in secrets. Adds client_id_secret_name to PendingOAuthFlow so the gateway callback handler persists the client_id alongside the tokens, matching the behavior of the CLI flow in authorize_mcp_server(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .as_ref() | ||
| .map(|p| format!("mcp:{}", p)) |
There was a problem hiding this comment.
The provider string used for persisting the MCP client_id secret is built as format!("mcp:{}", p) where p comes from flow.provider. If PendingOAuthFlow.provider is updated to already include the mcp: prefix (to match MCP token storage), this will double-prefix. Consider storing a fully-qualified provider string in the flow (e.g., mcp:{name}) and using it directly here (or otherwise ensure this is consistent with how MCP access/refresh tokens are stored).
| .as_ref() | |
| .map(|p| format!("mcp:{}", p)) | |
| .clone() |
There was a problem hiding this comment.
Fixed in b7376b7. flow.provider now uses the mcp: prefix directly, and the client_id secret storage uses flow.provider as-is (no double-prefix).
There was a problem hiding this comment.
Fixed in b7376b7. flow.provider now uses the mcp: prefix directly, and client_id secret storage uses flow.provider as-is (no double-prefix).
| #[test] | ||
| fn test_pending_flow_mcp_carries_client_id_secret_name() { | ||
| // MCP flows must set client_id_secret_name so refresh can find the client_id | ||
| let mcp_secret_name = format!("mcp_{}_client_id", "notion"); | ||
| assert_eq!(mcp_secret_name, "mcp_notion_client_id"); | ||
|
|
||
| // Simulate what auth_mcp_build_url sets for gateway mode | ||
| let has_secret_name = Some(mcp_secret_name.clone()); | ||
| assert!( | ||
| has_secret_name.is_some(), | ||
| "MCP PendingOAuthFlow must set client_id_secret_name for token refresh" | ||
| ); | ||
|
|
||
| // WASM flows should not set it | ||
| let wasm_secret_name: Option<String> = None; | ||
| assert!( | ||
| wasm_secret_name.is_none(), | ||
| "WASM PendingOAuthFlow should not set client_id_secret_name" | ||
| ); | ||
| } |
There was a problem hiding this comment.
test_pending_flow_mcp_carries_client_id_secret_name is effectively asserting that Some(String) is Some, without exercising any production logic (it doesn’t construct a PendingOAuthFlow or call auth_mcp_build_url). This test won’t catch regressions. Suggest either removing it or rewriting it to validate behavior via the relevant builder/flow-construction API (e.g., an integration-style test around the function that populates PendingOAuthFlow).
| #[test] | |
| fn test_pending_flow_mcp_carries_client_id_secret_name() { | |
| // MCP flows must set client_id_secret_name so refresh can find the client_id | |
| let mcp_secret_name = format!("mcp_{}_client_id", "notion"); | |
| assert_eq!(mcp_secret_name, "mcp_notion_client_id"); | |
| // Simulate what auth_mcp_build_url sets for gateway mode | |
| let has_secret_name = Some(mcp_secret_name.clone()); | |
| assert!( | |
| has_secret_name.is_some(), | |
| "MCP PendingOAuthFlow must set client_id_secret_name for token refresh" | |
| ); | |
| // WASM flows should not set it | |
| let wasm_secret_name: Option<String> = None; | |
| assert!( | |
| wasm_secret_name.is_none(), | |
| "WASM PendingOAuthFlow should not set client_id_secret_name" | |
| ); | |
| } | |
| // NOTE: Previous test here only asserted trivial properties of local values | |
| // and did not exercise production logic. It was removed to avoid a misleading, | |
| // non-meaningful test that could not catch regressions. |
| code_verifier, | ||
| access_token_field: "access_token".to_string(), | ||
| secret_name: server.token_secret_name(), | ||
| provider: Some(name.to_string()), |
There was a problem hiding this comment.
In gateway mode, PendingOAuthFlow.provider is set to Some(name.to_string()), but MCP secrets stored by the CLI flow use provider mcp:{server_name} (see store_tokens() in src/tools/mcp/auth.rs). This mismatch can break provider-based filtering/grouping in the secrets store. Consider setting provider here to Some(format!("mcp:{}", name)) (and then use the same provider string consistently when persisting the client_id secret in the callback handler).
| provider: Some(name.to_string()), | |
| provider: Some(format!("mcp:{}", name)), |
There was a problem hiding this comment.
Fixed in b7376b7. Provider is now set to format!("mcp:{}", name) in the flow.
There was a problem hiding this comment.
Fixed in b7376b7. Provider is now format\!("mcp:{}", name) in the flow.
| sse_sender: self.sse_sender.read().await.clone(), | ||
| gateway_token: self.gateway_token.clone(), | ||
| resource: Some(resource), | ||
| client_id_secret_name: Some(format!("mcp_{}_client_id", name)), |
There was a problem hiding this comment.
client_id_secret_name is always set for gateway MCP flows, even when the server has pre-configured OAuth (server.oauth.is_some()). Token refresh only needs the persisted client_id for DCR flows (CLI path stores it only when server_config.oauth.is_none()). Consider setting client_id_secret_name only for DCR cases to avoid writing unnecessary secrets.
| client_id_secret_name: Some(format!("mcp_{}_client_id", name)), | |
| client_id_secret_name: if server.oauth.is_none() { | |
| Some(format!("mcp_{}_client_id", name)) | |
| } else { | |
| None | |
| }, |
There was a problem hiding this comment.
Fixed in b7376b7. Now uses server.client_id_secret_name() only when server.oauth.is_none() (DCR flows).
There was a problem hiding this comment.
Fixed in b7376b7. Now uses server.client_id_secret_name() only when server.oauth.is_none() (DCR flows).
| if crate::cli::oauth_defaults::use_gateway_callback() { | ||
| return match self.auth_mcp_build_url(name, &server).await { | ||
| Ok(result) => Ok(result), | ||
| Err(_) => Ok(AuthResult::awaiting_token( | ||
| name, | ||
| ExtensionKind::McpServer, | ||
| format!( | ||
| "Server '{}' does not support OAuth. \ | ||
| Please provide an API token/key for this server.", | ||
| name | ||
| ), | ||
| None, | ||
| )), | ||
| }; |
There was a problem hiding this comment.
In gateway mode, any error from auth_mcp_build_url() is treated as “server does not support OAuth” and the user is prompted for a manual token. This can hide actionable failures (e.g., discovery network errors, unsafe endpoints, DCR failure) behind a misleading message. Consider propagating the underlying error (or including it in the awaiting_token message) and only falling back to manual token on a confirmed NotSupported-style condition.
There was a problem hiding this comment.
Fixed — see reply on the earlier duplicate comment.
| let result: Result<(), String> = async { | ||
| let token_response = if let Some(ref proxy_url) = exchange_proxy_url { | ||
| let gateway_token = flow.gateway_token.as_deref().unwrap_or_default(); | ||
| oauth_defaults::exchange_via_proxy( | ||
| proxy_url, | ||
| gateway_token, | ||
| &code, | ||
| &flow.redirect_uri, | ||
| flow.code_verifier.as_deref(), | ||
| &flow.access_token_field, | ||
| ) | ||
| .await | ||
| .map_err(|e| e.to_string())? | ||
| } else { | ||
| oauth_defaults::exchange_oauth_code( | ||
| oauth_defaults::exchange_oauth_code_with_resource( | ||
| &flow.token_url, | ||
| &flow.client_id, | ||
| flow.client_secret.as_deref(), | ||
| &code, | ||
| &flow.redirect_uri, | ||
| flow.code_verifier.as_deref(), | ||
| &flow.access_token_field, | ||
| flow.resource.as_deref(), |
There was a problem hiding this comment.
When IRONCLAW_OAUTH_EXCHANGE_URL is set, the callback always uses exchange_via_proxy(), which currently does not accept/forward the RFC 8707 resource parameter (and doesn’t use flow.token_url). For MCP flows this likely drops resource (and may route token exchange to a proxy that can’t handle MCP providers). Consider bypassing the proxy when flow.resource.is_some() (or extending the proxy request API to include resource / provider routing).
There was a problem hiding this comment.
Fixed — see reply on the earlier duplicate comment.
There was a problem hiding this comment.
Fixed — see reply on 2919349142.
activate_mcp() returned ActivationFailed for all errors including 401 auth responses, so the activate handler never triggered the OAuth flow. Now 401/auth errors return AuthRequired, which the handler detects and redirects to the OAuth flow — matching the WASM extension pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add explicit gateway_mode flag on ExtensionManager (set at startup by web gateway) so MCP OAuth returns auth URLs to the frontend instead of calling open::that() on the server machine. - Auto-activate extensions after successful OAuth callback so the UI transitions from "Activate" to "Active" without a second click. - Send ApprovalNeeded status (not generic "Awaiting approval") from thread_ops.rs for all three NeedApproval paths so the web UI shows approval cards for deferred tool calls. - Remove duplicate ApprovalNeeded send from agent_loop.rs (thread_ops.rs is now the canonical sender). - Skip approval for tool_auth in gateway mode since it only returns a URL. - Revert fragile active-server detection heuristic from system prompt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
scope: agent
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return match self.auth_mcp_build_url(name, &server).await { | ||
| Ok(result) => Ok(result), | ||
| Err(_) => Ok(AuthResult::awaiting_token( | ||
| name, | ||
| ExtensionKind::McpServer, | ||
| format!( | ||
| "Server '{}' does not support OAuth. \ | ||
| Please provide an API token/key for this server.", | ||
| name | ||
| ), | ||
| None, | ||
| )), | ||
| }; |
There was a problem hiding this comment.
In gateway mode, any error from auth_mcp_build_url() is currently treated as “server does not support OAuth” and the user is prompted for a manual token. This masks real discovery/network/config errors (e.g., transient HTTP failures) and can send users down the wrong path. Consider only falling back to manual token entry for a specific “OAuth not supported” error, and otherwise surface the underlying error message to the caller/UI.
| return match self.auth_mcp_build_url(name, &server).await { | |
| Ok(result) => Ok(result), | |
| Err(_) => Ok(AuthResult::awaiting_token( | |
| name, | |
| ExtensionKind::McpServer, | |
| format!( | |
| "Server '{}' does not support OAuth. \ | |
| Please provide an API token/key for this server.", | |
| name | |
| ), | |
| None, | |
| )), | |
| }; | |
| return self.auth_mcp_build_url(name, &server).await; |
There was a problem hiding this comment.
Fixed — see reply on the earlier duplicate comment.
| sse_sender: self.sse_sender.read().await.clone(), | ||
| gateway_token: self.gateway_token.clone(), | ||
| resource: Some(resource), | ||
| client_id_secret_name: Some(format!("mcp_{}_client_id", name)), |
There was a problem hiding this comment.
client_id_secret_name is set unconditionally for MCP OAuth gateway flows. The CLI path only persists a client_id secret for Dynamic Client Registration (when no client_id is preconfigured). Persisting it for preconfigured OAuth configs adds extra secrets unnecessarily and may confuse troubleshooting. Consider setting client_id_secret_name only when the flow was created via DCR, and prefer server.client_id_secret_name() over reformatting the secret name string here.
| client_id_secret_name: Some(format!("mcp_{}_client_id", name)), | |
| client_id_secret_name: server.client_id_secret_name(), |
There was a problem hiding this comment.
Fixed — see reply on the earlier duplicate comment.
There was a problem hiding this comment.
Fixed — see reply on 2921154104.
| .await | ||
| .map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
For MCP gateway OAuth, the access/refresh tokens are persisted via store_oauth_tokens(...) using flow.provider. The CLI MCP auth path stores secrets with provider tag mcp:{server_name}; if flow.provider is just the raw server name, gateway-mode tokens will be tagged differently than CLI tokens. Consider ensuring MCP flows pass a mcp:-prefixed provider string into store_oauth_tokens so provider tags remain consistent across auth paths.
There was a problem hiding this comment.
Fixed — flow.provider is now mcp:-prefixed, and store_oauth_tokens uses it directly. All secrets (token, refresh, client_id) share the same provider tag.
There was a problem hiding this comment.
Fixed — flow.provider is now mcp:-prefixed, and store_oauth_tokens uses it directly. All secrets share the same provider tag.
| async fn gateway_callback_redirect_uri(&self) -> Option<String> { | ||
| use crate::cli::oauth_defaults; | ||
| if oauth_defaults::use_gateway_callback() { | ||
| return Some(format!("{}/callback", oauth_defaults::callback_url())); |
There was a problem hiding this comment.
gateway_callback_redirect_uri() builds {callback_url()}/callback when IRONCLAW_OAUTH_CALLBACK_URL is set, but other gateway paths use /oauth/callback and the web server routes the handler at /oauth/callback. This can easily produce a redirect URI that 404s unless the env var is set to a non-obvious base path. Consider making the env-var branch generate the same /oauth/callback path (or clearly document/validate the required shape of IRONCLAW_OAUTH_CALLBACK_URL).
| return Some(format!("{}/callback", oauth_defaults::callback_url())); | |
| return Some(format!("{}/oauth/callback", oauth_defaults::callback_url())); |
There was a problem hiding this comment.
Fixed in b7376b7. Now generates /oauth/callback path to match the web server route.
There was a problem hiding this comment.
Fixed in b7376b7. Now generates /oauth/callback path to match the web server route.
| code_verifier, | ||
| access_token_field: "access_token".to_string(), | ||
| secret_name: server.token_secret_name(), | ||
| provider: Some(name.to_string()), |
There was a problem hiding this comment.
For MCP flows, provider is set to Some(name.to_string()), but the existing CLI MCP auth path stores secrets with provider tag mcp:{server_name}. Since the gateway callback uses flow.provider when persisting OAuth tokens, the provider tag will differ between CLI and gateway mode, which can break grouping/lookup by provider. Consider storing the fully-qualified provider string (e.g. mcp:{name}) in the flow (and avoid double-prefixing later), or otherwise ensure store_oauth_tokens receives the mcp:-prefixed provider.
| provider: Some(name.to_string()), | |
| provider: Some(format!("mcp:{}", name)), |
There was a problem hiding this comment.
Fixed — see reply on the earlier duplicate comment.
There was a problem hiding this comment.
Fixed — see reply on 2921154075.
| let params = crate::secrets::CreateSecretParams::new(client_id_secret, &flow.client_id) | ||
| .with_provider( | ||
| flow.provider | ||
| .as_ref() | ||
| .map(|p| format!("mcp:{}", p)) | ||
| .unwrap_or_default(), | ||
| ); |
There was a problem hiding this comment.
The new client_id_secret_name persistence uses with_provider(...unwrap_or_default()), which will set an empty provider string if flow.provider is missing. Even if MCP flows always set it today, it’s safer to only set a provider when it’s Some, and to avoid re-prefixing with mcp: if the flow already stores a fully-qualified provider tag.
| let params = crate::secrets::CreateSecretParams::new(client_id_secret, &flow.client_id) | |
| .with_provider( | |
| flow.provider | |
| .as_ref() | |
| .map(|p| format!("mcp:{}", p)) | |
| .unwrap_or_default(), | |
| ); | |
| let mut params = | |
| crate::secrets::CreateSecretParams::new(client_id_secret, &flow.client_id); | |
| if let Some(provider) = flow.provider.as_ref() { | |
| let provider = if provider.starts_with("mcp:") { | |
| provider.to_string() | |
| } else { | |
| format!("mcp:{}", provider) | |
| }; | |
| params = params.with_provider(provider); | |
| } |
There was a problem hiding this comment.
Fixed — see reply on 2921154036.
|
|
||
| /// Verify that auth-related activation errors are detected as AuthRequired | ||
| /// so the activate handler triggers OAuth instead of showing a generic failure. | ||
| #[test] | ||
| fn activation_error_auth_detection() { | ||
| // 401 errors from MCP servers should be treated as AuthRequired | ||
| let auth_msg = "MCP server 'notion' requires authentication. Run: ironclaw mcp auth notion"; | ||
| let is_auth = auth_msg.contains("requires authentication") || auth_msg.contains("401"); | ||
| assert!(is_auth, "Should detect auth error in: {}", auth_msg); | ||
|
|
||
| let raw_401 = "External service error: 401 Unauthorized"; | ||
| let is_auth = raw_401.contains("requires authentication") || raw_401.contains("401"); | ||
| assert!(is_auth, "Should detect 401 in: {}", raw_401); | ||
|
|
||
| // Non-auth errors should not trigger auth flow | ||
| let other_msg = "Connection refused"; | ||
| let is_auth = other_msg.contains("requires authentication") || other_msg.contains("401"); | ||
| assert!(!is_auth, "Should not detect auth in: {}", other_msg); | ||
| } |
There was a problem hiding this comment.
This test doesn’t validate the actual activation error classification logic; it only asserts that hard-coded strings contain substrings like "401". As written it can’t fail for the intended behavior and will provide false confidence. Consider rewriting it to exercise the real code path (e.g., whatever function maps activation errors to AuthRequired) or removing it.
| /// Verify that auth-related activation errors are detected as AuthRequired | |
| /// so the activate handler triggers OAuth instead of showing a generic failure. | |
| #[test] | |
| fn activation_error_auth_detection() { | |
| // 401 errors from MCP servers should be treated as AuthRequired | |
| let auth_msg = "MCP server 'notion' requires authentication. Run: ironclaw mcp auth notion"; | |
| let is_auth = auth_msg.contains("requires authentication") || auth_msg.contains("401"); | |
| assert!(is_auth, "Should detect auth error in: {}", auth_msg); | |
| let raw_401 = "External service error: 401 Unauthorized"; | |
| let is_auth = raw_401.contains("requires authentication") || raw_401.contains("401"); | |
| assert!(is_auth, "Should detect 401 in: {}", raw_401); | |
| // Non-auth errors should not trigger auth flow | |
| let other_msg = "Connection refused"; | |
| let is_auth = other_msg.contains("requires authentication") || other_msg.contains("401"); | |
| assert!(!is_auth, "Should not detect auth in: {}", other_msg); | |
| } |
There was a problem hiding this comment.
Removed in b7376b7. The real AuthRequired detection is now covered by the MCP extension lifecycle E2E test which hits a mock server returning 401.
…way-browser # Conflicts: # src/extensions/manager.rs
- Use Release/Acquire ordering for gateway_mode AtomicBool instead of Relaxed to ensure visibility across threads. - Report activation failure as error in OAuth callback SSE event instead of silently falling back to the success message. - Fix EnvGuard::drop to remove env var when original was unset. - Replace hardcoded /tmp/ path with std::env::temp_dir() in test helper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…erver Add a full MCP extension lifecycle E2E test that exercises: - Turn 1: tool_search → tool_install → text (extension discovery and install) - Token injection + activate (simulating OAuth completion) - Turn 2: MCP tool calls (notion-search → notion-fetch → text) Includes a mock MCP server (tests/support/mock_mcp_server.rs) with OAuth discovery, DCR, token exchange, and JSON-RPC endpoints. The mock server validates Bearer auth and serves pre-configured tool responses. Also adds inject_registry_entry() to ExtensionManager for test use and exposes extension_manager from TestRig. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 5. Turn 1: "setup mock-notion" → search → install → text. | ||
| rig.send_message("setup mock-notion").await; | ||
| let r1 = rig.wait_for_responses(1, TIMEOUT).await; | ||
| assert!(!r1.is_empty(), "Turn 1: no response"); | ||
|
|
||
| // 6. Simulate OAuth completion: inject token + activate. | ||
| // This mirrors what the gateway's oauth_callback_handler does after | ||
| // the user completes the OAuth flow in their browser. | ||
| let secret_name = "mcp_mock-notion_access_token"; | ||
| ext_mgr | ||
| .secrets() | ||
| .create( | ||
| "default", | ||
| ironclaw::secrets::CreateSecretParams::new(secret_name, "mock-access-token") | ||
| .with_provider("mcp:mock-notion".to_string()), | ||
| ) | ||
| .await | ||
| .expect("failed to inject test token"); | ||
|
|
||
| let activate_result = ext_mgr.activate("mock-notion").await; | ||
| assert!( | ||
| activate_result.is_ok(), | ||
| "activation failed: {:?}", | ||
| activate_result.err() | ||
| ); | ||
|
|
||
| // 7. Turn 2: "check what's in my notion" → notion-search → notion-fetch → text. | ||
| rig.send_message("it's done, check what's in my notion") | ||
| .await; | ||
| let r2 = rig.wait_for_responses(2, TIMEOUT).await; | ||
| assert!( | ||
| r2.len() >= 2, | ||
| "Turn 2: expected at least 2 total responses, got {}", | ||
| r2.len() | ||
| ); |
There was a problem hiding this comment.
wait_for_responses(n) returns all captured responses once the total count reaches n (it does not scope to the most recent turn). Here wait_for_responses(2, ...) for turn 2 can return immediately if turn 1 already produced ≥2 responses, so the test may pass without ever observing turn-2 behavior. Consider clearing the channel between turns (rig.clear().await), or waiting for r1.len() + expected_new responses after sending turn 2.
There was a problem hiding this comment.
Fixed in c8cc515. Now uses wait_for_responses(turn1_count + 1) to ensure at least one new turn-2 response is observed.
There was a problem hiding this comment.
Fixed in c8cc515. Now uses wait_for_responses(turn1_count + 1) to ensure at least one new turn-2 response is observed.
|
|
||
| /// Verify that auth-related activation errors are detected as AuthRequired | ||
| /// so the activate handler triggers OAuth instead of showing a generic failure. | ||
| #[test] | ||
| fn activation_error_auth_detection() { | ||
| // 401 errors from MCP servers should be treated as AuthRequired | ||
| let auth_msg = "MCP server 'notion' requires authentication. Run: ironclaw mcp auth notion"; | ||
| let is_auth = auth_msg.contains("requires authentication") || auth_msg.contains("401"); | ||
| assert!(is_auth, "Should detect auth error in: {}", auth_msg); | ||
|
|
||
| let raw_401 = "External service error: 401 Unauthorized"; | ||
| let is_auth = raw_401.contains("requires authentication") || raw_401.contains("401"); | ||
| assert!(is_auth, "Should detect 401 in: {}", raw_401); | ||
|
|
||
| // Non-auth errors should not trigger auth flow | ||
| let other_msg = "Connection refused"; | ||
| let is_auth = other_msg.contains("requires authentication") || other_msg.contains("401"); | ||
| assert!(!is_auth, "Should not detect auth in: {}", other_msg); | ||
| } |
There was a problem hiding this comment.
This test doesn't exercise any production code: it only asserts that hard-coded strings contain substrings. That provides false confidence for the new AuthRequired detection logic (which lives in ExtensionManager activation/error handling). Consider replacing with a unit/integration test that triggers the actual error path (e.g., mock an MCP server returning 401 from tools/list and assert activation yields ExtensionError::AuthRequired).
| /// Verify that auth-related activation errors are detected as AuthRequired | |
| /// so the activate handler triggers OAuth instead of showing a generic failure. | |
| #[test] | |
| fn activation_error_auth_detection() { | |
| // 401 errors from MCP servers should be treated as AuthRequired | |
| let auth_msg = "MCP server 'notion' requires authentication. Run: ironclaw mcp auth notion"; | |
| let is_auth = auth_msg.contains("requires authentication") || auth_msg.contains("401"); | |
| assert!(is_auth, "Should detect auth error in: {}", auth_msg); | |
| let raw_401 = "External service error: 401 Unauthorized"; | |
| let is_auth = raw_401.contains("requires authentication") || raw_401.contains("401"); | |
| assert!(is_auth, "Should detect 401 in: {}", raw_401); | |
| // Non-auth errors should not trigger auth flow | |
| let other_msg = "Connection refused"; | |
| let is_auth = other_msg.contains("requires authentication") || other_msg.contains("401"); | |
| assert!(!is_auth, "Should not detect auth in: {}", other_msg); | |
| } |
There was a problem hiding this comment.
Removed — see earlier reply.
| /// Verify that MCP flows set client_id_secret_name so the gateway callback | ||
| /// can persist the DCR client_id for token refresh. | ||
| #[test] | ||
| fn test_pending_flow_mcp_carries_client_id_secret_name() { | ||
| // MCP flows must set client_id_secret_name so refresh can find the client_id | ||
| let mcp_secret_name = format!("mcp_{}_client_id", "notion"); | ||
| assert_eq!(mcp_secret_name, "mcp_notion_client_id"); | ||
|
|
||
| // Simulate what auth_mcp_build_url sets for gateway mode | ||
| let has_secret_name = Some(mcp_secret_name.clone()); | ||
| assert!( | ||
| has_secret_name.is_some(), | ||
| "MCP PendingOAuthFlow must set client_id_secret_name for token refresh" | ||
| ); | ||
|
|
||
| // WASM flows should not set it | ||
| let wasm_secret_name: Option<String> = None; | ||
| assert!( | ||
| wasm_secret_name.is_none(), | ||
| "WASM PendingOAuthFlow should not set client_id_secret_name" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This test is effectively a tautology (it constructs Some(...) and asserts it’s Some, then constructs None and asserts it’s None). It doesn’t verify that MCP gateway flows actually populate client_id_secret_name in PendingOAuthFlow. Consider removing it or rewriting it to call the real builder (auth_mcp_build_url / the code that creates the flow) and assert the produced flow includes client_id_secret_name for MCP and not for WASM.
| /// Verify that MCP flows set client_id_secret_name so the gateway callback | |
| /// can persist the DCR client_id for token refresh. | |
| #[test] | |
| fn test_pending_flow_mcp_carries_client_id_secret_name() { | |
| // MCP flows must set client_id_secret_name so refresh can find the client_id | |
| let mcp_secret_name = format!("mcp_{}_client_id", "notion"); | |
| assert_eq!(mcp_secret_name, "mcp_notion_client_id"); | |
| // Simulate what auth_mcp_build_url sets for gateway mode | |
| let has_secret_name = Some(mcp_secret_name.clone()); | |
| assert!( | |
| has_secret_name.is_some(), | |
| "MCP PendingOAuthFlow must set client_id_secret_name for token refresh" | |
| ); | |
| // WASM flows should not set it | |
| let wasm_secret_name: Option<String> = None; | |
| assert!( | |
| wasm_secret_name.is_none(), | |
| "WASM PendingOAuthFlow should not set client_id_secret_name" | |
| ); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Removed — see earlier reply.
| return match self.auth_mcp_build_url(name, &server).await { | ||
| Ok(result) => Ok(result), | ||
| Err(_) => Ok(AuthResult::awaiting_token( | ||
| name, | ||
| ExtensionKind::McpServer, | ||
| format!( | ||
| "Server '{}' does not support OAuth. \ | ||
| Please provide an API token/key for this server.", | ||
| name | ||
| ), | ||
| None, | ||
| )), | ||
| }; |
There was a problem hiding this comment.
In gateway mode this swallows all errors from auth_mcp_build_url() and returns the fallback message "does not support OAuth". That will mislead users for real failures (discovery/DCR/network/invalid URLs) and makes debugging/auth recovery harder. Consider only falling back to manual-token flow for explicit "OAuth not supported" cases (e.g., when metadata lacks auth+registration endpoints), and otherwise propagate/return an AuthFailed message with the underlying error.
| return match self.auth_mcp_build_url(name, &server).await { | |
| Ok(result) => Ok(result), | |
| Err(_) => Ok(AuthResult::awaiting_token( | |
| name, | |
| ExtensionKind::McpServer, | |
| format!( | |
| "Server '{}' does not support OAuth. \ | |
| Please provide an API token/key for this server.", | |
| name | |
| ), | |
| None, | |
| )), | |
| }; | |
| return self.auth_mcp_build_url(name, &server).await; |
There was a problem hiding this comment.
Fixed — see reply on the earlier duplicate comment.
| access_token_field: "access_token".to_string(), | ||
| secret_name: server.token_secret_name(), | ||
| provider: Some(name.to_string()), | ||
| validation_endpoint: None, | ||
| scopes, | ||
| user_id: self.user_id.clone(), | ||
| secrets: Arc::clone(&self.secrets), | ||
| sse_sender: self.sse_sender.read().await.clone(), | ||
| gateway_token: self.gateway_token.clone(), | ||
| resource: Some(resource), | ||
| client_id_secret_name: Some(format!("mcp_{}_client_id", name)), |
There was a problem hiding this comment.
PendingOAuthFlow.provider is set to Some(name.to_string()) (e.g. "mock-notion"), but MCP secrets elsewhere are tagged with provider mcp:{server_name} (see store_tokens() / store_client_id() in src/tools/mcp/auth.rs). With the current value, store_oauth_tokens() will persist the access/refresh tokens under provider "mock-notion", which is inconsistent and can break any provider-based lookup/management. Set the flow's provider to the fully qualified mcp:{name} (and then avoid re-prefixing it later in the gateway callback).
There was a problem hiding this comment.
Fixed — see reply on the earlier duplicate comment.
There was a problem hiding this comment.
Fixed — see reply on 2921154075.
| if let Some(ref client_id_secret) = flow.client_id_secret_name { | ||
| let params = crate::secrets::CreateSecretParams::new(client_id_secret, &flow.client_id) | ||
| .with_provider( | ||
| flow.provider | ||
| .as_ref() | ||
| .map(|p| format!("mcp:{}", p)) | ||
| .unwrap_or_default(), | ||
| ); | ||
| flow.secrets | ||
| .create(&flow.user_id, params) | ||
| .await | ||
| .map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
This provider tag construction is inconsistent with how OAuth tokens are stored in store_oauth_tokens(): the access/refresh tokens use flow.provider directly, but here the client_id secret is stored with mcp:{flow.provider}. As written, MCP flows currently store tokens under provider "mock-notion" while the client_id secret is stored under "mcp:mock-notion". Once flow.provider is fixed to include the mcp: prefix, this code would also double-prefix to mcp:mcp:.... Prefer storing all related secrets (token, refresh, client_id) with the same provider tag (likely flow.provider as-is).
There was a problem hiding this comment.
Fixed — see earlier reply. Provider tag is now consistent across all secrets.
There was a problem hiding this comment.
Fixed — see reply on 2921154036. Provider tag is now consistent across all secrets.
| // After successful OAuth, auto-activate the extension so it moves | ||
| // from "Installed (Authenticate)" → "Active" without a second click. | ||
| let (final_success, final_message) = if success { | ||
| match ext_mgr.activate(&flow.extension_name).await { | ||
| Ok(result) => (true, result.message), | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| extension = %flow.extension_name, | ||
| error = %e, | ||
| "Auto-activation after OAuth failed" | ||
| ); | ||
| ( | ||
| false, | ||
| format!( | ||
| "{} authenticated but activation failed: {}. Try activating manually.", | ||
| flow.display_name, e | ||
| ), | ||
| ) | ||
| } | ||
| } | ||
| } else { | ||
| (false, message) | ||
| }; | ||
|
|
||
| // Broadcast SSE event to notify the web UI | ||
| if let Some(ref sender) = flow.sse_sender { | ||
| let _ = sender.send(SseEvent::AuthCompleted { | ||
| extension_name: flow.extension_name, | ||
| success, | ||
| message, | ||
| success: final_success, | ||
| message: final_message.clone(), | ||
| }); | ||
| } | ||
|
|
||
| let html = oauth_defaults::landing_html(&flow.display_name, success); | ||
| let html = oauth_defaults::landing_html(&flow.display_name, final_success); |
There was a problem hiding this comment.
When auto-activation fails, this sets final_success to false and uses that for both the SSE AuthCompleted.success and the landing page. That reports the OAuth step as "failed" even though tokens were successfully exchanged and stored, which is misleading and may cause the UI to regress to an unauthenticated state. Consider keeping success tied to OAuth/token persistence, and reporting activation failure separately (or include activation status in the message without flipping overall auth success).
There was a problem hiding this comment.
Fixed in c8cc515. OAuth success is now kept independent of activation — tokens are already stored, so auth succeeded. The SSE event and landing page reflect the actual OAuth result, with activation failure reported in the message text only.
There was a problem hiding this comment.
Fixed in c8cc515. OAuth success is now independent of activation — tokens are already stored. Activation failure is reported in the message text only, without flipping the auth success status.
- Only fall back to manual token entry on AuthNotSupported, propagate real errors from auth_mcp_build_url() instead of masking them - Use mcp:-prefixed provider string in PendingOAuthFlow for consistency with CLI MCP auth token storage - Only persist client_id_secret_name for DCR flows (not pre-configured OAuth) - Fix gateway_callback_redirect_uri to use /oauth/callback path - Bypass exchange proxy when flow has RFC 8707 resource parameter - Remove client_id double-prefix in oauth callback handler - Remove weak tests that didn't exercise production logic - Add clarifying comments for exchange_oauth_code delegation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nses scoping - OAuth success is now reported accurately even when auto-activation fails (tokens are already stored, so auth succeeded) - E2E test waits for turn1_count + 1 responses to ensure turn-2 behavior is actually observed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 2. Load trace fixture. | ||
| let trace = | ||
| LlmTrace::from_file(format!("{FIXTURES}/mcp_extension_lifecycle.json")).unwrap(); | ||
|
|
There was a problem hiding this comment.
This test loads a trace fixture but never calls rig.verify_trace_expects(...) (or run_and_verify_trace) to assert the fixture’s declarative expectations (tool order, min responses, etc.). As written, failures like missing/extra tool calls could slip through as long as the ad-hoc assertions still pass. Consider verifying the trace expects against the accumulated responses after turn 2.
| async fn gateway_callback_redirect_uri(&self) -> Option<String> { | ||
| use crate::cli::oauth_defaults; | ||
| if oauth_defaults::use_gateway_callback() { | ||
| return Some(format!("{}/oauth/callback", oauth_defaults::callback_url())); |
There was a problem hiding this comment.
gateway_callback_redirect_uri() appends /oauth/callback onto oauth_defaults::callback_url(), but callback_url() is an env-controlled string and can already include a path. If it already ends with /oauth/callback, this will generate a duplicated redirect URI (e.g. .../oauth/callback/oauth/callback) and break hosted auth. Consider parsing with url::Url and only appending the path segment when needed (or documenting/enforcing that IRONCLAW_OAUTH_CALLBACK_URL must be a base origin).
| return Some(format!("{}/oauth/callback", oauth_defaults::callback_url())); | |
| let callback = oauth_defaults::callback_url(); | |
| // Try to parse and only append `/oauth/callback` when needed. | |
| if let Ok(mut url) = url::Url::parse(&callback) { | |
| let path = url.path(); | |
| if path.ends_with("/oauth/callback") { | |
| // Already points at the desired callback path; use as-is. | |
| return Some(callback); | |
| } | |
| if path.is_empty() || path == "/" { | |
| // No meaningful path yet; set it directly. | |
| url.set_path("/oauth/callback"); | |
| } else { | |
| // Append the callback segment to the existing path. | |
| let new_path = format!("{}/oauth/callback", path.trim_end_matches('/')); | |
| url.set_path(&new_path); | |
| } | |
| return Some(url.to_string()); | |
| } | |
| // Fallback: preserve previous behavior with basic trailing slash handling. | |
| let base = callback.trim_end_matches('/'); | |
| return Some(format!("{}/oauth/callback", base)); |
| let oauth_result = oauth_defaults::build_oauth_url( | ||
| &metadata.authorization_endpoint, | ||
| &client_id, | ||
| &redirect_uri, | ||
| &metadata.scopes_supported, | ||
| Some(&pkce), | ||
| &std::collections::HashMap::new(), | ||
| None, | ||
| &scopes, | ||
| true, // Always use PKCE for MCP | ||
| &extra_params, |
There was a problem hiding this comment.
In auth_mcp_build_url, the discovered authorization_endpoint (and token endpoint via flow.token_url) is used to build an auth URL returned to the frontend without applying the same endpoint safety checks used in the CLI flow (authorize_mcp_server validates HTTPS/non-local via SSRF/phishing guard). This makes it easier for a malicious MCP server to supply an unsafe/phishing authorization URL. Consider adding a shared public validator in crate::tools::mcp::auth and rejecting/errored auth when the discovered endpoints fail validation.
| let platform_state = oauth_defaults::build_platform_state(&expected_state); | ||
| let auth_url = if platform_state != expected_state { | ||
| oauth_result.url.replace( | ||
| &format!("state={}", urlencoding::encode(&expected_state)), | ||
| &format!("state={}", urlencoding::encode(&platform_state)), |
There was a problem hiding this comment.
The state rewrite for platform routing is done with a raw string replace() on the full URL. This is brittle (e.g., if the substring happens to appear elsewhere in the URL or if query param ordering/encoding changes) and makes the logic harder to reason about. Prefer parsing the URL and updating just the state query parameter via url::Url so it’s guaranteed to be correct.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Code review for fix(mcp): open MCP OAuth in same browser as gateway
Verdict: Approve
This is a well-structured fix for a real UX problem (MCP OAuth opening in the OS default browser instead of the browser running the gateway UI). The changes are clean, well-commented, and follow existing patterns.
What I reviewed:
-
Code correctness and safety
- No
.unwrap()in production code. All error paths use propermap_errwith context. AtomicBoolforgateway_modeusesRelease/Acquireordering -- correct for cross-thread visibility.PendingOAuthFlowis properly populated with all fields including the newresourceandclient_id_secret_name.- The
EnvGuardhelper in tests correctly restores/removes env vars on drop.
- No
-
Error handling
- New
AuthNotSupportedvariant inExtensionErrorcleanly distinguishes "server doesn't do OAuth" from "OAuth failed" -- this is the right fix for the root cause whereActivationFailedwas returned for 401s. exchange_oauth_code_with_resource()properly delegates from the existingexchange_oauth_code()so callers without RFC 8707 needs are unaffected.- OAuth callback handler correctly reports auth success independently of auto-activation failure (tokens are stored regardless).
- New
-
Code conventions
- Follows existing patterns:
thiserrorfor errors,Arc<RwLock<>>for shared state,map_errwith context strings. should_use_gateway_mode()has a clear priority chain (explicit flag > env var > tunnel URL) with good doc comments.- The approval status refactor (moving
ApprovalNeededfromagent_loop.rstothread_ops.rs) is correct -- the canonical sender is now in one place instead of two.
- Follows existing patterns:
-
Security
- RFC 8707
resourceparameter scopes tokens to specific MCP servers -- good security practice. - Gateway mode correctly bypasses the exchange proxy when resource param is present (proxy doesn't forward it).
client_id_secret_nameis only persisted for DCR flows (not pre-configured OAuth), which is correct.tool_authskipping approval in gateway mode is safe since it only returns a URL, not launching a process.
- RFC 8707
-
Testing
- Good coverage: gateway mode detection tests (tunnel URL, loopback, explicit enable), redirect URI construction, RFC 8707 resource param in auth URL.
- Full E2E lifecycle test with mock MCP server exercising search -> install -> OAuth -> activate -> tool calls.
- Mock MCP server is well-implemented with proper JSON-RPC, OAuth discovery, and Bearer auth validation.
Minor observations (non-blocking):
- The
msg.contains("401")string matching for detecting auth errors is fragile (could match on unrelated "401" in error messages). Consider matching on a typed error variant fromMcpClientin a follow-up if the MCP client surfaces HTTP status codes. - The
.githooks/pre-pushaddition is unrelated to the MCP OAuth fix but is fine to include.
All CI checks pass. Well done.
* fix(mcp): use gateway callback for MCP OAuth so auth opens in same browser When MCP OAuth is triggered from the web gateway, the auth URL was being opened via `open::that()` which launches the OS default browser instead of the browser already running the gateway UI. This changes the MCP OAuth flow to use the same gateway callback pattern as WASM extensions: in gateway mode, the auth URL is returned to the frontend via SSE and opened with `window.open()`, keeping the user in the same browser. Also adds RFC 8707 `resource` parameter support to the gateway token exchange path, scoping issued tokens to the correct MCP server. Closes nearai#299 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: cargo fmt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): persist DCR client_id in gateway OAuth callback for token refresh The gateway callback handler stored access and refresh tokens but not the DCR client_id. When the token expired, refresh failed with "No client ID found" because get_client_id() could not find it in secrets. Adds client_id_secret_name to PendingOAuthFlow so the gateway callback handler persists the client_id alongside the tokens, matching the behavior of the CLI flow in authorize_mcp_server(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): return AuthRequired on 401 so activate triggers OAuth flow activate_mcp() returned ActivationFailed for all errors including 401 auth responses, so the activate handler never triggered the OAuth flow. Now 401/auth errors return AuthRequired, which the handler detects and redirects to the OAuth flow — matching the WASM extension pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): fix gateway OAuth flow, approval cards, and auto-activation - Add explicit gateway_mode flag on ExtensionManager (set at startup by web gateway) so MCP OAuth returns auth URLs to the frontend instead of calling open::that() on the server machine. - Auto-activate extensions after successful OAuth callback so the UI transitions from "Activate" to "Active" without a second click. - Send ApprovalNeeded status (not generic "Awaiting approval") from thread_ops.rs for all three NeedApproval paths so the web UI shows approval cards for deferred tool calls. - Remove duplicate ApprovalNeeded send from agent_loop.rs (thread_ops.rs is now the canonical sender). - Skip approval for tool_auth in gateway mode since it only returns a URL. - Revert fragile active-server detection heuristic from system prompt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings - Use Release/Acquire ordering for gateway_mode AtomicBool instead of Relaxed to ensure visibility across threads. - Report activation failure as error in OAuth callback SSE event instead of silently falling back to the success message. - Fix EnvGuard::drop to remove env var when original was unset. - Replace hardcoded /tmp/ path with std::env::temp_dir() in test helper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(mcp): add E2E trace test for MCP extension lifecycle with mock server Add a full MCP extension lifecycle E2E test that exercises: - Turn 1: tool_search → tool_install → text (extension discovery and install) - Token injection + activate (simulating OAuth completion) - Turn 2: MCP tool calls (notion-search → notion-fetch → text) Includes a mock MCP server (tests/support/mock_mcp_server.rs) with OAuth discovery, DCR, token exchange, and JSON-RPC endpoints. The mock server validates Bearer auth and serves pre-configured tool responses. Also adds inject_registry_entry() to ExtensionManager for test use and exposes extension_manager from TestRig. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings (round 2) - Only fall back to manual token entry on AuthNotSupported, propagate real errors from auth_mcp_build_url() instead of masking them - Use mcp:-prefixed provider string in PendingOAuthFlow for consistency with CLI MCP auth token storage - Only persist client_id_secret_name for DCR flows (not pre-configured OAuth) - Fix gateway_callback_redirect_uri to use /oauth/callback path - Bypass exchange proxy when flow has RFC 8707 resource parameter - Remove client_id double-prefix in oauth callback handler - Remove weak tests that didn't exercise production logic - Add clarifying comments for exchange_oauth_code delegation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: keep OAuth success independent of activation, fix wait_for_responses scoping - OAuth success is now reported accurately even when auto-activation fails (tokens are already stored, so auth succeeded) - E2E test waits for turn1_count + 1 responses to ensure turn-2 behavior is actually observed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* fix(mcp): use gateway callback for MCP OAuth so auth opens in same browser When MCP OAuth is triggered from the web gateway, the auth URL was being opened via `open::that()` which launches the OS default browser instead of the browser already running the gateway UI. This changes the MCP OAuth flow to use the same gateway callback pattern as WASM extensions: in gateway mode, the auth URL is returned to the frontend via SSE and opened with `window.open()`, keeping the user in the same browser. Also adds RFC 8707 `resource` parameter support to the gateway token exchange path, scoping issued tokens to the correct MCP server. Closes nearai#299 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: cargo fmt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): persist DCR client_id in gateway OAuth callback for token refresh The gateway callback handler stored access and refresh tokens but not the DCR client_id. When the token expired, refresh failed with "No client ID found" because get_client_id() could not find it in secrets. Adds client_id_secret_name to PendingOAuthFlow so the gateway callback handler persists the client_id alongside the tokens, matching the behavior of the CLI flow in authorize_mcp_server(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): return AuthRequired on 401 so activate triggers OAuth flow activate_mcp() returned ActivationFailed for all errors including 401 auth responses, so the activate handler never triggered the OAuth flow. Now 401/auth errors return AuthRequired, which the handler detects and redirects to the OAuth flow — matching the WASM extension pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): fix gateway OAuth flow, approval cards, and auto-activation - Add explicit gateway_mode flag on ExtensionManager (set at startup by web gateway) so MCP OAuth returns auth URLs to the frontend instead of calling open::that() on the server machine. - Auto-activate extensions after successful OAuth callback so the UI transitions from "Activate" to "Active" without a second click. - Send ApprovalNeeded status (not generic "Awaiting approval") from thread_ops.rs for all three NeedApproval paths so the web UI shows approval cards for deferred tool calls. - Remove duplicate ApprovalNeeded send from agent_loop.rs (thread_ops.rs is now the canonical sender). - Skip approval for tool_auth in gateway mode since it only returns a URL. - Revert fragile active-server detection heuristic from system prompt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings - Use Release/Acquire ordering for gateway_mode AtomicBool instead of Relaxed to ensure visibility across threads. - Report activation failure as error in OAuth callback SSE event instead of silently falling back to the success message. - Fix EnvGuard::drop to remove env var when original was unset. - Replace hardcoded /tmp/ path with std::env::temp_dir() in test helper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(mcp): add E2E trace test for MCP extension lifecycle with mock server Add a full MCP extension lifecycle E2E test that exercises: - Turn 1: tool_search → tool_install → text (extension discovery and install) - Token injection + activate (simulating OAuth completion) - Turn 2: MCP tool calls (notion-search → notion-fetch → text) Includes a mock MCP server (tests/support/mock_mcp_server.rs) with OAuth discovery, DCR, token exchange, and JSON-RPC endpoints. The mock server validates Bearer auth and serves pre-configured tool responses. Also adds inject_registry_entry() to ExtensionManager for test use and exposes extension_manager from TestRig. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review findings (round 2) - Only fall back to manual token entry on AuthNotSupported, propagate real errors from auth_mcp_build_url() instead of masking them - Use mcp:-prefixed provider string in PendingOAuthFlow for consistency with CLI MCP auth token storage - Only persist client_id_secret_name for DCR flows (not pre-configured OAuth) - Fix gateway_callback_redirect_uri to use /oauth/callback path - Bypass exchange proxy when flow has RFC 8707 resource parameter - Remove client_id double-prefix in oauth callback handler - Remove weak tests that didn't exercise production logic - Add clarifying comments for exchange_oauth_code delegation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: keep OAuth success independent of activation, fix wait_for_responses scoping - OAuth success is now reported accurately even when auto-activation fails (tokens are already stored, so auth succeeded) - E2E test waits for turn1_count + 1 responses to ensure turn-2 behavior is actually observed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
open::that()which launches the OS default browser — not the browser already running the gateway UIwindow.open()in the same browserresourceparameter to the gateway token exchange path, scoping issued tokens to the correct MCP serverresourcefield toPendingOAuthFlowso the gateway callback handler can include it in token exchangeChanges
src/extensions/manager.rs:auth_mcp()checksuse_gateway_callback()first;auth_mcp_build_url()rewritten to storePendingOAuthFlowin gateway mode with proper CSRF state, PKCE, platform routing, and resource parametersrc/cli/oauth_defaults.rs: Addedresourcefield toPendingOAuthFlow; addedexchange_oauth_code_with_resource()for RFC 8707 supportsrc/channels/web/server.rs: Gateway callback handler passesflow.resourcethrough to token exchangeTest plan
cargo clippy— zero warningscargo test— 2946 tests passingtest_build_oauth_url_includes_resource_via_extra_paramsverifies resource param is URL-encoded in auth URLCloses #299
🤖 Generated with Claude Code