-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix: pass conformance auth scenarios, add RFC 8707 resource validation #2010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 | |
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.