From 6e0e4e87fcecf9c2266ca679a10f918cb9859d31 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 00:59:33 +0000 Subject: [PATCH] fix(plugin-auth): collapse the three basePath derivations onto one chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AuthManager` derived its base path in three independent places — `getBasePath()`, `getAuthIssuer()` and `getMcpResourceUrl()` — each reading `this.config.basePath` and normalising it its own way. One of them built a value that is not a URL. `getMcpResourceUrl()` added no leading slash, so a `basePath` configured without one produced `http://localhost:3000api/v1/mcp`. Measured: `new URL()` throws on it (`3000api` is not a port), so `auth-plugin.ts`'s `new URL(manager.getMcpResourceUrl()).pathname` — which mounts the RFC 9728 §3.1 path-inserted well-known route — throws too, and `@better-auth/oauth-provider` 1.7.2 refuses it at plugin init: oauth-provider: skipping resource seed for http://localhost:3000api/v1/mcp — resource identifier ... must be an absolute URI (RFC 8707 §2) Post-#16780 that identifier seeds the `sys_oauth_resource` row (`resources`) and links every DCR client to it (`clientRegistrationDefaultResources`), with `enforcePerClientResources` at its `true` default — so the row is never written and every MCP client is refused. That input class could never mint or match a token, which is why repairing it re-selects nothing. There is now exactly ONE read of `this.config.basePath` in the file and one chain above it: configuredBasePath() the configured value VERBATIM — what better-auth is handed └─ rootedBasePath() + a leading slash when absent (better-auth's own rule) ├─ getAuthIssuer() = origin + this └─ getBasePath() = this, trailing slashes stripped └─ getMcpResourceUrl() = origin + this minus `/auth` + `/mcp` A fourth normaliser cannot be added without deleting a link of that chain. ⛔ `getAuthIssuer()` is NOT canonicalised, and the card's second defect is not repaired here because it does not exist on `main`. The card and its triage both read PR #16380 as having handed better-auth the STRIPPED base path while `getAuthIssuer()` broadcast the RETAINED one. #16380's last commit ("hand better-auth the configured basePath verbatim again") reverted exactly that, having measured that it rejects every token minted under a trailing-slash `basePath`. Measured here on a real `betterAuth()`, reading `(await auth.$context).baseURL` — the value the oauth-provider stamps as `iss` — `getAuthIssuer()` already equals it for all six spellings probed. A new case pins that equality against the real instance, so the divergence cannot be introduced by a later "canonicalisation". `getAuthIssuer()` and `getBasePath()` are byte-identical to before for every spelling. Only `getMcpResourceUrl()` moves, and only for a non-canonical `basePath`. The canonical-input control asserts all three getters unchanged. Refs #16399 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --- .../basepath-normaliser-consolidation.md | 33 ++++ .../src/auth-manager-base-path.test.ts | 165 +++++++++++++++++- .../plugins/plugin-auth/src/auth-manager.ts | 112 ++++++++---- 3 files changed, 272 insertions(+), 38 deletions(-) create mode 100644 .changeset/basepath-normaliser-consolidation.md diff --git a/.changeset/basepath-normaliser-consolidation.md b/.changeset/basepath-normaliser-consolidation.md new file mode 100644 index 0000000000..b402f663ba --- /dev/null +++ b/.changeset/basepath-normaliser-consolidation.md @@ -0,0 +1,33 @@ +--- +'@objectstack/plugin-auth': patch +--- + +fix(plugin-auth): one base-path normalisation chain, and an MCP resource identifier that is always a URL + +`AuthManager` derived its base path in three independent places. `getMcpResourceUrl()` +read `this.config.basePath` directly and added no leading slash, so a `basePath` +configured without one produced a value that is not a URL at all: + + basePath 'api/v1/auth' -> http://localhost:3000api/v1/mcp + +`new URL()` throws on that (`3000api` is not a port), so the RFC 9728 path-inserted +well-known route derived from it throws too, and `@better-auth/oauth-provider` 1.7.2 +refuses to seed the `sys_oauth_resource` row from it at plugin init ("resource +identifier ... must be an absolute URI (RFC 8707 §2)"). With +`enforcePerClientResources` at its `true` default, every MCP client was then refused +for want of a link row. That input class could never mint or match a token, so +repairing it re-selects nothing. + +There is now exactly one read of the configured value and one chain above it: + + configuredBasePath() the configured value VERBATIM — what better-auth is handed + └─ rootedBasePath() + a leading slash when absent (better-auth's own rule) + ├─ getAuthIssuer() = origin + this + └─ getBasePath() = this, trailing slashes stripped + └─ getMcpResourceUrl() = origin + this minus `/auth` + `/mcp` + +`getAuthIssuer()` and `getBasePath()` answer byte-identically to before for every +spelling. Only `getMcpResourceUrl()` moves, and only for a non-canonical `basePath`: +a missing leading slash (was not a URL), repeated trailing slashes, or a configured +`/` (was a `//mcp` path no mount serves). A canonical `basePath` is unchanged on all +three getters. diff --git a/packages/plugins/plugin-auth/src/auth-manager-base-path.test.ts b/packages/plugins/plugin-auth/src/auth-manager-base-path.test.ts index 790cca8edd..3839854f8d 100644 --- a/packages/plugins/plugin-auth/src/auth-manager-base-path.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager-base-path.test.ts @@ -4,11 +4,12 @@ // learn where better-auth serves. // // ⛔ NOT "the one definition" of that value, which an earlier spelling of this -// header claimed. Two more readers of `this.config.basePath` are live in -// `auth-manager.ts` — `getAuthIssuer()` and `getMcpResourceUrl()`, each with its -// own normaliser — and they are deliberately untouched: they are published OAuth -// identifiers, compared by exact string. The accessor's docblock carries the -// measurement and the reason. +// header claimed. #16399 gave the file one, a layer down: `configuredBasePath()` +// is now the ONLY read of `this.config.basePath`, `rootedBasePath()` the only +// place a leading slash is added, and `getBasePath()` the only place a trailing +// one is stripped. `getAuthIssuer()` and `getMcpResourceUrl()` read that chain +// instead of each re-deriving. Their two values still DIFFER on purpose — see +// the #16399 block at the bottom of this file, which pins why. // // ## Why this member is public, and why a rename is a breaking change // @@ -177,3 +178,157 @@ describe('#16025 the ownership walk follows getBasePath(), not the configured sp await expect(ownsGetSession('/api/v1/auth')).resolves.toBe(true); }); }); + +/** + * #16399 — the three derivations are ONE chain, and the MCP resource identifier + * is a URL for every spelling of `basePath`. + * + * ## What was wrong, measured on `origin/main` before this card + * + * basePath 'api/v1/auth' getMcpResourceUrl() -> http://localhost:3000api/v1/mcp + * basePath 'api/v1/auth/' getMcpResourceUrl() -> http://localhost:3000api/v1/mcp + * + * That is not an alternative spelling of the identifier, it is not a URL: + * `new URL()` throws on it (`3000api` is not a port), so `auth-plugin.ts`'s + * `new URL(manager.getMcpResourceUrl()).pathname` — which mounts the RFC 9728 + * §3.1 path-inserted well-known route — throws too, and + * `@better-auth/oauth-provider` 1.7.2 refuses to seed the `sys_oauth_resource` + * row from it at plugin init: + * + * oauth-provider: skipping resource seed for http://localhost:3000api/v1/mcp + * — resource identifier ... must be an absolute URI (RFC 8707 §2) + * + * ⇒ under that configuration no token could ever have been minted OR matched, + * so the repair re-selects nothing. + * + * ## ⛔ Why the ASSERTION is `new URL(...)` and not a string literal + * + * A literal is only as right as whoever typed it: writing + * `toBe('http://localhost:3000api/v1/mcp')` would have pinned the defect. These + * cases assert the PROPERTY that failed — that the value parses as an absolute + * URL, and that its path is the one the mount actually serves — and only then + * compare it with the canonical answer. + */ +describe('#16399 one normalisation chain, and an MCP resource URL that is always a URL', () => { + const withOrigin = (basePath?: string) => + new AuthManager({ + ...(basePath === undefined ? {} : { basePath }), + baseUrl: 'http://localhost:3000', + } as unknown as AuthManagerOptions); + + /** Every spelling of "mount better-auth under /api/v1/auth" a host might write. */ + const EQUIVALENT_SPELLINGS = [ + undefined, // unset -> the shipped default + '', // empty -> treated as unset + '/api/v1/auth', // canonical + 'api/v1/auth', // ⭐ no leading slash — defect 1 + '/api/v1/auth/', // trailing slash + 'api/v1/auth/', // ⭐ both — defect 1 + '/api/v1/auth///', // repeated trailing slashes + ] as const; + + it('⭐ builds a parseable absolute URL for EVERY spelling — the property that failed', () => { + for (const spelling of EQUIVALENT_SPELLINGS) { + const manager = withOrigin(spelling); + // `new URL` throws on a malformed value; letting it throw IS the assertion. + const resource = new URL(manager.getMcpResourceUrl()); + const issuer = new URL(manager.getAuthIssuer()); + expect(resource.protocol).toBe('http:'); + expect(resource.host).toBe('localhost:3000'); + expect(issuer.host).toBe('localhost:3000'); + } + }); + + it('⭐ answers the SAME resource identifier for every spelling of the same mount', () => { + for (const spelling of EQUIVALENT_SPELLINGS) { + expect(withOrigin(spelling).getMcpResourceUrl()).toBe('http://localhost:3000/api/v1/mcp'); + } + }); + + it("the resource path is where the mount actually serves — auth-plugin's `new URL(...).pathname`", () => { + // auth-plugin.ts registers `/.well-known/oauth-protected-resource${mcpPath}` + // off exactly this expression. Under the defect it threw instead. + for (const spelling of EQUIVALENT_SPELLINGS) { + const manager = withOrigin(spelling); + expect(new URL(manager.getMcpResourceUrl()).pathname).toBe('/api/v1/mcp'); + expect(manager.getBasePath()).toBe('/api/v1/auth'); + } + }); + + it('a base path that is not an auth path keeps its whole prefix', () => { + expect(withOrigin('/api/v9/identity').getMcpResourceUrl()).toBe( + 'http://localhost:3000/api/v9/identity/mcp', + ); + expect(withOrigin('api/v9/identity/').getMcpResourceUrl()).toBe( + 'http://localhost:3000/api/v9/identity/mcp', + ); + }); + + it('a configured root yields the bare /mcp resource, not a doubled slash', () => { + // `'/'` normalises to `''` (pinned above), so the resource is `/mcp`. + // Before this card it was `http://localhost:3000//mcp` — parseable, but a + // `//mcp` path that no mount serves. + expect(withOrigin('/').getMcpResourceUrl()).toBe('http://localhost:3000/mcp'); + expect(new URL(withOrigin('/').getMcpResourceUrl()).pathname).toBe('/mcp'); + }); + + /** + * ⭐ NEGATIVE CONTROL — an already-canonical `basePath` must answer byte for + * byte what it answered before this card, on ALL THREE getters. These are the + * values in the card's own "measured, on the real manager" table, row 1. + * If a canonical deployment's `iss` or `aud` moved, this card changed which + * tokens are accepted and the claim's `Clause-②: no` no longer holds. + */ + it('⭐ negative control — a canonical basePath moves NOTHING on all three getters', () => { + const manager = withOrigin('/api/v1/auth'); + expect(manager.getBasePath()).toBe('/api/v1/auth'); + expect(manager.getAuthIssuer()).toBe('http://localhost:3000/api/v1/auth'); + expect(manager.getMcpResourceUrl()).toBe('http://localhost:3000/api/v1/mcp'); + + const dflt = withOrigin(); + expect(dflt.getBasePath()).toBe('/api/v1/auth'); + expect(dflt.getAuthIssuer()).toBe('http://localhost:3000/api/v1/auth'); + expect(dflt.getMcpResourceUrl()).toBe('http://localhost:3000/api/v1/mcp'); + }); + + /** + * ⭐ The pin that stops defect 2 from being "fixed" into existence. + * + * The card and its triage both read PR #16380 as having created a divergence + * — better-auth handed the STRIPPED form while `getAuthIssuer()` broadcast + * the RETAINED one. That is not what landed: #16380's last commit ("hand + * better-auth the configured basePath verbatim again") reverted exactly that, + * because it rejects every token minted under a trailing-slash `basePath`. + * + * So there is nothing to align, and this case says so by measurement rather + * than by prose: it reads better-auth's OWN `ctx.context.baseURL` — the value + * `@better-auth/oauth-provider` 1.7.2 stamps as the access-token `iss` — off + * a real instance built by `createAuthInstance`, and requires + * `getAuthIssuer()` to equal it. Canonicalising `getAuthIssuer()` turns this + * RED, which is the point. + */ + it('⭐ getAuthIssuer() equals the issuer the AS is ACTUALLY configured with', async () => { + const withSecret = (basePath: string) => + new AuthManager({ + basePath, + secret: 'x'.repeat(40), + baseUrl: 'http://localhost:3000', + } as unknown as AuthManagerOptions); + + for (const configured of [ + '/api/v1/auth', + 'api/v1/auth', + '/api/v1/auth/', + 'api/v1/auth/', + '/api/v1/auth///', + '/api/v9/identity/', + ]) { + const manager = withSecret(configured); + const auth = (await manager.getAuthInstance()) as unknown as { + $context: Promise<{ baseURL: string }>; + }; + const stamped = (await auth.$context).baseURL; + expect(manager.getAuthIssuer()).toBe(stamped); + } + }); +}); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index c8ca805238..d791d62b39 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -5599,6 +5599,37 @@ export class AuthManager { return this.config.basePath || '/api/v1/auth'; } + /** + * [#16399] The configured base path with a leading slash GUARANTEED and + * everything else left alone. This is the one place in this file that adds a + * leading slash; every other base-path reader is derived from it. + * + * ## ⛔ This mirrors better-auth's rule — it is not a normalisation of ours + * + * better-auth resolves the string it is handed exactly this way before + * composing `ctx.context.baseURL`, the value `@better-auth/oauth-provider` + * 1.7.2 stamps as the access-token `iss`. So `getAuthIssuer()` is + * `getCanonicalOrigin()` + this, and the pair cannot drift. Measured on a + * real `betterAuth()` built by `createAuthInstance`, reading + * `(await auth.$context).baseURL`: + * + * handed 'api/v1/auth' ctx.baseURL http://localhost:3000/api/v1/auth + * handed '/api/v1/auth' ctx.baseURL http://localhost:3000/api/v1/auth + * handed '/api/v1/auth/' ctx.baseURL http://localhost:3000/api/v1/auth/ + * handed 'api/v1/auth/' ctx.baseURL http://localhost:3000/api/v1/auth/ + * handed '/api/v1/auth///' ctx.baseURL http://localhost:3000/api/v1/auth/// + * + * ⇒ a trailing slash SURVIVES into the issuer, so stripping one here would + * make this manager's own verifier reject every token its AS mints — the + * fail-closed break `configuredBasePath()` above records. ⛔ Never strip + * anything in this method. Stripping belongs one layer down in + * `getBasePath()`, which is a MOUNT path, not a published identifier. + */ + private rootedBasePath(): string { + const configured = this.configuredBasePath(); + return configured.startsWith('/') ? configured : `/${configured}`; + } + /** * [#16025] The path prefix better-auth's routes are reachable under, in the * single NORMALISED spelling an HTTP adapter can mount: a leading slash added @@ -5638,33 +5669,22 @@ export class AuthManager { * `/api/v1/auth/` — measured on the same probe, which drove its whole OAuth * exchange through that mount. * - * **It is NOT the single definition of the base path.** FOUR readers of - * `this.config.basePath` existed in this file; this card leaves THREE, by - * collapsing the string handed to better-auth and `betterAuthEndpointPath`'s - * normalising copy onto `configuredBasePath()`. The two that remain keep - * their own normalisers: - * - * getAuthIssuer() adds a leading slash, KEEPS a trailing one - * getMcpResourceUrl() adds nothing, strips a trailing `/auth` - * - * They are deliberately untouched, and collapsing them is not a free move. - * `getAuthIssuer()` is the `iss` this AS advertises and `getMcpResourceUrl()` - * is the RFC 8707 resource identifier a token's `aud` is matched against — - * both compared by exact string by relying parties, so moving either - * re-selects tokens. Measured on this manager, at this commit: - * - * basePath '/api/v1/auth/' getAuthIssuer() -> …/api/v1/auth/ (trailing slash KEPT — - * and better-auth is handed - * the same spelling, which is - * why the pair still agrees) - * basePath 'api/v1/auth' getMcpResourceUrl() -> http://localhost:3000api/v1/mcp - * (malformed; pre-existing, - * unchanged by this card) - * - * ⇒ ⛔ Do not read this method as licence to assume one answer exists. Two - * more spellings of "the auth base path" are live in this file, and retiring - * them is a decision about published OAuth identifiers, not a tidy-up. Filed - * as #16399 rather than taken on a mount card. + * **It is NOT the single definition of the base path — but there IS one, one + * layer down [#16399].** FOUR readers of `this.config.basePath` existed in + * this file; #16025 left THREE, each with its own normaliser. There is now + * exactly ONE read of `this.config.basePath` in this file + * (`configuredBasePath()`) and one chain above it: + * + * configuredBasePath() the configured value VERBATIM — what better-auth is handed + * └─ rootedBasePath() + a leading slash when absent (better-auth's own rule) + * ├─ getAuthIssuer() = origin + this (published `iss`) + * └─ getBasePath() = this, trailing slashes stripped (mount path) + * └─ getMcpResourceUrl() = origin + this minus `/auth` + `/mcp` + * + * ⇒ a fourth normaliser cannot be added without deleting a link of that + * chain. The two remaining values still DIFFER, and deliberately so: an + * issuer must mirror what better-auth stamps (trailing slash and all), while + * a mount path and the MCP resource URL must be canonical. * * ## ⛔ No value moves — what this card actually changed here * @@ -5688,8 +5708,7 @@ export class AuthManager { * which is the very move measured above to reject live tokens. */ getBasePath(): string { - const configured = this.configuredBasePath(); - return (configured.startsWith('/') ? configured : `/${configured}`).replace(/\/+$/, ''); + return this.rootedBasePath().replace(/\/+$/, ''); } /** @@ -6018,20 +6037,47 @@ export class AuthManager { * The OAuth issuer identifier: better-auth's `baseURL` INCLUDING `basePath` * (e.g. `https://acme.example.com/api/v1/auth`) — this is the `iss` claim * the jwt plugin stamps on access tokens and what the AS metadata reports. + * + * ⛔ [#16399] This value is NOT canonicalised, and must not be: it has to + * equal what better-auth composes from the string `createAuthInstance` hands + * it, byte for byte, because `verifyMcpAccessToken` gives jose this string as + * `issuer` and jose compares `iss` by exact string. `rootedBasePath()` is + * that composition — see its docblock for the measured table, and + * `auth-manager-base-path.test.ts` for the pin against a real `betterAuth()`. + * Stripping a configured trailing slash here rejects every MCP access token + * the deployment mints. */ getAuthIssuer(): string { - const basePath = this.config.basePath || '/api/v1/auth'; - return `${this.getCanonicalOrigin()}${basePath.startsWith('/') ? basePath : `/${basePath}`}`; + return `${this.getCanonicalOrigin()}${this.rootedBasePath()}`; } /** * The MCP resource identifier (RFC 8707 `resource` / token `aud`): * `/mcp`. Derived from the auth basePath so the two can * never disagree about the API prefix. + * + * ## [#16399] Derived from the NORMALISED base path, unlike `getAuthIssuer()` + * + * This one is a location on this host — `auth-plugin.ts` reads a path back + * out of it with `new URL(...).pathname` to mount the RFC 9728 §3.1 + * path-inserted well-known route — so it takes `getBasePath()`, not the + * configured spelling. Before that it read `this.config.basePath` directly + * and added no leading slash, so a `basePath` written without one produced a + * value that is not a URL at all: + * + * basePath 'api/v1/auth' -> http://localhost:3000api/v1/mcp + * + * `new URL()` THROWS on that (`3000api` is not a port), and + * `@better-auth/oauth-provider` 1.7.2 refuses it outright — measured, at + * plugin init: `skipping resource seed for http://localhost:3000api/v1/mcp — + * resource identifier … must be an absolute URI (RFC 8707 §2)`. So the + * `sys_oauth_resource` row is never seeded, `enforcePerClientResources` + * stays at its `true` default, and every MCP client is refused for want of a + * link row. That input class could never mint or match a token, which is why + * repairing it re-selects nothing. */ getMcpResourceUrl(): string { - const basePath = this.config.basePath || '/api/v1/auth'; - const apiPrefix = basePath.replace(/\/auth\/?$/, ''); + const apiPrefix = this.getBasePath().replace(/\/auth$/, ''); return `${this.getCanonicalOrigin()}${apiPrefix}/mcp`; }