fix: pass conformance auth scenarios, add RFC 8707 resource validation#2010
fix: pass conformance auth scenarios, add RFC 8707 resource validation#2010felixweinberger wants to merge 3 commits intomainfrom
Conversation
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
You're wrong @claude - the CLAUDE.md literally says not to use Test Classes but this PR has them. |
|
Claude finished @felixweinberger's task in 46s —— View job Code Review - CLAUDE.md Violations FoundYou're absolutely right - I apologize for the incorrect review. This PR does violate CLAUDE.md guidelines. Issues Foundtests/client/test_auth.py:822-951 - Uses Test classes which violates CLAUDE.md:
The CLAUDE.md explicitly states:
And:
Looking at Additional Issuetests/client/test_auth.py:890 - The review comment from @Kludex suggests using snapshots instead of manual assertions, which would align better with the patterns in RecommendationThe tests need to be refactored:
Would you like me to refactor these test classes into functions following the | |
| except json.JSONDecodeError: | ||
| pass |
There was a problem hiding this comment.
same comment as before: why wouldn't want to see the exception?
The conformance test suite was broken by @modelcontextprotocol/conformance@0.1.13 introducing new auth scenarios that require: 1. Pre-registered client credentials from MCP_CONFORMANCE_CONTEXT 2. RFC 8707 resource validation (PRM resource must match server URL) SDK changes: - Add _validate_resource_match() to OAuthClientProvider that validates the Protected Resource Metadata resource field matches the server URL before proceeding with the auth flow - Add validate_resource_url callback parameter for custom validation Conformance client changes: - Pre-load client credentials from MCP_CONFORMANCE_CONTEXT into token storage when available, allowing the existing flow to skip DCR when pre-registered credentials are present CI: bump conformance package from 0.1.10 to 0.1.13
Address review feedback: - Convert TestResourceValidation class to standalone test functions - Use inline_snapshot for assertion values
Reverts the disable from #2007 now that the conformance client supports the new auth scenarios.
cca61b9 to
d56bdbc
Compare
Summary
Fixes the broken conformance CI that is blocking all PRs with
client-conformancefailures.Motivation and Context
The conformance test suite was broken after
@modelcontextprotocol/conformance@0.1.13introduced new auth scenarios requiring:MCP_CONFORMANCE_CONTEXTThis was causing the
client-conformancecheck to fail on nearly all open PRs (thetools_callscenario returns 403 Forbidden). Builds on the work from #1999.How Has This Been Tested?
Breaking Changes
No breaking changes. The new
validate_resource_urlparameter onOAuthClientProvideris optional.Types of changes
Checklist
Additional context
Changes across 4 files:
src/mcp/client/auth/oauth2.py: Adds_validate_resource_match()method that validates PRM resource matches the server URL per RFC 8707 before proceeding with the OAuth flow. Adds optionalvalidate_resource_urlcallback parameter for custom validation logic..github/actions/conformance/client.py: Pre-loads client credentials fromMCP_CONFORMANCE_CONTEXTinto token storage when available, allowing the existing auth flow to skip DCR when pre-registered credentials are present..github/workflows/conformance.yml: Bumps conformance package from 0.1.10 to 0.1.13.tests/client/test_auth.py: AddsTestResourceValidationtest class with 6 tests covering resource validation (mismatched resources, matching resources, custom callbacks, trailing slash normalization, fallback behavior). Updates existing test resource URLs to match server URLs.