[auth]: support oauth client_secret_basic / none / custom methods by ochafik · Pull Request #720 · modelcontextprotocol/typescript-sdk

ochafik · 2025-07-01T17:15:29Z

This is an attempt to merge #531 and #552

Motivation and Context

This merges two sets of OAuth changes:

PR Allow OAuth client provider to implement token endpoint authentication methods #531 (@jaredhanson): Adds extensible authentication via OAuthClientProvider.addClientAuthentication callback (renamed from PR's proposed authToTokenEndpoint) for custom auth methods
PR Add client_secret_basic and none authentication methods #552 (@SightStudio): Adds built-in support for standard OAuth 2.1 authentication methods (client_secret_basic, none)

How Has This Been Tested?

WIP: testing in inspector

Breaking Changes

n/a

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

Additional context

Main changes from McpError: MCP error -32601: Method not found #415:
- Renamed callback to addClientAuthentication
- Passed callback explicitly to refreshAuthorization & exchangeAuthorization, to avoid the awkwardness of them accepting a provider + other redundant parameters (codeVerifier, redirectUri)

… during authorization code exchange.

… during refresh token exchange.

… refreshAuthorization to maintain compatibility.

… into ochafik/auth-merge-531-552

…ods' into ochafik/auth-merge-531-552

…1-552

The applyBasicAuth function was incorrectly trying to set headers using array notation on a Headers object. Fixed by using the proper Headers.set() method instead of treating it as a plain object. This ensures that HTTP Basic authentication works correctly when client_secret_basic is the selected authentication method. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

jaredhanson · 2025-07-02T00:05:13Z

Thanks @ochafik ! This is looking pretty good. I'm experimenting with the branch right now, and have a suggested modification that I'll propose later tonight or tomorrow.

jaredhanson · 2025-07-02T04:08:51Z

Hi @ochafik - I opened #723, with my proposed changes. Feel free to pull commits from that branch into this one, if you find them acceptable. Thanks for taking up these changes - really looking forward to them landing in a release. Let me know if I can do anything else.

…ication.

SightStudio

I know my feedback came a bit late, but it looks good to me.
I don't have any major comments to add.
Thank you for reviewing it positively!

…1-552

felixweinberger

only some nits, looks clean to me!

felixweinberger · 2025-07-09T15:36:23Z

src/client/auth.test.ts

  type OAuthClientProvider,
 } from "./auth.js";
+import { OAuthMetadata } from 'src/shared/auth.js';


nit: the project seems to use ../shared/auth.js as a pattern, i.e. relative imports (but I actually think this is better)

see discussion: https://anthropic.slack.com/archives/C0840HQ2Y2V/p1752075120212319

Gone relative, thanks for the link

felixweinberger · 2025-07-09T15:47:59Z

src/client/auth.ts

+   * @param url - The token endpoint URL being called
+   * @param headers - The request headers (can be modified to add authentication)
+   * @param params - The request body parameters (can be modified to add credentials)
+   */


ultra-nit: @params are in a different order than the args

Ooops, fixed

felixweinberger · 2025-07-09T15:51:15Z

src/client/auth.ts

+  clientInformation: OAuthClientInformation,
+  supportedMethods: string[]
+): string {
+  const hasClientSecret = !!clientInformation.client_secret;


nit: we don't seem to use !! notation anywhere, maybe prefer explicitness?

const hasClientSecret = clientInformation.client_secret !== undefined && clientInformation.client_secret !== '';

Tightened this / only testing against undefined

felixweinberger · 2025-07-09T15:54:35Z

src/client/auth.ts

+function selectClientAuthMethod(
+  clientInformation: OAuthClientInformation,
+  supportedMethods: string[]
+): string {


should this maybe return an enum?

enum ClientAuthMethod { BASIC = "client_secret_basic", POST = "client_secret_post", NONE = "none" }

felixweinberger · 2025-07-09T15:58:22Z

src/client/auth.ts

+  if (method === "client_secret_basic") {
+    applyBasicAuth(client_id, client_secret, headers);
+    return;
+  }
+
+  if (method === "client_secret_post") {
+    applyPostAuth(client_id, client_secret, params);
+    return;
+  }
+
+  if (method === "none") {
+    applyPublicAuth(client_id, params);
+    return;
+  }


could make this a switch with the enum potentially

felixweinberger · 2025-07-09T16:05:55Z

src/client/auth.ts

+  if (addClientAuthentication) {
+    addClientAuthentication(headers, params, authorizationServerUrl, metadata);
+  } else {
+    // Determine and apply client authentication method
+    const supportedMethods = metadata?.token_endpoint_auth_methods_supported ?? [];
+    const authMethod = selectClientAuthMethod(clientInformation, supportedMethods);
+
+    applyClientAuthentication(authMethod, clientInformation, headers, params);


felixweinberger · 2025-07-09T16:10:52Z

src/client/auth.test.ts

+      const body = request.body as URLSearchParams;
+      expect(body.get("client_id")).toBe("client123");
+      expect(body.get("client_secret")).toBe("secret123");
+    });


should we have a test if it supports everything, that it chooses client_secret_basic

Good idea, added!

felixweinberger · 2025-07-09T16:12:15Z

src/client/auth.test.ts

+      expect(body.get("client_id")).toBeNull();     // should not be in body
+      expect(body.get("client_secret")).toBeNull(); // should not be in body


nit: comments seem redundant

ochafik

Thanks Felix!

ochafik · 2025-07-09T17:23:09Z

src/client/auth.test.ts

+      expect(body.get("client_id")).toBeNull();     // should not be in body
+      expect(body.get("client_secret")).toBeNull(); // should not be in body


ochafik · 2025-07-09T17:23:26Z

src/client/auth.test.ts

+      const body = request.body as URLSearchParams;
+      expect(body.get("client_id")).toBe("client123");
+      expect(body.get("client_secret")).toBe("secret123");
+    });


Good idea, added!

ochafik · 2025-07-09T17:23:48Z

src/client/auth.ts

+   * @param url - The token endpoint URL being called
+   * @param headers - The request headers (can be modified to add authentication)
+   * @param params - The request body parameters (can be modified to add credentials)
+   */


Ooops, fixed

ochafik · 2025-07-09T17:24:14Z

src/client/auth.ts

+function selectClientAuthMethod(
+  clientInformation: OAuthClientInformation,
+  supportedMethods: string[]
+): string {


ochafik · 2025-07-09T17:25:20Z

src/client/auth.ts

+  clientInformation: OAuthClientInformation,
+  supportedMethods: string[]
+): string {
+  const hasClientSecret = !!clientInformation.client_secret;


Tightened this / only testing against undefined

ochafik · 2025-07-09T17:25:26Z

src/client/auth.ts

+  if (method === "client_secret_basic") {
+    applyBasicAuth(client_id, client_secret, headers);
+    return;
+  }
+
+  if (method === "client_secret_post") {
+    applyPostAuth(client_id, client_secret, params);
+    return;
+  }
+
+  if (method === "none") {
+    applyPublicAuth(client_id, params);
+    return;
+  }


ochafik · 2025-07-09T17:26:31Z

src/client/auth.test.ts

  type OAuthClientProvider,
 } from "./auth.js";
+import { OAuthMetadata } from 'src/shared/auth.js';


Gone relative, thanks for the link

ochafik · 2025-07-10T16:30:51Z

Oh absolutely, fixed, thanks for bringing it up!

…

On Thu, Jul 10, 2025 at 4:34 PM Dong Min Seol ***@***.***> wrote: *SightStudio* left a comment (modelcontextprotocol/typescript-sdk#720) <#720 (comment)> https://github.com/modelcontextprotocol/typescript-sdk/releases/tag/1.15.1 @jerome3o-anthropic <https://github.com/jerome3o-anthropic> It would be nice if co-authors could also be added to that list of contributors. ☺️ — Reply to this email directly, view it on GitHub <#720 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACC3RDBYXLKVW3ITFRLPA33H2BWPAVCNFSM6AAAAACARWCVGKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTANJXHE2TKNJTGY> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

-- -- Olivier

jaredhanson and others added 19 commits May 21, 2025 19:14

Allow OAuthClientProvider to control authentication to token endpoint…

9b2915e

… during authorization code exchange.

Include mockProvider in exchangeAuthorization tests.

c7737a8

Add test case for authToTokenEndpoint during exchangeAuthorization.

439a8d3

Allow OAuthClientProvider to control authentication to token endpoint…

92f9030

… during refresh token exchange.

Add test case for authToTokenEndpoint during refreshAuthorization.

58c2332

Make provider an optional final argument to exchangeAuthorization and…

9ef072f

… refreshAuthorization to maintain compatibility.

Add url argument to authToTokenEndpoint.

c0e9ef6

feature. Add client_secret_basic and none authentication methods

a055fb3

Merge branch 'main' into feat/oauth-client-auth-methods

562d487

Merge branch 'main' into feat/oauth-client-auth-methods

ab2f890

Merge remote-tracking branch 'jaredhanson/jaredhanson/token-auth-ext'…

b803121

… into ochafik/auth-merge-531-552

Merge remote-tracking branch 'SightStudio/feat/oauth-client-auth-meth…

38244b8

…ods' into ochafik/auth-merge-531-552

Merge remote-tracking branch 'origin/main' into ochafik/auth-merge-53…

8bae46e

…1-552

rename authToTokenEndpoint -> addClientAuthentication

afe1279

Merge branch 'main' into ochafik/auth-merge-531-552

474db45

update tests

c7f9e36

revert / minimize diffs

3444b67

cleanup

3ccff1c

This was referenced Jul 1, 2025

Allow OAuth client provider to implement token endpoint authentication methods #531

Closed

Add client_secret_basic and none authentication methods #552

Closed

ochafik added this to the auth milestone Jul 1, 2025

ochafik requested a review from pcarleton July 1, 2025 17:24

jaredhanson mentioned this pull request Jul 2, 2025

[auth]: revision of support oauth client_secret_basic / none / custom methods #723

Merged

9 tasks

jaredhanson and others added 4 commits July 3, 2025 21:18

Add authorizationServerUrl and metadata arguments to addClientAuthent…

8875e21

…ication.

Fix tests.

0bd770b

Update addClientAuthentication tests for metadata.

b797e9c

Fix optional metadata arg type signature in auth.test.ts

2568a21

SightStudio reviewed Jul 7, 2025

View reviewed changes

ochafik added 2 commits July 7, 2025 13:32

test: fix lint

1600a56

Merge remote-tracking branch 'origin/main' into ochafik/auth-merge-53…

638deb2

…1-552

ochafik marked this pull request as ready for review July 7, 2025 12:32

Merge branch 'main' into ochafik/auth-merge-531-552

4bfd005

felixweinberger approved these changes Jul 9, 2025

View reviewed changes

apply suggested updates

906005c

ochafik commented Jul 9, 2025

View reviewed changes

ochafik merged commit 031dfc2 into main Jul 9, 2025
5 checks passed

ochafik deleted the ochafik/auth-merge-531-552 branch July 9, 2025 17:31

jaredhanson mentioned this pull request Jul 11, 2025

Allow Agent instance to supply custom OAuth provider. cloudflare/agents#298

Merged

kyle-leonhard mentioned this pull request Jul 24, 2025

Register oauth clients with a preferred auth method modelcontextprotocol/inspector#642

Open

9 tasks

		expect(body.get("client_id")).toBeNull(); // should not be in body
		expect(body.get("client_secret")).toBeNull(); // should not be in body

Conversation

ochafik commented Jul 1, 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!

jaredhanson commented Jul 2, 2025

Uh oh!

jaredhanson commented Jul 2, 2025

Uh oh!

SightStudio left a comment

Choose a reason for hiding this comment

Uh oh!

felixweinberger left a comment

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!

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!

ochafik left a comment

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!

Uh oh!

ochafik commented Jul 10, 2025 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ochafik commented Jul 1, 2025 •

edited

Loading