Repository navigation
Conversation
aheritier
added
area/mcp
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Issue #4590 is valid: dynamic registration omits token_endpoint_auth_method (which defaults to client_secret_basic), while the token exchange sends client_secret_post. This PR addresses the right root cause, but needs changes before merging.
Blocking findings:
- oauthflow.go:172 and :446 — OAuth Basic credentials must be individually application/x-www-form-urlencoded before SetBasicAuth. Raw credentials containing reserved characters produce incorrect headers. Please encode both credentials and add exchange/refresh tests with reserved characters.
- oauth.go:236,259 and oauthflow.go:387–388 — When token_endpoint_auth_methods_supported is omitted, RFC 8414 defaults to client_secret_basic, not client_secret_post. Please preserve that default and test registration against a Basic-only server with omitted metadata.
- Add regression coverage for negotiated refresh: persist/reload the method, refresh an expired token, assert Basic headers and absence of form credentials, and verify the method survives refresh. Also cover none, legacy-empty compatibility, and standalone propagation.
Should fix:
4. oauthflow.go:413 — Reject unsupported negotiated methods (for example client_secret_jwt/private_key_jwt) instead of silently sending client_secret_post, and fail clearly when no supported method is advertised.
CI at reviewed head 6445649 is not green: lint and the required gate failed. Replace context.Background() with t.Context() in authmethod_test.go at lines 29, 54, and 79.
Validation: source inspection and read-only discovery confirm the reported mismatch and server metadata. No authenticated Structured login or patched-binary execution was performed. Local tests were baseline-only; PR-head test results were taken from CI.
DCR left token_endpoint_auth_method unset, so servers defaulted to client_secret_basic while the token call posted the secret. Ask for a method we can send, and use whatever registration returns for the code exchange and refresh. Signed-off-by: Dean Chen <862469039@qq.com>
RFC 8414 treats a missing token_endpoint_auth_methods_supported list as client_secret_basic. Encode each basic credential before SetBasicAuth, and reject jwt methods instead of posting the secret. An empty stored method still posts, so tokens from before this field keep working. Signed-off-by: Dean Chen <862469039@qq.com>
6445649 to
53165ed
Compare
|
omitted metadata stays basic now. basic user and secret are encoded on their own before the header, so a colon or a space doesn't get folded into user:pass. refresh keeps the method that was stored. empty still posts the secret, that's the tokens from before this field. none stays none. jwt methods error instead of quietly posting. those tests use t.Context() now. |
There was a problem hiding this comment.
🔵 Needs a closer look
Authentication changes span multiple login paths and need human review, with a registration-response fallback issue still unresolved.
1 open issue
What changed in this PR
Aligns MCP OAuth registration, token exchange, and refresh authentication to address #4590.
Changes:
- Selects and honors registered token authentication methods.
- Persists the method for refresh across login paths.
- Adds authentication-method and compatibility tests.
| File | Description |
|---|---|
| pkg/tools/mcp/oauthflow/types.go | Stores the authentication method. |
| pkg/tools/mcp/oauthflow/oauthflow.go | Implements method selection and credential placement. |
| pkg/tools/mcp/oauthflow/authmethod_test.go | Tests supported methods and legacy behavior. |
| pkg/tools/mcp/oauth.go | Propagates methods through transport flows. |
| pkg/tools/mcp/oauth_test.go | Tests exchange and refresh integration. |
| pkg/tools/mcp/oauth_login.go | Updates standalone login handling. |
| pkg/tools/mcp/oauth_login_test.go | Verifies standalone authentication behavior. |
| pkg/tools/mcp/oauth_helpers.go | Updates the registration wrapper signature. |
| pkg/runtime/remote_runtime.go | Propagates methods through remote OAuth. |
Review effort: Balanced
| if respBody.TokenEndpointAuthMethod != "" { | ||
| authMethod = respBody.TokenEndpointAuthMethod | ||
| } |
RFC 7591 treats a missing token_endpoint_auth_method on the registration response as client_secret_basic, not as agreement with the method we requested. Signed-off-by: Dean Chen <862469039@qq.com>
|
registration response that leaves the method out is basic now, even when we asked for post. a missing field is the rfc default, not the server agreeing with the request. lint also wanted those jwt rejection calls to stop using three blanks. |
DCR never set token_endpoint_auth_method, so a server that defaults to client_secret_basic rejected the token call. We were posting the secret. Registration now asks for post unless the server only offers basic or none, and the code exchange plus refresh follow whatever method comes back.
Fixes #4590