[auth] Scenarios for scope selection by pcarleton · Pull Request #36 · modelcontextprotocol/conformance

pcarleton · 2025-11-18T13:14:20Z

Motivation and Context

fixes #32

How Has This Been Tested?

Breaking Changes

tests and negative tests

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
- needs a typescript sdk change to pass
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

I did a few related refactors:

Inlined tests
allowed piping test output to jq
some fixes around how we handled WARNING, since it's a bunch of SHOULD's in this case

pcarleton · 2025-11-18T13:39:29Z

src/runner/client.ts

  const scenario = getScenario(scenarioName)!;

-  console.log(`Starting scenario: ${scenarioName}`);
+  console.error(`Starting scenario: ${scenarioName}`);


this is to make it possible to pipe verbose output to jq or jless

pcarleton · 2025-11-18T13:40:59Z

src/scenarios/client/auth/helpers/createServer.ts

+  server.setRequestHandler(
+    CallToolRequestSchema,
+    async (request): Promise<CallToolResult> => {
+      if (request.params.name === 'test-tool') {


this is so we can test an "auth only required on a tool call" scenario. It's via setRequestHandler because we're in the low level interface

pcarleton · 2025-11-18T13:42:26Z

src/scenarios/client/auth/helpers/mockTokenVerifier.ts

+    private expectedScopes: string[] = []
+  ) {}
+
+  registerToken(token: string, scopes: string[]) {


we're keeping track of the scopes here as a convenience. we could also fetch them from the AS, and might want to switch to doing that in the future.

pcarleton · 2025-11-18T13:43:31Z

src/scenarios/client/auth/index.test.ts

+  InlineClientRunner
 } from './test_helpers/testClient.js';
-import path from 'path';
+import { runClient as goodClient } from '../../../../examples/clients/typescript/auth-test.js';


I refactored the client tests to allow running them "inline" rather than spawning a subshell.

A subshell took a good 600ms, so the full auth suite took about 13s. With this, it's about 200ms for the full suite, so vitest watch is like a live refresh.

pcarleton · 2025-11-18T13:43:49Z

src/scenarios/client/auth/index.test.ts

-    'examples/clients/typescript/auth-test.ts'
-  );
+beforeAll(() => {
+  setLogLevel('error');


without this, we get a bunch of annoying stdout lines littering the test output

pcarleton · 2025-11-18T13:45:44Z

src/scenarios/client/auth/test_helpers/testClient.ts

      // Verify that only the expected checks failed
-      const failures = nonInfoChecks.filter((c) => c.status === 'FAILURE');
+      const failures = nonInfoChecks.filter(
+        (c) => c.status === 'FAILURE' || c.status === 'WARNING'


I'm counting warning as failure in this test because we want the examples to not have warnings

pcarleton · 2025-11-18T13:48:18Z

This is ready for review, but needs a typsecript change for tests to pass, will push that shortly.

pcarleton · 2025-11-18T14:09:21Z

typescript-sdk PR to have this pass:
modelcontextprotocol/typescript-sdk#1133

I think I'll comment out the broken ones for now since we'll need to wait for typescript release i think to get the fix. (unless we want to patch release?)

pcarleton · 2025-11-18T14:25:58Z

okay I skipped tests, we can unskip them later

felixweinberger · 2025-11-18T16:37:39Z

examples/clients/typescript/auth-test-ignore-403.ts

+ * Broken client that only responds to 401, not 403.
+ * BUG: Ignores 403 responses which are used for step-up auth.
+ */
+export async function runClient(serverUrl: string): Promise<void> {


more of a style nit: all these "broken" examples look very similar (identical?) except for the specific way in which they're "broken".

Could consider having a single implementation and having the specific way it's actually broken be a param to createBrokenHandler, a higher level function that creates the handle401broken.

type ClientBehavior = | { type: 'well-behaved' } // or just omit for default | { type: 'ignore-resource-metadata' } | { type: 'ignore-scope' } | { type: 'partial-scopes'; scope: string } | { type: 'ignore-403' };

Could make this less verbose.

Potentially the well-behaved client could also share the same implementation.

Maybe we prefer explicitness over conciseness though for conformance tests, so definitely just a thought 🤔

agreed on the repetitiveness, i've been resisting the urge to de-dupe too aggressively in order to let abstractions fall out from a few more iterations on tests before committing to one.

felixweinberger · 2025-11-18T16:45:40Z

package.json

    "@types/express": "^5.0.3",
    "@types/node": "^22.10.2",
    "@typescript/native-preview": "^7.0.0-dev.20251030.1",
+    "cors": "^2.8.5",


Is adding cors here intentional for this PR? Doesn't seem like we're importing this anywhere?

when I ran npm link this started complaining, i think because the everything server uses it (even if we have a package.json in the server client repo).

I'd prefer to keep it to let the link flow be smoother even if it's a little odd.

pcarleton added 16 commits November 18, 2025 09:49

wip scope change

e37c97f

rm useless bool

aa16772

fix-me: package changes for linking

ef31f40

negative test

53ae690

negative test for multiple scopes

70fcc3a

fix displays of warnings

194fbae

wip step-up

7f94fed

negative test for step up auth

8b378be

move warning into main request flow

8d24f02

clean up more checks

ab7ecd5

cleanup step-up scenario

258d859

cleanup and simplify

778c933

cleanup middleware

e7185ff

inline negative tests

7bab0bb

inline the other test

ec554f5

refactor logging and inlining for standalone execution too

8961318

pcarleton commented Nov 18, 2025

View reviewed changes

pcarleton marked this pull request as ready for review November 18, 2025 13:48

pcarleton requested a review from felixweinberger November 18, 2025 13:48

pcarleton mentioned this pull request Nov 18, 2025

[auth] Adjust scope management to line up with SEP-835 modelcontextprotocol/typescript-sdk#1133

Merged

9 tasks

skip pending tests

ee9015e

felixweinberger approved these changes Nov 18, 2025

View reviewed changes

pcarleton merged commit 5c15113 into main Nov 18, 2025
4 of 7 checks passed

pcarleton deleted the pcarleton/scopes-selection branch November 18, 2025 17:16

Conversation

pcarleton commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcarleton commented Nov 18, 2025

Uh oh!

pcarleton commented Nov 18, 2025

Uh oh!

pcarleton commented Nov 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixweinberger Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pcarleton commented Nov 18, 2025 •

edited

Loading

felixweinberger Nov 18, 2025 •

edited

Loading