feat(sep-1932): AS negative probes and nonce checks (stacked on #396) - #525
Open
nbarbettini wants to merge 8 commits into
Open
nbarbettini wants to merge 8 commits into
nbarbettini wants to merge 8 commits into
Conversation
…l#370) Follow-up on the DPoP client PR (shared foundation: createAuthServer DPoP core + dpopProof/dpopToken helpers). Adds an authorization-server scenario testing DPoP (SEP-1932 / RFC 9449): metadata (dpop_signing_alg_values_supported present, asymmetric-only), token binding (cnf.jkt + token_type=DPoP), and no-proof enforcement when dpop_bound_access_tokens is advertised. Probes a live AS via authorization_code + PKCE (auto-follows a direct redirect, falls back to an interactive callback) and returns four sep-1932-as-* checks (compliant run + four one-defect-isolation misbehaving configs). - authorization-server/dpop.ts (+ acceptance test, spec-references). - dpopToken: adds readTokenBinding() (reads token_type + cnf.jkt back out of a token response) — introduced here because this scenario is its only consumer. Depends only on the shared DPoP foundation; independent of the server PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
negotiateProofAlg fell back to ES256 for a present-but-non-array dpop_signing_alg_values_supported (e.g. the string "RS256"), contradicting its docstring and risking a token-binding mis-score for that malformed shape. Treat a present-but-non-array value as null (SKIP), like a non-empty list with no supported alg; only an absent/empty list still falls back to ES256. Defensive against malformed metadata; not independently exercised by a fixture (would need a malformed-metadata AS option), consistent with the htu-strip defensive fixes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The round-4 non-array guard carved out `null` (advertised !== null), so metadata with "dpop_signing_alg_values_supported": null passed the support gate (which only tests === undefined), skipped the guard, and fell through to the ES256 fallback — the exact binding mis-score the fix targeted. Extract the negotiation to an exported pure function negotiateProofAlg(advertised) and treat ANY present-but-non-array shape (string, null, number, object) as malformed → null (SKIP). Only an empty array still falls back to ES256. Correct the docstring (an absent field never reaches here — the support gate SKIPs upstream). Add unit tests for every shape (array / empty / no-overlap / string / null / number / object), pinning the fix against a silent refactor regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Broaden the negotiateProofAlg fallback test to also assert undefined → ES256
and correct its title ("empty array or absent field") — the contract covers
both, though absent is gated upstream in the scenario.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…yaml Add a one-line comment above the sep-1932-as-token-binding requirement noting that its binding mechanics are defined in RFC 9449 (§6 cnf/jkt thumbprint, §5 token_type: DPoP) — which the SEP builds on rather than restating — so a reader can see where the requirement text derives from. Addresses review feedback on modelcontextprotocol#396; the check id and text are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The DPoP AS scenario drives its own authorization_code + PKCE flow but did not forward the `resource` parameter, unlike authorization-code-grant.ts after modelcontextprotocol#466. Send it on both the authorization request and the token request when supplied (guarded by options.resource, so it's a no-op otherwise). Keeps the two AS scenarios consistent and lets the DPoP binding checks be evaluated cleanly against a resource-enforcing AS. Addresses review feedback on modelcontextprotocol#396. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Public-client refresh-token binding (issue modelcontextprotocol#370) is not exercisable today: the shared conformance test AS (createAuthServer) doesn't issue refresh tokens or handle the refresh_token grant, so a conformant-vs-misbehaving pair can't be built to validate the check under the suite's "prove it passes and fails" rule. Record it as an excluded: row in sep-1932.yaml with this rationale; deferred as a follow-up until the test AS gains refresh support. Addresses review feedback on modelcontextprotocol#396. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An authorization server that binds a token without checking the proof, or that mishandles a DPoP nonce, now fails. Negative probes run on headless redirects and stay not-testable on a login-gated server unless opted in. Co-authored-by: Cursor <cursoragent@cursor.com>
commit: |
nbarbettini
changed the title
nbarbettini
marked this pull request as ready for review
September 25, 2026 13:26
This branch has not been deployed
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.
Stacked on #396 (
PieterKas/conformance:dpop-as, heade1c4fb9). This branch contains that PR's commits plusff169cc. Rebase ontomainonce #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-bindingis SUCCESS, otherwise are skipped.sep-1932-as-rejects-invalid-proofRFC 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_proofas the value of theerrorparameter."Two checks for this:
htuset to a URL other than the token endpointHTTP 400 with
error=invalid_dpop_proofis SUCCESS. HTTP 200 with an access token is FAILURE. HTTP 400 with a different error is WARNING.sep-1932-as-nonceRelevant 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:
DPoP-Nonceheader. §8 states this as the content of the response ("The authorization server includes aDPoP-NonceHTTP header in the response supplying a nonce value..."). A missing header used to fall throughexchangeWithProofand leavesep-1932-as-token-bindingSKIPPED. It is now a FAILURE on this check.nonceclaim 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 passnpm run check(tsgo + eslint + prettier) — cleanMade with Cursor