Skip to content

Review fixes for SEP-2468 iss scenarios (stacks on #220)#1

Closed
pcarleton wants to merge 9 commits into
max-stytch:feat/iss-parameter-conformancefrom
modelcontextprotocol:paulc/sep-2468-review
Closed

Review fixes for SEP-2468 iss scenarios (stacks on #220)#1
pcarleton wants to merge 9 commits into
max-stytch:feat/iss-parameter-conformancefrom
modelcontextprotocol:paulc/sep-2468-review

Conversation

@pcarleton

Copy link
Copy Markdown

Stacked on modelcontextprotocol#220 — review fixes for the SEP-2468 iss parameter scenarios. One commit per finding so you can take or leave each independently; merge this into feat/iss-parameter-conformance (or cherry-pick) and it'll flow through to modelcontextprotocol#220.

Commits

  1. fix: createAuthServer iss option type/guard and NotAdvertised scenario duplication — kept the on-by-default behavior (mock AS models a well-behaved server) but issParameterSupported is now boolean | null so callers can pass null to omit the metadata field, matching the codeChallengeMethodsSupported pattern. The !== undefined guard at L155 was unreachable with a = true destructure default, so there was no way to omit — and IssParameterNotAdvertisedScenario was silently advertising support (a duplicate of IssParameterSupported). It and IssParameterUnexpected now pass null/'omit' explicitly. Doc comments updated to match.
  2. fix: rejection scenarios silently pass when client never reaches auth endpointcorrectlyRejected = !tokenRequestMade reported SUCCESS on a setup failure. Now gated on authReached.
  3. fix: iss-unexpected scenario contradicts SEP-2468 spec table row 3 — spec says absent + iss present → Compare, not reject. Scenario sent a correct iss and FAILed compliant clients for proceeding. Now sends a mismatched iss so rejection is the spec-required outcome; reference client updated to compare-when-present.
  4. refactor: replace harness-config checks with client-proceeded checksiss-advertised-in-metadata etc. fired in onAuthorizationRequest and asserted only that the harness was configured correctly. Replaced with one check per positive scenario keyed on tokenRequestMade.
  5. refactor: rename check IDs to sep-2468-* — aligned with the spec table rows; auth/iss-supported and auth/iss-wrong-issuer now share sep-2468-client-compare-iss-supported.
  6. feat: add sep-2468.yaml requirement traceability — 8 check: rows, 1 excluded:.

Coverage after this stack

sep-2468.yaml check: covered by status
client-compare-iss-supported auth/iss-supported, auth/iss-wrong-issuer
client-reject-missing-iss auth/iss-supported-missing
client-compare-iss-unadvertised auth/iss-unexpected
client-proceed-no-iss auth/iss-not-advertised
client-validate-metadata-issuer gap (host candidate: client/auth/discovery-metadata.ts)
client-no-normalization gap
as-include-iss gap (AS suite, ok to defer)
as-advertise-iss-supported gap (AS suite, ok to defer)

Left as prose (not in this stack)

npx vitest run src/scenarios/client/auth/index.test.ts → 35/35 pass on this stack.

max-stytch and others added 8 commits April 8, 2026 18:24
Adds 5 draft conformance scenarios testing RFC 9207 issuer parameter
validation in OAuth authorization responses:

- auth/iss-supported: server advertises support and sends correct iss
- auth/iss-not-advertised: server omits iss parameter entirely
- auth/iss-supported-missing: client must reject missing iss when required
- auth/iss-wrong-issuer: client must reject mismatched iss value
- auth/iss-unexpected: client must reject iss when not advertised

Also adds auth-test-iss-validation.ts, a reference client that correctly
validates iss per RFC 9207, and negative tests confirming the standard
client fails all three rejection scenarios.

TODO: Update RFC_9207_ISS_PARAMETER spec reference once SEP-2468
(modelcontextprotocol/modelcontextprotocol#2468) is merged.
…o duplication

The doc comments said 'Default: not included' but the destructure defaulted
to true/'correct', and the `!== undefined` guard at L155 was unreachable —
so there was no way to omit the metadata field, and IssParameterNotAdvertised
silently advertised support (a duplicate of IssParameterSupported).

Kept the on-by-default behavior (mock AS models a well-behaved server) but
made issParameterSupported `boolean | null` so callers pass null to omit,
matching the codeChallengeMethodsSupported pattern. Doc comments now match.
Scenarios that need omission pass null/'omit' explicitly.
… endpoint

correctlyRejected = !tokenRequestMade reports SUCCESS if the client errors
out before hitting /authorize. Gate on authReached so a setup failure shows
as FAILURE with authReached:false in details.
The spec table says: supported=false/absent + iss present -> *Compare* to
the recorded issuer (not reject). The scenario sent a *correct* iss and
FAILed compliant clients for proceeding after a successful comparison.

Now sends a mismatched iss so the comparison fails and rejection is the
spec-required outcome. Reference client updated to compare-when-present
instead of throw-on-presence.
iss-advertised-in-metadata / iss-sent-in-redirect (and the not-* variants)
fired in onAuthorizationRequest before the redirect happened, asserting only
that the harness was configured correctly — a client that ignores iss passes
identically. Replaced with one check per scenario keyed on tokenRequestMade,
which observes that the client actually proceeded through the iss path.
One ID per spec table row; auth/iss-supported and auth/iss-wrong-issuer
both emit sep-2468-client-compare-iss-supported (same comparison, opposite
input) per the same-slug-for-SUCCESS-and-FAIL convention.
8 check rows (4 client table-row checks, 1 metadata-issuer, 2 AS-side,
1 no-normalization), 1 excluded (error-display is UI-facing). The
record-issuer MUST is merged into the compare-iss-supported row text
since it has no independent wire observation.
Replaces `specVersions: ['draft']` with `source: { introducedIn: DRAFT_PROTOCOL_VERSION }`
in the 5 iss-parameter scenarios.

This commit typechecks once the stack is rebased onto main >= #265 (the
ScenarioSource migration). Adding it now so the rebase is mechanical.
@pcarleton pcarleton force-pushed the feat/iss-parameter-conformance branch 2 times, most recently from 2958374 to 28a83ae Compare May 19, 2026 22:34
@pcarleton pcarleton closed this May 19, 2026
@pcarleton pcarleton deleted the paulc/sep-2468-review branch May 19, 2026 22:35
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.

2 participants