client/dpop: follow-ups from review (stacked on #394)#400
Draft
claude[bot] wants to merge 21 commits into
Draft
Conversation
…368) Client-anchored packaging: carries the shared DPoP foundation (dpopProof / dpopToken helpers, the createAuthServer DPoP core, and sep-1932.yaml) together with the client scenario, so it can merge first; the server (#369) and authorization-server (#370) scenarios are follow-ups that depend on it. DPoP is treated as a draft/not-yet-official feature (mirroring the WIF SEP-1933 scenario): the scenario's source is `introducedIn: DRAFT_PROTOCOL_VERSION` and it is registered in draftScenariosList (informational, not scored). It is NOT registered in EXTENSION_IDS, since the ext-auth DPoP extension is still on a branch rather than merged. Client checks (the two extension MUSTs, RFC 9449 §7.1 / §4.2-4.3): - sep-1932-client-dpop-auth-scheme — token presented with the DPoP scheme. - sep-1932-client-fresh-proof — a fresh, well-formed DPoP proof per request. The test MCP server judges the client via an opt-in DPoP middleware (dpopResourceAuth) passed through createServer's authMiddleware hook; a compliant example client (covered by the Client Draft Scenarios loop) plus deliberately- broken bearer/replay variants drive the pass/fail acceptance tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Server-provided nonces are optional in RFC 9449 (AS §8 "MAY", RS §9 "can also choose"), and the nonce-less flow is the common case. The single auth/dpop scenario hard-wired nonce enforcement on both the test AS and MCP server, so a plain nonce-less proof was never accepted end-to-end and a client that only does plain proofs was never exercised on its happy path. Parameterize DPoPClientScenario with `requireNonce` and register it twice: - auth/dpop (requireNonce=false) — nonce-less baseline; emits the three baseline checks (token-request-proof, dpop-auth-scheme, fresh-proof). - auth/dpop-nonce (requireNonce=true) — prior behavior; adds as-nonce and rs-nonce (five checks). Check IDs are reused (no new IDs, no sep-1932.yaml change); the two nonce checks are emitted only by auth/dpop-nonce, so traceability stays intact. Add auth-test-dpop-no-nonce.ts (nonce-incapable but otherwise compliant) asserting SUCCESS against auth/dpop — proof that a client with no nonce handling still completes DPoP when the server does not require a nonce. Baseline negatives stay on auth/dpop (nonce-independent); the two nonce negatives are re-pointed to auth/dpop-nonce. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review findings on the auth/dpop-nonce posture: - Record the token-request proof observation BEFORE the §8 nonce challenge, so a client that is challenged but does not retry is no longer mis-reported by token-request-proof as "never completed a token request" (its proof was just validated). (#2) - Record the resource-request jti/replay on ANY valid proof (before the §9 nonce gate), so fresh-proof is never asserted on zero evidence and a client that never honours the RS nonce fails ONLY rs-nonce. (#5) - Require challengeIssued && honored for as-nonce/rs-nonce SUCCESS, closing a vacuous-pass path (a client that pre-sends the nonce is never challenged). (#3a) - Gate the AS nonce observation on the authorization_code exchange, matching recordTokenRequestProof (a refresh exchange must not satisfy §8). (#3c) - Collapse duplicate shared token-flow check IDs (token-request, pkce-*) that the §8 round-trip re-POST would otherwise emit twice. (#4) - Example client: retry the token request only on a use_dpop_nonce error code, not any 400 carrying a DPoP-Nonce header (consistent with the RS path). (#6) - Fix the DpopTokenRequestObservation docstring ("last write wins" → sticky-failure). (#7) Adds an expectedSuccessSlugs option to the client test helper and tightens the two nonce negative tests to pin the now-clean behavior (token-request-proof SUCCESS for no-as-nonce; only rs-nonce fails for no-rs-nonce). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- dedupeSharedChecks was masking failures: it kept the last occurrence of a duplicate check ID regardless of status and ran in BOTH postures, so a FAILURE on a challenged POST (or an earlier attempt in a restarted flow) was silently dropped. Replace with an exported, unit-tested collapseDuplicateChecks that prefers the MOST-SEVERE occurrence, and only apply it in the nonce posture (the baseline keeps genuinely distinct repeated attempts). (R5 #1) - Reword the token-request-proof description: it can be SUCCESS for a client that presented a valid proof but never obtained a token (challenged, no retry), so it no longer claims "obtaining a DPoP-bound access token". (R5 #2) - Reword the as-nonce/rs-nonce failure messages to cover a client that DID retry but whose retry proof was rejected (not only "did not retry"). (R5 #3) - Document the deliberate replay decision: recording jti on challenged requests means re-sending the identical proof across the challenge/retry boundary trips replay detection — a genuine RFC 9449 §4.2 jti-uniqueness violation. (R5 #5) Adds src/scenarios/client/auth/dpop.test.ts unit-testing collapseDuplicateChecks (prefers failures, keeps INFO, preserves order) — the dedupe fix is now pinned. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Add an integration assertion that the §8/§9 double-POST is collapsed to one token-request/pkce-* entry each in auth/dpop-nonce, so the requireNonce gating of collapseDuplicateChecks can't be silently deleted or inverted (previously mutation-survivable). runClientAgainstScenario now returns the emitted checks so callers can make occurrence-level assertions (backward-compatible). - Docstring: note the severity ladder ranks SKIPPED below SUCCESS, and that equal-severity ties keep the last occurrence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…xtension instead of the draft timeline
…of a personal working branch
…e/iss, and surfaces the OAuth error body
… per RFC 3986/9449 §4.3
…ad of silently ignoring them
…reject unadvertised algs (RFC 9449 §5.1)
…(single-defect isolation)
…AS issuer key and the jkt bound at the token endpoint
…e iat-window to the spec's unconditional statement; align token-binding with RFC 9449 §6
…w) in htm/htu/iat rejection messages
…algs in the PRM (RFC 9728 §2)
…FC 9449 §11); negative variants opt in
…le HTTP, no SSE stream)
… servers MUST 401)
commit: |
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
2 participants
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.
Requested by Paul Carleton · Slack thread
Follow-ups to #394 from review feedback. Includes and depends on #394 — the
dpop-clientbranch lives in PieterKas's fork, so this PR targetsmainand carries #394's commits plus the fixes below; it should land after (or be folded into) #394.Before / After
Before: the DPoP scenarios rode the draft-spec timeline, the suite's spec link pointed at a personal working branch, the resource judge trusted whatever JWT the client presented (unverified
decodeJwt), a spurious trailing slash inhtupassed silently, the AS advertised ES256-only while enforcing ten algs, and failing implementers got rejection strings like "htu does not match the request URI" with no values to act on.After:
auth/dpopandauth/dpop-nonceare registered as theio.modelcontextprotocol/dpopextension (selected via--suite extensions, never promoted into the scored core suite), the spec link is commit-pinned, the judge verifies the presented token against the test AS issuer key and the exactcnf.jktbound at the token endpoint,htucomparison is strict RFC 3986/9449 §4.3 normalization, advertisement matches enforcement (with unadvertised algs rejected per RFC 9449 §5.1), the PRM advertisesdpop_bound_access_tokens_required/dpop_signing_alg_values_supported(RFC 9728 §2) so discovery-driven clients know DPoP is expected, and every htm/htu/iat rejection reports observed vs expected values (including the ±300s window).How (one commit per fix)
auth/dpop/auth/dpop-noncemove fromdraftScenariosList(introducedIn: DRAFT_PROTOCOL_VERSION) toextensionScenariosListwithsource: { extensionId: 'io.modelcontextprotocol/dpop' }, mirroring the other ext-auth extensions.SpecReferences.DPOP_EXTENSIONandsep-1932.yaml'sspec_urlnow use a commit-pinned blob URL instead of thepieterkas-dpop-extensionbranch path, with a TODO to switch to the main-branch path once the doc merges.resourceparameter in both the authorization and token requests, validatesstate(andisswhen returned) on the callback, and surfaces the OAutherror/error_descriptionbody in token-request failures instead of bareHTTP 400.htunormalization — dropped the trailing-slash leniency in both validators; only RFC 3986 syntax-based normalization (case, default port, empty path) is applied. Tests updated: the trailing-slash case now pins rejection, with a genuine-normalization acceptance case added.dpopRequireNonce/dpopTokenRequestObson a DPoP-disabled test AS now throws instead of silently doing nothing.DPOP_ASYMMETRIC_ALGSconstant; the scenario's AS metadata advertises exactly what the validators enforce, and the token-endpoint validator rejects proofs signed with an alg outside the advertised list (RFC 9449 §5.1) with an asymmetric-only floor (§11.6).expectedSuccessSlugs, so a regression that breaks an unrelated check can't hide behind the subset assertion.cnf.jktagainst the jkt recorded at the token endpoint, closing the self-minted-token loophole.server-iat-windowrow is scoped to the spec's unconditional verbatim sentence (the stateless-server conditional moved to an excluded row);as-token-bindingreworded to RFC 9449 §6's actual requirement (cnf.jktfor JWT tokens, introspection permitted).htu/iatget their own messages distinct from mismatches.dpop_bound_access_tokens_required: trueanddpop_signing_alg_values_supported(RFC 9728 §2).generateDpopKeyPairdefaults toextractable: false(RFC 9449 §11 guidance); only theembedPrivateKeynegative variants opt in.proof.test.ts→dpopProof.test.ts; removed the pointer to nonexistent "project notes".Skipped
Testing
npm test— 37 files / 423 tests passnpm run check(tsgo + eslint + prettier) — cleanGenerated by Claude Code