feat: add positive tests for the Authorization Code Grant by Michito-Okai · Pull Request #267 · modelcontextprotocol/conformance

Michito-Okai · 2026-05-12T05:09:11Z

Motivation and Context

Described in #208

How Has This Been Tested?

npm test (Test Files 10 passed, Tests 126 passed)
npm run start -- authorization --url http://localhost:8080/realms/mcp --client-id testClient --secret qeVFKk0g4qVTyeiX93ZW9ffdOA19L0QO --port 3000

Breaking Changes

add file options for authorization server mode (--client-id --secret --port )

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
I have added appropriate error handling
I have added or updated documentation as needed

pkg-pr-new · 2026-05-12T05:09:45Z

Open in StackBlitz

npx https://pkg.pr.new/@modelcontextprotocol/conformance@267

commit: eeed17f

tnorimat · 2026-05-14T01:36:16Z

 export const AuthorizationServerOptionsSchema = z.object({
-  url: z.string().url('Invalid authorization server URL')
+  url: z.string().url('Invalid authorization server URL'),
+  clientId: z.string().min(1, 'Client id cannot be empty'),


The schema REQUIRES clientId and secret while CLI requires them as optional parameter, which causes contradiction.
One way for resolving is that the schema requires the parameters as optional.

Assuming this PR will be merged, the clientId and secret were optional CLI parameters. I have made clientId and secret mandatory CLI parameters.

tnorimat · 2026-05-14T01:36:30Z

+- Token response MUST return a JSON response including access_token and token_type`;
+
+  async run(
+    option: any,


I think it might be better to be type-safe as much as we can.
How about using AuthorizationServerOptions defined by schemas.ts ?

I changed it from any to AuthorizationServerOptions.

tnorimat · 2026-05-14T01:44:19Z

+import { request } from 'undici';
+import { randomBytes } from 'crypto';
+
+const STATE = randomBytes(32).toString('base64url');


When only the module is loaded, a pseudo-random number is generated and assigned to STATE, which means that multiple runs share that. For now, it might not cause any issue, but in the future, it might cause the issue.

If possible, I think it might be better to generate the value and assign it to STATE per run.

I modified it so that the value of state changes each time run is called.

tnorimat · 2026-05-14T01:46:52Z

+const STATE = randomBytes(32).toString('base64url');
+const REDIRECT_URI_ORIGIN = 'http://localhost';
+const REDIRECT_URI_PATH = '/callback';
+const CODE_VERIFIER = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk';


It seems that CODE_VERIFIER and CODE_CHALLENGE values are from https://datatracker.ietf.org/doc/html/rfc7636#appendix-B as a valid combination of code_challenge and code_verifier.
How about mentioning that here?

I added a comment about CODE_VERIFIER and CODE_CHALLENGE values.

tnorimat · 2026-05-14T01:55:12Z

+    resolveFn = resolve;
+  });
+
+  const server = app.listen(port, () => {


app.listen(port) binds to 0.0.0.0 by default, which means that it listens all IP addresses of the machine running the test. I am afraid that it may cause some security issue.

How about binding explicitly to the loopback address?

const server = app.listen(port, '127.0.0.1', () => { ... });

tnorimat · 2026-05-14T01:58:23Z

+      }
+      const body = resultMetadata.body;
+
+      const callback = startCallbackServer(option.port);


If any request hits the server on a path other than /callback, express returns a default 404 but the server stays open indefinitely (until timeout).

If possible, it might be good if the server receives a request other than /callback, then it server closes and the test finishes as failure.

The callback accepts any path, and the path check is performed during the redirect_uri validation to cause failure if it does not match.

tnorimat · 2026-05-14T02:18:25Z

+      grant_type: 'authorization_code',
+      code,
+      redirect_uri: REDIRECT_URI_ORIGIN + ':' + option.port + REDIRECT_URI_PATH,
+      client_id: option.clientId,


It includes client_id and client_secret are included into HTTP Form body and assumes that a target authorization server supports client_secret_post as client authentication method.

I am afraid that if an authorization server does not support client_secret_post but support client_secret_basic, then this token request fails.

Therefore, it might be better to firstly look at token_endpoint_auth_methods_supported of server metadata and determine which authentication method is used.

I fixed.
If the authorization server does not support client_secret_post and client_secret_basic, it is set to SKIPPED. The implementations of client_secret_jwt, private_key_jwt, and tls_client_auth are out of scope and have been added to the To do list of this ISSUE.

tnorimat · 2026-05-14T02:23:31Z

+      specReferences: [
+        {
+          id: 'Authorization-Code-Grant',
+          url: 'https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-15#section-4.1'


I think It might be better to point to MCP spec's reference that mandate https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-15#section-4.1 .

I fixed the version of the OAuth 2.1 URL.
Also, I added OAUTH_2_1_AUTHORIZATION_CODE_GRANT to spec-refercenses.ts.

tnorimat · 2026-05-14T02:23:40Z

+      specReferences: [
+        {
+          id: 'Authorization-Code-Grant',
+          url: 'https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-15#section-4.1'


I think It might be better to point to MCP spec's reference that mandate https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-15#section-4.1 .

Also, it might be good to refer OAuth 2.1 v13 instead of v15 because:

the latest MCP revision, namely 2025-11-25, refers v13.

other MCP client and server tests refers v13.

Also, how about following the MCP client and server test convention for the way of referring the spec?
E.g, spec-refercenses.ts:

export const SpecReferences: { [key: string]: SpecReference } = { RFC_PRM_DISCOVERY: { id: 'RFC-9728', url: '[https://www.rfc-editor.org/rfc/rfc9728.html#section-3.1'](https://www.rfc-editor.org/rfc/rfc9728.html#section-3.1%27) },

I fixed the version of the OAuth 2.1 URL.
Also, I added OAUTH_2_1_AUTHORIZATION_CODE_GRANT to spec-refercenses.ts.

Michito-Okai mentioned this pull request May 12, 2026

Add positive tests for the Authorization Code Grant #208

Open

tnorimat suggested changes May 14, 2026

View reviewed changes

feat: add positive tests for the Authorization Code Grant

eeed17f

Michito-Okai force-pushed the athorization-code-grant branch from 2f8846b to eeed17f Compare May 14, 2026 06:59

Conversation

Michito-Okai commented May 12, 2026

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Uh oh!

pkg-pr-new Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Michito-Okai May 14, 2026 • 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!

Michito-Okai May 14, 2026 • 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pkg-pr-new Bot commented May 12, 2026 •

edited

Loading

Michito-Okai May 14, 2026 •

edited

Loading

Michito-Okai May 14, 2026 •

edited

Loading