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

Closed
BA
24d ago
+293-310

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 #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.