SEP-2350: scope-accumulation check in auth/scope-step-up#282
Merged
Conversation
SEP-2350 clarifies that on step-up re-authorization, clients SHOULD compute the union of previously-granted and newly-challenged scopes so they don't lose permissions for other operations. - Traceability yaml: 1 client check (two spec sentences merged), 1 server check tracked for a future server-side scenario, 1 excluded reword - ScopeStepUpAuthScenario now challenges with only the missing scope so union accumulation is observable on the second authorization request - New check sep-2350-scope-union-on-reauth (WARNING when the previously granted scope is dropped) - everything-client withOAuthRetry now accumulates prior token scope into the re-auth scope (passing example) - New auth-test-echo-scope negative client + vitest case
commit: |
* feat: merge latest request-metadata scenario stacked branch * test: implement optional capability conditional checks and simulated negotiation retry
The sep-2243 scenario check IDs drifted from the requirement-traceability yaml in #259 — the code emitted one ID per test case while the yaml declares one per normative requirement, so the IDs no longer matched. Rename the emitted IDs so a check ID maps to a single MUST/SHOULD and is emitted once per case (the per-case detail moves to name/description), matching the repo's existing 'same id, vary status/message' convention: - mcp-method-header-* / mcp-name-header-* -> client-includes-standard-headers - reject-invalid-tool-* / keep-valid-tool -> client-reject-invalid-tool - server-accepts-{lower,upper}case-name -> header-name-case-insensitive - server reject status checks (mismatch/missing/case x method/name) -> server-reject-invalid-headers - their error-code variants -> server-reject-error-code Merge the yaml's server-reject-mismatch + server-reject-status into one server-reject-invalid-headers MUST (HTTP 400 on a header-validation failure); keep server-reject-error-code as the SHOULD. Base64/custom-header rejection checks keep their own param-validation IDs (out of scope here). Gates and the whitespace-acceptance check keep their scenario-matching names. No behavior change — only check IDs, descriptions, and the yaml.
…277) * feat: add sdk subcommand to run conformance against any SDK ref (#250) * Revise sdk runner: explicit --mode, KNOWN_SDKS-only config, v1/v2 entries Addresses review feedback on the sdk subcommand: - Require --mode (client|server) and remove "both". Each invocation now tests exactly one side with its own exit code; the old default ran client then server but combined exit codes with ||=, which skipped the server side entirely whenever the client run failed. - Resolve build/run config from KNOWN_SDKS + CLI flags only; drop the conformance.config.yaml loader (no SDK ships one yet). The Zod schema stays as the type for the built-in entries. - Split the typescript entry by major version: typescript-sdk (v2/main, pnpm install + build:all, expected-failures.yaml) and typescript-sdk-v1 (v1.x, npm ci + build, conformance-baseline.yml). An entry may set `repo` (real clone target for an alias) and `defaultRef` (branch used when no @ref is given). parseSdkSpec now leaves ref undefined when omitted so defaultRef can apply; a trailing @ is treated as no ref. - Key the clone cache by ref (<repo>/<ref>) so different refs of the same repo no longer share one checkout. - Bound the server readiness probe with a per-request AbortSignal timeout so a server that accepts the socket but never responds can't hang past the overall deadline. * fix(sdk-runner): resolve -o to absolute path; add --expected-failures override; replaceAll for safeName
squashed; see PR #288 body
* feat: add server conformance tests for SEP-2575 * refactor and add new checks * Fix stateless scenario source field and report SKIPPED for inapplicable capability checks - Add the required `source` field (DRAFT) so the scenario typechecks and is selectable via --spec-version. Without it the class did not satisfy the ClientScenario interface and s.source was dereferenced by the spec-version filter at list time. - Teach runCheck about a SKIPPED status and use it for the two client-capability checks when the server does not return -32003, instead of reporting a green PASS for a requirement that was never exercised. * Remove unimplemented placeholder checks from stateless server scenario The subscriptions/listen, statelessness-invariant, list-changed and disconnect-is-cancel checks emitted SUCCESS without probing the server, which inflated coverage in the traceability report. Drop them until they have real assertions; the corresponding rows remain declared in src/seps/sep-2575.yaml so traceability shows them as not-yet-covered. Also fix the checkErrorId helper to push under the sep-2575-http-server-error-jsonrpc-id slug so its failure path actually short-circuits the aggregate SUCCESS check at the end of the scenario. --------- Co-authored-by: Paul Carleton <paulc@anthropic.com>
* Add SEP-2352 authorization-server migration scenario SEP-2352 requires that client credentials are bound to the issuing authorization server: when PRM authorization_servers changes to a new issuer, clients MUST re-register and MUST NOT reuse the previous AS's client credentials. - Traceability yaml: 3 checks, 3 excluded (internal state / UI) - New auth/authorization-server-migration scenario (draft suite): two auth servers; PRM flips from AS1 to AS2 after the first authenticated request; AS2 asserts it received a fresh /register and never saw AS1's client_id at /authorize or /token - ConformanceOAuthProvider gains invalidateCredentials and bindIssuer so the everything-client can key credentials by issuer (passing example) - everything-client adds an issuer-aware handler for this scenario that re-reads PRM on each 401 and rebinds before re-authorizing - auth-test-reuse-credentials negative client + vitest case * Drop application_type from DCR metadata (not in SDK OAuthClientMetadata type) --------- Co-authored-by: Paul Carleton <paulc@anthropic.com>
The refresh run failed with 'pnpm: not found' — the reference SDK's build command (typescript-sdk: pnpm install && pnpm run build:all) needs pnpm on PATH. Add 'corepack enable' to the run job.
…290) The `sdk` command exits non-zero when the SDK has conformance failures not in its baseline. The traceability manifest only needs the emitted check IDs (written regardless of pass/fail), so a failing SDK must not fail the refresh. Add `|| true`; the existing 'no results produced' guard catches a genuinely broken run.
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Member
|
Follow-up to track, not for this PR: |
…nion check can't be satisfied by scopes_supported ∪ challenge A client that unions scopes_supported with the challenge — instead of its prior grant with the challenge — would have requested mcp:basic mcp:write and falsely passed sep-2350-scope-union-on-reauth, since scopesSupported happened to coincide with the previously-granted scope. Setting it to a disjoint value makes the check actually require the prior token's scope. Spec backing: clients MUST NOT assume any set relationship between the challenged scope set and scopes_supported, so a disjoint advertisement is realistic.
* Add SEP-837 application_type check to DCR registration SEP-837 requires MCP clients to specify an appropriate application_type during Dynamic Client Registration so OIDC authorization servers can apply the correct redirect-URI constraints. - Traceability yaml: 1 check (presence + valid value), 4 excluded (class-specific SHOULDs unobservable; UI/robustness) - Check added in the shared createAuthServer DCR handler so it fires in every auth scenario that performs DCR (no new scenario) - withOAuthRetry now sets application_type: native (passing example; the conformance example clients are CLI tools) - New auth-test-no-application-type negative client + vitest case * Widen ConformanceOAuthProvider metadata type for application_type Fixes CI typecheck (TS2353 at withOAuthRetry.ts:76): the SDK's OAuthClientMetadataSchema doesn't include application_type yet. registerClient() spreads clientMetadata verbatim into the /register POST body, so a local type intersection is sufficient to get the field on the wire without an SDK release. * Set application_type in runAuthMigrationClient The SEP-2352 authorization-server-migration handler constructs its own ConformanceOAuthProvider (not via withOAuthRetry), so it was missing application_type after rebasing onto main. The scenario does DCR twice (old AS, new AS) and the SEP-837 check fires on each. --------- Co-authored-by: Paul Carleton <paulc@anthropic.com>
Resolve import-line conflict in src/scenarios/client/auth/index.test.ts: both main (#290 added auth-test-reuse-credentials) and this branch added an import to the same line. Keep both.
localden
dismissed
pcarleton’s stale review
The merge-base changed after approval.
Resolve import-line conflict in src/scenarios/client/auth/index.test.ts: both #284 (auth-test-no-application-type) and this branch (auth-test-echo-scope) added an import to the same line. Keep both.
* feat: add conformance tests for iss parameter (SEP-2468) 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. * update scenarios * fix: createAuthServer iss option type/guard and NotAdvertised scenario 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. * fix: rejection scenarios silently pass when client never reaches auth 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. * fix: iss-unexpected scenario contradicts SEP-2468 spec table row 3 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. * refactor: replace harness-config checks with client-proceeded checks 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. * refactor: rename check IDs to sep-2468-* and align with spec table rows 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. * feat: add sep-2468.yaml requirement traceability 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. * fix: migrate iss scenarios specVersions->source (post-#265) 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. * fix: include application_type in iss-validation example DCR (post-#284) The SEP-837 application_type check now runs in every auth scenario; the hand-rolled DCR in auth-test-iss-validation.ts was omitting the field. --------- Co-authored-by: Paul Carleton <paulc@anthropic.com>
Same import-line conflict in src/scenarios/client/auth/index.test.ts; keep both noIssValidationClient (from #220) and echoScopeClient.
localden
dismissed
pcarleton’s stale review
The merge-base changed after approval.
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
5 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.
Closes #281.
Adds the
sep-2350-scope-union-on-reauthcheck to the existingauth/scope-step-upscenario per SEP-2350: on re-authorization after a 403insufficient_scope, clients SHOULD compute the union of previously-granted and newly-challenged scopes.What changed
src/seps/sep-2350.yaml: traceability — 1 clientcheck:, 1 servercheck:(sep-2350-server-single-challenge, tracked for a future server-side scenario), 1excluded:(no-normative-delta reword)ScopeStepUpAuthScenario: the 403 challenge now lists only the missing scope (mcp:write), andtools/callgates onmcp:writealone so the scenario completes for non-accumulating clients (which then surface as a WARNING, not a hang). New check asserts the second authorization request still includes the previously-grantedmcp:basic.examples/.../helpers/withOAuthRetry.tsnow unions the prior token'sscopewith the challenge scope before re-auth.auth-test-echo-scope.ts(re-authorizes with the challenge scope verbatim) + vitest case asserting the WARNING fires.Output —
node dist/index.js client --scenario auth/scope-step-upeverything-client(accumulates)sep-2350-scope-union-on-reauthSUCCESSauth-test-echo-scope(echoes challenge)sep-2350-scope-union-on-reauthWARNINGnpm test: auth suite 28/28 every run.server/all-scenarios.test.tsflakes ~2/3 in the full suite (port contention with the auth servers; passes in isolation) — pre-existing, unrelated to this change.