Skip to content

fix(client): preserve existing query params on OAuth authorization_endpoint#2779

Open
Bartok9 wants to merge 1 commit into
modelcontextprotocol:mainfrom
Bartok9:fix/2776-auth-endpoint-query
Open

fix(client): preserve existing query params on OAuth authorization_endpoint#2779
Bartok9 wants to merge 1 commit into
modelcontextprotocol:mainfrom
Bartok9:fix/2776-auth-endpoint-query

Conversation

@Bartok9
Copy link
Copy Markdown

@Bartok9 Bartok9 commented Jun 4, 2026

Summary

  • The OAuth authorization-code grant now preserves any query parameters already present on the server-advertised authorization_endpoint instead of producing a malformed double-? URL.
  • User-facing impact: clients can complete OAuth against servers whose authorization_endpoint carries query params (e.g. Salesforce's .../authorize?prompt=select_account).

Motivation

Closes #2776.

_perform_authorization_code_grant built the redirect URL with:

authorization_url = f"{auth_endpoint}?{urlencode(auth_params)}"

When the server's authorization_endpoint already has a query string, this yields an invalid URL with two ? separators. The reporter's real-world example:

  • Salesforce advertises https://test.salesforce.com/services/oauth2/authorize?prompt=select_account
  • The client then navigated to ...authorize?prompt=select_account?response_type=code&... — the second ? is invalid, so the existing prompt param 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:

  • urlparse the endpoint, merge its existing query (parse_qsl) with the flow-generated auth_params, then urlunparse back into a single well-formed query string.
  • Flow-generated params win on key conflicts (the client's 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 as Mapping[str, str | None]).

Diff: 2 files, +103/-3.

Tests

Added TestAuthorizationEndpointWithQuery in tests/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; server prompt survives 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 xfailed
  • uv run pyright src/mcp/client/auth/oauth2.py — 0 errors
  • uv run ruff format / ruff check — clean
  • Did NOT change any unrelated behavior; flow param precedence and None handling preserve the previous effective output for endpoints without a query string.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OAuth handler doesn't support redirect URLs with params

1 participant