From 1d29c78117d36805157ee391c87bedf518329c25 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 03:36:56 +0000 Subject: [PATCH 1/4] test(plugin-auth): make the MCP OAuth resource check falsifiable The predecessor check asserted `opts.validAudiences` on the options object captured from a mocked `oauthProvider`. The provider never consumes 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. Replace it with checks whose subject is what the REAL provider does: - an option-surface liveness scan over the INSTALLED provider dist, carrying a two-way control so a 0-hit reading is a measurement rather than silence; - an end-to-end block that boots a real authorization server from the exact options AuthManager produces and drives discovery -> DCR -> `authorize?resource=` -> consent -> token; - a guard that the per-client resource check stays ON, so satisfying the flow by switching a security check off turns this red instead. This commit is deliberately red: it is the reproduction. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --- .../auth-manager.mcp-oauth-resource.test.ts | 357 ++++++++++++++++++ .../src/auth-manager.mcp-oauth.test.ts | 22 +- 2 files changed, 372 insertions(+), 7 deletions(-) create mode 100644 packages/plugins/plugin-auth/src/auth-manager.mcp-oauth-resource.test.ts 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..9b71d142bd --- /dev/null +++ b/packages/plugins/plugin-auth/src/auth-manager.mcp-oauth-resource.test.ts @@ -0,0 +1,357 @@ +// 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. + */ + +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'; + +// 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`; +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()`. */ +async function captureProviderOptions(): Promise { + process.env.OS_MCP_SERVER_ENABLED = 'true'; + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const manager = new AuthManager({ + secret: 'test-secret-at-least-32-chars-long', + baseUrl: BASE_URL, + }); + await manager.getAuthInstance(); + } finally { + warnSpy.mockRestore(); + } + const opts = (oauthProvider as any).mock.calls.at(-1)?.[0]; + expect(opts, 'AuthManager must register the oauthProvider plugin').toBeDefined(); + return opts; +} + +/** + * 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] = []; + + 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: [jwt(), plugin as any], + }); + // Forces plugin `init` — which is where the provider seeds `resources`. + await auth.$context; + + const resourceModel = pluginSchema?.oauthResource?.modelName ?? 'oauthResource'; + const clientResourceModel = pluginSchema?.oauthClientResource?.modelName ?? 'oauthClientResource'; + + return { + auth, + db, + rows: () => ({ + resource: db[resourceModel]?.length ?? 0, + clientResource: db[clientResourceModel]?.length ?? 0, + }), + resourceRows: () => db[resourceModel] ?? [], + }; +} + +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 = (res.headers.get('set-cookie') ?? '') + .split(',') + .map((c) => c.split(';')[0]!.trim()) + .join('; '); + return cookie; +} + +async function authorizeWithResource(auth: any, clientId: string, cookie: string) { + 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', MCP_RESOURCE); + const res = await auth.handler(new Request(url.toString(), { method: 'GET', headers: { cookie } })); + return { status: res.status, location: res.headers.get('location') ?? '' }; +} + +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 = 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); + + const az = await authorizeWithResource(server.auth, reg.body.client_id, cookie); + expect(az.location).not.toContain('invalid_target'); + + const consentRes = await server.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: az.location.slice(az.location.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(); + + 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: code!, + redirect_uri: REDIRECT_URI, + client_id: reg.body.client_id, + code_verifier: PKCE_VERIFIER, + resource: MCP_RESOURCE, + }).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(MCP_RESOURCE); + expect(payload.iss).toBe(ISSUER); + }); + + 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 cookie = await signUp(server.auth); + + // A client created WITHOUT the registration defaults gets no link row. + const unlinkedClientId = 'unlinked-test-client'; + server.db.oauthClient!.push({ + id: 'unlinked-row-id', + clientId: unlinkedClientId, + clientSecret: null, + name: 'Unlinked client', + redirectURLs: [REDIRECT_URI], + type: 'public', + applicationType: 'native', + tokenEndpointAuthMethod: 'none', + grantTypes: ['authorization_code'], + responseTypes: ['code'], + scopes: ['openid', 'profile', 'email', 'offline_access', 'data:read'], + clientCredentialsScopes: [], + disabled: false, + skipConsent: false, + requirePkce: true, + createdAt: new Date(), + updatedAt: new Date(), + }); + + const az = await authorizeWithResource(server.auth, unlinkedClientId, 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'); + }); +}); 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..6676ca2c2e 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,9 +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'); + // ⛔ 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('silences the false-positive oauthAuthServerConfig warning (#3420)', async () => { From 8a417f01b2d2af35f3b88e91b3fe8fac0d75f3b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 03:46:40 +0000 Subject: [PATCH 2/4] fix(plugin-auth): register the MCP resource so RFC 8707 authorize can succeed `@better-auth/oauth-provider` 1.7.2 resolves a requested `resource` from the `oauthResource` table and, with `enforcePerClientResources` at its `true` default, requires the client to be linked in `oauthClientResource`. Neither row was ever written, so every MCP client that sends `resource=` was refused at `/oauth2/authorize` with `invalid_target: requested resource is not configured`. No token could be minted on 17.3.0. Route (a): declare the resource rather than relax the check. - `resources: [mcpResourceUrl]` seeds the sys_oauth_resource row from the provider's own `init`, idempotently and `insertOnly`, so an admin's later policy edits survive a restart. - `clientRegistrationDefaultResources: [mcpResourceUrl]` links every newly registered client inside the DCR transaction -- the only place the link can happen, since a client registers anonymously about a second before login. - `enforcePerClientResources` stays at its `true` default. A client with no link row is still refused, and a test asserts that. Two dead options removed. Neither `validAudiences` nor `silenceWarnings` occurs anywhere in the installed `@better-auth/oauth-provider` or `better-auth` (0 hits each, against positive controls that fire), and the `oauthAuthServerConfig` notice `silenceWarnings` claimed to suppress no longer exists in 1.7.2 either. A field that is passed and read by nobody looks like configuration and enforces nothing -- that is how this defect survived a version bump, so the new option-surface liveness check refuses any such field rather than allowlisting these two. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --- .changeset/mcp-oauth-resource-registration.md | 14 ++++ .../auth-manager.mcp-oauth-resource.test.ts | 75 +++++++++++++------ .../src/auth-manager.mcp-oauth.test.ts | 10 --- .../plugins/plugin-auth/src/auth-manager.ts | 62 +++++++++------ 4 files changed, 107 insertions(+), 54 deletions(-) create mode 100644 .changeset/mcp-oauth-resource-registration.md diff --git a/.changeset/mcp-oauth-resource-registration.md b/.changeset/mcp-oauth-resource-registration.md new file mode 100644 index 0000000000..8124369b66 --- /dev/null +++ b/.changeset/mcp-oauth-resource-registration.md @@ -0,0 +1,14 @@ +--- +"@objectstack/plugin-auth": patch +--- + +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. + +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 index 9b71d142bd..c9e5f83dbd 100644 --- 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 @@ -37,6 +37,7 @@ 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 @@ -137,19 +138,34 @@ async function bootRealAuthorizationServer(opts: any) { 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: [jwt(), plugin as any], + 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, @@ -158,7 +174,15 @@ async function bootRealAuthorizationServer(opts: any) { 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] ?? [], }; } @@ -271,6 +295,13 @@ describe('MCP resource registration against the real provider (RFC 8707)', () => const opts = await captureProviderOptions(); 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); @@ -313,6 +344,19 @@ describe('MCP resource registration against the real provider (RFC 8707)', () => const aud = Array.isArray(payload.aud) ? payload.aud : [payload.aud]; expect(aud, 'the minted token must be audienced to the MCP resource').toContain(MCP_RESOURCE); 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 () => { @@ -323,31 +367,16 @@ describe('MCP resource registration against the real provider (RFC 8707)', () => 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); - // A client created WITHOUT the registration defaults gets no link row. - const unlinkedClientId = 'unlinked-test-client'; - server.db.oauthClient!.push({ - id: 'unlinked-row-id', - clientId: unlinkedClientId, - clientSecret: null, - name: 'Unlinked client', - redirectURLs: [REDIRECT_URI], - type: 'public', - applicationType: 'native', - tokenEndpointAuthMethod: 'none', - grantTypes: ['authorization_code'], - responseTypes: ['code'], - scopes: ['openid', 'profile', 'email', 'offline_access', 'data:read'], - clientCredentialsScopes: [], - disabled: false, - skipConsent: false, - requirePkce: true, - createdAt: new Date(), - updatedAt: new Date(), - }); + // 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, unlinkedClientId, cookie); + 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 — ' 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 6676ca2c2e..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 @@ -310,16 +310,6 @@ describe('oauthProvider plugin wiring (DCR + scopes — options we pass, not beh // and drives `authorize?resource=` to a minted token. }); - 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 }); - }); - it('OS_OIDC_DCR_ENABLED=false forces DCR off even with MCP on', async () => { const opts = await capturePluginOpts({ OS_MCP_SERVER_ENABLED: 'true', OS_OIDC_DCR_ENABLED: 'false' }); expect(opts.allowDynamicClientRegistration).toBe(false); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 08634124f5..97aa3c347f 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -3415,23 +3415,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 @@ -3439,10 +3433,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 From f8a61bdd656c3d1684281ca1f13747e15f03baec Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 04:55:32 +0000 Subject: [PATCH 3/4] fix(plugin-auth): settle auth plugin init, and key the dev in-memory fallback by modelName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Registering the MCP resource made the oauth-provider seed a `sys_oauth_resource` row from its plugin `init` — the first write this package ever performs during better-auth construction. Two latent boot-path defects became reachable as soon as it did, and both are fixed here: * `betterAuth()` returns synchronously and runs plugin `init` behind `auth.$context`, so anything a plugin does at init was a promise nobody held. A failure escaped as an UNHANDLED REJECTION (fatal to the process by default) and, in tests, as a boot write racing its engine teardown. `createAuthInstance` now awaits `$context`, making the seed part of "the instance is ready" and a boot failure a rejection of the call that asked for it. * The no-`dataEngine` fallback handed better-auth no `database` at all, which makes it build an in-memory store keyed by the schema KEY while every read resolves by `modelName`. Measured on better-auth 1.7.2: every renamed model — `user`/`sys_user` included, not just the oauth ones — answered "Model not found" on that path. The fallback now builds the store itself, keyed the way the adapter reads it. Production is unaffected: it returns the ObjectQL adapter factory above this branch. The pin that asserted `database === undefined` is replaced rather than edited — it pinned exactly the branch this removes, and it read the value we passed rather than what that value does. Its successor drives the factory and asks the adapter for a renamed model. Two suites had their measurement windows corrected, not their assertions weakened: the sign-up refusal test now drains boot writes before arming its insert recorder, and the membership-policy double stops filing every insert as a membership regardless of which object it named. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --- .changeset/mcp-oauth-resource-registration.md | 5 ++ .../auth-manager.mcp-oauth-resource.test.ts | 51 ++++++++++++--- .../plugin-auth/src/auth-manager.test.ts | 22 ++++++- .../plugins/plugin-auth/src/auth-manager.ts | 64 +++++++++++++++++-- .../src/membership-policy-setting.test.ts | 22 +++++-- .../signup-existing-address-refusal.test.ts | 10 +++ 6 files changed, 151 insertions(+), 23 deletions(-) diff --git a/.changeset/mcp-oauth-resource-registration.md b/.changeset/mcp-oauth-resource-registration.md index 8124369b66..2b7a3e59ab 100644 --- a/.changeset/mcp-oauth-resource-registration.md +++ b/.changeset/mcp-oauth-resource-registration.md @@ -11,4 +11,9 @@ On 17.3.0 no MCP client could ever obtain a token. `plugin-auth` configured `@be - **`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 index c9e5f83dbd..f7bc5d6034 100644 --- 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 @@ -78,22 +78,34 @@ afterEach(() => { } }); -/** The options AuthManager actually hands `oauthProvider()`. */ +/** + * 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; + return { opts, discovery }; } /** @@ -213,14 +225,19 @@ async function signUp(auth: any) { body: JSON.stringify({ email: 'dev@acme.example.com', password: 'password-12345', name: 'Dev' }), }), ); - const cookie = (res.headers.get('set-cookie') ?? '') + const cookie = String(res.headers.get('set-cookie') ?? '') .split(',') - .map((c) => c.split(';')[0]!.trim()) + .map((c: string) => c.split(';')[0]!.trim()) .join('; '); return cookie; } -async function authorizeWithResource(auth: any, clientId: string, cookie: string) { +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); @@ -229,7 +246,7 @@ async function authorizeWithResource(auth: any, clientId: string, cookie: string url.searchParams.set('state', 'st'); url.searchParams.set('code_challenge', PKCE_CHALLENGE); url.searchParams.set('code_challenge_method', 'S256'); - url.searchParams.set('resource', MCP_RESOURCE); + 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') ?? '' }; } @@ -292,7 +309,21 @@ describe('MCP resource registration against the real provider (RFC 8707)', () => }); it('mints a token whose audience is the MCP resource (discovery → DCR → authorize → consent → token)', async () => { - const opts = await captureProviderOptions(); + 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({ @@ -306,7 +337,7 @@ describe('MCP resource registration against the real provider (RFC 8707)', () => 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); + const az = await authorizeWithResource(server.auth, reg.body.client_id, cookie, advertisedResource); expect(az.location).not.toContain('invalid_target'); const consentRes = await server.auth.handler( @@ -332,7 +363,7 @@ describe('MCP resource registration against the real provider (RFC 8707)', () => redirect_uri: REDIRECT_URI, client_id: reg.body.client_id, code_verifier: PKCE_VERIFIER, - resource: MCP_RESOURCE, + resource: advertisedResource, }).toString(), }), ); @@ -342,7 +373,7 @@ describe('MCP resource registration against the real provider (RFC 8707)', () => 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(MCP_RESOURCE); + 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. ⚠️ 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 97aa3c347f..7368f50192 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1252,7 +1252,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 @@ -2413,7 +2413,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; } /** @@ -3737,7 +3757,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 @@ -3755,9 +3775,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); From 11854b605481d0f50ccf49d191c7db9b8e393969 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 12:36:55 +0000 Subject: [PATCH 4/4] test(plugin-auth): add the wrong-resource negative control; grade the changeset minor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Contract review of PR #16780 returned CHANGES REQUIRED on two points. F1 — the changeset level. The PR declares `Clause-②: yes` (the accept set of `/oauth2/authorize` grows) while grading `@objectstack/plugin-auth` `patch`. The maintainer's 2026-09-04 ruling (decision batch #35, the WHICH LEVEL prose in `pr-automation.yml`) settles the order between that and "a bug fix in a released package takes `patch`": a purely additive widening of a published package's public surface — "a new accepted key or value" — takes at least `minor`, and the commit type never lowers the bump below what the act requires. Graded `minor`. F2 — the missing negative control. The body claimed "a request naming any other resource is still refused exactly as before" and nothing tested it: `authorizeWithResource`'s `resource` parameter was never varied. Without that control a green suite cannot tell "the MCP resource is registered" from "resource checking is off" — the same axis as this card's original defect, where the assertion read the options we passed rather than what the provider does. The control is written as a DIFFERENTIAL against the real provider: one run, one booted AS, one DCR client, one session, and two authorize requests that differ only in `resource`. The registered MCP resource must reach consent; an identifier that was never registered must answer `invalid_target`. That shape reddens from both sides — remove the registration and the granted half fails, seed the second resource and link clients to it and the refused half does — where a bare refusal assertion would stay green under either. It asserts nothing about the options object; the resource inventory it checks at the end is read out of the AS's own store. A second control covers the token leg: a code bound to the MCP resource at authorize, redeemed with a `resource` the grant never carried, must be refused `invalid_target` and mint nothing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --- .changeset/mcp-oauth-resource-registration.md | 2 +- .../auth-manager.mcp-oauth-resource.test.ts | 132 ++++++++++++++++-- 2 files changed, 120 insertions(+), 14 deletions(-) diff --git a/.changeset/mcp-oauth-resource-registration.md b/.changeset/mcp-oauth-resource-registration.md index 2b7a3e59ab..efccae745b 100644 --- a/.changeset/mcp-oauth-resource-registration.md +++ b/.changeset/mcp-oauth-resource-registration.md @@ -1,5 +1,5 @@ --- -"@objectstack/plugin-auth": patch +"@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`. 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 index f7bc5d6034..19a2e15f9c 100644 --- 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 @@ -28,6 +28,15 @@ * 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'; @@ -56,6 +65,10 @@ 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'; @@ -251,6 +264,27 @@ async function authorizeWithResource( 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); @@ -340,18 +374,7 @@ describe('MCP resource registration against the real provider (RFC 8707)', () => const az = await authorizeWithResource(server.auth, reg.body.client_id, cookie, advertisedResource); expect(az.location).not.toContain('invalid_target'); - const consentRes = await server.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: az.location.slice(az.location.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(); + const code = await consentToCode(server.auth, az.location, cookie); const tokenRes = await server.auth.handler( new Request(`${ISSUER}/oauth2/token`, { @@ -359,7 +382,7 @@ describe('MCP resource registration against the real provider (RFC 8707)', () => headers: { 'content-type': 'application/x-www-form-urlencoded' }, body: new URLSearchParams({ grant_type: 'authorization_code', - code: code!, + code, redirect_uri: REDIRECT_URI, client_id: reg.body.client_id, code_verifier: PKCE_VERIFIER, @@ -414,4 +437,87 @@ describe('MCP resource registration against the real provider (RFC 8707)', () => + '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(); + }); });