fix(mcp): fail closed when the JWT verifier has no pinned algorithm#41296
fix(mcp): fail closed when the JWT verifier has no pinned algorithm#41296rusackas wants to merge 1 commit into
Conversation
DetailedJWTVerifier.load_access_token only rejected the "none" algorithm and otherwise compared the token algorithm against self.algorithm — but only when self.algorithm was truthy. The pinned value is currently always present because the upstream JWTVerifier defaults it to RS256, so the verifier relied on that upstream default for a security property. Make the pinning explicit: reject tokens when no algorithm is pinned rather than validating against an unconstrained algorithm family. This removes the implicit dependency on the upstream default and fails closed if the pinned algorithm is ever absent. Adds a unit test asserting an unpinned verifier rejects signed tokens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code Review Agent Run #7088bdActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_unpinned_algorithm_is_rejected(hs256_verifier): |
There was a problem hiding this comment.
Suggestion: Add full type annotations to the new async test function, including the hs256_verifier parameter type and an explicit -> None return type. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added Python async test function, and it omits both a parameter type hint for
hs256_verifier and an explicit -> None return annotation. That matches the custom rule
requiring new Python functions to be fully typed.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_jwt_verifier.py
**Line:** 87:87
**Comment:**
*Custom Rule: Add full type annotations to the new async test function, including the `hs256_verifier` parameter type and an explicit `-> None` return type.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The suggestion to add type annotations to the new async test function is correct and aligns with best practices for maintaining a typed codebase. You can resolve this by updating the function signature in @pytest.mark.asyncio
async def test_unpinned_algorithm_is_rejected(hs256_verifier: Any) -> None:(Note: Ensure There are no other comments on this PR to address. Would you like me to proceed with any other changes? tests/unit_tests/mcp_service/test_jwt_verifier.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41296 +/- ##
==========================================
- Coverage 64.34% 64.34% -0.01%
==========================================
Files 2653 2653
Lines 144952 144957 +5
Branches 33433 33434 +1
==========================================
- Hits 93272 93267 -5
- Misses 49996 50004 +8
- Partials 1684 1686 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
SUMMARY
DetailedJWTVerifier.load_access_tokenrejected thenonealgorithm and otherwise compared the token'salgagainstself.algorithm— but only inside anif self.algorithmguard. The pinned value is currently always populated because the upstreamJWTVerifierdefaultsalgorithmtoRS256, so the verifier was effectively relying on that upstream default to keep the algorithm pinned.This makes the pinning explicit and self-contained: if no algorithm is pinned, the verifier now rejects the token (fail closed) rather than validating against whatever algorithm family the key/library would otherwise permit. It removes the implicit dependency on the upstream default.
This is a small defense-in-depth change — under current behavior
self.algorithmis always set — so there is no functional change for normally constructed verifiers.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend verification behavior.
TESTING INSTRUCTIONS
Unit test added in
tests/unit_tests/mcp_service/test_jwt_verifier.py(test_unpinned_algorithm_is_rejected): an unpinned verifier rejects a signed token with reason"No signing algorithm pinned".Run:
pytest tests/unit_tests/mcp_service/test_jwt_verifier.pyADDITIONAL INFORMATION