|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | + |
| 3 | +/** |
| 4 | + * [#17176] One boot builds ONE better-auth instance — and therefore seeds the |
| 5 | + * RFC 8707 `sys_oauth_resource` row once. |
| 6 | + * |
| 7 | + * ## The reported symptom, and what it actually is |
| 8 | + * |
| 9 | + * The card reports `Insert operation failed {object: sys_oauth_resource}` on a |
| 10 | + * boot, and reads it as a bootstrap row being "re-inserted on every boot". |
| 11 | + * Neither half of that survived measurement: |
| 12 | + * |
| 13 | + * - The insert is `@better-auth/oauth-provider`'s, from its own plugin `init`, |
| 14 | + * and 1.7.2 already seeds check-then-insert: `findOne` by `identifier`, then |
| 15 | + * `create` only on a miss, with the UNIQUE refusal caught and treated as a |
| 16 | + * no-op ("one wins, the other catches the constraint error", its own |
| 17 | + * docblock). `seedsOnce` below is the reading: a second init over the same |
| 18 | + * store attempts zero inserts. |
| 19 | + * - What made it collide anyway is on OUR side. `AuthManager.getOrCreateAuth()` |
| 20 | + * assigned `this.auth` only after `createAuthInstance()` resolved, so every |
| 21 | + * caller arriving inside that window started its own build. Overlapping |
| 22 | + * callers exist at boot: `auth-plugin.ts` dispatches |
| 23 | + * `registerOidcDiscoveryRoutes()` with `void` from one `kernel:ready` hook |
| 24 | + * and a later hook reads the instance for the account-issuer backfill. N |
| 25 | + * instances run the vendor `init` N times; on a FRESH database all N miss on |
| 26 | + * `findOne` together and all N insert, and the unique index refuses N - 1. |
| 27 | + * |
| 28 | + * ⇒ First boot of a fresh project only, which is exactly the shape measured |
| 29 | + * downstream (boot 1: one occurrence; boots 2-4: none) and exactly what |
| 30 | + * `packages/objectql/src/engine.ts` records as the cost of moving that log |
| 31 | + * line off `error`. |
| 32 | + * |
| 33 | + * ## Why these checks can fail |
| 34 | + * |
| 35 | + * `singleFlight` counts DISTINCT instance objects, and carries a firing |
| 36 | + * control: two separate managers must yield two distinct objects, so a green |
| 37 | + * "one instance" can never come from an identity comparison that cannot tell |
| 38 | + * objects apart. `seedsOnce` / `concurrentInitsCollide` boot the REAL provider |
| 39 | + * from the REAL options `AuthManager` produces, over one shared store whose |
| 40 | + * `identifier` column refuses a duplicate the way `sys_oauth_resource`'s |
| 41 | + * unique index does — and they are a differential: same store, same options, |
| 42 | + * one variable (whether the two inits overlap). If a provider bump ever made |
| 43 | + * the seed unconditional, `seedsOnce` reddens and this fix is no longer the |
| 44 | + * right one. |
| 45 | + * |
| 46 | + * ⛔ No log level is asserted here and none is changed by the fix: the write |
| 47 | + * doors' `warn` is a separate ruling, and the cure for a duplicated step is |
| 48 | + * not doing it twice. |
| 49 | + */ |
| 50 | + |
| 51 | +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; |
| 52 | + |
| 53 | +import { AuthManager } from './auth-manager'; |
| 54 | +import { buildJwtPluginSchema } from './auth-schema-config.js'; |
| 55 | + |
| 56 | +// better-auth itself stays REAL: the defect is about how many instances |
| 57 | +// `AuthManager` constructs, and a mocked constructor is exactly the thing that |
| 58 | +// cannot tell one from three. Only the oauth provider is stubbed, so the |
| 59 | +// manager's own build stays cheap; the authorization servers the seed checks |
| 60 | +// run against are separate, REAL ones booted from `vi.importActual` below. |
| 61 | +vi.mock('@better-auth/oauth-provider', () => ({ |
| 62 | + oauthProvider: vi.fn((opts: any) => ({ id: 'oauth-provider', _opts: opts })), |
| 63 | +})); |
| 64 | + |
| 65 | +import { oauthProvider } from '@better-auth/oauth-provider'; |
| 66 | + |
| 67 | +const BASE_URL = 'https://acme.example.com'; |
| 68 | +const SECRET = 'test-secret-at-least-32-chars-long'; |
| 69 | +const UNIQUE_REFUSAL = 'UNIQUE constraint failed: sys_oauth_resource.identifier'; |
| 70 | + |
| 71 | +const ENV_KEYS = ['OS_MCP_SERVER_ENABLED', 'OS_OIDC_PROVIDER_ENABLED', 'OS_OIDC_DCR_ENABLED'] as const; |
| 72 | +const savedEnv: Record<string, string | undefined> = {}; |
| 73 | + |
| 74 | +beforeEach(() => { |
| 75 | + vi.clearAllMocks(); |
| 76 | + for (const k of ENV_KEYS) { |
| 77 | + savedEnv[k] = process.env[k]; |
| 78 | + delete process.env[k]; |
| 79 | + } |
| 80 | +}); |
| 81 | +afterEach(() => { |
| 82 | + for (const k of ENV_KEYS) { |
| 83 | + if (savedEnv[k] === undefined) delete process.env[k]; |
| 84 | + else process.env[k] = savedEnv[k]; |
| 85 | + } |
| 86 | +}); |
| 87 | + |
| 88 | +function newManager(): AuthManager { |
| 89 | + return new AuthManager({ secret: SECRET, baseUrl: BASE_URL }); |
| 90 | +} |
| 91 | + |
| 92 | +describe('[#17176] AuthManager.getAuthInstance() is single-flight', () => { |
| 93 | + it('singleFlight: concurrent callers share one instance, separate managers do not', async () => { |
| 94 | + const manager = newManager(); |
| 95 | + const concurrent = await Promise.all([ |
| 96 | + manager.getAuthInstance(), |
| 97 | + manager.getAuthInstance(), |
| 98 | + manager.getAuthInstance(), |
| 99 | + ]); |
| 100 | + |
| 101 | + // The defect: each caller that arrived before `this.auth` was assigned |
| 102 | + // built its own better-auth instance, and every one of them ran the |
| 103 | + // vendor plugin `init` that seeds the resource row. |
| 104 | + expect(new Set(concurrent).size, 'three concurrent callers must share ONE better-auth instance').toBe(1); |
| 105 | + |
| 106 | + // Same manager, no overlap — the already-cached path. |
| 107 | + const cachedFirst = await manager.getAuthInstance(); |
| 108 | + const cachedSecond = await manager.getAuthInstance(); |
| 109 | + expect(new Set([...concurrent, cachedFirst, cachedSecond]).size).toBe(1); |
| 110 | + |
| 111 | + // ⭐ Firing control. Without it, "distinct count is 1" would also be the |
| 112 | + // reading if these objects compared equal for some reason unrelated to |
| 113 | + // the fix. Two managers are two instances, and the same comparison sees it. |
| 114 | + const other = await newManager().getAuthInstance(); |
| 115 | + expect( |
| 116 | + new Set([cachedFirst, other]).size, |
| 117 | + 'control: two separate managers must yield two DISTINCT instances', |
| 118 | + ).toBe(2); |
| 119 | + }); |
| 120 | + |
| 121 | + it('applyConfigPatch discards an in-flight build instead of adopting it', async () => { |
| 122 | + const manager = newManager(); |
| 123 | + const inFlight = manager.getAuthInstance(); |
| 124 | + // Invalidate while the build is still running: the disowned build must not |
| 125 | + // install itself as the manager's instance. |
| 126 | + manager.applyConfigPatch({ baseUrl: 'https://patched.example.com' }); |
| 127 | + const disowned = await inFlight; |
| 128 | + const rebuilt = await manager.getAuthInstance(); |
| 129 | + expect(rebuilt).not.toBe(disowned); |
| 130 | + // ⭐ Control: the rebuilt instance is itself cached, so the assertion above |
| 131 | + // is about invalidation and not about the cache having been switched off. |
| 132 | + expect(await manager.getAuthInstance()).toBe(rebuilt); |
| 133 | + }); |
| 134 | +}); |
| 135 | + |
| 136 | +/** |
| 137 | + * The options `AuthManager` actually hands `oauthProvider()`. Read off the |
| 138 | + * capturing stub rather than hand-written, so the servers below are configured |
| 139 | + * exactly the way a deployment is. |
| 140 | + */ |
| 141 | +async function captureProviderOptions(): Promise<any> { |
| 142 | + process.env.OS_MCP_SERVER_ENABLED = 'true'; |
| 143 | + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); |
| 144 | + try { |
| 145 | + await newManager().getAuthInstance(); |
| 146 | + } finally { |
| 147 | + warnSpy.mockRestore(); |
| 148 | + } |
| 149 | + const opts = (oauthProvider as any).mock.calls.at(-1)?.[0]; |
| 150 | + expect(opts, 'AuthManager must register the oauthProvider plugin').toBeDefined(); |
| 151 | + return opts; |
| 152 | +} |
| 153 | + |
| 154 | +/** |
| 155 | + * One store, shared by every authorization server a run boots — the way one |
| 156 | + * database is shared by every better-auth instance a single boot constructs. |
| 157 | + * `create` on the resource model is counted and refuses a duplicate |
| 158 | + * `identifier`, mirroring `sys_oauth_resource`'s unique index; that refusal is |
| 159 | + * the line the card reported. |
| 160 | + */ |
| 161 | +async function makeSharedStore() { |
| 162 | + const [{ betterAuth }, { memoryAdapter }, { jwt }, { oauthProvider: realOauthProvider }] = await Promise.all([ |
| 163 | + vi.importActual<typeof import('better-auth')>('better-auth'), |
| 164 | + import('better-auth/adapters/memory'), |
| 165 | + import('better-auth/plugins'), |
| 166 | + vi.importActual<typeof import('@better-auth/oauth-provider')>('@better-auth/oauth-provider'), |
| 167 | + ]); |
| 168 | + |
| 169 | + const opts = await captureProviderOptions(); |
| 170 | + const probePlugin = realOauthProvider(opts); |
| 171 | + const pluginSchema = (probePlugin as any).schema as Record<string, { modelName?: string }>; |
| 172 | + const resourceModel = pluginSchema?.oauthResource?.modelName ?? 'oauthResource'; |
| 173 | + |
| 174 | + const db: Record<string, any[]> = {}; |
| 175 | + for (const m of ['user', 'session', 'account', 'verification', 'jwks']) db[m] = []; |
| 176 | + for (const [model, def] of Object.entries(pluginSchema ?? {})) db[def.modelName ?? model] = []; |
| 177 | + |
| 178 | + // ⛔ `jwt()` must be given the SAME schema the platform gives it: 1.7.2's |
| 179 | + // `jwt()` MUTATES its shared default schema object, so a bare `jwt()` after |
| 180 | + // AuthManager has built one silently comes back mapped to `sys_jwks`. |
| 181 | + const jwtPlugin = jwt({ schema: buildJwtPluginSchema() as any }); |
| 182 | + const jwksModel = (jwtPlugin as any).schema?.jwks?.modelName ?? 'jwks'; |
| 183 | + db[jwksModel] = db[jwksModel] ?? []; |
| 184 | + |
| 185 | + const insertAttempts: string[] = []; |
| 186 | + const uniqueRefusals: string[] = []; |
| 187 | + // A unique index refuses at the moment of the insert, not after an async |
| 188 | + // round-trip: the first insert reserves the value and the database |
| 189 | + // serialises the second against it. Reading the row array back instead would |
| 190 | + // let two overlapping inserts both pass — modelling no constraint at all. |
| 191 | + const claimedIdentifiers = new Set<string>(); |
| 192 | + |
| 193 | + const baseAdapter = memoryAdapter(db); |
| 194 | + const countingAdapter = (options: any) => { |
| 195 | + const adapter: any = (baseAdapter as any)(options); |
| 196 | + const create = adapter.create.bind(adapter); |
| 197 | + adapter.create = async (args: any) => { |
| 198 | + if (args?.model === resourceModel) { |
| 199 | + const identifier = args?.data?.identifier; |
| 200 | + insertAttempts.push(identifier); |
| 201 | + if (claimedIdentifiers.has(identifier)) { |
| 202 | + uniqueRefusals.push(identifier); |
| 203 | + throw new Error(UNIQUE_REFUSAL); |
| 204 | + } |
| 205 | + claimedIdentifiers.add(identifier); |
| 206 | + } |
| 207 | + return create(args); |
| 208 | + }; |
| 209 | + return adapter; |
| 210 | + }; |
| 211 | + |
| 212 | + const boot = () => |
| 213 | + betterAuth({ |
| 214 | + baseURL: BASE_URL, |
| 215 | + basePath: '/api/v1/auth', |
| 216 | + secret: SECRET, |
| 217 | + database: countingAdapter as any, |
| 218 | + emailAndPassword: { enabled: true }, |
| 219 | + plugins: [jwtPlugin, realOauthProvider(opts) as any], |
| 220 | + }); |
| 221 | + |
| 222 | + return { |
| 223 | + boot, |
| 224 | + resourceModel, |
| 225 | + insertAttempts, |
| 226 | + uniqueRefusals, |
| 227 | + rowCount: () => db[resourceModel]?.length ?? 0, |
| 228 | + }; |
| 229 | +} |
| 230 | + |
| 231 | +describe('[#17176] @better-auth/oauth-provider 1.7.2 seeds sys_oauth_resource check-then-insert', () => { |
| 232 | + it('seedsOnce: a second init over the same store attempts NO insert', async () => { |
| 233 | + const store = await makeSharedStore(); |
| 234 | + |
| 235 | + await store.boot().$context; // fresh store — the seed inserts |
| 236 | + expect(store.insertAttempts.length, 'the fresh store must be seeded exactly once').toBe(1); |
| 237 | + expect(store.rowCount()).toBe(1); |
| 238 | + |
| 239 | + await store.boot().$context; // warm store — findOne hits, nothing is inserted |
| 240 | + await store.boot().$context; |
| 241 | + |
| 242 | + // ⇒ "re-inserted on every boot" is FALSE of this provider: the second and |
| 243 | + // third inits attempt zero inserts, so the refusal cannot come from a boot |
| 244 | + // that merely repeats an earlier boot's work. |
| 245 | + expect(store.insertAttempts.length, 'warm inits must attempt no further insert').toBe(1); |
| 246 | + expect(store.uniqueRefusals, 'a warm init never reaches the unique index').toEqual([]); |
| 247 | + expect(store.rowCount()).toBe(1); |
| 248 | + }); |
| 249 | + |
| 250 | + it('concurrentInitsCollide: overlapping inits on a FRESH store produce the reported refusal', async () => { |
| 251 | + const store = await makeSharedStore(); |
| 252 | + |
| 253 | + // Two instances built in one process, the way an un-serialised |
| 254 | + // `getOrCreateAuth()` built them. Both `findOne` miss before either |
| 255 | + // `create` lands. |
| 256 | + const [first, second] = [store.boot(), store.boot()]; |
| 257 | + await Promise.all([first.$context, second.$context]); |
| 258 | + |
| 259 | + // The differential against `seedsOnce`: same store, same options, the only |
| 260 | + // variable is whether the inits overlap. |
| 261 | + expect(store.insertAttempts.length, 'both overlapping inits attempt the insert').toBe(2); |
| 262 | + expect(store.uniqueRefusals.length, 'the unique index refuses exactly one of them').toBe(1); |
| 263 | + // The refusal is a no-op for the data: the row is present exactly once, |
| 264 | + // which is why nothing downstream breaks and only the log line shows it. |
| 265 | + expect(store.rowCount()).toBe(1); |
| 266 | + }); |
| 267 | +}); |
0 commit comments