diff --git a/.changeset/mcp-oauth-resource-registration.md b/.changeset/mcp-oauth-resource-registration.md new file mode 100644 index 0000000000..efccae745b --- /dev/null +++ b/.changeset/mcp-oauth-resource-registration.md @@ -0,0 +1,19 @@ +--- +"@objectstack/plugin-auth": minor +--- + +MCP OAuth can complete again: the MCP resource is registered as an RFC 8707 resource and DCR-registered clients are linked to it, so `authorize?resource=` no longer answers `invalid_target`. + +On 17.3.0 no MCP client could ever obtain a token. `plugin-auth` configured `@better-auth/oauth-provider` with `validAudiences: [authIssuer, mcpResourceUrl]`, an option the pinned 1.7.2 does not read — the string does not occur once in its dist. In 1.7.2 a requested `resource` is resolved from the `oauthResource` table (`sys_oauth_resource`) and `enforcePerClientResources` defaults to `true`, so the client must also be linked in `oauthClientResource` (`sys_oauth_client_resource`). Neither row was ever written, so every client that sends `resource=` — Claude Code does — was refused at `/oauth2/authorize` with `invalid_target: requested resource is not configured`. Discovery, dynamic client registration and the login page all worked; the flow died one step before consent. + +- **`resources: [mcpResourceUrl]`** seeds the `sys_oauth_resource` row from the provider's own `init`. Seeding is idempotent and defaults to `insertOnly`, so an administrator's later edits to the row's token policy are never reverted by a restart. +- **`clientRegistrationDefaultResources: [mcpResourceUrl]`** links each newly registered client to that resource inside the DCR transaction. This is the only place the link can be made: a client registers anonymously about one second before the browser login, leaving no window for an administrator to insert the row by hand. +- **`enforcePerClientResources` is left at its `true` default.** The per-client linkage check stays on — the fix makes the link exist rather than switching the check off. A client with no link row is still refused with `invalid_target`, and a test asserts that. +- **`validAudiences` is removed.** It was passed and read by nobody, which is precisely how the defect survived a version bump: it looked like configuration and enforced nothing. + +Two boot-path defects the resource seed uncovered are fixed in the same change, because seeding is the first thing this package ever wrote from a plugin `init`: + +- **`getAuthInstance()` now settles better-auth's plugin `init` hooks before it resolves.** `betterAuth()` returns synchronously and runs those hooks behind `auth.$context`, so a failure inside one had no catcher and escaped as an unhandled rejection — which Node terminates the process for by default. A boot failure now rejects the call that asked for the instance. +- **The no-`dataEngine` development fallback builds its own in-memory adapter instead of letting better-auth build one.** better-auth 1.7.2 keys that store by the schema *key* while every read resolves by `modelName`, so on that path every model this package renames was unreachable — `user`/`sys_user` as much as `oauthResource`/`sys_oauth_resource` — answering `Model not found`. Production never took this branch (it uses the ObjectQL adapter); development and tests did. + +No configuration change is required. Deployments that already ran 17.3.0 get the resource row on the next boot; MCP clients that failed to connect need to reconnect so a fresh registration picks up the link. diff --git a/packages/plugins/plugin-auth/src/auth-manager.mcp-oauth-resource.test.ts b/packages/plugins/plugin-auth/src/auth-manager.mcp-oauth-resource.test.ts new file mode 100644 index 0000000000..19a2e15f9c --- /dev/null +++ b/packages/plugins/plugin-auth/src/auth-manager.mcp-oauth-resource.test.ts @@ -0,0 +1,523 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * MCP OAuth — RFC 8707 resource registration, verified against the REAL + * `@better-auth/oauth-provider` rather than against the options object we + * hand it. + * + * ## Why this file exists + * + * The predecessor check asserted `opts.validAudiences` on the object + * captured from a mocked `oauthProvider`. The provider never consumed that + * object, so the assertion was green whether or not the installed version + * read the option — and 1.7.2 does not read it at all. An assertion that + * cannot fail is indistinguishable from one that passed. + * + * Every check here is refutable by the one fact the predecessor could not + * see — "the provider does not consume this option": + * + * 1. `option surface liveness` reads the INSTALLED provider's dist and + * refuses any option name AuthManager passes that does not occur in it. + * It carries its own two-way control: a name the provider demonstrably + * reads must be found, and a name nothing could read must not be. + * 2. The end-to-end block boots a real authorization server from the exact + * options AuthManager produces and drives discovery → DCR → + * `authorize?resource=` → consent → token. `invalid_target` + * at the authorize step is the production symptom, so the flow reaching + * a minted token audienced to the MCP resource is the reading. + * 3. `enforcePerClientResources` stays ON and is asserted ON by behaviour: + * a client that is NOT linked to the resource must still be refused. + * Switching the check off to make (2) pass would turn that check red. + * 4. The wrong-resource control asks the SAME server, over the SAME client and + * session, for a resource that was never registered, and requires + * `invalid_target`. Without it a green suite cannot tell "we registered the + * MCP resource" from "resource checking is off": seeding by wildcard, or a + * build that stopped resolving `resource` at all, leaves 1-3 green. It is + * written as a DIFFERENTIAL — accepted resource and refused resource in one + * run, one variable apart — so it reddens from either side: remove the + * registration and the accepted half fails; widen it and the refused half + * does. + */ + +import { createRequire } from 'node:module'; +import path from 'node:path'; +import fs from 'node:fs'; + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +import { AuthManager } from './auth-manager'; +import { buildJwtPluginSchema } from './auth-schema-config.js'; + +// Same pattern as auth-manager.mcp-oauth.test.ts: better-auth is mocked so +// AuthManager's own instance build stays cheap. The authorization server the +// assertions run against is a SEPARATE, REAL one booted below from the +// options captured here. +vi.mock('better-auth', () => ({ + betterAuth: vi.fn(() => ({ handler: vi.fn(), api: {} })), +})); +vi.mock('@better-auth/oauth-provider', () => ({ + oauthProvider: vi.fn((opts: any) => ({ id: 'oauth-provider', _opts: opts })), +})); + +import { oauthProvider } from '@better-auth/oauth-provider'; + +const BASE_URL = 'https://acme.example.com'; +const AUTH_BASE_PATH = '/api/v1/auth'; +const ISSUER = `${BASE_URL}${AUTH_BASE_PATH}`; +const MCP_RESOURCE = `${BASE_URL}/api/v1/mcp`; +// Never registered as an `oauthResource` row, and never advertised by the RFC +// 9728 document. The AS must refuse it for the same client that the MCP +// resource is granted to. +const UNREGISTERED_RESOURCE = `${BASE_URL}/api/v1/other`; +const REDIRECT_URI = 'http://localhost:56789/callback'; +// RFC 7636 Appendix B verifier/challenge pair. +const PKCE_VERIFIER = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; +const PKCE_CHALLENGE = 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM'; + +const ENV_KEYS = ['OS_MCP_SERVER_ENABLED', 'OS_OIDC_PROVIDER_ENABLED', 'OS_OIDC_DCR_ENABLED'] as const; +const savedEnv: Record = {}; + +beforeEach(() => { + vi.clearAllMocks(); + for (const k of ENV_KEYS) { + savedEnv[k] = process.env[k]; + delete process.env[k]; + } +}); +afterEach(() => { + for (const k of ENV_KEYS) { + if (savedEnv[k] === undefined) delete process.env[k]; + else process.env[k] = savedEnv[k]; + } +}); + +/** + * The options AuthManager actually hands `oauthProvider()`, together with the + * RFC 9728 document the SAME manager advertises. Both come from one manager on + * purpose: the defect class here is a drift between the resource identifier a + * client is TOLD to request and the one the AS will accept, and only a reading + * that carries both can see it. + */ +async function captureProviderOptions(): Promise { + return (await captureManagerSurface()).opts; +} + +async function captureManagerSurface(): Promise<{ opts: any; discovery: any }> { + process.env.OS_MCP_SERVER_ENABLED = 'true'; + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + let discovery: any; + try { + const manager = new AuthManager({ + secret: 'test-secret-at-least-32-chars-long', + baseUrl: BASE_URL, + }); + await manager.getAuthInstance(); + discovery = manager.getMcpProtectedResourceMetadata(); + } finally { + warnSpy.mockRestore(); + } + const opts = (oauthProvider as any).mock.calls.at(-1)?.[0]; + expect(opts, 'AuthManager must register the oauthProvider plugin').toBeDefined(); + return { opts, discovery }; +} + +/** + * Every source byte of the INSTALLED `@better-auth/oauth-provider`, resolved + * through Node's own resolver so the reading is about the version this + * checkout actually runs. Throws (fails the test) if it cannot be read — + * "the scanner found nothing" must never be spellable as a pass. + */ +function installedProviderDistText(): string { + const require = createRequire(import.meta.url); + const distDir = path.dirname(require.resolve('@better-auth/oauth-provider')); + const files = fs.readdirSync(distDir).filter((f) => f.endsWith('.mjs') || f.endsWith('.d.mts')); + expect(files.length, `no dist sources found under ${distDir}`).toBeGreaterThan(0); + return files.map((f) => fs.readFileSync(path.join(distDir, f), 'utf8')).join('\n'); +} + +/** + * Boots a REAL better-auth authorization server carrying the REAL + * `@better-auth/oauth-provider` configured with `opts`, backed by the + * in-memory adapter. Returns the instance plus the raw row store, so a + * check can read `oauthResource` / `oauthClientResource` row counts the + * same way the bug report read `sys_oauth_resource` / `sys_oauth_client_resource`. + */ +async function bootRealAuthorizationServer(opts: any) { + const [{ betterAuth }, { memoryAdapter }, { jwt }, { oauthProvider: realOauthProvider }] = await Promise.all([ + vi.importActual('better-auth'), + import('better-auth/adapters/memory'), + import('better-auth/plugins'), + vi.importActual('@better-auth/oauth-provider'), + ]); + + // Constructing the real plugin is itself load-bearing: 1.7.2 throws + // `clientRegistrationDefaultResources resource not found in resources` + // when the two options disagree. + const plugin = realOauthProvider(opts); + + // The memory adapter refuses a model it has no array for, so derive the + // table list from the plugin's own resolved schema rather than a hand-kept + // list that would rot on the next provider bump. + const pluginSchema = (plugin as any).schema as Record; + const db: Record = {}; + for (const m of ['user', 'session', 'account', 'verification', 'jwks']) db[m] = []; + for (const [model, def] of Object.entries(pluginSchema ?? {})) db[def.modelName ?? model] = []; + + // ⛔ `jwt()` must be given the SAME schema the platform gives it. Two + // reasons, and the second is a trap: (1) the harness should carry the + // deployment's real table names; (2) better-auth 1.7.2's `jwt()` MUTATES its + // shared default schema object, so once anything in this process has built + // `jwt({ schema: buildJwtPluginSchema() })` — AuthManager does, above — a + // later bare `jwt()` silently comes back mapped to `sys_jwks` too, and the + // token endpoint 500s on a model this store never created. + const jwtPlugin = jwt({ schema: buildJwtPluginSchema() as any }); + const jwksModel = (jwtPlugin as any).schema?.jwks?.modelName ?? 'jwks'; + db[jwksModel] = db[jwksModel] ?? []; + + const auth = betterAuth({ + baseURL: BASE_URL, + basePath: AUTH_BASE_PATH, + secret: 'test-secret-at-least-32-chars-long', + database: memoryAdapter(db), + emailAndPassword: { enabled: true }, + plugins: [jwtPlugin, plugin as any], + }); + // Forces plugin `init` — which is where the provider seeds `resources`. + await auth.$context; + + // The schema maps these onto ObjectStack's `sys_oauth_*` tables, so resolve + // the store keys through it rather than assuming better-auth's model names. + const resourceModel = pluginSchema?.oauthResource?.modelName ?? 'oauthResource'; + const clientResourceModel = pluginSchema?.oauthClientResource?.modelName ?? 'oauthClientResource'; + const accessTokenModel = pluginSchema?.oauthAccessToken?.modelName ?? 'oauthAccessToken'; + const refreshTokenModel = pluginSchema?.oauthRefreshToken?.modelName ?? 'oauthRefreshToken'; + + return { + auth, + db, + rows: () => ({ + resource: db[resourceModel]?.length ?? 0, + clientResource: db[clientResourceModel]?.length ?? 0, + }), + /** Keyed by the platform table names the bug report counted. */ + tableCounts: () => ({ + [resourceModel]: db[resourceModel]?.length ?? 0, + [clientResourceModel]: db[clientResourceModel]?.length ?? 0, + [accessTokenModel]: db[accessTokenModel]?.length ?? 0, + [refreshTokenModel]: db[refreshTokenModel]?.length ?? 0, + }), + resourceRows: () => db[resourceModel] ?? [], + clientResourceRows: () => db[clientResourceModel] ?? [], + }; +} + +async function registerDcrClient(auth: any) { + const res = await auth.handler( + new Request(`${ISSUER}/oauth2/register`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + client_name: 'Claude Code (test)', + redirect_uris: [REDIRECT_URI], + grant_types: ['authorization_code', 'refresh_token'], + response_types: ['code'], + token_endpoint_auth_method: 'none', + application_type: 'native', + scope: 'openid profile email offline_access data:read', + }), + }), + ); + return { status: res.status, body: (await res.json().catch(() => null)) as any }; +} + +async function signUp(auth: any) { + const res = await auth.handler( + new Request(`${ISSUER}/sign-up/email`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ email: 'dev@acme.example.com', password: 'password-12345', name: 'Dev' }), + }), + ); + const cookie = String(res.headers.get('set-cookie') ?? '') + .split(',') + .map((c: string) => c.split(';')[0]!.trim()) + .join('; '); + return cookie; +} + +async function authorizeWithResource( + auth: any, + clientId: string, + cookie: string, + resource: string = MCP_RESOURCE, +) { + const url = new URL(`${ISSUER}/oauth2/authorize`); + url.searchParams.set('client_id', clientId); + url.searchParams.set('redirect_uri', REDIRECT_URI); + url.searchParams.set('response_type', 'code'); + url.searchParams.set('scope', 'openid profile email offline_access data:read'); + url.searchParams.set('state', 'st'); + url.searchParams.set('code_challenge', PKCE_CHALLENGE); + url.searchParams.set('code_challenge_method', 'S256'); + url.searchParams.set('resource', resource); + const res = await auth.handler(new Request(url.toString(), { method: 'GET', headers: { cookie } })); + return { status: res.status, location: res.headers.get('location') ?? '' }; +} + +/** + * Drives the consent POST the authorize redirect asks for and returns the + * authorization code. Fails loudly rather than returning an empty code, so a + * caller can never mistake "consent broke" for "the grant was refused". + */ +async function consentToCode(auth: any, azLocation: string, cookie: string): Promise { + const consentRes = await auth.handler( + new Request(`${ISSUER}/oauth2/consent`, { + method: 'POST', + headers: { 'content-type': 'application/json', cookie, origin: BASE_URL }, + body: JSON.stringify({ accept: true, oauth_query: azLocation.slice(azLocation.indexOf('?')) }), + }), + ); + const consentBody: any = await consentRes.json().catch(() => null); + const target = consentBody?.redirect_uri ?? consentBody?.url; + expect(target, `consent did not produce a redirect: ${JSON.stringify(consentBody)}`).toBeTruthy(); + const code = new URL(target).searchParams.get('code'); + expect(code, `consent returned no authorization code: ${target}`).toBeTruthy(); + return code!; +} + +function decodeJwtPayload(token: string): any { + const parts = token.split('.'); + expect(parts.length, 'access token must be a signed JWT').toBe(3); + return JSON.parse(Buffer.from(parts[1]!, 'base64url').toString('utf8')); +} + +describe('oauthProvider option surface liveness (installed 1.7.2)', () => { + // Two-way control on the scanner itself: it must be able to answer BOTH + // "present" and "absent", or a 0-hit reading proves nothing. + it('the dist scan fires in both directions', () => { + const dist = installedProviderDistText(); + expect(dist.includes('enforcePerClientResources'), 'positive control: an option the provider reads').toBe(true); + expect(dist.includes('objectstackOptionThatCannotExist'), 'negative control: a name nothing reads').toBe(false); + }); + + it('every option AuthManager passes occurs in the installed provider', async () => { + const opts = await captureProviderOptions(); + const dist = installedProviderDistText(); + const dead = Object.keys(opts).filter((key) => !dist.includes(key)); + expect( + dead, + 'options passed to @better-auth/oauth-provider that the INSTALLED version never reads — ' + + 'a field passed and read by nobody looks like configuration and enforces nothing; ' + + 'delete it or replace it with the option this version actually honours', + ).toEqual([]); + }); +}); + +describe('MCP resource registration against the real provider (RFC 8707)', () => { + it('seeds the MCP resource as an oauthResource row at plugin init', async () => { + const opts = await captureProviderOptions(); + const server = await bootRealAuthorizationServer(opts); + expect(server.rows().resource, 'the MCP resource must exist before any client asks for it').toBe(1); + expect(server.resourceRows()[0]?.identifier).toBe(MCP_RESOURCE); + }); + + it('links a DCR-registered client to the MCP resource without an admin step', async () => { + const opts = await captureProviderOptions(); + const server = await bootRealAuthorizationServer(opts); + expect(server.rows().clientResource).toBe(0); + const reg = await registerDcrClient(server.auth); + expect(reg.status, JSON.stringify(reg.body)).toBe(201); + // A client that registers anonymously one second before the login cannot + // be linked by an admin in between — the link has to happen here. + expect(server.rows().clientResource).toBe(1); + }); + + it('does NOT answer invalid_target for authorize?resource= (the production symptom)', async () => { + const opts = await captureProviderOptions(); + const server = await bootRealAuthorizationServer(opts); + const reg = await registerDcrClient(server.auth); + const cookie = await signUp(server.auth); + const az = await authorizeWithResource(server.auth, reg.body.client_id, cookie); + expect(az.location, 'authorize must not refuse the advertised MCP resource').not.toContain('invalid_target'); + expect(az.location, 'authorize must hand off to the consent page').toContain('/oauth/consent'); + }); + + it('mints a token whose audience is the MCP resource (discovery → DCR → authorize → consent → token)', async () => { + const { opts, discovery } = await captureManagerSurface(); + + // -- discovery leg (RFC 9728) ---------------------------------------- + // The client learns the resource identifier HERE and asks for exactly this + // string below - it is never re-typed from a constant. That is the point: + // an AS that seeds one spelling while advertising another reproduces this + // very defect, and a flow that hard-codes the resource cannot see it. + expect(discovery?.resource, 'discovery must advertise the MCP resource').toBe(MCP_RESOURCE); + expect(discovery?.authorization_servers).toEqual([ISSUER]); + const advertisedResource: string = discovery.resource; + expect( + opts.resources, + 'the AS must be seeded with the SAME identifier discovery advertises', + ).toContain(advertisedResource); + + const server = await bootRealAuthorizationServer(opts); + + expect(server.tableCounts()).toEqual({ + sys_oauth_resource: 1, + sys_oauth_client_resource: 0, + sys_oauth_access_token: 0, + sys_oauth_refresh_token: 0, + }); + + const reg = await registerDcrClient(server.auth); + expect(reg.status, JSON.stringify(reg.body)).toBe(201); + const cookie = await signUp(server.auth); + + const az = await authorizeWithResource(server.auth, reg.body.client_id, cookie, advertisedResource); + expect(az.location).not.toContain('invalid_target'); + + const code = await consentToCode(server.auth, az.location, cookie); + + const tokenRes = await server.auth.handler( + new Request(`${ISSUER}/oauth2/token`, { + method: 'POST', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams({ + grant_type: 'authorization_code', + code, + redirect_uri: REDIRECT_URI, + client_id: reg.body.client_id, + code_verifier: PKCE_VERIFIER, + resource: advertisedResource, + }).toString(), + }), + ); + const tokenBody: any = await tokenRes.json().catch(() => null); + expect(tokenRes.status, JSON.stringify(tokenBody)).toBe(200); + expect(tokenBody.access_token, 'no token was minted').toBeTruthy(); + + const payload = decodeJwtPayload(tokenBody.access_token); + const aud = Array.isArray(payload.aud) ? payload.aud : [payload.aud]; + expect(aud, 'the minted token must be audienced to the MCP resource').toContain(advertisedResource); + expect(payload.iss).toBe(ISSUER); + + // The three tables the bug report counted, plus the refresh row. ⚠️ + // `sys_oauth_access_token` legitimately stays 0: the jwt plugin is on, so + // the access token is a signed JWT and 1.7.2 only persists a row for the + // OPAQUE variant (`createOpaqueAccessToken`). The minted-token evidence is + // the JWT above; the persisted evidence of a completed grant is the + // refresh row, which `offline_access` earns. + expect(server.tableCounts()).toEqual({ + sys_oauth_resource: 1, + sys_oauth_client_resource: 1, + sys_oauth_access_token: 0, + sys_oauth_refresh_token: 1, + }); + }); + + it('keeps the per-client resource check ON — an unlinked client is still refused', async () => { + const opts = await captureProviderOptions(); + // Route (b) — `enforcePerClientResources: false` — would make the checks + // above pass by switching a security check off instead of satisfying it. + // This guard is what tells the two routes apart. + expect(opts.enforcePerClientResources, 'the per-client resource check must not be disabled').not.toBe(false); + + const server = await bootRealAuthorizationServer(opts); + const reg = await registerDcrClient(server.auth); + expect(reg.status, JSON.stringify(reg.body)).toBe(201); + const cookie = await signUp(server.auth); + + // Drop the link the DCR defaults created. Everything else about the client + // is untouched, so the ONLY difference from the passing flow above is the + // per-client authorisation this check is supposed to enforce. + server.clientResourceRows().length = 0; + + const az = await authorizeWithResource(server.auth, reg.body.client_id, cookie); + expect( + az.location, + 'a client with no oauthClientResource row must NOT be able to request the MCP resource — ' + + 'if this passes, enforcePerClientResources has been switched off', + ).toContain('invalid_target'); + }); + + it('still refuses an UNREGISTERED resource at authorize — the fix registers ONE resource, it does not switch resource checking off', async () => { + // ⛔ No assertion here on `opts.resources`. Asserting the option we passed + // is the very shape this file exists to replace — it would stay green under + // a provider that ignored the option. Every reading below is taken from the + // running AS. + const opts = await captureProviderOptions(); + const server = await bootRealAuthorizationServer(opts); + const reg = await registerDcrClient(server.auth); + expect(reg.status, JSON.stringify(reg.body)).toBe(201); + const cookie = await signUp(server.auth); + + // DIFFERENTIAL. Same server, same client, same session, same scopes — the + // ONLY difference between these two requests is the `resource` value, so + // the pair isolates exactly the thing under test. Asserting both halves in + // one run is what makes this control refutable from both sides: drop the + // registration and the granted half fails, widen the registration and the + // refused half does. + const granted = await authorizeWithResource(server.auth, reg.body.client_id, cookie, MCP_RESOURCE); + expect(granted.location, 'the registered MCP resource must still be granted').not.toContain('invalid_target'); + expect(granted.location, 'the registered MCP resource must reach consent').toContain('/oauth/consent'); + + const refused = await authorizeWithResource( + server.auth, + reg.body.client_id, + cookie, + UNREGISTERED_RESOURCE, + ); + expect( + refused.location, + `a resource that was never registered must still be refused, and this client was just ` + + `granted ${MCP_RESOURCE} in the same run — if this passes, the AS is accepting ` + + `resources it was never told about, which is "resource checking is off", not "the MCP ` + + `resource is registered"`, + ).toContain('invalid_target'); + expect(refused.location, 'the refusal must not leak into a consent hand-off').not.toContain('/oauth/consent'); + + // Read from the AS's own store, not from the options: exactly one resource + // was seeded and it is the MCP one. A wildcard or catch-all seed shows up + // here, and would have shown up one assertion earlier as a granted + // redirect for a resource nobody registered. + expect( + server.resourceRows().map((r: any) => r.identifier), + 'the AS must hold exactly the one resource this fix registers', + ).toEqual([MCP_RESOURCE]); + }); + + it('refuses at /oauth2/token a resource that was not bound at authorize', async () => { + const opts = await captureProviderOptions(); + const server = await bootRealAuthorizationServer(opts); + const reg = await registerDcrClient(server.auth); + expect(reg.status, JSON.stringify(reg.body)).toBe(201); + const cookie = await signUp(server.auth); + + // A complete, VALID grant for the MCP resource — so the only defect the + // exchange below can carry is the swapped `resource`. + const az = await authorizeWithResource(server.auth, reg.body.client_id, cookie, MCP_RESOURCE); + expect(az.location).not.toContain('invalid_target'); + const code = await consentToCode(server.auth, az.location, cookie); + + const tokenRes = await server.auth.handler( + new Request(`${ISSUER}/oauth2/token`, { + method: 'POST', + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams({ + grant_type: 'authorization_code', + code, + redirect_uri: REDIRECT_URI, + client_id: reg.body.client_id, + code_verifier: PKCE_VERIFIER, + resource: UNREGISTERED_RESOURCE, + }).toString(), + }), + ); + const tokenBody: any = await tokenRes.json().catch(() => null); + expect( + tokenRes.status, + `redeeming a code bound to ${MCP_RESOURCE} against ${UNREGISTERED_RESOURCE} must fail: ` + + JSON.stringify(tokenBody), + ).not.toBe(200); + expect(tokenBody?.error, JSON.stringify(tokenBody)).toBe('invalid_target'); + expect(tokenBody?.access_token, 'no token may be minted for an unbound resource').toBeFalsy(); + }); +}); diff --git a/packages/plugins/plugin-auth/src/auth-manager.mcp-oauth.test.ts b/packages/plugins/plugin-auth/src/auth-manager.mcp-oauth.test.ts index cee60e36d0..49ac167bb0 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.mcp-oauth.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.mcp-oauth.test.ts @@ -7,9 +7,13 @@ * Token verification tests use REAL jose-signed JWTs against a locally * generated JWKS (mocked `getApi().getJwks`), so the crypto path — signature, * issuer, audience, expiry — is exercised for real, fail-closed on each axis. - * The full discovery → DCR → PKCE browser flow is covered end-to-end against - * a live dev server (see the PR's verification notes); better-auth's own - * endpoint behavior is not re-tested here. + * + * ⚠️ The `oauthProvider plugin wiring` block below reads the options object + * this package passes to a MOCKED `oauthProvider`. That subject can only + * answer "did we pass X", never "does the provider honour X" — so ⛔ never + * assert protocol behaviour here. Anything whose truth depends on what the + * installed provider DOES belongs in auth-manager.mcp-oauth-resource.test.ts, + * which boots the real provider and drives the flow end to end. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; @@ -272,7 +276,7 @@ describe('verifyMcpAccessToken (local JWKS verification, fail-closed)', () => { }); }); -describe('oauthProvider plugin wiring (DCR + scopes + audiences)', () => { +describe('oauthProvider plugin wiring (DCR + scopes — options we pass, not behaviour)', () => { async function capturePluginOpts(env: Record): Promise { for (const [k, v] of Object.entries(env)) process.env[k] = v; (betterAuth as any).mockImplementation((config: any) => ({ handler: vi.fn(), api: {}, _cfg: config })); @@ -297,19 +301,13 @@ describe('oauthProvider plugin wiring (DCR + scopes + audiences)', () => { expect(opts.allowUnauthenticatedClientRegistration).toBe(true); for (const scope of MCP_OAUTH_SCOPES) expect(opts.scopes).toContain(scope); expect(opts.scopes).toEqual(expect.arrayContaining(['openid', 'profile', 'email', 'offline_access'])); - // RFC 8707: the MCP resource must be a valid audience or token minting fails. - expect(opts.validAudiences).toContain('https://acme.example.com/api/v1/mcp'); - expect(opts.validAudiences).toContain('https://acme.example.com/api/v1/auth'); - }); - - it('silences the false-positive oauthAuthServerConfig warning (#3420)', async () => { - // registerOidcDiscoveryRoutes mounts /.well-known/oauth-authorization-server - // (and the /api/v1/auth path-insertion variant) at the issuer ROOT ourselves, - // so better-auth's boot-time "Please ensure … exists" reminder is a false - // positive. It must be silenced via the documented option, or the stock - // showcase prints it (twice) on every `os dev`. Regression guard for the fix. - const opts = await capturePluginOpts({ OS_MCP_SERVER_ENABLED: 'true' }); - expect(opts.silenceWarnings).toEqual({ oauthAuthServerConfig: true }); + // ⛔ RFC 8707 audience binding is NOT asserted here. The subject available + // in this describe block is the options object we passed in, and the + // provider never consumes it — an assertion on it is green whether or not + // the installed version reads the option, which is exactly how a dead + // `validAudiences` survived a version bump. The refutable form lives in + // auth-manager.mcp-oauth-resource.test.ts, which boots the REAL provider + // and drives `authorize?resource=` to a minted token. }); it('OS_OIDC_DCR_ENABLED=false forces DCR off even with MCP on', async () => { diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index 15161954d4..564c3b9213 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -333,7 +333,15 @@ describe('AuthManager', () => { })); }); - it('should return undefined (in-memory fallback) when no dataEngine is provided', async () => { + // ⛔ This used to assert `database === undefined` — "let better-auth build + // its own in-memory store". That store is keyed by the schema KEY while + // every read resolves by `modelName`, so on better-auth 1.7.2 EVERY model + // this package renames (`user`/`sys_user`, `oauthResource`/ + // `sys_oauth_resource`, …) was unreachable on that path with + // "Model not found". The old assertion could not see that: it read + // the value we passed, never what the value does. This one drives the + // factory and asks the adapter for a renamed model. + it('the no-dataEngine fallback yields an adapter that can resolve a RENAMED model', async () => { let capturedConfig: any; (betterAuth as any).mockImplementation((config: any) => { capturedConfig = config; @@ -348,9 +356,17 @@ describe('AuthManager', () => { }); await manager.getAuthInstance(); - - expect(capturedConfig.database).toBeUndefined(); warnSpy.mockRestore(); + + // An AdapterFactory, not `undefined`. + expect(typeof capturedConfig.database).toBe('function'); + + const adapter = capturedConfig.database(capturedConfig); + // `sys_user` is `user` renamed via `modelName`; a store keyed by the + // schema key answers "Model sys_user not found" here instead of `null`. + await expect( + adapter.findOne({ model: 'sys_user', where: [{ field: 'id', value: 'nobody' }] }), + ).resolves.toBeNull(); }); }); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 0bdbb9a540..c8ca805238 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1256,7 +1256,7 @@ export class AuthManager { basePath: this.configuredBasePath(), // Database adapter configuration - database: this.createDatabaseConfig(), + database: await this.createDatabaseConfig(), // Model/field mapping: camelCase (better-auth) → snake_case (ObjectStack) // These declarations tell better-auth the actual table/column names used @@ -2417,7 +2417,27 @@ export class AuthManager { } : {}), }; - return betterAuth(betterAuthConfig); + const auth = betterAuth(betterAuthConfig); + + // ⛔ Do not return before better-auth's plugin `init` hooks have settled. + // + // `betterAuth()` returns synchronously and runs those hooks behind + // `auth.$context`, so anything a plugin does at init is a promise NOBODY + // holds. That was harmless while init did no I/O. It stopped being harmless + // when the oauth-provider began seeding the RFC 8707 `sys_oauth_resource` + // row from its own `init`: a failure there had no catcher and surfaced as + // an UNHANDLED REJECTION — which Node terminates the process for by default + // — and, in tests, as a boot write racing its engine's teardown and failing + // with "No driver available for object 'sys_oauth_resource'" long after the + // test that triggered it had passed. + // + // Awaiting it here makes the seed part of "the instance is ready": a boot + // failure now rejects THIS call, where callers can see and handle it, + // instead of escaping the stack. `$context` is absent when better-auth is + // mocked, and `await undefined` is a no-op, so this is safe on that path. + await (auth as { $context?: Promise } | undefined)?.$context; + + return auth; } /** @@ -3463,23 +3483,17 @@ export class AuthManager { loginPage: this.getConsolePageUrl('/login'), consentPage: this.getConsolePageUrl('/oauth/consent'), schema: buildOauthProviderPluginSchema(), - // better-auth's oauth-provider cannot see the well-known documents we - // mount ourselves at the issuer ROOT (RFC 8414 §3 requires them there, - // not under the auth basePath) — registerOidcDiscoveryRoutes serves - // /.well-known/oauth-authorization-server AND the path-insertion variant - // (`…/api/v1/auth`) the notice names. Its "Please ensure … exists" - // reminder is therefore a false positive on every stock example. - // - // #3420 root cause of the DOUBLE print: the notice fires in the - // oauth-provider plugin's `init(ctx)`, which better-auth runs once per - // `betterAuth()` construction — and the instance is built more than once - // at boot (an initial lazy build, then a rebuild once boot-time auth - // *settings* are applied — applyConfigPatch() nulls the cached instance - // so the next request rebuilds with the new policy). Gating the emitter - // here silences the one requirement we've already satisfied across every - // build path, independent of how many times auth is constructed, so an - // official dev boot stays warning-free. - silenceWarnings: { oauthAuthServerConfig: true }, + // ⛔ No `silenceWarnings` here. It was added for the #3420 double + // print of oauth-provider's "Please ensure /.well-known/… exists" + // notice — a false positive, because registerOidcDiscoveryRoutes + // mounts those documents at the issuer ROOT where RFC 8414 §3 requires + // them. The pinned 1.7.2 emits no such notice: neither the option name + // nor the `oauthAuthServerConfig` key nor the notice text occurs + // anywhere in `@better-auth/oauth-provider` or `better-auth`, so the + // option silenced nothing and was the same dead-option shape as the + // `validAudiences` defect below. If a future bump reintroduces the + // notice, re-add the silencer with a fresh reading — do NOT restore it + // on the strength of this comment. // ── MCP OAuth track (#2698) ──────────────────────────────── // Coarse tool-family scopes for the platform's own MCP endpoint, // advertised alongside the standard OIDC scopes. Names are @@ -3487,10 +3501,36 @@ export class AuthManager { // tool layer cannot drift. scopes: ['openid', 'profile', 'email', 'offline_access', ...MCP_OAUTH_SCOPES], // MCP clients bind tokens to the resource via RFC 8707 - // (`resource=`); the AS only mints audiences it knows. - // The auth base (better-auth's default audience) stays valid for - // plain OIDC SSO flows. - validAudiences: [this.getAuthIssuer(), this.getMcpResourceUrl()], + // (`resource=`). In @better-auth/oauth-provider 1.7.2 a + // requested `resource` is resolved from the `oauthResource` table + // (`sys_oauth_resource`) — a miss is refused at /oauth2/authorize with + // `invalid_target: requested resource is not configured` — and + // `enforcePerClientResources` defaults to TRUE, so the client must + // additionally be linked in `oauthClientResource` + // (`sys_oauth_client_resource`). Both rows have to exist before the + // first Connect, so both are declared here: + // + // • `resources` seeds the sys_oauth_resource row from the plugin's + // own `init` (idempotent, `resourceSeedMode: "insertOnly"` by + // default, so an admin's later CRUD edits are never reverted); + // • `clientRegistrationDefaultResources` links every newly + // registered client to it inside the DCR transaction — a client + // that registers anonymously one second before the login cannot + // be linked by an admin in between. + // + // ⛔ `enforcePerClientResources` is deliberately NOT passed: the + // per-client linkage check stays at its `true` default. The fix makes + // the link happen; it does not switch the check off. A client with no + // link row is still refused, and + // auth-manager.mcp-oauth-resource.test.ts asserts exactly that. + // + // ⛔ Do not reintroduce `validAudiences`: 1.7.2 reads no such option + // (0 occurrences in its dist), and audience validation now runs + // through the resource table instead. A field that is passed and read + // by nobody looks like configuration and enforces nothing — that is + // how this defect survived a version bump. + resources: [this.getMcpResourceUrl()], + clientRegistrationDefaultResources: [this.getMcpResourceUrl()], // RFC 7591 Dynamic Client Registration. `allowUnauthenticated…` is // required: MCP clients register BEFORE any user is logged in (the // whole point of the self-serve flow). Registration is rate-limited @@ -3765,7 +3805,7 @@ export class AuthManager { * silently. We therefore wrap the ObjectQL adapter in a factory function * so it is correctly recognised as a `DBAdapterInstance`. */ - private createDatabaseConfig(): any { + private async createDatabaseConfig(): Promise { // Use ObjectQL adapter factory if dataEngine is provided if (this.config.dataEngine) { // createObjectQLAdapterFactory returns an AdapterFactory @@ -3783,9 +3823,41 @@ export class AuthManager { 'Please provide a dataEngine instance (e.g., ObjectQL) in AuthManagerOptions.' ); - // Return a minimal in-memory configuration as fallback - // This allows the system to work in development/testing without a real database - return undefined; // better-auth will use its default in-memory adapter + // ⛔ NOT `undefined`, and ⛔ do not "simplify" it back to that. + // + // Handing better-auth no `database` makes it build its own in-memory store + // in `getBaseAdapter`, and that store is keyed by the schema KEY while + // every read resolves by `modelName`. Measured on better-auth 1.7.2: + // + // getAuthTables(options) -> { oauthResource: { modelName: 'sys_oauth_resource' }, … } + // its memoryDB -> { oauthResource: [] } // keyed by KEY + // the adapter then asks -> 'sys_oauth_resource' // resolved by modelName + // => Error: Model sys_oauth_resource not found + // + // So on that path EVERY model this package renames is unreachable — + // `user`/`sys_user` included. It stayed invisible for as long as nothing + // touched a renamed model during boot; the RFC 8707 resource seed does, + // from the oauth-provider plugin's `init`, where the throw surfaces as an + // UNHANDLED REJECTION rather than a failed request. + // + // Keying the store by `modelName` is what the adapter actually reads, so + // this fixes the dev/test fallback instead of working around it. Production + // never reaches this branch — it returns the ObjectQL factory above. + // + // The import is dynamic on purpose (the rest of better-auth is loaded the + // same way here); `createAuthInstance` awaits this method, and better-auth + // then calls the returned factory SYNCHRONOUSLY, so the module has to be + // resolved before we hand it over, not inside it. + const [{ memoryAdapter }, { getAuthTables }] = await Promise.all([ + import('better-auth/adapters/memory'), + import('@better-auth/core/db'), + ]); + return (options: any) => { + const tables = getAuthTables(options) as Record; + const db: Record = {}; + for (const [key, table] of Object.entries(tables)) db[table?.modelName ?? key] = []; + return memoryAdapter(db)(options); + }; } /** diff --git a/packages/plugins/plugin-auth/src/membership-policy-setting.test.ts b/packages/plugins/plugin-auth/src/membership-policy-setting.test.ts index d9c490b275..f710ec2096 100644 --- a/packages/plugins/plugin-auth/src/membership-policy-setting.test.ts +++ b/packages/plugins/plugin-auth/src/membership-policy-setting.test.ts @@ -60,8 +60,17 @@ function makeEngine(seed: { users?: Array<{ id: string }>; members?: Array<{ use ]; return { _members: members, - insert: vi.fn(async (_object: string, row: any) => { - members.push({ organization_id: row.organization_id, user_id: row.user_id }); + // ⛔ Table-AWARE on purpose. This double used to push every insert into + // `members` whatever object it named, so any unrelated write — auth boot + // seeds an `sys_oauth_resource` row for the MCP RFC 8707 resource — landed + // in `_members` as `{organization_id: undefined, user_id: undefined}` and + // reddened the membership assertions with a row that is not a membership. + // A double that answers about the wrong table is not a cheaper double, it + // is a wrong one. + insert: vi.fn(async (object: string, row: any) => { + if (object === 'sys_member') { + members.push({ organization_id: row.organization_id, user_id: row.user_id }); + } return row; }), find: vi.fn(async (object: string, query: any) => { @@ -253,7 +262,10 @@ describe('auth.membership_policy — the setting', () => { // And the corroborating evidence: `no-target-org` can only be reached by // ASKING for the target org. Under the policy skip it is never consulted. expect(defaultOrgId).not.toHaveBeenCalled(); - expect(engine.insert).not.toHaveBeenCalled(); + // Scoped to `sys_member`: auth boot legitimately inserts the MCP + // `sys_oauth_resource` seed row, which says nothing about membership. + expect(engine.insert.mock.calls.map((c: any[]) => c[0])).not.toContain('sys_member'); + expect(engine._members).toEqual([]); }); it('the unset default still auto-binds a new sign-up (unchanged behaviour)', async () => { @@ -294,7 +306,9 @@ describe('auth.membership_policy — the setting', () => { reason: 'policy', }); expect(defaultOrgId).not.toHaveBeenCalled(); - expect(engine.insert).not.toHaveBeenCalled(); + // Same scoping as the sign-up leg above, same reason. + expect(engine.insert.mock.calls.map((c: any[]) => c[0])).not.toContain('sys_member'); + expect(engine._members).toEqual([]); }); it('the backfill still binds pre-existing member-less users under the unset default', async () => { diff --git a/packages/plugins/plugin-auth/src/signup-existing-address-refusal.test.ts b/packages/plugins/plugin-auth/src/signup-existing-address-refusal.test.ts index 94b2ab276c..f43c63e78c 100644 --- a/packages/plugins/plugin-auth/src/signup-existing-address-refusal.test.ts +++ b/packages/plugins/plugin-auth/src/signup-existing-address-refusal.test.ts @@ -272,6 +272,16 @@ describe('[#15587] sign-up for an address that already exists is refused, not fa await seedPopulation(engine); const manager = makeManager(engine, EMAIL_DOMAIN_POSTURE); const before = await readAll(engine, 'sys_user'); + // ⛔ Build the auth instance BEFORE arming the recorder. `handleRequest` + // builds it lazily on the first call, and that build is boot work, not + // sign-up work — it seeds the MCP `sys_oauth_resource` row (RFC 8707). + // Recording from before the build would attribute a boot-time write to + // this sign-up; the assertion below is meant to be total, so the window + // has to start where the sign-up does. + // `getAuthInstance()` now settles better-auth's plugin `init` hooks before + // it resolves, so this drains the boot writes (the RFC 8707 + // `sys_oauth_resource` seed among them) outside the window. + await manager.getAuthInstance(); const inserts = instrumentInserts(engine); const res = await signUp(manager, EXISTING);