Repository navigation
feat(sep-1932): DPoP discovery signals (stacked on #528) - #529
nbarbettini wants to merge 9 commits into
Conversation
…ocol#369) Follow-up on the DPoP client PR (shared foundation). Adds the server-role conformance for SEP-1932 / RFC 9449: the framework acts as a DPoP client against the MCP server under test and emits the sep-1932-server-* checks across the RFC 9449 §4.3 validation surface — proof validation, the ±5-minute iat window, asymmetric-only algorithms, the 401 + WWW-Authenticate challenge, token audience validation under DPoP, and the optional server-provided nonce. - src/scenarios/server/auth/dpop.ts (+ test, spec-references): the scenario. - examples/servers/typescript/sep-1932-{compliant,broken}-server.ts: passing and failing fixtures proving every check passes and fails. - Registered in the pending + all-client scenario lists. Depends only on the shared DPoP foundation (dpopProof/dpopToken); independent of the authorization-server PR. No DPoP-capable MCP SDK exists yet, so correctness rests on RFC vectors + an independent verifier + the fixtures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Refresh the held DPoP nonce from every response's DPoP-Nonce header (RFC 9449 §8.2 newest-wins) instead of capturing it once, so a server that rotates or single-uses its nonce no longer turns the negative probes into false "not testable" failures (or vacuous passes). (modelcontextprotocol#1) - Widen the stale iat probe to -303 s (matching the future side) so the ±1 s quantization of the whole-second Date header can't pull it onto the ±300 s boundary and be false-accepted. (modelcontextprotocol#2) - Update the isNonceChallenge comment and the untestable message: probes now carry the held nonce, so a use_dpop_nonce challenge is about the nonce lifetime (rotated/stale/single-use), not a missing nonce. (modelcontextprotocol#3) - Fixture: parse integer env vars NaN-safely (intEnv helper) so a malformed DPOP_CLOCK_OFFSET_SECONDS / DPOP_IAT_SKEW_SECONDS falls back to its default instead of silently disabling the iat window. (modelcontextprotocol#4) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Soften the heldNonce comment: newest-wins refresh is correct for ROTATING servers; a strict single-use server that re-arms only via challenges can still push alternate negatives to untestable (correctly reported, not mis-scored) — the previous "rotate or single-use" over-claimed. (R5) - Fix the clock-skew test comment: the stale probe is now -303s (~273s old), not -301s/~271s. (R5) Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A crash or 404 must not count as correctly refusing a DPoP-bound token presented as Bearer. Co-authored-by: Cursor <cursoragent@cursor.com>
A challenge with no error code was passing. The code is a SHOULD, so a wrong or missing value is a warning and the existing MUST checks stay as they are. Co-authored-by: Cursor <cursoragent@cursor.com>
A client that has never met the server has to learn the requirement from the 401 challenge and from protected resource metadata. A missing challenge or metadata field is a failure, and the advertisement has to match what the server enforces. Co-authored-by: Cursor <cursoragent@cursor.com>
f20bf07 to
5d213b9
Compare
|
@nbarbettini I am reviewing the PR and also revisiting the SEP1932 text. I think the following is overly strict (or at least as I think the tests are implemented):
Here are my thoughts process. I think if the MCP server supports DPoP it MUST advertise that support in protected resource metadata (PRM). However, there will be a lot of MCP servers out there that does not support it and won't advertise that they don't support it, they just won't have that field set. According to RFC9728, absence of a field means "not supported". I don''t think we want to register absence of the field as a failure, but rather as being equivalent to being set to "false". I feel like this is a more reasonable approach and avoids a lot of servers having to advertise "false" when they can achieve the same by just not advertising support. |
|
Big-picture: the test fixtures (the fake servers the suite runs against) have no way to trigger the situations described in the other review comments — so the grading fixes those comments ask for can't be tested, and could silently regress later. This comment collects the fixture modes and test cases needed to close that gap. Fixture modes (sep-1932-compliant-server.ts):
Test arms with no coverage today, worth pinning while in here:
Each new mode should flip exactly its targeted check and leave the rest green, per the suite's one-defect convention. Generated with the aid of Claude (Fable 5). |
PieterKas
left a comment
There was a problem hiding this comment.
Thorough review done (adversarial pass with the aid of Claude, Fable 5 — findings verified empirically against live fixtures). The architecture is good: discovery probes are cleanly separated, the consistency probe reuses the positive token minus cnf so rejections are attributable, and the suites run green.
Requesting changes for two themes, detailed in 9 comments:
- Grading drift against the proposed SEP text: the "MCP Server DPoP Advertisement" section proposed in modelcontextprotocol/ext-auth#33 grades these behaviors differently than this PR assumes — absent
dpop_bound_access_tokens_requiredis valid (line 591), the challenge is two-tier SHOULD/MUST (line 524), "reject" isn't "401" (line 622), and the yaml rows should quote the SEP sentences (yaml comment, with suggestion). - Verdicts without evidence: the consistency check asserts metadata contents when no document was read (line 666), and documents are consumed without the RFC 9728 §3.3 resource check (line 507); plus fetcher robustness (line 456) and a details nit (line 623).
The Conversation-tab comment collects the fixture modes and tests needed to pin all of the above. Sequencing: ext-auth#33 needs to land first so the yaml quotes track its final wording; happy to re-review quickly after.
I completely agree - I was too eager here and didn't mean to imply that all servers MUST advertise their DPoP stance. I meant it in the same way you described above: "if the MCP server supports DPoP it MUST advertise that support in protected resource metadata (PRM)" I will take a pass at updating this PR with your feedback, thanks! |
An omitted dpop_bound_access_tokens_required is the RFC 9728 default, a DPoP challenge is a SHOULD unless that field is true, and metadata is only usable when its resource identifier matches. Co-authored-by: Cursor <cursoragent@cursor.com>
A use_dpop_nonce 401 to an unbound Bearer token is about the nonce, so it no longer counts as unadvertised DPoP enforcement. Any 4xx now counts as rejecting that token when metadata says DPoP is required. Metadata at the root well-known URL may name the origin as its resource, which is the RFC 9728 identifier for that URL and what MCP clients accept. Details record the URL that served the metadata after a redirect. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@PieterKas thanks again for the thorough review. I pushed three more commits and updated the PR description to stay current. I think I captured all the points in the updated SEP/ext-auth text, but let me know if I missed something! Grading against ext-auth#33
Evidence
Fixtures: every mode you suggested is in, plus a few more:
Updated with the help of Cursor! |
A resource identifier that fails RFC 9728 §3.3 makes the document unusable, the same as an unreachable one or the wrong Content-Type. PrmDpop now reports it not testable instead of failing it as a DPoP problem. Co-authored-by: Cursor <cursoragent@cursor.com>
|
One follow-up for outside this PR: If a server's metadata says DPoP is required, the SEP says every |
Stacked on #528 -> #527 -> #395. Rebase onto
mainonce #528, #527, and #395 merge.Review the commits after
9771ecb(5d213b9onward). Everything before that belongs to the PRs below it in the stack.The grading follows the "MCP Server DPoP Advertisement" section of SEP-1932 proposed in modelcontextprotocol/ext-auth#33, and the yaml rows quote it word for word.
What each check asserts
#395 checks that a malformed proof is rejected. This PR checks how a client that has never met the server learns that it supports or requires DPoP, and that the advertisement matches what the server enforces.
A server that supports DPoP MUST publish
dpop_signing_alg_values_supported. It does not have to publishdpop_bound_access_tokens_required. When that field is absent, the RFC 9728 default applies: DPoP-bound tokens are not required.sep-1932-server-advertises-dpopSends an unauthenticated POST, with no
Authorizationand noDPoPheader. This check is graded after metadata discovery, because whether the challenge is a SHOULD or a MUST depends ondpop_bound_access_tokens_required.algs: SUCCESSalgs: WARNING (SHOULD)dpop_bound_access_tokens_required: true: FAILURE (MUST)false, or could not be read: WARNING (SHOULD)The check looks for a DPoP challenge anywhere in the header; other challenges alongside it are fine.
resource_metadatais read from the DPoP challenge, or from a sibling challenge such asBearer(RFC 9728 §5.1).sep-1932-server-prm-dpopFetches the Protected Resource Metadata (PRM). It tries the
resource_metadataURL when the challenge has one; otherwise the path-based well-known URL, then the root one. That's the same order as the client discovery scenarios.resource_metadataURL fails, the check does not fall back to a different document.application/json(RFC 9728 §3.2).resourcemust identify the server (RFC 9728 §3.3). For theresource_metadataand path-based URLs, that means exactly the server URL. For the root URL, the RFC's identifier is the origin. The check also accepts the server URL there, because MCP clients such as the TypeScript SDK accept both.dpop_signing_alg_values_supportedmissing, not an array, empty, or containing a non-string entry: FAILURE, with a separate message for each casedpop_signing_alg_values_supportedlistsnoneorHS*: FAILUREdpop_bound_access_tokens_requiredabsent: fine (it defaults tofalse)dpop_bound_access_tokens_requiredpresent but not a boolean: FAILURE (malformed)resourcemissing or not matching, metadata unreachable, not 200, or the wrong Content-Type: not testable (FAILURE). The document is unusable, so its DPoP fields are not graded.sep-1932-server-prm-consistencySends an unbound token (
omitCnf) asBearer, with no DPoP header. The probe differs from the accepted valid-proof request only in its token, so the response can be attributed to the token. Runs only if the valid-proof check passed and a usable PRM document was read. Otherwise it is not testable, and it doesn't report a status.When the metadata sets
dpop_bound_access_tokens_required: true:When the metadata omits the field or sets it to
false:use_dpop_nonce401: not testable (the challenge is about the nonce, not the token)The check's details include the response's
WWW-Authenticateheader and itserrorcode.Testing
DPoP server scenario tests pass (37). The fixture modes cover:
dpop_bound_access_tokens_required;noneandHS256listed.resource; metadata served only at the root URL, with the origin and with a different origin asresource; no metadata route; an HTML Content-Type; a redirect to the document.resource_metadatain the challenge, which exercises the well-known fallback.use_dpop_nonce401.Each mode changes only the check it targets; every other check stays as it is against the compliant server.
Full suite passes
npm run check(typecheck, eslint, prettier) andnpm run buildare cleanMade with Cursor