feat(sep-1932): AS negative probes and nonce checks (stacked on #396)

[nbarbettini]

Stacked on #396 (PieterKas/conformance:dpop-as, head e1c4fb9). This branch contains that PR's commits plus ff169cc. Rebase onto main once #396 merges.

The new commit (ff169cc) is the only one to review.

What each check asserts

#396 covers the happy path: metadata, and that a valid proof yields a token bound to the proof key.

These additional checks fail an authorization server that binds a token to any proof without verifying it, or that mishandles nonces. They run only after sep-1932-as-token-binding is SUCCESS, otherwise are skipped.

sep-1932-as-rejects-invalid-proof

RFC 9449 §5, graded MUST from "The DPoP HTTP header field MUST contain a valid DPoP proof JWT [...] If the DPoP proof is invalid, the authorization server issues an error response per Section 5.2 of [RFC6749] with invalid_dpop_proof as the value of the error parameter."

Two checks for this:

• tampered signature
• htu set to a URL other than the token endpoint

HTTP 400 with error=invalid_dpop_proof is SUCCESS. HTTP 200 with an access token is FAILURE. HTTP 400 with a different error is WARNING.

sep-1932-as-nonce

Relevant only when the first token exchange is use_dpop_nonce. Supplying a nonce is MAY (RFC 9449 §8: "An authorization server MAY supply a nonce value..."), so an AS that never challenges does not emit the check.

When relevant, we now check for:

• The challenge carries a DPoP-Nonce header. §8 states this as the content of the response ("The authorization server includes a DPoP-Nonce HTTP header in the response supplying a nonce value..."). A missing header used to fall through exchangeWithProof and leave sep-1932-as-token-binding SKIPPED. It is now a FAILURE on this check.
• The retry with the supplied nonce is accepted (HTTP 200 with an access token). This was already exercised; it is now recorded.
• A proof carrying a different nonce is rejected, on a fresh authorization code, and that exchange is not retried (a retry would hide the rejection). §4.3 step 10 is under "the receiving server MUST ensure", and §8 says "If the nonce claim in the DPoP proof does not exactly match a nonce recently supplied by the authorization server to the client, the authorization server MUST reject the request." Issuing a token is FAILURE. Any token-less 4xx counts as a rejection. Other statuses are SKIPPED.

Testing

• npm test — 46 files / 616 tests pass
• npm run check (tsgo + eslint + prettier) — clean

Made with Cursor

[pkg-pr-new]

Open in StackBlitz

npx https://pkg.pr.new/@modelcontextprotocol/conformance@525

commit: ff169cc

[tnorimat]

@nbarbettini Hello, I will review the PR. Please wait for a moment.

[PieterKas]

A fully conformant nonce-requiring AS gets two WARNING marks here, and the thing these probes exist to test never actually gets tested against it.

The scenario may already hold the AS's nonce by the time the negative probes run (suppliedNonce is in scope at the call site, line 699), but the tampered-signature and wrong-htu proofs are built without it. RFC 9449 §4.3 lets a server run its checks "in any order" — an AS that checks the nonce first answers both probes 400 use_dpop_nonce, which grades as WARNING ("wrong error code") against a compliant server. Worse: because the probes never get past the nonce gate, the signature/htu validation they target is never exercised — a nonce-first AS that accepts tampered signatures looks identical to a strict one. (Reproduced empirically against a nonce-first in-process AS: binding and nonce checks all SUCCESS, both invalid-proof probes WARNING. The bundled fixture hides this only because it happens to validate proofs before nonces.)

Fix: pass the held nonce into the negative proofs — the injected defect is still present, so a validating AS then rejects for the right reason. Same shape as the heldNonce fix the server scenario got in conformance#395's review rounds.

Generated with the aid of Claude (Fable 5).

[PieterKas]

An AS that never compares nonces at all can pass this check.

This accepts ANY token-less 4xx as proof of nonce-mismatch rejection — but the probe's exchange can fail 4xx for reasons that have nothing to do with the nonce: invalid_grant from a code policy, invalid_client, rate limiting. Each gets credited as "the AS validated the nonce" when no comparison ever ran. The sibling judge for invalid proofs already gets this right (SUCCESS only for the prescribed error code, other 400s → WARNING); suggest the same here: SUCCESS for error in {use_dpop_nonce, invalid_dpop_proof}, inconclusive otherwise. (The unit test currently pins the any-4xx behavior, so it would need updating too.)

Generated with the aid of Claude (Fable 5).

[PieterKas]

One non-DPoP hiccup on the retry turns into a hard FAILURE (and empirically, four of them).

RFC 9449 §8 places no MUST on the AS to accept the retry — and the retry can fail for reasons the AS is allowed: client auth we didn't send, a second rotated use_dpop_nonce challenge (§8 leaves re-challenge timing to the server), or the AS having burned the single-use code on the challenge itself. Reproduced against a code-burning AS: the binding check correctly grades the same response "inconclusive (non-DPoP reason)", while this check reports FAILURE "retry not accepted" — and the gated probes stack three more untestable-FAILUREs on top. Suggest mirroring bindingCheckFor's inconclusive rule for non-DPoP error codes.

Generated with the aid of Claude (Fable 5).

[PieterKas]

An AS that accepted the harness's valid proof and issued a token can be reported as "rejects everything" — three FAILUREs for probes that were never sent.

The flow here: the harness (playing the client) first presents a valid DPoP proof at the token endpoint; if that baseline goes well, it then presents deliberately broken proofs on fresh codes and checks they're refused. This line is the gate between those two stages, and it asks the wrong question. The bad-proof probes need exactly one thing from the baseline: "the AS accepts proofs and issues tokens" — they never look inside a token. But the gate asks "did the binding check return SUCCESS?", and the binding check has an extra sensitivity the probes don't share: it must decode cnf.jkt out of the issued token.

Those two questions give different answers whenever the token was issued but can't be inspected. The concrete case (reproduced empirically): an AS issuing opaque access tokens. Baseline: valid proof in, 200 + token_type: DPoP out — accepted. Binding: SKIPPED, correctly, since an opaque token can't be decoded. The gate then withholds all the probes and reports them as untestable-FAILUREs with the reason "did not issue a DPoP-bound token for a valid proof … rejects everything" — describing a rejection that never happened. (The unbound-token path hits the same false text: binding FAILURE is deserved there, but the proof was accepted, so the probes are attributable and would be informative.)

Fix: gate on "valid-proof exchange succeeded and a token was issued" instead of the binding verdict. Opaque-token ASs then keep their honest binding SKIP and get real probe coverage; the genuinely blocked case (baseline exchange failed, no token) still hits the gate, with a reason that's true.

Generated with the aid of Claude (Fable 5).

[PieterKas]

This judge decides what a response proves by matching exact status codes, and the edges are wrong in both directions: clear evidence that the AS validates proofs gets thrown away, while clear evidence that it doesn't can escape ungraded.

Three cases, each with a concrete victim:

1. An AS rejects the tampered proof with 401 + error=invalid_dpop_proof. That's unambiguous evidence it validated the proof — the status just deviates from RFC 6749 §5.2's prescribed 400. Because only exactly-400 is matched, this falls through to SKIPPED and vanishes from pass/fail entirely. Meanwhile an AS answering 400 with the wrong error code gets a WARNING. So the smaller deviation (right behavior, odd status) is treated more harshly than silence, and the stronger evidence counts for nothing — the asymmetry is backwards.

2. An AS issues a token for the tampered proof — the worst possible outcome, the thing this whole check exists to catch — but answers 201 instead of 200. The FAILURE arm matches only statusCode === 200, so this non-conformant AS lands in SKIPPED "inconclusive" and the defect disappears from the counts.

3. An AS answers 200 with no access_token in the body. The binding check one screen up explicitly calls that "a plainly broken AS" and fails it; this judge lets the same response fall to SKIPPED. Same PR, same response, two verdicts.

Suggested boundaries: a token issued on any 2xx → FAILURE; a token-less 2xx → FAILURE (matching the binding check's rule); 4xx with error=invalid_dpop_proof → SUCCESS whether 400 or 401 (optionally noting a status deviation in details); other 4xx codes keep the current WARNING.

Generated with the aid of Claude (Fable 5).

[PieterKas]

The MUST this row derives the FAILURE from binds the client, not the authorization server — "The DPoP HTTP header field MUST contain a valid DPoP proof JWT" is about what the request must contain, and the AS-side sentence in the quote ("the authorization server issues an error response…") carries no 2119 keyword. An AS vendor disputing their FAILURE would follow this citation, find a client requirement, and conclude the verdict is ungrounded — when a solid ground exists one section over: RFC 9449 §4.3 binds every proof-receiving server directly ("To validate a DPoP proof, the receiving server MUST ensure…"), and an AS minting a token for a tampered proof has violated it. Suggested re-grounding:

Suggested change

	# RFC 9449 §5. The SEP does not restate this sentence; it requires conformance to RFC 9449.
	# Graded MUST from "MUST contain a valid DPoP proof JWT". A 400 that names some other
	# error is WARNING: the prescribed `invalid_dpop_proof` code is stated without its own keyword.
	- check: sep-1932-as-rejects-invalid-proof
	text: 'The `DPoP` HTTP header field MUST contain a valid DPoP proof JWT. If the DPoP proof is invalid, the authorization server issues an error response per Section 5.2 of [RFC6749] with `invalid_dpop_proof` as the value of the `error` parameter.'
	url: https://www.rfc-editor.org/rfc/rfc9449.html#section-5
	# RFC 9449 §4.3 ("MUST ensure") binds the proof-receiving server; the §5 sentence
	# describes the AS's error response. The SEP does not restate these sentences; it
	# requires conformance to RFC 9449. A 400 that names some other error is WARNING:
	# the prescribed `invalid_dpop_proof` code is stated without its own keyword.
	- check: sep-1932-as-rejects-invalid-proof
	text: 'To validate a DPoP proof, the receiving server MUST ensure the following: … The JWT signature verifies with the public key contained in the `jwk` JOSE Header Parameter. … The `htu` claim matches the HTTP URI value for the HTTP request in which the JWT was received, ignoring any query and fragment parts. / If the DPoP proof is invalid, the authorization server issues an error response per Section 5.2 of [RFC6749] with `invalid_dpop_proof` as the value of the `error` parameter.'
	url: https://www.rfc-editor.org/rfc/rfc9449.html#section-4.3

(The ellipses mark the elided items of §4.3's numbered checklist — the two quoted items are the ones these probes exercise, signature and htu; the / join follows this file's existing multi-sentence convention.)

Generated with the aid of Claude (Fable 5).

[PieterKas]

Default behavior question: after this merges, a compliant login-gated AS that passes today's dpop scenario will start reporting FAILUREs simply because the operator upgraded the conformance suite — with no change on the AS side.

The new probes are off by default and the off state is reported via untestableCheck (FAILURE, "Not testable:"). Both halves are firsts for the suite: no other check is gated behind a flag (the merged AS scenario's own interactive flow — the closest analogue of an expensive probe — runs by default and reports non-completion as SKIPPED), and all existing untestableCheck users are server-side-prerequisite cases per #248, not harness-side choices. If the cost argues for opt-in here, precedent argues the not-opted-in state should render as SKIPPED (or the probes should run by default like the interactive flow they extend); keeping untestable-FAILURE for a harness choice would be new policy, worth a maintainer ruling rather than a side effect of this PR.

Whichever way it lands, the migration should be visible: the flag currently appears only in --help and the scenario description — README and the example settings file don't mention it. Related messaging nit: the opt-in text says "an additional authorization code" (singular) — a full nonce run spends three extra codes, which on a login-gated AS means up to three more interactive 300s waits (~15 min); worth stating the real cost where operators decide.

Generated with the aid of Claude (Fable 5).

[PieterKas]

Nit: CHECK_DEFS registers name 'DpopNonce' but no emission path uses it — the id is emitted under three path-dependent names (NonceChallengeHeader/NonceRetryAccepted/NonceRejectsWrongValue), which also drop the file's Dpop- name prefix; report consumers keying on (id, name) see the check set mutate between runs. Consider per-case ids or fixed names.

Generated with the aid of Claude (Fable 5).

[PieterKas]

Nit: this catch wraps the token exchange too, but the reason always says "could not obtain an authorization code" — a token-endpoint failure (e.g. ECONNRESET during the tampered-signature exchange) sends the operator to debug the wrong endpoint. Same pattern at the wrong-nonce probe's catch.

Generated with the aid of Claude (Fable 5).

[PieterKas]

Nit: this header comment still claims "each defect fails EXACTLY its target check" but the unbound-token case now produces three FAILUREs, and the loop no longer asserts the absence of unexpected failures — comment and assertions should both catch up.

Generated with the aid of Claude (Fable 5).

[PieterKas]

Nit: this describe deletes MCP_CONFORMANCE_DPOP_NEGATIVE_PROBES in afterEach instead of save/restore like the sibling login-gated tests — a developer running the suite with the var exported in their shell has it silently destroyed for later tests in the same worker. Same save/restore (or vi.stubEnv) pattern as the siblings would do.

Generated with the aid of Claude (Fable 5).

[PieterKas]

Thorough review done (adversarial pass with the aid of Claude, Fable 5 .

Much here is good: fresh authorization code per probe with no reuse, post-signing tamper so the proof is structurally valid, honest SKIP reasons on the prerequisite paths, faithful MAY-gating of the nonce check, and the shared test-AS changes are cleanly opt-in — both the AS suite (51/51) and the client auth suite (135/135) pass unchanged.

Requesting changes for two themes, detailed in the inline comments:

1. Attribution discipline — several arms grade responses without establishing the response was about the injected defect: the negative probes omit the nonce the scenario already holds, so a conformant nonce-first AS gets WARNINGs while the signature/htu MUSTs go unprobed (line 772); the wrong-nonce check credits any token-less 4xx (line 146); the nonce retry hard-fails on non-DPoP errors the binding check treats as inconclusive (line 639); the probe gate keys on the binding verdict rather than "valid proof accepted", mislabeling opaque-token ASs (line 532); and the response judge discards strong evidence (401 + invalid_dpop_proof) while letting a 201-issued token or a token-less 200 escape (line 113).

2. Grounding and policy: the yaml derives the FAILURE from a client-side MUST — RFC 9449 §4.3's "MUST ensure" is the right citation (yaml comment, with suggestion); and the default-off probes reporting as untestable-FAILURE is a suite-first that flips passing setups red on upgrade — raised as a maintainer policy question on the Conversation tab.

Plus four minor nits inline (check naming, a stale isolation-matrix comment, a catch mislabel, env-var hygiene in tests). Happy to re-review quickly once these land.