fix(client): preserve existing query params on OAuth authorization_endpoint#2779
Open
Bartok9 wants to merge 1 commit into
Open
fix(client): preserve existing query params on OAuth authorization_endpoint#2779Bartok9 wants to merge 1 commit into
Bartok9 wants to merge 1 commit into
Conversation
…dpoint Closes modelcontextprotocol#2776 The authorization code grant built the redirect URL with `f"{auth_endpoint}?{urlencode(auth_params)}"`, which produces an invalid URL when the server-advertised authorization_endpoint already carries a query string. For example Salesforce advertises `.../services/oauth2/authorize?prompt=select_account`, yielding `...authorize?prompt=select_account?response_type=code&...` (two `?` separators), so the client navigates to a malformed URL and the server rejects the request. Fix: parse the endpoint, merge its existing query params with the flow-generated auth_params (flow params win on conflict), and re-encode into a single well-formed query string. None-valued params are dropped rather than serialized as the literal "None". Tests: add TestAuthorizationEndpointWithQuery covering the helper (no/with/conflicting existing query) plus an end-to-end _perform_authorization_code_grant assertion that the captured redirect URL preserves the server param and stays well-formed. 101 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
authorization_endpointinstead of producing a malformed double-?URL.authorization_endpointcarries query params (e.g. Salesforce's.../authorize?prompt=select_account).Motivation
Closes #2776.
_perform_authorization_code_grantbuilt the redirect URL with:When the server's
authorization_endpointalready has a query string, this yields an invalid URL with two?separators. The reporter's real-world example:https://test.salesforce.com/services/oauth2/authorize?prompt=select_account...authorize?prompt=select_account?response_type=code&...— the second?is invalid, so the existingpromptparam is swallowed into the value and the request is rejected.Root cause
Blind string concatenation of
?+ encoded params, with no handling for a pre-existing query component on the endpoint.Fix
New
_build_authorization_url(auth_endpoint, auth_params)helper:urlparsethe endpoint, merge its existing query (parse_qsl) with the flow-generatedauth_params, thenurlunparseback into a single well-formed query string.response_type,state, PKCE challenge, etc. are authoritative).None-valued params are dropped rather than serialized as the literal string"None"(preserves prior effective behavior; keeps the signature type-correct asMapping[str, str | None]).Diff: 2 files, +103/-3.
Tests
Added
TestAuthorizationEndpointWithQueryintests/client/test_auth.py:test_build_authorization_url_no_existing_query— baseline, single?.test_build_authorization_url_preserves_existing_query— the OAuth handler doesn't support redirect URLs with params #2776 Salesforce case; serverpromptsurvives alongside flow params, exactly one?.test_build_authorization_url_flow_params_win_on_conflict— precedence rule.test_perform_authorization_preserves_endpoint_query— end-to-end through_perform_authorization_code_grant, asserting the captured redirect URL is well-formed.Verification
uv run pytest tests/client/test_auth.py— 100 passed, 1 xfaileduv run pyright src/mcp/client/auth/oauth2.py— 0 errorsuv run ruff format/ruff check— cleanNonehandling preserve the previous effective output for endpoints without a query string.