diff --git a/.changeset/25111-sso-domain-verification-instructed-record.md b/.changeset/25111-sso-domain-verification-instructed-record.md new file mode 100644 index 00000000000..10f2927b433 --- /dev/null +++ b/.changeset/25111-sso-domain-verification-instructed-record.md @@ -0,0 +1,14 @@ +--- +'@objectstack/plugin-auth': patch +--- + +fix(plugin-auth): SSO domain verification accepts the DNS TXT record that request-domain-verification tells the operator to publish (#25111) + +Clause-②: no — the verify door now accepts the record the platform already names, as `@better-auth/sso` designed it; the one record it stops accepting is `undefined=TOKEN`, which only the defect produced and nothing instructs. +Metadata-protocol: none + +`POST /api/v1/auth/admin/sso/request-domain-verification` tells the operator to publish a TXT record named `_better-auth-token-PROVIDER.DOMAIN` with the value `_better-auth-token-PROVIDER=TOKEN`. `POST /api/v1/auth/admin/sso/verify-domain` (and the vendor route behind it, `POST /api/v1/auth/sso/verify-domain`) refused exactly that record with `502 DOMAIN_VERIFICATION_FAILED`. The vendor builds the record it accepts from the verification row's `identifier`, and the storage adapter handed it that row without the column (`sys_verification.identifier` is `internal`), so it expected `undefined=TOKEN`. + +- **Now.** The adapter hands better-auth the row's `identifier` the same way it already hands back `value`. The instructed record verifies on both doors and marks the provider verified. +- **Still refused.** A wrong token, the token under another provider's label, the record published under another provider's name, and `undefined=TOKEN`: each answers `502 DOMAIN_VERIFICATION_FAILED` and leaves the provider unverified. +- **Unchanged.** A record holding only the token is still accepted, as the vendor designed it. The generic data API still omits `identifier` from every `sys_verification` row. diff --git a/packages/plugins/plugin-auth/src/internal-field-readback.test.ts b/packages/plugins/plugin-auth/src/internal-field-readback.test.ts index 46cdc362392..1c886867915 100644 --- a/packages/plugins/plugin-auth/src/internal-field-readback.test.ts +++ b/packages/plugins/plugin-auth/src/internal-field-readback.test.ts @@ -184,6 +184,44 @@ describe('[#24061] a column the read was keyed on is answered from the lookup', }); }); +describe('[#25111] sys_verification.identifier — @better-auth/sso verify-domain reads it off the row', () => { + // `findVerificationValue(identifier)` is `findMany(verification, where + // identifier = …)`; the vendor then builds the TXT record it accepts as + // `${row.identifier}=${row.value}`. The real-handler pin is + // `sso-domain-verification-instructed-record.test.ts`; this pins the seam. + const byIdentifier = (value: string) => [ + { field: 'identifier', value, operator: 'eq', connector: 'AND', mode: 'sensitive' }, + ]; + + it('a stripped row (the findMany shape, no lookup) gets BOTH columns back — one batched call per column', async () => { + const resolveInternalField = resolver({ v1: { value: 'TOKEN', identifier: '_better-auth-token-acme' } }); + const rows: any[] = [{ id: 'v1', expires_at: '2099-01-01' }]; + await reattachInternalFieldsOnRead({ resolveInternalField }, 'sys_verification', rows); + expect(rows[0].value).toBe('TOKEN'); + expect(rows[0].identifier).toBe('_better-auth-token-acme'); + expect(resolveInternalField).toHaveBeenCalledTimes(2); + expect(resolveInternalField).toHaveBeenCalledWith('sys_verification', ['v1'], 'identifier'); + }); + + it('a read keyed on identifier answers it from the lookup — only `value` costs a privileged read', async () => { + const resolveInternalField = resolver({ v1: { value: 'TOKEN', identifier: 'never-read' } }); + const row: any = { id: 'v1' }; + await reattachInternalFieldsOnRead( + { resolveInternalField }, 'sys_verification', row, undefined, byIdentifier('_better-auth-token-acme'), + ); + expect(row.identifier).toBe('_better-auth-token-acme'); + expect(row.value).toBe('TOKEN'); + expect(resolveInternalField).toHaveBeenCalledTimes(1); + expect(resolveInternalField).toHaveBeenCalledWith('sys_verification', ['v1'], 'value'); + }); + + it('fails closed on a row the strip emptied of identifier when the engine offers no accessor', async () => { + // `required: true`, so absence can only be the strip — the `value` posture. + const row: any = { id: 'v1', value: 'TOKEN' }; + await expect(reattachInternalFieldsOnRead({}, 'sys_verification', row)).rejects.toThrow(/'identifier'/); + }); +}); + describe('#7987 reattachInternalFieldsOnRead — sys_account OAuth columns', () => { const ACCOUNT_ROW = () => ({ id: 'a1', user_id: 'u1', provider_id: 'google' }); diff --git a/packages/plugins/plugin-auth/src/internal-field-readback.ts b/packages/plugins/plugin-auth/src/internal-field-readback.ts index 65aab815166..e8e98f4e53e 100644 --- a/packages/plugins/plugin-auth/src/internal-field-readback.ts +++ b/packages/plugins/plugin-auth/src/internal-field-readback.ts @@ -195,7 +195,8 @@ const READBACK_FIELDS: Readonly> = { // mirror that honours the same flag — carries it, and each is here because a // better-auth route reads it back OFF a row its storage adapter returned // (traced in the then-installed better-auth / @better-auth/* dist, 1.7.3; - // ⚠️ [#25096] NOT re-stamped for 1.7.12 — see the `identifier` note below): + // ⚠️ [#25096] NOT re-stamped for 1.7.12 as a whole — only the + // `sys_verification` row below was re-traced there, by #25111): // // - `sys_jwks.private_key` — the jwt plugin's key read (`findMany`, no // projection) feeds signing. MEASURED before this row existed: the flag @@ -205,13 +206,24 @@ const READBACK_FIELDS: Readonly> = { // - `sys_verification.value` — reset-password, email/phone OTP, magic link // and OIDC code redemption read it off the found or CONSUMED row (the // adapter's `consumeOne` re-attaches through this table too). - // `required: true`. `identifier` is deliberately absent: no flow reads it - // back off a row — it is only ever the lookup key. - // ⚠️ [#25096] That census is wrong, on 1.7.3 and on 1.7.12 alike: - // `@better-auth/sso`'s `/sso/verify-domain` builds the TXT record it - // expects from `identifier` read back off the row, so on this store it - // expects `undefined=TOKEN`. Measured and reported on #25096; nothing - // here is changed by that card. + // `required: true`. + // - `sys_verification.identifier` [#25111] — `@better-auth/sso`'s + // `/sso/verify-domain` (traced on 1.7.12, the same code on 1.7.3) builds + // the TXT record it accepts as `${activeVerification.identifier}=${value}` + // from the row `findVerificationValue` read back, beside the bare value. + // Stripped, that row made it expect `undefined=TOKEN`: the record + // `request-domain-verification` tells the operator to publish + // (`_better-auth-token-PROVIDER=TOKEN`, `register-sso-provider.ts`) + // answered 502 DOMAIN_VERIFICATION_FAILED, and a record spelling + // `undefined=TOKEN` verified. It is the ONLY reader of the column off a + // row in better-auth, @better-auth/sso, /oauth-provider and /scim 1.7.12; + // every other flow uses it as the lookup key alone. It hands the vendor + // nothing it does not already hold: every verification read better-auth + // issues is keyed on `identifier` (`findVerificationValue`, + // `consumeVerificationValue`), a `findOne` keyed on it is answered from + // the lookup with no privileged read (#24061), and no vendor route echoes + // a verification row to its caller. The generic exits still strip it. + // `required: true` ⇒ absence can only be the strip. // - `sys_two_factor.secret` (`required: true`) and `.backup_codes` (empty // until generated) — TOTP and backup-code verification read both off the // enrolment row. @@ -222,7 +234,10 @@ const READBACK_FIELDS: Readonly> = { // digest off the client row to verify a presented secret; empty on a // public client. [SystemObjectName.JWKS]: [{ field: 'private_key', absenceProvesStrip: true }], - [SystemObjectName.VERIFICATION]: [{ field: 'value', absenceProvesStrip: true }], + [SystemObjectName.VERIFICATION]: [ + { field: 'value', absenceProvesStrip: true }, + { field: 'identifier', absenceProvesStrip: true }, + ], [SystemObjectName.TWO_FACTOR]: [ { field: 'secret', absenceProvesStrip: true }, { field: 'backup_codes', absenceProvesStrip: false }, @@ -252,7 +267,8 @@ const FAIL_CLOSED_CONSEQUENCE: Readonly> = { + '(no set-auth-jwt header, no OIDC id_token)', [SystemObjectName.VERIFICATION]: 'better-auth one-time verification flows (password reset, OTP, magic link, OIDC code redemption) ' - + 'would find no stored value on such rows and refuse every outstanding code', + + 'would find no stored value on such rows and refuse every outstanding code, and SSO domain ' + + 'verification would refuse the DNS record request-domain-verification names', [SystemObjectName.TWO_FACTOR]: 'better-auth two-factor verification would find no TOTP secret on such rows and refuse every code', }; diff --git a/packages/plugins/plugin-auth/src/sso-domain-verification-instructed-record.test.ts b/packages/plugins/plugin-auth/src/sso-domain-verification-instructed-record.test.ts new file mode 100644 index 00000000000..63968b1dfe8 --- /dev/null +++ b/packages/plugins/plugin-auth/src/sso-domain-verification-instructed-record.test.ts @@ -0,0 +1,331 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#25111] SSO domain verification accepts the TXT record the platform tells + * the operator to publish. + * + * `POST /admin/sso/request-domain-verification` answers the record to publish: + * name `_better-auth-token-PROVIDER.DOMAIN`, value + * `_better-auth-token-PROVIDER=TOKEN` (`register-sso-provider.ts`). The vendor + * route `POST /sso/verify-domain` (`@better-auth/sso`) accepts a TXT record at + * that name whose value is EITHER `IDENTIFIER=VALUE` OR the bare `VALUE`, and it + * builds `IDENTIFIER` from the verification row it read back through the + * storage adapter (`activeVerification.identifier`). `sys_verification.identifier` + * is declared `internal`, so the engine strips it from that row, and until the + * read-back seam (`internal-field-readback.ts`) re-attached it the vendor + * compared the instructed record against `undefined=TOKEN`: the instructed + * record answered 502 `DOMAIN_VERIFICATION_FAILED`, and a record literally + * spelling `undefined=TOKEN` verified. + * + * Real `ObjectQL` over `@objectstack/driver-sql` + better-sqlite3 with this + * plugin's own manifest (the strip is live), a real `AuthManager` with the real + * `sso({ domainVerification })` plugin, and the plugin's REAL route + * registration on a real Hono app for the bridge door. Only DNS is a double: + * `node:dns/promises.resolveTxt` answers from a table this file writes. The + * vendor loads that builtin natively (vitest does not transform it), so the + * double is installed on the builtin's exports object and published to ES + * importers with `syncBuiltinESMExports()`, the way + * `__tests__/scrypt-memo.testkit.ts` reaches `node:crypto`. + * + * Pinned, on both doors (the vendor route and the plugin's bridge): + * - the instructed record verifies, and the provider row is marked verified; + * - a wrong token, a wrong provider label in the value, the instructed value + * at another provider's NAME, and the `undefined=TOKEN` record the strip + * made the vendor expect are each refused with `code` + `status`, and leave + * the provider unverified. + * Beside them, the vendor's own second form (the bare token) is pinned as the + * vendor designed it, so a later change to what this door accepts is visible. + */ + +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import dnsPromises from 'node:dns/promises'; +import { syncBuiltinESMExports } from 'node:module'; +import { Hono } from 'hono'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import type { PluginContext } from '@objectstack/core'; +import { AuthManager } from './auth-manager.js'; +import { AuthPlugin } from './auth-plugin.js'; +import { authIdentityObjects } from './manifest.js'; +import { memoizeScrypt } from './__tests__/scrypt-memo.testkit.js'; + +const ORIGIN = 'http://localhost:3000'; +const BASE = '/api/v1/auth'; +const SECRET = 'test-secret-at-least-32-chars-long-25111'; +const PASSWORD = 'S3cure!Passw0rd-25111'; +const DOMAIN = 'acme-25111.example.com'; +const SYS = { context: { isSystem: true } } as const; + +type Row = Record; + +// ── The DNS double ───────────────────────────────────────────────────────── + +/** TXT records by fully-qualified name, as `resolveTxt` returns them (chunks per record). */ +const published = new Map(); +/** Every name the vendor asked for, in order: the positive control that the double is reached. */ +const queried: string[] = []; +const dnsExports = dnsPromises as unknown as { resolveTxt: typeof dnsPromises.resolveTxt }; +const originalResolveTxt = dnsExports.resolveTxt; + +function installDnsDouble(): void { + dnsExports.resolveTxt = (async (name: string) => { + queried.push(name); + const records = published.get(name); + if (records) return records; + throw Object.assign(new Error(`queryTxt ENOTFOUND ${name}`), { code: 'ENOTFOUND', hostname: name }); + }) as typeof dnsPromises.resolveTxt; + syncBuiltinESMExports(); +} + +function restoreDns(): void { + dnsExports.resolveTxt = originalResolveTxt; + syncBuiltinESMExports(); +} + +/** Publish exactly one TXT record at `name`, replacing whatever was there. */ +function publish(name: string, value: string): void { + published.clear(); + published.set(name, [[value]]); +} + +// ── The stack ────────────────────────────────────────────────────────────── + +const mockCtx = (): PluginContext => + ({ + registerService: vi.fn(), + getService: vi.fn((name: string) => (name === 'manifest' ? { register: vi.fn() } : undefined)), + getServices: vi.fn(() => new Map()), + hook: vi.fn(), + trigger: vi.fn(), + logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() }, + getKernel: vi.fn(), + }) as any; + +interface Driver { + find(object: string, query: unknown): Promise; + create(object: string, data: Row): Promise; + update(object: string, id: string, data: Row): Promise; +} + +let engine: ObjectQL; +let driver: Driver; +let manager: AuthManager; +let app: Hono; +let bearer: string; +let adminId: string; +const scrypt = memoizeScrypt(); + +beforeAll(async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + installDnsDouble(); + + engine = new ObjectQL(); + engine.registerDriver( + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }), + true, + ); + await engine.init(); + for (const object of authIdentityObjects) engine.registry.registerObject(object as never, '@objectstack/plugin-auth'); + await engine.syncSchemas(); + driver = (engine as unknown as { getDriver(o: string): Driver }).getDriver('sys_verification'); + + manager = new AuthManager({ + secret: SECRET, + baseUrl: ORIGIN, + dataEngine: engine as never, + plugins: { admin: true, sso: true, ssoDomainVerification: true }, + } as never); + + const email = 'admin.25111@example.com'; + const signUp = await vendor('/sign-up/email', { email, password: PASSWORD, name: 'Platform Admin' }); + expect(signUp.status, `sign-up: ${await signUp.clone().text()}`).toBe(200); + const [admin] = (await driver.find('sys_user', { where: { email } })) as Row[]; + adminId = String(admin!.id); + // The legacy scalar `isPlatformAdminUser` accepts, so the bridge's `gateAdmin` admits. + await driver.update('sys_user', adminId, { role: 'admin' }); + + const signIn = await vendor('/sign-in/email', { email, password: PASSWORD }); + expect(signIn.status, `sign-in: ${await signIn.clone().text()}`).toBe(200); + bearer = String(signIn.headers.get('set-auth-token') ?? ''); + if (!bearer) throw new Error('sign-in emitted no set-auth-token'); + + app = new Hono(); + const ctx = mockCtx(); + const plugin = new AuthPlugin({ secret: SECRET }); + await plugin.init(ctx); + (plugin as any).authManager = manager; + (plugin as any).registerAuthRoutes({ getRawApp: () => app, getPort: () => 0 }, ctx); +}, 120_000); + +afterAll(async () => { + restoreDns(); + scrypt.restore(); + vi.restoreAllMocks(); + try { + await (engine as unknown as { destroy?(): Promise })?.destroy?.(); + } catch { + /* noop */ + } +}); + +/** A request straight at better-auth's handler: the vendor door. */ +function vendor(path: string, body: unknown, withBearer = false): Promise { + return manager.handleRequest( + new Request(`${ORIGIN}${BASE}${path}`, { + method: 'POST', + headers: { + 'content-type': 'application/json', + origin: ORIGIN, + ...(withBearer ? { authorization: `Bearer ${bearer}` } : {}), + }, + body: JSON.stringify(body), + }), + ); +} + +/** A request at the plugin's mounted `/admin/sso/*` route: the bridge door. */ +async function bridge(door: 'request-domain-verification' | 'verify-domain', body: unknown): Promise<{ status: number; json: any }> { + const res = await app.request( + new Request(`${ORIGIN}${BASE}/admin/sso/${door}`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: ORIGIN, authorization: `Bearer ${bearer}` }, + body: JSON.stringify(body), + }), + ); + return { status: res.status, json: await res.json() }; +} + +/** An org-less provider the admin registered (`checkProviderAccess` grants the registrar), unverified. */ +async function seedProvider(providerId: string): Promise { + const now = new Date().toISOString(); + await driver.create('sys_sso_provider', { + id: `ssop_${providerId}`, + provider_id: providerId, + issuer: `https://idp.${DOMAIN}`, + domain: DOMAIN, + user_id: adminId, + organization_id: null, + domain_verified: false, + created_at: now, + updated_at: now, + }); +} + +async function domainVerified(providerId: string): Promise { + const [row] = (await driver.find('sys_sso_provider', { where: { provider_id: providerId } })) as Row[]; + return row?.domain_verified === true || row?.domain_verified === 1; +} + +/** Ask the bridge for the record to publish, the way the admin's action does. */ +async function instructedRecord(providerId: string): Promise<{ name: string; value: string; token: string }> { + const { status, json } = await bridge('request-domain-verification', { providerId, domain: DOMAIN }); + expect(status, JSON.stringify(json)).toBe(200); + expect(json.data.dnsRecordType).toBe('TXT'); + return { name: json.data.dnsRecordName, value: json.data.dnsRecordValue, token: json.data.token }; +} + +/** One verify call on either door, normalised to status + error code. */ +async function verify(door: 'vendor' | 'bridge', providerId: string): Promise<{ status: number; code: string | undefined; body: unknown }> { + if (door === 'vendor') { + const res = await vendor('/sso/verify-domain', { providerId }, true); + const text = await res.text(); + const body = text ? JSON.parse(text) : undefined; + return { status: res.status, code: body?.code, body }; + } + const { status, json } = await bridge('verify-domain', { providerId }); + return { status, code: json?.error?.code, body: json }; +} + +// ── Preconditions ────────────────────────────────────────────────────────── + +describe('[#25111] preconditions — the strip is live and the record names the provider', () => { + it('the generic read omits `identifier` from the verification row; storage holds it', async () => { + const providerId = 'p25111-precondition'; + await seedProvider(providerId); + const record = await instructedRecord(providerId); + const label = `_better-auth-token-${providerId}`; + expect(record.name).toBe(`${label}.${DOMAIN}`); + expect(record.value).toBe(`${label}=${record.token}`); + + const generic = ((await (engine as any).find('sys_verification', { ...SYS })) as Row[]).filter( + (r) => r.id !== undefined, + ); + expect(generic.length).toBeGreaterThan(0); + for (const row of generic) expect(row, 'the generic read omits the column').not.toHaveProperty('identifier'); + const atRest = await driver.find('sys_verification', { where: { identifier: label } }); + expect(atRest, 'storage holds the row keyed by the provider label').toHaveLength(1); + expect(atRest[0]!.value).toBe(record.token); + }); +}); + +// ── The pins, per door ───────────────────────────────────────────────────── + +for (const door of ['vendor', 'bridge'] as const) { + const accepted = door === 'vendor' ? 204 : 200; + + describe(`[#25111] ${door} door — POST ${door === 'vendor' ? '/sso/verify-domain' : '/admin/sso/verify-domain'}`, () => { + const providerId = `p25111-${door}`; + let record: { name: string; value: string; token: string }; + + beforeAll(async () => { + await seedProvider(providerId); + record = await instructedRecord(providerId); + }); + + const refusals: Array<[string, () => [string, string]]> = [ + ['a wrong token under the right label', () => [record.name, `_better-auth-token-${providerId}=${'x'.repeat(record.token.length)}`]], + ['the right token under another provider’s label', () => [record.name, `_better-auth-token-p25111-other=${record.token}`]], + ['the instructed value published at another provider’s name', () => [`_better-auth-token-p25111-other.${DOMAIN}`, record.value]], + ]; + + for (const [label, recordFor] of refusals) { + it(`refuses ${label}: 502 DOMAIN_VERIFICATION_FAILED, provider left unverified`, async () => { + const [name, value] = recordFor(); + publish(name, value); + queried.length = 0; + const res = await verify(door, providerId); + expect(res.status, JSON.stringify(res.body)).toBe(502); + expect(res.code, JSON.stringify(res.body)).toBe('DOMAIN_VERIFICATION_FAILED'); + expect(queried, 'the vendor asked DNS for the instructed name').toEqual([record.name]); + expect(await domainVerified(providerId)).toBe(false); + }); + } + + // Its own provider: on a store that hands the vendor the stripped row this + // record VERIFIES, and a verified provider would turn every later case on + // the shared one into 409 DOMAIN_VERIFIED instead of the answer it pins. + it('refuses `undefined=TOKEN`, the record the stripped row made the vendor expect: 502 DOMAIN_VERIFICATION_FAILED, provider left unverified', async () => { + const own = `${providerId}-undefined`; + await seedProvider(own); + const ownRecord = await instructedRecord(own); + publish(ownRecord.name, `undefined=${ownRecord.token}`); + const res = await verify(door, own); + expect(res.status, JSON.stringify(res.body)).toBe(502); + expect(res.code, JSON.stringify(res.body)).toBe('DOMAIN_VERIFICATION_FAILED'); + expect(await domainVerified(own)).toBe(false); + }); + + it(`accepts the instructed record: ${accepted}, and the provider is marked verified`, async () => { + publish(record.name, record.value); + queried.length = 0; + const res = await verify(door, providerId); + expect(res.status, JSON.stringify(res.body)).toBe(accepted); + if (door === 'bridge') expect((res.body as any).data.verified).toBe(true); + expect(queried).toEqual([record.name]); + expect(await domainVerified(providerId), 'the provider row is verified at rest').toBe(true); + }); + }); +} + +describe('[#25111] the vendor’s second form, unchanged by this card', () => { + it('the bare token at the instructed name verifies (the vendor accepts `VALUE` beside `IDENTIFIER=VALUE`)', async () => { + const providerId = 'p25111-bare'; + await seedProvider(providerId); + const record = await instructedRecord(providerId); + publish(record.name, record.token); + const res = await verify('vendor', providerId); + expect(res.status, JSON.stringify(res.body)).toBe(204); + expect(await domainVerified(providerId)).toBe(true); + }); +});