feat(server-auth-legacy): add frozen v1 Authorization-Server package#1908
feat(server-auth-legacy): add frozen v1 Authorization-Server package#1908felixweinberger wants to merge 5 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 2fc7351 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/server-auth-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
felixweinberger
force-pushed
the
fweinberger/v2-bc-server-auth-legacy
branch
2 times, most recently
from
652b1db to
099bdf1
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/v2-bc-server-auth-legacy
branch
3 times, most recently
from
49747fd to
5700f66
Compare
felixweinberger
force-pushed
the
fweinberger/v2-bc-server-auth-legacy
branch
from
5700f66 to
10c6b28
Compare
|
@claude review |
felixweinberger
force-pushed
the
fweinberger/v2-bc-server-auth-legacy
branch
from
10c6b28 to
7cac712
Compare
|
@claude review |
|
@claude review |
|
@claude review |
Reviewer guide — what's copy-paste vs. modifiedThis package is a frozen copy of v1's Verbatim from v1 (skim — review is "did we copy faithfully?")
Modified / new (actually review these)
Maintenance contractFrozen. Security patches only. Consumers should migrate to an external AS (better-auth, etc.) for new work; this exists so v1 OAuth-provider servers (e.g. shortcut-mcp-server) can upgrade without rewriting their AS. |
There was a problem hiding this comment.
51eabff resolves both remaining threads (express is back to a required peer, and the OAuth-error-consolidation sections in migration.md/migration-SKILL.md now scope the subclass→OAuthError rewrite to client-side) — nothing further from me. Leaving final sign-off to a human given this adds a new published package + OAuth AS code + a publish.yml change, and the npm deprecated banner / README forward-reference @modelcontextprotocol/express exports that only exist once #1907 lands.
Extended reasoning...
Overview
Adds @modelcontextprotocol/server-auth-legacy, a new workspace package that is a frozen copy of v1's src/server/auth/ (router, handlers, middleware, ProxyOAuthServerProvider, error hierarchy), plus the full ported v1 test suite (10 files / ~140 cases), package scaffolding (tsdown/eslint/vitest/typedoc), a changeset, an entry in .changeset/pre.json, an extra path in .github/workflows/publish.yml, and migration-doc edits in docs/migration.md, docs/migration-SKILL.md, and docs/faq.md.
State since last review
My two open inline comments from the previous round are addressed by 51eabff:
package.jsonoptional express peer — thepeerDependenciesMetablock is gone;expressis again a required peer (matching sibling@modelcontextprotocol/express).- OAuth-error-consolidation contradiction — both
docs/migration.md(§OAuth error refactoring) anddocs/migration-SKILL.md(§5) now carry an explicit note that the v1 subclass hierarchy /OAUTH_ERRORSremain available fromserver-auth-legacyfor AS implementations, with theOAuthError+OAuthErrorCoderewrite scoped to client-side.
The bug-hunting system found nothing new at this commit.
Security risks
The package is an OAuth 2.0 Authorization Server implementation (token issuance, PKCE verification, client-secret comparison, bearer-token verification, dynamic client registration). It's a verbatim port of code that has shipped in v1 for a long time and is covered by the ported v1 test suite, but it is unambiguously security-sensitive surface.
Level of scrutiny
High — new published npm package, security-relevant code paths, CI/publish workflow change, and migration-guide edits that will steer every v1→v2 consumer. The remaining intentional forward-reference (README L11, package.json "deprecated" field, changeset, src/index.ts @deprecated JSDoc all point at RS helpers in @modelcontextprotocol/express that ship in #1907) is a release-coordination decision the maintainer has explicitly accepted, but it does mean merge order with #1907 matters for the published deprecate banner.
Other factors
This PR has been through seven review rounds with substantive fixes each time (barrel completeness, full v1 test port, pkg-pr-new inclusion, doc self-consistency, peer-dep correctness). It's in good shape now; deferring solely on size/sensitivity, not on any open defect.
Frozen, deprecated copy of the v1 SDK's src/server/auth/ Authorization Server helpers (mcpAuthRouter, ProxyOAuthServerProvider, OAuth handlers, middleware, and error subclasses) as a standalone package for v1 -> v2 migration. The package carries a package.json "deprecated" field directing users to a dedicated IdP plus the Resource Server helpers in @modelcontextprotocol/express. Imports of OAuth types/schemas are rewritten to @modelcontextprotocol/core; AuthInfo is re-exported from core for structural compatibility with v2 request-handler context. Minimal edits vs v1 source: override modifiers and noUncheckedIndexedAccess fixes to satisfy the v2 strict tsconfig; behaviour is unchanged.
…a describe block Ports the two remaining v1 test files/blocks flagged in review: - test/middleware/bearerAuth.test.ts (20 cases): requireBearerAuth coverage - test/router.test.ts: append 'MCP Auth Metadata Router' describe (4 cases) - test/helpers/http.ts: createExpressResponseMock (used by bearerAuth tests) All 10 v1 server/auth test files now ported. 142 tests pass.
…subclass availability in migration docs
felixweinberger
force-pushed
the
fweinberger/v2-bc-server-auth-legacy
branch
from
51eabff to
2fc7351
Compare
| */ | ||
|
|
||
| export type { OAuthRegisteredClientsStore } from './clients.js'; | ||
| export * from './errors.js'; |
There was a problem hiding this comment.
🟡 nit: this is the only export * in an otherwise fully-explicit barrel — CLAUDE.md §Public API Exports (L74) says "Use explicit named exports, not export *, in package index.ts files", and the reviewer guide above flags this file as a "curated public surface". All 19 errors.ts symbols are intentionally public so there's no leak risk; consider listing them explicitly (OAuthError, the 17 subclasses, OAUTH_ERRORS) for consistency with the rest of the file. Non-blocking — feel free to close if you'd rather keep the frozen-copy barrel terse.
Extended reasoning...
What the issue is
packages/server-auth-legacy/src/index.ts:14 does:
export * from './errors.js';while every other re-export in the same barrel (lines 13, 15-34) uses explicit named exports — export { authorizationHandler, redirectUriMatches } from ..., export type { TokenHandlerOptions } from ..., etc. This is the lone wildcard in an otherwise fully-curated file.
The documented convention
CLAUDE.md:74 (§Public API Exports) states verbatim:
Use explicit named exports, not
export *, in packageindex.tsfiles andcore/public.
and REVIEW.md:41's checklist cross-references it: "Every new export is intentional (see CLAUDE.md § Public API Exports)". The maintainer's own reviewer-guide comment on this PR lists src/index.ts under "Modified / new (actually review these)" as a "curated public surface", so the convention is in scope for this file.
Step-by-step
| Step | Observation |
|---|---|
| 1 | src/index.ts has 21 export statements; 20 are explicit named exports, 1 (L14) is export * |
| 2 | src/errors.ts exports 19 symbols: OAuthError, 16 concrete subclasses, CustomOAuthError, and OAUTH_ERRORS |
| 3 | All 19 are intentionally public — docs/migration-SKILL.md:196 and docs/migration.md:801 explicitly tell users the v1 subclass hierarchy + OAUTH_ERRORS are "still available from @modelcontextprotocol/server-auth-legacy" |
| 4 | So there is no accidental-surface-leak risk here — the rule's stated rationale (CLAUDE.md:75: "Adding a symbol … makes it public API — do so intentionally") is satisfied in substance |
| 5 | The remaining concern is purely the documented convention + internal consistency of this barrel |
Addressing the counter-arguments
One verifier argued this isn't worth flagging because (a) other packages already use export *, (b) all exports are intentional, (c) it's a frozen deprecated package, and (d) it's not egregious.
On (a): packages/client/src/index.ts:81 and packages/server/src/index.ts:52 do use export *, but only as export * from '@modelcontextprotocol/core/public' — i.e. re-exporting an already-curated surface that is itself governed by the same rule. That's structurally different from wildcarding an internal source module. packages/core/src/index.ts does use export * extensively, but CLAUDE.md explicitly distinguishes the internal core barrel from core/public. So the precedent isn't as broad as it first appears, though it does show the rule has some flex.
On (b)/(c)/(d): all fair, which is why this is a nit, not a blocker. There's zero functional impact, and the maintainer could reasonably decline.
The reason it's still worth a one-line mention: this barrel went through several review rounds specifically auditing which symbols are reachable (ClientAuthenticationMiddlewareOptions, redirectUriMatches), the maintainer explicitly described it as "curated", and it's the single line in the file that doesn't enumerate what it exports. If someone later adds an internal helper to errors.ts, it silently becomes public API — exactly what the explicit-export convention guards against.
Fix
Replace L14 with:
export {
OAuthError,
InvalidRequestError,
InvalidClientError,
InvalidGrantError,
UnauthorizedClientError,
UnsupportedGrantTypeError,
InvalidScopeError,
AccessDeniedError,
ServerError,
TemporarilyUnavailableError,
UnsupportedResponseTypeError,
UnsupportedTokenTypeError,
InvalidTokenError,
MethodNotAllowedError,
TooManyRequestsError,
InvalidClientMetadataError,
InsufficientScopeError,
InvalidTargetError,
CustomOAuthError,
OAUTH_ERRORS
} from './errors.js';Mechanical, no behavior change.
1 participant
Part of the v2 backwards-compatibility series — see reviewer guide.
New
@modelcontextprotocol/server-auth-legacypackage — frozen v1 copy ofmcpAuthRouter/ProxyOAuthServerProvider/handlers.npm deprecated on publish with a message pointing to a real IdP + the RS helpers in/express.Motivation and Context
New
@modelcontextprotocol/server-auth-legacypackage — frozen v1 copy ofmcpAuthRouter/ProxyOAuthServerProvider/handlers.npm deprecated on publish with a message pointing to a real IdP + the RS helpers in/express.v1 vs v2 pattern & evidence
v1 pattern:
`import { mcpAuthRouter } from '@modelcontextprotocol/sdk/server/auth/router.js'`v2-native:
Evidence: Unblocks consumers on the v1 AS impl with a clear migration signal.
How Has This Been Tested?
v2-bc-integrationvalidation branchpnpm typecheck:all && pnpm lint:all && pnpm test:allgreenBreaking Changes
None — additive
@deprecatedshim. Removed in v3.Types of changes
Checklist
Additional context
Stacks on: none