fix(client/auth): bind negative-check verdicts to an observed prerequisite - #483
Open
OfficialAbhinavSingh wants to merge 1 commit into
Conversation
…isite A negative check that reads only the final verdict scores SUCCESS whenever the client fails to reach the requirement at all: "declined" and "never got far enough to decide" are the same observation. auth/resource-mismatch decided on !authorizationRequestMade alone, so a client that threw before making a single HTTP request passed it. Gate each verdict on the prerequisite the harness already records: prm-pathbased-requested for the PRM resource comparison, and authorization-server-metadata for the RFC 9207 iss comparisons, which SEP-2468 conditions on the issuer recorded from the validated metadata document. When the prerequisite is not observed the requirement was never exercised, so report it via notTestable() (modelcontextprotocol#248) rather than SUCCESS. auth/metadata-issuer-mismatch already gated on the metadata fetch but reported the unreached case as a plain violation; it now reports untestable too. All six checks record propertyReached and stopReason in details. Adds an inert client and a vitest case, so the checks are shown to catch something and not merely to avoid false-positiving.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #467.
What
Negative checks in the client auth suite bound SUCCESS to a verdict alone, so "the client declined" and "the client never got far enough to decide" were the same observation.
auth/resource-mismatchis the clearest case: its verdict is!authorizationRequestMade, and a client that throws before making a single HTTP request satisfies it.Before, against a client that performs no discovery request at all:
After:
auth/metadata-issuer-mismatchalready gated its verdict on the metadata fetch; the other five checks did not. This makes the policy uniform and routes the unreached case throughnotTestable()(#248) instead of SUCCESS, since the requirement was not violated — it was never exercised.Changes
resource-mismatch.ts— verdict requiresprm-pathbased-requested; absent it, untestable.issuer-parameter.ts— the four RFC 9207 rejection checks requireauthorization-server-metadataandauthReached. SEP-2468 conditions each of them on the issuer recorded "from the selected authorization server validated metadata document", so a client that never fetched it cannot have made the comparison. Policy lives in oneissRejectionCheckfunction: the duplication is what let five of six sites drift.issuer-parameter.ts—metadata-issuer-mismatchalready detected the unreached case but reported it as a plain FAILURE; it now reports untestable.propertyReachedandstopReasonindetails, per the machine-readable shape in the issue.examples/clients/typescript/auth-test-inert.ts— inert client, plus a vitest case asserting the untestable report.What this does not close
A client that receives the redirect and then aborts before the token request for an unrelated reason remains indistinguishable from one that rejected on
iss. Separating those needs a signal from inside the client that a black-box harness does not have. This closes the "never reached the requirement at all" class.Validation
npm test— 525 tests passed, 44 filesnpm run typecheck,npx eslint src/ examples/,npx prettier --check .— all cleannpm run buildexamples/clients/typescript/everything-client.ts(built on@modelcontextprotocol/sdk), and the prerequisite checks fire for it —prm-pathbased-requestedandauthorization-server-metadataboth SUCCESS.❌ OVERALL: FAILED, exit 1. The same client scored SUCCESS before this change.Written with Claude Code, against this specific issue rather than a generic bug-hunt prompt.