OAuth client: refresh before re-authorizing, and discover before refreshing - #3328
OAuth client: refresh before re-authorizing, and discover before refreshing#3328maxisbey wants to merge 5 commits into
Code review found 4 new issues (3 earlier findings still open)
Found 11 candidates, confirmed 7: 4 posted in this review, 3 from earlier reviews still open. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 3 |
| 🟡 Nit | 4 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/mcp/client/auth/oauth2.py:758 |
Security: 401-branch Step 5 still POSTs a refresh token of unconfirmed issuer provenance to whatever AS the resource ser |
| 🔴 Important | src/mcp/client/auth/oauth2.py:694 |
Legacy no-PRM path trusts the resource-server-origin ASM's self-declared issuer without validation (validate_metadata_is |
| 🟡 Nit | src/mcp/client/auth/oauth2.py:0 |
[quality] Step 4's CIMD comment "the issuer stamp is informational" is made false by this PR: the new _apply_issuer_bind |
| 🟡 Nit | tests/interaction/auth/_harness.py:0 |
[quality] connect_with_oauth's docstring ("in that case ... 'headless' [is] unused (the yielded HeadlessOAuth is never i |
Annotations
Check failure on line 758 in src/mcp/client/auth/oauth2.py
claude / Claude Code Review
Security: 401-branch Step 5 still POSTs a refresh token of unconfirmed issuer provenance to whatever AS the resource server's PRM currently names — for pre-registered/unstamped registrations always, and for CIMD across restarts because the SEP-2352 toke
Security: 401-branch Step 5 still POSTs a refresh token of unconfirmed issuer provenance to whatever AS the resource server's PRM currently names — for pre-registered/unstamped registrations always, and for CIMD across restarts because the SEP-2352 token drop in _apply_issuer_binding is memory-only (clear_tokens never touches storage). [also at: src/mcp/client/auth/oauth2.py:756 - Re-filing still-present security gap: for pre-registered/unstamped (non-CIMD) client_info, the new 401-branch Step
Check failure on line 694 in src/mcp/client/auth/oauth2.py
claude / Claude Code Review
Legacy no-PRM path trusts the resource-server-origin ASM's self-declared issuer without validation (validate_metadata_issuer at lines 684-685 is skipped when auth_server_url is None), so the SEP-2352 stamp check in _apply_issuer_binding is spoofable and t
Legacy no-PRM path trusts the resource-server-origin ASM's self-declared issuer without validation (validate_metadata_issuer at lines 684-685 is skipped when auth_server_url is None), so the SEP-2352 stamp check in _apply_issuer_binding is spoofable and the new Step-5 refresh (lines 756-758) silently POSTs the stored refresh token plus client secret to the forged metadata's token_endpoint — defeating issuer binding even for stamped, SDK-minted registrations, which the PRM path does protect (th