Skip to content

Commit 6e0e4e8

Browse files
committed
fix(plugin-auth): collapse the three basePath derivations onto one chain
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
1 parent 7cd5874 commit 6e0e4e8

3 files changed

Lines changed: 272 additions & 38 deletions

File tree

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
---
2+
'@objectstack/plugin-auth': patch
3+
---
4+
5+
fix(plugin-auth): one base-path normalisation chain, and an MCP resource identifier that is always a URL
6+
7+
`AuthManager` derived its base path in three independent places. `getMcpResourceUrl()`
8+
read `this.config.basePath` directly and added no leading slash, so a `basePath`
9+
configured without one produced a value that is not a URL at all:
10+
11+
basePath 'api/v1/auth' -> http://localhost:3000api/v1/mcp
12+
13+
`new URL()` throws on that (`3000api` is not a port), so the RFC 9728 path-inserted
14+
well-known route derived from it throws too, and `@better-auth/oauth-provider` 1.7.2
15+
refuses to seed the `sys_oauth_resource` row from it at plugin init ("resource
16+
identifier ... must be an absolute URI (RFC 8707 §2)"). With
17+
`enforcePerClientResources` at its `true` default, every MCP client was then refused
18+
for want of a link row. That input class could never mint or match a token, so
19+
repairing it re-selects nothing.
20+
21+
There is now exactly one read of the configured value and one chain above it:
22+
23+
configuredBasePath() the configured value VERBATIM — what better-auth is handed
24+
└─ rootedBasePath() + a leading slash when absent (better-auth's own rule)
25+
├─ getAuthIssuer() = origin + this
26+
└─ getBasePath() = this, trailing slashes stripped
27+
└─ getMcpResourceUrl() = origin + this minus `/auth` + `/mcp`
28+
29+
`getAuthIssuer()` and `getBasePath()` answer byte-identically to before for every
30+
spelling. Only `getMcpResourceUrl()` moves, and only for a non-canonical `basePath`:
31+
a missing leading slash (was not a URL), repeated trailing slashes, or a configured
32+
`/` (was a `//mcp` path no mount serves). A canonical `basePath` is unchanged on all
33+
three getters.

packages/plugins/plugin-auth/src/auth-manager-base-path.test.ts

Lines changed: 160 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@
44
// learn where better-auth serves.
55
//
66
// ⛔ NOT "the one definition" of that value, which an earlier spelling of this
7-
// header claimed. Two more readers of `this.config.basePath` are live in
8-
// `auth-manager.ts` — `getAuthIssuer()` and `getMcpResourceUrl()`, each with its
9-
// own normaliser — and they are deliberately untouched: they are published OAuth
10-
// identifiers, compared by exact string. The accessor's docblock carries the
11-
// measurement and the reason.
7+
// header claimed. #16399 gave the file one, a layer down: `configuredBasePath()`
8+
// is now the ONLY read of `this.config.basePath`, `rootedBasePath()` the only
9+
// place a leading slash is added, and `getBasePath()` the only place a trailing
10+
// one is stripped. `getAuthIssuer()` and `getMcpResourceUrl()` read that chain
11+
// instead of each re-deriving. Their two values still DIFFER on purpose — see
12+
// the #16399 block at the bottom of this file, which pins why.
1213
//
1314
// ## Why this member is public, and why a rename is a breaking change
1415
//
@@ -177,3 +178,157 @@ describe('#16025 the ownership walk follows getBasePath(), not the configured sp
177178
await expect(ownsGetSession('/api/v1/auth')).resolves.toBe(true);
178179
});
179180
});
181+
182+
/**
183+
* #16399 — the three derivations are ONE chain, and the MCP resource identifier
184+
* is a URL for every spelling of `basePath`.
185+
*
186+
* ## What was wrong, measured on `origin/main` before this card
187+
*
188+
* basePath 'api/v1/auth' getMcpResourceUrl() -> http://localhost:3000api/v1/mcp
189+
* basePath 'api/v1/auth/' getMcpResourceUrl() -> http://localhost:3000api/v1/mcp
190+
*
191+
* That is not an alternative spelling of the identifier, it is not a URL:
192+
* `new URL()` throws on it (`3000api` is not a port), so `auth-plugin.ts`'s
193+
* `new URL(manager.getMcpResourceUrl()).pathname` — which mounts the RFC 9728
194+
* §3.1 path-inserted well-known route — throws too, and
195+
* `@better-auth/oauth-provider` 1.7.2 refuses to seed the `sys_oauth_resource`
196+
* row from it at plugin init:
197+
*
198+
* oauth-provider: skipping resource seed for http://localhost:3000api/v1/mcp
199+
* — resource identifier ... must be an absolute URI (RFC 8707 §2)
200+
*
201+
* ⇒ under that configuration no token could ever have been minted OR matched,
202+
* so the repair re-selects nothing.
203+
*
204+
* ## ⛔ Why the ASSERTION is `new URL(...)` and not a string literal
205+
*
206+
* A literal is only as right as whoever typed it: writing
207+
* `toBe('http://localhost:3000api/v1/mcp')` would have pinned the defect. These
208+
* cases assert the PROPERTY that failed — that the value parses as an absolute
209+
* URL, and that its path is the one the mount actually serves — and only then
210+
* compare it with the canonical answer.
211+
*/
212+
describe('#16399 one normalisation chain, and an MCP resource URL that is always a URL', () => {
213+
const withOrigin = (basePath?: string) =>
214+
new AuthManager({
215+
...(basePath === undefined ? {} : { basePath }),
216+
baseUrl: 'http://localhost:3000',
217+
} as unknown as AuthManagerOptions);
218+
219+
/** Every spelling of "mount better-auth under /api/v1/auth" a host might write. */
220+
const EQUIVALENT_SPELLINGS = [
221+
undefined, // unset -> the shipped default
222+
'', // empty -> treated as unset
223+
'/api/v1/auth', // canonical
224+
'api/v1/auth', // ⭐ no leading slash — defect 1
225+
'/api/v1/auth/', // trailing slash
226+
'api/v1/auth/', // ⭐ both — defect 1
227+
'/api/v1/auth///', // repeated trailing slashes
228+
] as const;
229+
230+
it('⭐ builds a parseable absolute URL for EVERY spelling — the property that failed', () => {
231+
for (const spelling of EQUIVALENT_SPELLINGS) {
232+
const manager = withOrigin(spelling);
233+
// `new URL` throws on a malformed value; letting it throw IS the assertion.
234+
const resource = new URL(manager.getMcpResourceUrl());
235+
const issuer = new URL(manager.getAuthIssuer());
236+
expect(resource.protocol).toBe('http:');
237+
expect(resource.host).toBe('localhost:3000');
238+
expect(issuer.host).toBe('localhost:3000');
239+
}
240+
});
241+
242+
it('⭐ answers the SAME resource identifier for every spelling of the same mount', () => {
243+
for (const spelling of EQUIVALENT_SPELLINGS) {
244+
expect(withOrigin(spelling).getMcpResourceUrl()).toBe('http://localhost:3000/api/v1/mcp');
245+
}
246+
});
247+
248+
it("the resource path is where the mount actually serves — auth-plugin's `new URL(...).pathname`", () => {
249+
// auth-plugin.ts registers `/.well-known/oauth-protected-resource${mcpPath}`
250+
// off exactly this expression. Under the defect it threw instead.
251+
for (const spelling of EQUIVALENT_SPELLINGS) {
252+
const manager = withOrigin(spelling);
253+
expect(new URL(manager.getMcpResourceUrl()).pathname).toBe('/api/v1/mcp');
254+
expect(manager.getBasePath()).toBe('/api/v1/auth');
255+
}
256+
});
257+
258+
it('a base path that is not an auth path keeps its whole prefix', () => {
259+
expect(withOrigin('/api/v9/identity').getMcpResourceUrl()).toBe(
260+
'http://localhost:3000/api/v9/identity/mcp',
261+
);
262+
expect(withOrigin('api/v9/identity/').getMcpResourceUrl()).toBe(
263+
'http://localhost:3000/api/v9/identity/mcp',
264+
);
265+
});
266+
267+
it('a configured root yields the bare /mcp resource, not a doubled slash', () => {
268+
// `'/'` normalises to `''` (pinned above), so the resource is `/mcp`.
269+
// Before this card it was `http://localhost:3000//mcp` — parseable, but a
270+
// `//mcp` path that no mount serves.
271+
expect(withOrigin('/').getMcpResourceUrl()).toBe('http://localhost:3000/mcp');
272+
expect(new URL(withOrigin('/').getMcpResourceUrl()).pathname).toBe('/mcp');
273+
});
274+
275+
/**
276+
* ⭐ NEGATIVE CONTROL — an already-canonical `basePath` must answer byte for
277+
* byte what it answered before this card, on ALL THREE getters. These are the
278+
* values in the card's own "measured, on the real manager" table, row 1.
279+
* If a canonical deployment's `iss` or `aud` moved, this card changed which
280+
* tokens are accepted and the claim's `Clause-②: no` no longer holds.
281+
*/
282+
it('⭐ negative control — a canonical basePath moves NOTHING on all three getters', () => {
283+
const manager = withOrigin('/api/v1/auth');
284+
expect(manager.getBasePath()).toBe('/api/v1/auth');
285+
expect(manager.getAuthIssuer()).toBe('http://localhost:3000/api/v1/auth');
286+
expect(manager.getMcpResourceUrl()).toBe('http://localhost:3000/api/v1/mcp');
287+
288+
const dflt = withOrigin();
289+
expect(dflt.getBasePath()).toBe('/api/v1/auth');
290+
expect(dflt.getAuthIssuer()).toBe('http://localhost:3000/api/v1/auth');
291+
expect(dflt.getMcpResourceUrl()).toBe('http://localhost:3000/api/v1/mcp');
292+
});
293+
294+
/**
295+
* ⭐ The pin that stops defect 2 from being "fixed" into existence.
296+
*
297+
* The card and its triage both read PR #16380 as having created a divergence
298+
* — better-auth handed the STRIPPED form while `getAuthIssuer()` broadcast
299+
* the RETAINED one. That is not what landed: #16380's last commit ("hand
300+
* better-auth the configured basePath verbatim again") reverted exactly that,
301+
* because it rejects every token minted under a trailing-slash `basePath`.
302+
*
303+
* So there is nothing to align, and this case says so by measurement rather
304+
* than by prose: it reads better-auth's OWN `ctx.context.baseURL` — the value
305+
* `@better-auth/oauth-provider` 1.7.2 stamps as the access-token `iss` — off
306+
* a real instance built by `createAuthInstance`, and requires
307+
* `getAuthIssuer()` to equal it. Canonicalising `getAuthIssuer()` turns this
308+
* RED, which is the point.
309+
*/
310+
it('⭐ getAuthIssuer() equals the issuer the AS is ACTUALLY configured with', async () => {
311+
const withSecret = (basePath: string) =>
312+
new AuthManager({
313+
basePath,
314+
secret: 'x'.repeat(40),
315+
baseUrl: 'http://localhost:3000',
316+
} as unknown as AuthManagerOptions);
317+
318+
for (const configured of [
319+
'/api/v1/auth',
320+
'api/v1/auth',
321+
'/api/v1/auth/',
322+
'api/v1/auth/',
323+
'/api/v1/auth///',
324+
'/api/v9/identity/',
325+
]) {
326+
const manager = withSecret(configured);
327+
const auth = (await manager.getAuthInstance()) as unknown as {
328+
$context: Promise<{ baseURL: string }>;
329+
};
330+
const stamped = (await auth.$context).baseURL;
331+
expect(manager.getAuthIssuer()).toBe(stamped);
332+
}
333+
});
334+
});

packages/plugins/plugin-auth/src/auth-manager.ts

Lines changed: 79 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -5599,6 +5599,37 @@ export class AuthManager {
55995599
return this.config.basePath || '/api/v1/auth';
56005600
}
56015601

5602+
/**
5603+
* [#16399] The configured base path with a leading slash GUARANTEED and
5604+
* everything else left alone. This is the one place in this file that adds a
5605+
* leading slash; every other base-path reader is derived from it.
5606+
*
5607+
* ## ⛔ This mirrors better-auth's rule — it is not a normalisation of ours
5608+
*
5609+
* better-auth resolves the string it is handed exactly this way before
5610+
* composing `ctx.context.baseURL`, the value `@better-auth/oauth-provider`
5611+
* 1.7.2 stamps as the access-token `iss`. So `getAuthIssuer()` is
5612+
* `getCanonicalOrigin()` + this, and the pair cannot drift. Measured on a
5613+
* real `betterAuth()` built by `createAuthInstance`, reading
5614+
* `(await auth.$context).baseURL`:
5615+
*
5616+
* handed 'api/v1/auth' ctx.baseURL http://localhost:3000/api/v1/auth
5617+
* handed '/api/v1/auth' ctx.baseURL http://localhost:3000/api/v1/auth
5618+
* handed '/api/v1/auth/' ctx.baseURL http://localhost:3000/api/v1/auth/
5619+
* handed 'api/v1/auth/' ctx.baseURL http://localhost:3000/api/v1/auth/
5620+
* handed '/api/v1/auth///' ctx.baseURL http://localhost:3000/api/v1/auth///
5621+
*
5622+
* ⇒ a trailing slash SURVIVES into the issuer, so stripping one here would
5623+
* make this manager's own verifier reject every token its AS mints — the
5624+
* fail-closed break `configuredBasePath()` above records. ⛔ Never strip
5625+
* anything in this method. Stripping belongs one layer down in
5626+
* `getBasePath()`, which is a MOUNT path, not a published identifier.
5627+
*/
5628+
private rootedBasePath(): string {
5629+
const configured = this.configuredBasePath();
5630+
return configured.startsWith('/') ? configured : `/${configured}`;
5631+
}
5632+
56025633
/**
56035634
* [#16025] The path prefix better-auth's routes are reachable under, in the
56045635
* single NORMALISED spelling an HTTP adapter can mount: a leading slash added
@@ -5638,33 +5669,22 @@ export class AuthManager {
56385669
* `/api/v1/auth/` — measured on the same probe, which drove its whole OAuth
56395670
* exchange through that mount.
56405671
*
5641-
* **It is NOT the single definition of the base path.** FOUR readers of
5642-
* `this.config.basePath` existed in this file; this card leaves THREE, by
5643-
* collapsing the string handed to better-auth and `betterAuthEndpointPath`'s
5644-
* normalising copy onto `configuredBasePath()`. The two that remain keep
5645-
* their own normalisers:
5646-
*
5647-
* getAuthIssuer() adds a leading slash, KEEPS a trailing one
5648-
* getMcpResourceUrl() adds nothing, strips a trailing `/auth`
5649-
*
5650-
* They are deliberately untouched, and collapsing them is not a free move.
5651-
* `getAuthIssuer()` is the `iss` this AS advertises and `getMcpResourceUrl()`
5652-
* is the RFC 8707 resource identifier a token's `aud` is matched against —
5653-
* both compared by exact string by relying parties, so moving either
5654-
* re-selects tokens. Measured on this manager, at this commit:
5655-
*
5656-
* basePath '/api/v1/auth/' getAuthIssuer() -> …/api/v1/auth/ (trailing slash KEPT —
5657-
* and better-auth is handed
5658-
* the same spelling, which is
5659-
* why the pair still agrees)
5660-
* basePath 'api/v1/auth' getMcpResourceUrl() -> http://localhost:3000api/v1/mcp
5661-
* (malformed; pre-existing,
5662-
* unchanged by this card)
5663-
*
5664-
* ⇒ ⛔ Do not read this method as licence to assume one answer exists. Two
5665-
* more spellings of "the auth base path" are live in this file, and retiring
5666-
* them is a decision about published OAuth identifiers, not a tidy-up. Filed
5667-
* as #16399 rather than taken on a mount card.
5672+
* **It is NOT the single definition of the base path — but there IS one, one
5673+
* layer down [#16399].** FOUR readers of `this.config.basePath` existed in
5674+
* this file; #16025 left THREE, each with its own normaliser. There is now
5675+
* exactly ONE read of `this.config.basePath` in this file
5676+
* (`configuredBasePath()`) and one chain above it:
5677+
*
5678+
* configuredBasePath() the configured value VERBATIM — what better-auth is handed
5679+
* └─ rootedBasePath() + a leading slash when absent (better-auth's own rule)
5680+
* ├─ getAuthIssuer() = origin + this (published `iss`)
5681+
* └─ getBasePath() = this, trailing slashes stripped (mount path)
5682+
* └─ getMcpResourceUrl() = origin + this minus `/auth` + `/mcp`
5683+
*
5684+
* ⇒ a fourth normaliser cannot be added without deleting a link of that
5685+
* chain. The two remaining values still DIFFER, and deliberately so: an
5686+
* issuer must mirror what better-auth stamps (trailing slash and all), while
5687+
* a mount path and the MCP resource URL must be canonical.
56685688
*
56695689
* ## ⛔ No value moves — what this card actually changed here
56705690
*
@@ -5688,8 +5708,7 @@ export class AuthManager {
56885708
* which is the very move measured above to reject live tokens.
56895709
*/
56905710
getBasePath(): string {
5691-
const configured = this.configuredBasePath();
5692-
return (configured.startsWith('/') ? configured : `/${configured}`).replace(/\/+$/, '');
5711+
return this.rootedBasePath().replace(/\/+$/, '');
56935712
}
56945713

56955714
/**
@@ -6018,20 +6037,47 @@ export class AuthManager {
60186037
* The OAuth issuer identifier: better-auth's `baseURL` INCLUDING `basePath`
60196038
* (e.g. `https://acme.example.com/api/v1/auth`) — this is the `iss` claim
60206039
* the jwt plugin stamps on access tokens and what the AS metadata reports.
6040+
*
6041+
* ⛔ [#16399] This value is NOT canonicalised, and must not be: it has to
6042+
* equal what better-auth composes from the string `createAuthInstance` hands
6043+
* it, byte for byte, because `verifyMcpAccessToken` gives jose this string as
6044+
* `issuer` and jose compares `iss` by exact string. `rootedBasePath()` is
6045+
* that composition — see its docblock for the measured table, and
6046+
* `auth-manager-base-path.test.ts` for the pin against a real `betterAuth()`.
6047+
* Stripping a configured trailing slash here rejects every MCP access token
6048+
* the deployment mints.
60216049
*/
60226050
getAuthIssuer(): string {
6023-
const basePath = this.config.basePath || '/api/v1/auth';
6024-
return `${this.getCanonicalOrigin()}${basePath.startsWith('/') ? basePath : `/${basePath}`}`;
6051+
return `${this.getCanonicalOrigin()}${this.rootedBasePath()}`;
60256052
}
60266053

60276054
/**
60286055
* The MCP resource identifier (RFC 8707 `resource` / token `aud`):
60296056
* `<origin><apiPrefix>/mcp`. Derived from the auth basePath so the two can
60306057
* never disagree about the API prefix.
6058+
*
6059+
* ## [#16399] Derived from the NORMALISED base path, unlike `getAuthIssuer()`
6060+
*
6061+
* This one is a location on this host — `auth-plugin.ts` reads a path back
6062+
* out of it with `new URL(...).pathname` to mount the RFC 9728 §3.1
6063+
* path-inserted well-known route — so it takes `getBasePath()`, not the
6064+
* configured spelling. Before that it read `this.config.basePath` directly
6065+
* and added no leading slash, so a `basePath` written without one produced a
6066+
* value that is not a URL at all:
6067+
*
6068+
* basePath 'api/v1/auth' -> http://localhost:3000api/v1/mcp
6069+
*
6070+
* `new URL()` THROWS on that (`3000api` is not a port), and
6071+
* `@better-auth/oauth-provider` 1.7.2 refuses it outright — measured, at
6072+
* plugin init: `skipping resource seed for http://localhost:3000api/v1/mcp —
6073+
* resource identifier … must be an absolute URI (RFC 8707 §2)`. So the
6074+
* `sys_oauth_resource` row is never seeded, `enforcePerClientResources`
6075+
* stays at its `true` default, and every MCP client is refused for want of a
6076+
* link row. That input class could never mint or match a token, which is why
6077+
* repairing it re-selects nothing.
60316078
*/
60326079
getMcpResourceUrl(): string {
6033-
const basePath = this.config.basePath || '/api/v1/auth';
6034-
const apiPrefix = basePath.replace(/\/auth\/?$/, '');
6080+
const apiPrefix = this.getBasePath().replace(/\/auth$/, '');
60356081
return `${this.getCanonicalOrigin()}${apiPrefix}/mcp`;
60366082
}
60376083

0 commit comments

Comments
 (0)