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

[Bartok9]

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.

[Bartok9]

CI note — rebased onto current main. The only remaining red cell (test (3.14, lowest-direct, …)) is a pre-existing, main-wide failure unrelated to this PR: tests/interaction/transports/test_stdio.py::test_tool_call_and_notification_round_trip… asserts byte-exact child stderr, but anyio==4.9 (the lowest-direct floor) has return inside a finally: in from_thread.py:119, which Python 3.14 emits as SyntaxWarning to stderr and pollutes the snapshot. This PR does not touch stdio or that test. Full root-cause + the 6 parallel fixes already in flight are consolidated in #2734 (comment). All other cells (incl. the previously-flaky termination test, now run in-process via #2767) are green.

[Bartok9]

Rebased onto latest upstream `main` to resolve the merge conflict (an import collision in `tests/client/test_auth.py` — kept both upstream's `OAuthTokenError` and this PR's `_build_authorization_url` import; `oauth2.py` auto-merged cleanly). The OAuth query-param fix is unchanged.

Local verification on the rebased branch:

• `uv run pytest tests/client/test_auth.py` — 136 passed, 1 xfailed (incl. the 4 new `TestAuthorizationEndpointWithQuery` cases)
• `uv run ruff check` / `ruff format --check` — clean
• `uv run pyright src/mcp/client/auth/oauth2.py` — 0 errors

PR now shows mergeable. Ready for review.

[STiFLeR7]

Reviewed the merge logic in _build_authorization_url. The precedence is correctly safety-conscious: auth_params (flow-generated redirect_uri, state, client_id, code_challenge, etc.) always wins over anything already on the endpoint's query string, since it's applied via .update() after seeding from parse_qsl. That closes off a parameter-pollution risk a naive merge could introduce (e.g. a misbehaving/misconfigured server-advertised authorization_endpoint overriding redirect_uri or state) — confirmed by checking the call site in _perform_authorization_code_grant, where all the security-relevant fields are present in auth_params.

One edge case worth a look: dict(parse_qsl(parsed.query, keep_blank_values=True)) silently collapses duplicate keys in the existing endpoint query string down to the last occurrence. If a server ever advertises something like ?scope=a&scope=b (multi-value query params are rare for OAuth authorization endpoints but not disallowed by the URL spec), only scope=b survives. Given how narrow the practical impact is or OAuth authorization endpoints specifically, this is likely fine to ship as-is, but might be worth either a one-line comment noting the tradeoff, or confirming no provider in the wild actually does this.

Test coverage otherwise looks solid — good catch on asserting url.count("?") == 1 to pin the exact bug (the old f-string producing a double ?).

[Bartok9]

Thanks for the careful review, @STiFLeR7 — the precedence read is exactly right.

Good catch on the duplicate-key edge case. dict(parse_qsl(...)) did collapse multi-value existing params (?scope=a&scope=b → only scope=b). Rather than just document the tradeoff, I fixed it: _build_authorization_url now builds the merged query from an ordered pair list — it keeps every existing endpoint pair (including duplicates) except keys the flow overrides, then appends the authoritative auth_params. So multi-value server params survive, and flow params still win on conflict.

Added a regression test test_build_authorization_url_preserves_multi_value_existing_query asserting scope=[a, b] survives alongside the flow's response_type, with exactly one ?.

Verification on the pushed commit:

• uv run pytest tests/client/test_auth.py -k AuthorizationEndpointWithQuery — 5 passed
• uv run ruff check / ruff format --check — clean
• uv run pyright src/mcp/client/auth/oauth2.py — 0 errors

[cubic-dev-ai]

No issues found across 2 files

_{Re-trigger cubic}

[maxisbey]

Thanks for the PR, and sorry it sat here without a proper review.

We're closing most of the open PR backlog. v2 is out and changed a lot of the SDK, so many older PRs no longer apply as written, and we're a small team that realistically doesn't have the capacity to work through the rest.

If this still matters to you on v2, the most useful thing you can do is open an issue (or comment on the existing one) with your use case and a repro. Hearing why it matters to you is what we use to decide what to prioritise.

_{AI Disclaimer}

[Bartok9]

Thanks for the note, and no worries on the wait.

Understood that v2 moved and the backlog is being closed rather than reviewed in place. I won't reopen this PR.

The original case (authorization URL built with a second ? when authorization_endpoint already has query params) is tracked on #2776, with a later v2-era report on #3505. I'll follow those rather than this PR.