fix: throw error on auth fallback for non-root AS paths by guoyangzhen · Pull Request #1724 · modelcontextprotocol/typescript-sdk

guoyangzhen · 2026-03-20T15:13:34Z

Summary

When authorization server metadata discovery fails and the server URL has a non-root path (e.g., https://example.com/admin), the fallback to /authorize, /token, and /register endpoints uses absolute paths that silently lose the URL path prefix.

For example:

new URL('/authorize', 'https://example.com/admin') → https://example.com/authorize (wrong!)
Correct would be: https://example.com/admin/authorize

Problem

Three locations in packages/client/src/client/auth.ts construct fallback URLs using absolute paths:

startAuthorization() line ~1204: new URL('/authorize', authorizationServerUrl)
executeTokenRequest() line ~1286: new URL('/token', authorizationServerUrl)
registerClient() line ~1533: new URL('/register', authorizationServerUrl)

Per RFC 6749, the /.well-known defaults only apply to authorization servers at the domain root. For non-root paths, metadata discovery is required.

Fix

Added checks for non-root paths. When metadata discovery fails and the AS URL has a non-root pathname, a descriptive error is thrown instead of silently constructing a wrong URL.

Impact

n- Before: Silent redirect to wrong URL, broken auth flow with no diagnostic info

After: Clear error message indicating metadata discovery failed for a non-root AS

Fixes modelcontextprotocol#1716 When authorization server metadata discovery fails and the server URL has a non-root path, the fallback to /authorize, /token, and /register endpoints silently constructs wrong URLs (losing the path prefix). This fix throws a descriptive error instead of silently redirecting to nonexistent endpoints. Affected locations: - startAuthorization: /authorize fallback - executeTokenRequest: /token fallback - registerClient: /register fallback

changeset-bot · 2026-03-20T15:13:45Z

🦋 Changeset detected

Latest commit: 237bff5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@modelcontextprotocol/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

boomboomtxzzit-coder

..

felixweinberger

Thanks for this. Seeing a bunch of CI failures here. authorizationServerUrl is string | URL (auth.ts:621), so the .pathname checks need new URL(authorizationServerUrl).pathname to avoid hitting undefined on the string case.

… string | URL address review feedback from felixweinberger

guoyangzhen · 2026-04-01T04:07:07Z

Fixed! Changed all three authorizationServerUrl.pathname accesses to new URL(authorizationServerUrl).pathname to handle the string | URL type correctly.

pkg-pr-new · 2026-04-01T04:08:36Z

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@1724

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@1724

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@1724

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@1724

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@1724

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@1724

commit: 237bff5

- Fix test mock to handle AS metadata discovery with /oauth path - Refactor ternary in executeTokenRequest to if/else to satisfy lint - Flip pathname checks to use positive condition (=== '/') first

felixweinberger · 2026-04-02T17:26:27Z

@claude review

claude · 2026-04-02T17:36:24Z

packages/client/test/client/auth.test.ts

+                } else if (urlString === 'https://auth.example.com/.well-known/oauth-authorization-server/path/name' ||
+                           urlString === 'https://auth.example.com/.well-known/oauth-authorization-server/oauth') {
+                    // Path-aware discovery on AS with path from serverUrl or AS path


🟡 The first branch of the OR condition in the fetch mock (urlString === 'https://auth.example.com/.well-known/oauth-authorization-server/path/name') is dead code that can never match, giving a false impression that the /path/name discovery path is being tested. Remove the unreachable /path/name branch and keep only the /oauth branch that actually gets triggered.

Extended reasoning...

Dead code in test mock: /path/name branch is unreachable

What the bug is:
In the test fetches AS metadata with path from serverUrl when PRM returns external AS (around line 3038 in auth.test.ts), the PR changed the mock condition from a single check for /path/name to an OR of two conditions adding /oauth while keeping the old /path/name. The /path/name branch will never match given the current test setup.

The code path that triggers the issue:
The PRM mock returns authorization_servers: ['https://auth.example.com/oauth']. The auth() flow extracts authorizationServerUrl = 'https://auth.example.com/oauth'. Then discoverAuthorizationServerMetadata('https://auth.example.com/oauth') calls buildDiscoveryUrls('https://auth.example.com/oauth'), which generates exactly these URLs:

https://auth.example.com/.well-known/oauth-authorization-server/oauth

https://auth.example.com/.well-known/openid-configuration/oauth

https://auth.example.com/oauth/.well-known/openid-configuration

None of these is https://auth.example.com/.well-known/oauth-authorization-server/path/name.

Why existing code does not prevent this:
The test still passes because the /oauth branch of the OR correctly handles the actual fetch request. The dead /path/name branch is simply never executed, but the test does not assert that both branches are hit, so the mistake goes undetected.

Where /path/name comes from:
/path/name is part of the resource server URL (serverUrl: 'https://my.resource.com/path/name'), not the authorization server URL. A previous version of the test apparently placed the resource server path into the AS mock URL by mistake. The PR fixed the active branch to use /oauth matching the PRM response, but inadvertently preserved the old incorrect branch as dead code.

Proof step by step:

PRM mock responds with authorization_servers: ['https://auth.example.com/oauth']

authorizationServerUrl becomes 'https://auth.example.com/oauth'

buildDiscoveryUrls('https://auth.example.com/oauth') enters the non-root branch and returns URLs all containing /oauth, never /path/name

The fetch mock is called with one of those three /oauth-containing URLs

The condition urlString === 'https://auth.example.com/.well-known/oauth-authorization-server/path/name' is evaluated but is never true

The test assertion (expect(calls[1]![0].toString()).toBe('https://auth.example.com/.well-known/oauth-authorization-server/oauth')) confirms only the /oauth URL is ever requested

How to fix:
Remove the dead urlString === 'https://auth.example.com/.well-known/oauth-authorization-server/path/name' condition and its associated comment, keeping only the /oauth branch that is actually triggered.

claude · 2026-04-02T17:36:24Z

packages/client/src/client/auth.ts


    if (!response.ok) {
        throw await parseErrorResponse(response);
    }


🟡 The PR removes the trailing newline at the end of packages/client/src/client/auth.ts (line 1742), which violates Prettier formatting rules. This will cause pnpm lint:all (which runs prettier --check .) to fail in CI -- easily fixed by adding a newline after the closing }.

Extended reasoning...

What the bug is: The last line of packages/client/src/client/auth.ts is the closing brace of the registerClient function, and the PR introduces it without a trailing newline character. The diff explicitly shows the POSIX warning No newline at end of file on the added line at position 1742, confirming the omission.

The specific code path: The change is at the very end of the file -- the closing brace of the registerClient export. The PR restructured the fallback logic inside that function and in doing so dropped the trailing newline that was present in the original file.

Why existing code does not prevent it: There is no pre-commit hook enforcing newlines at save time. The CI lint step runs Prettier in check mode, but that check only catches the problem after the PR is submitted -- it does not prevent the omission locally.

Impact: Prettier enforces a trailing newline by default. The client package's lint script runs prettier --check ., so any CI run that executes pnpm lint:all on this branch will report a formatting error for auth.ts and fail the build. No runtime behavior is affected.

How to fix: Add a single newline character after the closing brace on the last line of the file. This restores the file to POSIX-compliant and Prettier-compliant form.

Step-by-step proof:

The original file ended with a closing brace followed by a newline character.

The PR diff ends with the added line carrying the POSIX warning -- the new version omits the trailing newline.

Running od -c packages/client/src/client/auth.ts | tail -1 confirms the last byte is } with no following newline.

Running npx prettier --check packages/client/src/client/auth.ts exits with code 1, warning about this file.

This causes pnpm lint:all to fail in CI.

guoyangzhen · 2026-04-03T03:19:41Z

Thanks for the review! I've fixed the trailing newline issue that was causing the prettier check to fail.

Regarding the type concern — the code already uses in all three locations (lines 1374, 1465, 1719), which correctly handles both and types since the constructor accepts both.

The build should pass now. Could you take another look?

guoyangzhen requested a review from a team as a code owner March 20, 2026 15:13

add changeset

ea8ea36

boomboomtxzzit-coder reviewed Mar 22, 2026

View reviewed changes

felixweinberger requested changes Mar 31, 2026

View reviewed changes

felixweinberger added the auth Issues and PRs related to Authentication / OAuth label Mar 31, 2026

fix: use new URL() for pathname access when authorizationServerUrl is…

30afe5e

… string | URL address review feedback from felixweinberger

young and others added 2 commits April 2, 2026 13:24

fix: flip negated conditions for unicorn/no-negated-condition lint rule

b7bb8b1

- Fix test mock to handle AS metadata discovery with /oauth path - Refactor ternary in executeTokenRequest to if/else to satisfy lint - Flip pathname checks to use positive condition (=== '/') first

Merge branch 'main' into fix/auth-fallback-non-root-path

c7446b7

felixweinberger requested a review from pcarleton April 2, 2026 17:27

claude bot reviewed Apr 2, 2026

View reviewed changes

fix: add missing trailing newline for prettier

a7d98ce

guoyangzhen added 5 commits April 3, 2026 11:22

fix: apply prettier formatting to auth.ts

0ccd8e0

fix: apply prettier formatting to auth.test.ts

78ba5ab

fix: apply prettier 3.6.2 formatting to auth.test.ts

2950bec

fix: apply project prettier config to auth.ts

25d2d07

fix: apply project prettier config to auth.test.ts

237bff5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: throw error on auth fallback for non-root AS paths#1724

fix: throw error on auth fallback for non-root AS paths#1724
guoyangzhen wants to merge 11 commits intomodelcontextprotocol:mainfrom
guoyangzhen:fix/auth-fallback-non-root-path

guoyangzhen commented Mar 20, 2026

Uh oh!

changeset-bot bot commented Mar 20, 2026 •

edited

Loading

Uh oh!

boomboomtxzzit-coder left a comment •

edited

Loading

Uh oh!

felixweinberger left a comment

Uh oh!

guoyangzhen commented Apr 1, 2026

Uh oh!

pkg-pr-new bot commented Apr 1, 2026 •

edited

Loading

Uh oh!

felixweinberger commented Apr 2, 2026

Uh oh!

claude bot Apr 2, 2026

Uh oh!

claude bot Apr 2, 2026

Uh oh!

guoyangzhen commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

guoyangzhen commented Mar 20, 2026

Summary

Problem

Fix

Impact

Uh oh!

changeset-bot bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

boomboomtxzzit-coder left a comment • 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!

felixweinberger left a comment

Choose a reason for hiding this comment

Uh oh!

guoyangzhen commented Apr 1, 2026

Uh oh!

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

Uh oh!

Uh oh!

felixweinberger commented Apr 2, 2026

Uh oh!

claude bot Apr 2, 2026

Choose a reason for hiding this comment

Dead code in test mock: /path/name branch is unreachable

Uh oh!

claude bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

guoyangzhen commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

changeset-bot bot commented Mar 20, 2026 •

edited

Loading

boomboomtxzzit-coder left a comment •

edited

Loading

pkg-pr-new bot commented Apr 1, 2026 •

edited

Loading

Dead code in test mock: `/path/name` branch is unreachable