fix(plugin-auth): collapse the three basePath derivations onto one normalisation chain

.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.

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);
+    }
+  });
+});

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`):
    * `<origin><apiPrefix>/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`;
   }