Skip to content

fix(client/auth): bind negative-check verdicts to an observed prerequisite - #483

Open
OfficialAbhinavSingh wants to merge 1 commit into
modelcontextprotocol:mainfrom
OfficialAbhinavSingh:fix/reason-bound-negative-checks
Open

fix(client/auth): bind negative-check verdicts to an observed prerequisite#483
OfficialAbhinavSingh wants to merge 1 commit into
modelcontextprotocol:mainfrom
OfficialAbhinavSingh:fix/reason-bound-negative-checks

Conversation

@OfficialAbhinavSingh

Copy link
Copy Markdown

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-mismatch is 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:

SUCCESS  resource-mismatch-rejected
"Client correctly rejected authorization when PRM resource does not match server URL"

After:

FAILURE  resource-mismatch-rejected
Error: Not testable: client never requested the Protected Resource Metadata
document, so it never read the resource value it was required to validate

auth/metadata-issuer-mismatch already gated its verdict on the metadata fetch; the other five checks did not. This makes the policy uniform and routes the unreached case through notTestable() (#248) instead of SUCCESS, since the requirement was not violated — it was never exercised.

Changes

  • resource-mismatch.ts — verdict requires prm-pathbased-requested; absent it, untestable.
  • issuer-parameter.ts — the four RFC 9207 rejection checks require authorization-server-metadata and authReached. 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 one issRejectionCheck function: the duplication is what let five of six sites drift.
  • issuer-parameter.tsmetadata-issuer-mismatch already detected the unreached case but reported it as a plain FAILURE; it now reports untestable.
  • All six record propertyReached and stopReason in details, 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 files
  • npm run typecheck, npx eslint src/ examples/, npx prettier --check . — all clean
  • npm run build
  • Real SDK through the CLI runner: all six affected scenarios PASS against examples/clients/typescript/everything-client.ts (built on @modelcontextprotocol/sdk), and the prerequisite checks fire for it — prm-pathbased-requested and authorization-server-metadata both SUCCESS.
  • Same runner, inert client: ❌ 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.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prevent false greens when a negative check rejects for the wrong reason

1 participant