fix(authentication): Allow connecting to Oauth servers that use protected-resource fallback instead of the WWW-authenticate header#8148
Conversation
57e7b06 to
2756728
Compare
DOsinga
left a comment
There was a problem hiding this comment.
Good fix — the core logic correctly handles MCP servers that return bare HTTP 401 without a WWW-Authenticate header.
I merged main and trimmed the tests down to the two that exercise real code paths (AuthRequired and UnexpectedServerResponse with 401). The other five were just asserting that pattern matching and starts_with work as expected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d963a1e1fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(http_err) = error.downcast_ref::<StreamableHttpError<reqwest::Error>>() { | ||
| match http_err { | ||
| StreamableHttpError::AuthRequired(_) => true, | ||
| StreamableHttpError::UnexpectedServerResponse(body) => body.starts_with("HTTP 401"), |
There was a problem hiding this comment.
Restrict OAuth fallback beyond generic HTTP 401
should_attempt_oauth_fallback now treats any UnexpectedServerResponse that starts with HTTP 401 as OAuth-capable, so ordinary unauthorized responses (for example, bad API-key/header credentials on non-OAuth servers) will trigger the OAuth flow path. In this code path, a failed oauth_flow replaces the original transport error with a generic setup error, which both misdirects users into an irrelevant auth flow and hides the actionable server response they need to fix credentials.
Useful? React with 👍 / 👎.
af2b2af to
ea70df2
Compare
ce4a742 to
a386544
Compare
Some MCP servers (e.g. Datadog) return 401 without a WWW-Authenticate header, causing rmcp to surface the failure as an opaque "unexpected server response" rather than a typed AuthRequired error. Goose would then skip OAuth and silently continue without the extension. Add a string-based fallback in should_attempt_oauth_fallback that matches the rendered rmcp error message, enabling OAuth for these servers while preserving the typed-error path for standards-compliant servers that send WWW-Authenticate. Also broadens the UnexpectedServerResponse arm to match "HTTP 401" without requiring the trailing colon, and flattens the nested match into a single pattern for readability. Per the MCP draft auth spec, clients must support protected-resource discovery via well-known metadata when WWW-Authenticate is absent. This fallback bridges the gap until rmcp exposes a typed path for bare 401 responses. Signed-off-by: Cameron Yick <cameron.yick@datadoghq.com>
Covers all branches of the new OAuth fallback detection: - typed AuthRequired error (original path) - UnexpectedServerResponse with "HTTP 401" body prefix (new typed path) - string fallback when downcast to StreamableHttpError<reqwest::Error> fails - negative cases: HTTP 403, 401 not at body start, unrelated transport error, and non-transport ClientInitializeError variants Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Cameron Yick <cameron.yick@datadoghq.com>
Drop 5 tests that only assert pattern matching and starts_with behavior. Keep the two tests that exercise the actual code paths: AuthRequired and UnexpectedServerResponse with HTTP 401. Signed-off-by: Douwe Osinga <douwe@squareup.com> Signed-off-by: Cameron Yick <cameron.yick@datadoghq.com>
If a server returns a bare HTTP 401 without a WWW-Authenticate header, the OAuth fallback is attempted. Previously, if oauth_flow failed (e.g. the server is not OAuth-capable), the original transport error was discarded and replaced with a generic "auth error", hiding the actionable 401 body from the user. Now the original ClientInitializeError is surfaced when oauth_flow fails, so non-OAuth 401s (bad API key, wrong credentials) show the server response rather than misdirecting the user into an auth flow. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Cameron Yick <cameron.yick@datadoghq.com>
Signed-off-by: Cameron Yick <cameron.yick@datadoghq.com>
a386544 to
236babb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 236babb905
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let Some(http_err) = error.downcast_ref::<StreamableHttpError<reqwest::Error>>() { | ||
| match http_err { | ||
| StreamableHttpError::AuthRequired(_) => true, | ||
| StreamableHttpError::UnexpectedServerResponse(body) => body.starts_with("HTTP 401"), |
There was a problem hiding this comment.
Restrict OAuth fallback for generic HTTP 401 failures
should_attempt_oauth_fallback treats any UnexpectedServerResponse whose body starts with HTTP 401 as OAuth-capable, so non-OAuth servers that use API keys/basic auth and return plain 401s will still enter oauth_flow. In that scenario users are pushed through an irrelevant OAuth attempt (browser flow/discovery) before getting the original transport error, which is a behavior regression for unauthorized-but-non-OAuth endpoints and can significantly confuse authentication troubleshooting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think after tracing through the rmcp source, this is not an issue. start_authorization calls discover_metadata().await? before any browser interaction. That method tries to fetch OAuth server metadata from the target server , so if the server doesn't support OAuth, both discovery attempts fail and the error propagates via ? out of oauth_flow before a browser ever opens. The call site then reaches Err(_) => Ok(Box::new(client_res?)) and returns the original 401 error.
In other words, for a non-OAuth server returning a bare 401, the worst case is two extra failing HTTP requests to the discovery endpoints. The browser will not open, and the caller still gets the original error back.
* origin/main: (32 commits) docs: rework homepage and add aaif migration blog post (#8356) chore(aaif): rename a bunch of repository references (#8152) fix: use OPENAI_API_KEY secret for recipe security scanner (#8358) feat: configurable extension timeouts via ACP _meta and global default (#8295) fix: hide hidden extensions in UI (#8346) refactor: skills as its own platform ext (#8244) fix baseUrl (#8347) Fix desktop slash commands (#8341) fix(cli): display platform-correct secrets path in keyring config dialog (#8328) feat(acp): add reusable ACP provider controls (#8314) fix: resolve MDX compilation error in using-goosehints.md (#8332) fix: use v1beta1 API version for Google/MaaS models on GCP Vertex AI (#8278) docs: add MCP Roots guide (#8252) rust acp client for extension methods (#8227) fix: reconsolidate split tool-call messages to follow OpenAI format (#7921) fix: clean up MCP subprocesses after abrupt parent exit (#8242) build: raise default stack reserve to 8 MB (#8234) fix(config): honour GOOSE_DISABLE_KEYRING from config.yaml at startup (#8219) feat: add configurable fast_model for declarative providers (#8194) fix(authentication): Allow connecting to Oauth servers that use protected-resource fallback instead of the WWW-authenticate header (#8148) ...
Motivation
Some MCP servers (Datadog) return a bare
HTTP 401without aWWW-Authenticateheader, which rmcp surfaces asUnexpectedServerResponserather than a typedAuthRequirederror.The MCP draft authorization spec says clients must support discovery through
WWW-Authenticate(what Linear does) ORThe draft does not require the header to be present if the well-known metadata is available, but the Goose code seems to always require the header. Previous code only checked for the typed error, so these servers silently failed instead of triggering the OAuth flow.
Changes:
extract_auth_error→should_attempt_oauth_fallback(returnsbool; theOption<&AuthRequiredError>return value was never used beyond.is_some())UnexpectedServerResponsewhose body starts with"HTTP 401"error.to_string().contains("unexpected server response: HTTP 401")) for servers whose transport wraps the error in a type that fails thedowncast_ref::<StreamableHttpError<reqwest::Error>>()Testing
Added unit tests to
extension_manager.rsfor these branchestest_oauth_fallback_on_typed_auth_requiredAuthRequired(regression guard for original path)test_oauth_fallback_on_unexpected_response_http_401_prefixUnexpectedServerResponsestarting with"HTTP 401"test_oauth_fallback_via_string_when_downcast_failsNonetest_no_oauth_fallback_on_http_403test_no_oauth_fallback_when_401_not_at_body_startstarts_withvscontains(is a mutation guard)test_no_oauth_fallback_on_unrelated_transport_error_ => falsearmtest_no_oauth_fallback_on_non_transport_client_error_ => falsearmRelated Issues
Relates to #5107 (comment)
Screenshots/Demos (for UX changes)