diff --git a/.changeset/sso-oidc-client-secret-at-rest.md b/.changeset/sso-oidc-client-secret-at-rest.md new file mode 100644 index 0000000000..5187b94934 --- /dev/null +++ b/.changeset/sso-oidc-client-secret-at-rest.md @@ -0,0 +1,31 @@ +--- +'@objectstack/plugin-auth': minor +--- + +security: encrypt the OIDC SSO `clientSecret` at rest + +`sys_sso_provider.oidc_config` stored the OIDC `clientSecret` in cleartext inside +its JSON blob — measured on a real registration, byte for byte. That secret +authenticates the platform itself to the identity provider, and the object is +readable through the generic data API (`apiMethods: ['get','list']`), so anyone +who could read the row could impersonate the platform's OIDC client. + +The secret now lives in `sys_sso_provider.oidc_client_secret`, a `Field.secret()` +column on the engine's encrypted credential channel: the engine wraps it with the +registered `ICryptoProvider`, stores the ciphertext as a `sys_secret` row, keeps +only an opaque ref on the provider row, and returns a mask on every generic read. +`oidc_config` keeps the rest of the config in cleartext on purpose, so the admin +UI can still render endpoints, scopes and mapping. + +Both better-auth write doors are covered (`/sso/register` and +`/sso/update-provider`), and the adapter recovers the plaintext server-side for +`/sso/callback`, so federated login is unchanged. + +Existing provider rows are migrated forward automatically at start. A row that +cannot be migrated — no `ICryptoProvider` wired — keeps working and is reported +with a warning rather than silently left looking protected. + +⚠️ Registering or updating an SSO provider now REFUSES rather than storing +cleartext when no `ICryptoProvider` is registered. Self-hosted deployments get +`LocalCryptoProvider` automatically from `serve`; set `OS_SECRET_KEY` (or swap in +a KMS/Vault provider) so secrets survive a restart. diff --git a/packages/plugins/plugin-auth/objectstack.config.ts b/packages/plugins/plugin-auth/objectstack.config.ts index 1af4263a4f..f55903a44d 100644 --- a/packages/plugins/plugin-auth/objectstack.config.ts +++ b/packages/plugins/plugin-auth/objectstack.config.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { defineStack } from '@objectstack/spec'; -import { authIdentityObjects, authPluginManifestHeader } from './src/manifest'; +import { authIdentityObjects, authObjectExtensions, authPluginManifestHeader } from './src/manifest'; /** * ObjectStack Configuration for plugin-auth @@ -14,4 +14,7 @@ import { authIdentityObjects, authPluginManifestHeader } from './src/manifest'; export default defineStack({ manifest: authPluginManifestHeader, objects: authIdentityObjects, + // [#8009] Declared in the same canonical source as `objects`, so the + // compile-time and runtime registration paths cannot drift (D7). + objectExtensions: authObjectExtensions, }); diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 73c0ed3c00..26e0838679 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -50,8 +50,10 @@ import { runResendVerificationEmail } from './send-verification-email.js'; import type { CounterStore } from './rate-limit-storage.js'; import { authIdentityObjects, + authObjectExtensions, authPluginManifestHeader, } from './manifest.js'; +import { scheduleLegacySsoSecretMigration } from './sso-client-secret.js'; /** @@ -512,6 +514,11 @@ export class AuthPlugin implements Plugin { // write-side guardrail that keeps an ungoverned capability grant // unrepresentable. objects: authIdentityObjects, + // [#8009] `sys_sso_provider.oidc_client_secret` — the encrypted home of the + // OIDC client secret that used to sit in cleartext inside `oidc_config`. + // See `manifest.ts` for why the field is declared here and not on the + // object file. + objectExtensions: authObjectExtensions, // ADR-0048 — Setup/Studio/Account apps (and the Setup nav contributions) // moved to their own one-app packages (@objectstack/{setup,studio,account}), // each registering under its own package id so /apps/ resolves @@ -583,6 +590,22 @@ export class AuthPlugin implements Plugin { throw new Error('Auth manager not initialized'); } + // [#8009] Move any provider row still holding a CLEARTEXT OIDC client + // secret in `oidc_config` into the encrypted column. Registered + // unconditionally (not under `registerRoutes`) because the disposition of + // an already-stored secret does not depend on whether this process serves + // the auth routes. The returned unsubscribe is deliberately dropped: the + // engine outlives the plugin and there is no `stop()` to unwind it in. + ctx.hook('kernel:ready', async () => { + let ql: IDataEngine | undefined; + try { ql = ctx.getService('objectql'); } catch { ql = undefined; } + if (!ql) { + try { ql = ctx.getService('data'); } catch { ql = undefined; } + } + if (!ql) return; + scheduleLegacySsoSecretMigration(ql, ctx.logger); + }); + // Setup App translations are now loaded by `PlatformObjectsPlugin` // (in @objectstack/platform-objects). Translation bundles belong with // the package that defines them; auth-plugin no longer piggy-backs on diff --git a/packages/plugins/plugin-auth/src/manifest.ts b/packages/plugins/plugin-auth/src/manifest.ts index 62058ba5f7..77da8033cb 100644 --- a/packages/plugins/plugin-auth/src/manifest.ts +++ b/packages/plugins/plugin-auth/src/manifest.ts @@ -33,6 +33,7 @@ import { SysUserPreference, SysVerification, } from '@objectstack/platform-objects/identity'; +import { Field } from '@objectstack/spec/data'; export const AUTH_PLUGIN_ID = 'com.objectstack.plugin-auth'; export const AUTH_PLUGIN_VERSION = '3.0.1'; @@ -64,6 +65,45 @@ export const authIdentityObjects: any[] = [ SysScimProvider, ]; +/** + * [#8009] Fields plugin-auth adds to identity objects it registers. + * + * `sys_sso_provider.oidc_client_secret` is the encrypted home of the OIDC + * `clientSecret`, which was measured landing BYTE-FOR-BYTE IN CLEARTEXT inside + * the `oidc_config` JSON textarea (#8009 step 0). `type: 'secret'` puts it on + * the engine's encrypted credential channel: the engine wraps the value with the + * registered `ICryptoProvider`, persists the ciphertext as a `sys_secret` row, + * keeps only an opaque `secret:` ref on this column, and returns the mask on + * every generic read. `plugin-auth/src/sso-client-secret.ts` is the seam that + * moves the value in and out; `objectql-adapter.ts` calls it. + * + * ⚠️ Declared HERE rather than on the object itself because the definition file + * (`sys-sso-provider.object.ts`) lives in `packages/platform-objects`, which is + * `domain:metadata`'s package, while this object is registered and owned by + * plugin-auth (`authIdentityObjects` below) and the mechanism is `domain:identity`'s. + * The engine merges an `objectExtensions` field into the resolved schema exactly + * as if it had been declared inline — measured on #8009, including DDL, the + * encrypt-on-write path and the privileged dereference. Consolidating the + * declaration onto the object file is the tidier end state and is worth doing + * the next time that file is opened; it is a move, not a behaviour change. + */ +export const authObjectExtensions = [ + { + extend: 'sys_sso_provider', + fields: { + oidc_client_secret: Field.secret({ + label: 'OIDC Client Secret', + required: false, + description: + 'OAuth client secret issued by the IdP, in the engine\'s encrypted credential channel. ' + + 'Encrypted at rest into sys_secret; reads return a mask, never the secret. Written and ' + + 'read back only by the better-auth adapter seam (register / update-provider / callback).', + group: 'Protocol', + }), + }, + }, +]; + /** Manifest header shared by compile-time config and runtime registration. */ export const authPluginManifestHeader = { id: AUTH_PLUGIN_ID, diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.ts b/packages/plugins/plugin-auth/src/objectql-adapter.ts index 5f8d6f2e84..b866059fd8 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.ts @@ -11,6 +11,11 @@ import { hideRevokedSessionRow, reconcileSessionDelete, } from './session-tombstone.js'; +import { + injectClientSecretOnRead, + liftClientSecretForWrite, + type SecretResolvingEngine, +} from './sso-client-secret.js'; /** * Mapping from better-auth model names to ObjectStack protocol object names. @@ -689,6 +694,12 @@ export const withSystemReadContext = withSystemContext; */ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { const dataEngine = withSystemContext(rawDataEngine); + // [#8009] The OIDC `clientSecret` seam needs the engine's PRIVILEGED secret + // dereference, which `withSystemContext` deliberately does not carry (it + // exposes the CRUD verbs only). `resolveSecretField` is a separately-named + // privileged verb for exactly that reason (#7823), so it comes off the raw + // engine. See `sso-client-secret.ts` for why the seam sits here at all. + const secretEngine = rawDataEngine as unknown as SecretResolvingEngine; // Field-name bridging for better-auth plugins that expose NO `schema` option // (e.g. @better-auth/sso): when a model is remapped via AUTH_MODEL_TO_PROTOCOL, // its camelCase model fields are also converted to snake_case columns on the @@ -724,6 +735,10 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { const bridged = objectName !== model; const payload = normaliseIdentifierWrite(model, data); const row = bridged ? remapKeys(payload, camelToSnake) : payload; + // [#8009] Registration write door #1. Lift the OIDC `clientSecret` out + // of the cleartext JSON blob into the `secret`-typed column so the + // ENGINE encrypts it; `oidc_config` keeps everything else. + liftClientSecretForWrite(objectName, row); // [#7725] `sys_member` declares `{organization_id, user_id}` unique, and // the platform auto-binds every user at sign-up (ADR-0093 D1/D2), so // better-auth's accept-invitation `createMember` collides on a pair that @@ -760,6 +775,10 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { // [#7732] A revoked session is not a session — see `session-tombstone.ts`. if (await hideRevokedSessionRow(objectName, result)) return null; if (revokedAtIsBorrowed) delete (result as Record).revoked_at; + // [#8009] Read half — MANDATORY. `/sso/callback` reads the provider back + // and authenticates to the IdP with the plaintext; encrypt-on-write + // without this breaks every federated login. + await injectClientSecretOnRead(secretEngine, objectName, result); const norm = normaliseLegacyDates(model, result); return (bridged ? remapKeys(norm, snakeToCamel) : norm) as T; }, @@ -786,6 +805,9 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { }); // [#7732] A revoked session is not a session — see `session-tombstone.ts`. const results = await filterRevokedSessionRows(objectName, found); + // [#8009] Same read half, per row — better-auth reaches the provider + // through findMany as well as findOne. + for (const r of results) await injectClientSecretOnRead(secretEngine, objectName, r); return results.map((r) => { const norm = normaliseLegacyDates(model, r as Record); @@ -815,6 +837,10 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { const normalised = normaliseIdentifierWrite(model, update as any); const patch = bridged ? remapKeys(normalised, camelToSnake) : normalised; + // [#8009] Write door #2 — `/sso/update-provider`. A create-only seam + // would encrypt at registration and then write cleartext back on the + // first config edit, leaving a column that only LOOKS protected. + liftClientSecretForWrite(objectName, patch); const result = await dataEngine.update(objectName, { ...patch, id: record.id }); if (!result) return null; const norm = normaliseLegacyDates(model, result); @@ -832,6 +858,9 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { const records = await dataEngine.find(objectName, { where: filter }); const normalised = normaliseIdentifierWrite(model, update); const patch = bridged ? remapKeys(normalised, camelToSnake) : normalised; + // [#8009] Write door #3. Same column, same rule — a bulk edit must not + // be the one path that writes the secret back in cleartext. + liftClientSecretForWrite(objectName, patch); for (const record of records) { await dataEngine.update(objectName, { ...patch, id: record.id }); } diff --git a/packages/plugins/plugin-auth/src/sso-client-secret-at-rest.test.ts b/packages/plugins/plugin-auth/src/sso-client-secret-at-rest.test.ts new file mode 100644 index 0000000000..92fba37429 --- /dev/null +++ b/packages/plugins/plugin-auth/src/sso-client-secret-at-rest.test.ts @@ -0,0 +1,435 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +/** + * [#8009] The OIDC `clientSecret` must not be at rest in cleartext — and the + * federated login it authenticates must still work. + * + * Both halves are asserted, deliberately. A test that only checked "no cleartext + * at rest" would pass on a build where every SSO login is broken, because + * deleting the secret satisfies it perfectly. So each case below pins one of: + * + * ① after register, and after update-provider, the stored row carries NO + * plaintext in ANY column — and the secret column holds a `secret:` ref + * backed by a real `sys_secret` ciphertext row; + * ② better-auth reads the provider back through the adapter and gets the + * CORRECT plaintext, which is what `/sso/callback` hands the IdP. + * + * The write path under test is the real one: a real `betterAuth` with the real + * `sso()` plugin over the real `createObjectQLAdapterFactory` on a real ObjectQL + * engine and a real better-sqlite3 database, with a real signed-up admin and a + * real session cookie. Only the IdP's discovery document is stubbed — that is + * network, not storage. + * + * ⚠️ Registration is driven against `/sso/register` directly rather than through + * the `register_sso_provider` UI action, because that bridge currently 400s on + * an unrelated defect (#8193: it always sends `oidcConfig.mapping.id`, which + * `@better-auth/sso@1.7.0-rc.2` rejects with a `z.strictObject`). Same adapter, + * same write door; #8193 is filed separately and is not this card's scope. + */ +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { Field } from '@objectstack/spec/data'; +import type { ICryptoProvider, CryptoHandle, CryptoContext } from '@objectstack/spec/contracts'; +import { betterAuth } from 'better-auth'; +import { sso } from '@better-auth/sso'; +import { createObjectQLAdapterFactory } from './objectql-adapter.js'; +import { + SSO_CLIENT_SECRET_FIELD, + SSO_PROVIDER_OBJECT, + liftClientSecretForWrite, + migrateLegacySsoClientSecrets, +} from './sso-client-secret.js'; +import { + AUTH_USER_CONFIG, + AUTH_SESSION_CONFIG, + AUTH_ACCOUNT_CONFIG, + AUTH_VERIFICATION_CONFIG, +} from './auth-schema-config.js'; +import { + SysUser, + SysSession, + SysAccount, + SysVerification, + SysSsoProvider, + SysSecret, +} from '@objectstack/platform-objects'; +import { authObjectExtensions } from './manifest.js'; + +/** + * The secret under test. Chosen HERE, by the test, and fed in through the public + * register endpoint — the implementation never supplies this value, so an + * assertion against it cannot be satisfied by the code under test agreeing with + * itself. + */ +const SECRET_UNDER_TEST = 'super-secret-oidc-client-secret-8009-impl'; +const ROTATED_SECRET = 'rotated-oidc-client-secret-8009-impl'; +const BASE = 'http://localhost:3000'; +const IDP = 'https://idp.acme.com'; + +const engines: ObjectQL[] = []; +afterEach(async () => { + while (engines.length) { + const e = engines.pop(); + try { await (e as unknown as { destroy?(): Promise })?.destroy?.(); } catch { /* noop */ } + } +}); + +/** + * A stand-in `ICryptoProvider`. The host injects the real one (`serve.ts` wires + * `LocalCryptoProvider`, AES-256-GCM off `OS_SECRET_KEY`); objectql takes no + * dependency on an implementation, so neither does this test. Base64 is enough + * to tell ciphertext from plaintext, which is the only property asserted. + */ +function makeFakeCrypto(): ICryptoProvider { + let n = 0; + return { + async encrypt(plain: string, _ctx: CryptoContext): Promise { + n += 1; + return { + id: `sec_${n}`, kmsKeyId: 'local', alg: 'test-b64', version: 1, + ciphertext: Buffer.from(plain, 'utf8').toString('base64'), + }; + }, + async decrypt(handle: CryptoHandle): Promise { + return Buffer.from(handle.ciphertext, 'base64').toString('utf8'); + }, + async rotateKey(handle: CryptoHandle): Promise { + return { ...handle, version: handle.version + 1 }; + }, + digest(plain: string): string { return `d:${plain.length}`; }, + }; +} + +/** The `oidc_client_secret` column, exactly as the plugin manifest declares it. */ +function ssoProviderWithSecretColumn(): unknown { + const ext = authObjectExtensions.find((e) => e.extend === SSO_PROVIDER_OBJECT); + if (!ext) throw new Error('plugin-auth no longer declares the sys_sso_provider extension'); + return { ...(SysSsoProvider as object), fields: { ...(SysSsoProvider as { fields: object }).fields, ...ext.fields } }; +} + +async function bootEngine(opts: { withCrypto?: boolean; withSecretColumn?: boolean } = {}) { + const engine = new ObjectQL(); + engines.push(engine); + engine.registerDriver( + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }), + true, + ); + await engine.init(); + for (const o of [SysUser, SysSession, SysAccount, SysVerification, SysSecret]) { + engine.registry.registerObject(o as never); + } + engine.registry.registerObject( + (opts.withSecretColumn === false ? SysSsoProvider : ssoProviderWithSecretColumn()) as never, + ); + await engine.syncSchemas(); + if (opts.withCrypto !== false) engine.setCryptoProvider(makeFakeCrypto()); + return engine; +} + +/** Discovery document the register endpoint hydrates from (network, not storage). */ +function stubDiscovery() { + const realFetch = globalThis.fetch; + globalThis.fetch = (async (input: unknown, init?: unknown) => { + const url = typeof input === 'string' ? input : String((input as { url?: string })?.url ?? input); + if (url.includes('.well-known/openid-configuration')) { + return new Response(JSON.stringify({ + issuer: IDP, + authorization_endpoint: `${IDP}/authorize`, + token_endpoint: `${IDP}/token`, + userinfo_endpoint: `${IDP}/userinfo`, + jwks_uri: `${IDP}/jwks`, + response_types_supported: ['code'], + subject_types_supported: ['public'], + id_token_signing_alg_values_supported: ['RS256'], + }), { status: 200, headers: { 'content-type': 'application/json' } }); + } + return (realFetch as (i: unknown, x?: unknown) => Promise)(input, init); + }) as typeof globalThis.fetch; + return () => { globalThis.fetch = realFetch; }; +} + +function makeAuth(engine: ObjectQL) { + return betterAuth({ + secret: 'test-secret-at-least-32-chars-long-xxx', + baseURL: BASE, + basePath: '/api/v1/auth', + trustedOrigins: [BASE, IDP], + emailAndPassword: { enabled: true }, + user: { ...AUTH_USER_CONFIG }, + session: { ...AUTH_SESSION_CONFIG }, + account: { ...AUTH_ACCOUNT_CONFIG }, + verification: { ...AUTH_VERIFICATION_CONFIG }, + database: createObjectQLAdapterFactory(engine as never), + plugins: [sso({ organizationProvisioning: { defaultRole: 'member' } })], + }); +} + +async function signUpAdmin(auth: ReturnType): Promise { + const res = await auth.handler(new Request(`${BASE}/api/v1/auth/sign-up/email`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: BASE }, + body: JSON.stringify({ email: 'admin@acme.com', password: 'Password123!', name: 'Admin' }), + })); + expect(res.status).toBeLessThan(400); + return (res.headers.get('set-cookie') ?? '') + .split(',').map((c) => c.split(';')[0].trim()).filter(Boolean).join('; '); +} + +function registerBody(secret: string) { + return { + providerId: 'acme-okta', + issuer: IDP, + domain: 'acme.com', + oidcConfig: { + clientId: 'acme-client-id', + clientSecret: secret, + scopes: ['openid', 'email', 'profile'], + mapping: { email: 'email', name: 'name' }, + }, + }; +} + +/** The stored row, read at DRIVER level — below every engine read mask. */ +async function readRowAtRest(engine: ObjectQL): Promise> { + const driver = (engine as unknown as { getDriver(o: string): { find(o: string, q: unknown): Promise } }) + .getDriver(SSO_PROVIDER_OBJECT); + const found = await driver.find(SSO_PROVIDER_OBJECT, { where: {} }); + const rows = Array.isArray(found) ? found : [found]; + return rows[0] as Record; +} + +describe('[#8009] sys_sso_provider OIDC clientSecret at rest', () => { + it('① register: no plaintext in ANY column, and the column holds a real ciphertext ref', async () => { + const restore = stubDiscovery(); + try { + const engine = await bootEngine(); + const auth = makeAuth(engine); + const cookie = await signUpAdmin(auth); + + const res = await auth.handler(new Request(`${BASE}/api/v1/auth/sso/register`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: BASE, cookie }, + body: JSON.stringify(registerBody(SECRET_UNDER_TEST)), + })); + expect(res.status).toBe(200); + + const row = await readRowAtRest(engine); + // Whole-row scan, not just oidc_config: moving the secret into some other + // cleartext column would satisfy a column-scoped check and change nothing. + expect(JSON.stringify(row)).not.toContain(SECRET_UNDER_TEST); + expect(String(row.oidc_config)).not.toContain(SECRET_UNDER_TEST); + expect(String(row.oidc_config)).not.toContain('clientSecret'); + // …and the rest of the config is still readable, which is the whole reason + // only `clientSecret` was split out rather than the blob as a whole. + const blob = JSON.parse(String(row.oidc_config)) as Record; + expect(blob.clientId).toBe('acme-client-id'); + expect(blob.tokenEndpoint).toBe(`${IDP}/token`); + + // The column holds a ref, and the ciphertext behind it is not the plaintext. + expect(String(row[SSO_CLIENT_SECRET_FIELD])).toMatch(/^secret:/); + const secrets = await engine.find('sys_secret', {} as never) as Record[]; + expect(secrets.length).toBe(1); + expect(String(secrets[0].ciphertext)).not.toContain(SECRET_UNDER_TEST); + expect(secrets[0].namespace).toBe(SSO_PROVIDER_OBJECT); + expect(secrets[0].key).toBe(SSO_CLIENT_SECRET_FIELD); + + // The generic data API (apiMethods get/list) returns the mask, never the value. + const viaApi = await engine.find(SSO_PROVIDER_OBJECT, {} as never) as Record[]; + expect(String(viaApi[0][SSO_CLIENT_SECRET_FIELD])).not.toContain(SECRET_UNDER_TEST); + } finally { restore(); } + }, 60_000); + + it('② callback path: better-auth reads the provider back with the CORRECT plaintext', async () => { + const restore = stubDiscovery(); + try { + const engine = await bootEngine(); + const auth = makeAuth(engine); + const cookie = await signUpAdmin(auth); + const res = await auth.handler(new Request(`${BASE}/api/v1/auth/sso/register`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: BASE, cookie }, + body: JSON.stringify(registerBody(SECRET_UNDER_TEST)), + })); + expect(res.status).toBe(200); + + // Read the provider exactly the way `/sso/callback` does — through + // better-auth's OWN adapter, by provider id. This is the read that feeds + // the IdP token exchange; if it does not yield the plaintext, every + // federated login is broken. + const adapter = (auth as unknown as { + $context: Promise<{ adapter: { findOne(a: unknown): Promise | null> } }>; + }).$context; + const ctx = await adapter; + const provider = await ctx.adapter.findOne({ + model: 'ssoProvider', + where: [{ field: 'providerId', value: 'acme-okta', operator: 'eq', connector: 'AND' }], + }); + expect(provider).toBeTruthy(); + + const recovered = JSON.parse(String(provider!.oidcConfig)) as Record; + // Compared against the TEST's literal — never against anything the seam + // produced, which would be an assertion that cannot fail. + expect(recovered.clientSecret).toBe(SECRET_UNDER_TEST); + // …and the rest of the config survived the round trip intact. + expect(recovered.clientId).toBe('acme-client-id'); + expect(recovered.tokenEndpoint).toBe(`${IDP}/token`); + // better-auth must never be handed the internal column or its mask. + expect(provider!.oidcClientSecret).toBeUndefined(); + expect(JSON.stringify(provider)).not.toContain('••'); + } finally { restore(); } + }, 60_000); + + it('③ update-provider: the second write door does not put cleartext back', async () => { + const restore = stubDiscovery(); + try { + const engine = await bootEngine(); + const auth = makeAuth(engine); + const cookie = await signUpAdmin(auth); + await auth.handler(new Request(`${BASE}/api/v1/auth/sso/register`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: BASE, cookie }, + body: JSON.stringify(registerBody(SECRET_UNDER_TEST)), + })); + + // Drive better-auth's own update door with a rotated secret. + const upd = await auth.handler(new Request(`${BASE}/api/v1/auth/sso/update-provider`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: BASE, cookie }, + body: JSON.stringify({ + providerId: 'acme-okta', + oidcConfig: { + clientId: 'acme-client-id', + clientSecret: ROTATED_SECRET, + scopes: ['openid', 'email', 'profile'], + mapping: { email: 'email', name: 'name' }, + }, + }), + })); + + const row = await readRowAtRest(engine); + const serialized = JSON.stringify(row); + // Whichever way the endpoint resolved, NEITHER secret may be at rest. + expect(serialized).not.toContain(ROTATED_SECRET); + expect(serialized).not.toContain(SECRET_UNDER_TEST); + expect(String(row[SSO_CLIENT_SECRET_FIELD])).toMatch(/^secret:/); + + // If the update landed, the new secret must be what reads back. + if (upd.status === 200) { + const ctx = await (auth as unknown as { + $context: Promise<{ adapter: { findOne(a: unknown): Promise | null> } }>; + }).$context; + const provider = await ctx.adapter.findOne({ + model: 'ssoProvider', + where: [{ field: 'providerId', value: 'acme-okta', operator: 'eq', connector: 'AND' }], + }); + const recovered = JSON.parse(String(provider!.oidcConfig)) as Record; + expect(recovered.clientSecret).toBe(ROTATED_SECRET); + } + } finally { restore(); } + }, 60_000); + + it('④ fail-closed: with no CryptoProvider the write is REFUSED, never stored cleartext', async () => { + const restore = stubDiscovery(); + try { + const engine = await bootEngine({ withCrypto: false }); + const auth = makeAuth(engine); + const cookie = await signUpAdmin(auth); + + const res = await auth.handler(new Request(`${BASE}/api/v1/auth/sso/register`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: BASE, cookie }, + body: JSON.stringify(registerBody(SECRET_UNDER_TEST)), + })); + // The engine refuses rather than falling back to cleartext, so the + // registration fails loudly. What must NOT happen is a 200 with the + // secret on disk. + expect(res.status).not.toBe(200); + const rows = await engine.find(SSO_PROVIDER_OBJECT, {} as never) as Record[]; + expect(JSON.stringify(rows)).not.toContain(SECRET_UNDER_TEST); + } finally { restore(); } + }, 60_000); + + it('⑤ legacy rows: a pre-existing cleartext row still logs in, and migrates forward', async () => { + const engine = await bootEngine(); + // A row exactly as the shipped code wrote it: secret inside the blob, + // encrypted column empty. Written through the DRIVER so the engine's own + // secret handling cannot pre-empt the very state under test. + const driver = (engine as unknown as { + getDriver(o: string): { create(o: string, r: unknown, x?: unknown): Promise }; + }).getDriver(SSO_PROVIDER_OBJECT); + await driver.create(SSO_PROVIDER_OBJECT, { + id: 'legacy-1', + provider_id: 'legacy-okta', + issuer: IDP, + domain: 'legacy.com', + oidc_config: JSON.stringify({ clientId: 'legacy-id', clientSecret: SECRET_UNDER_TEST, tokenEndpoint: `${IDP}/token` }), + }); + + // Before migration: it is cleartext, and the read half must still hand + // better-auth a WORKING secret (case 2) — otherwise the upgrade breaks + // every existing federated login. + const auth = makeAuth(engine); + const ctx = await (auth as unknown as { + $context: Promise<{ adapter: { findOne(a: unknown): Promise | null> } }>; + }).$context; + const before = await ctx.adapter.findOne({ + model: 'ssoProvider', + where: [{ field: 'providerId', value: 'legacy-okta', operator: 'eq', connector: 'AND' }], + }); + expect(JSON.parse(String(before!.oidcConfig)).clientSecret).toBe(SECRET_UNDER_TEST); + + // The migration moves it into the encrypted channel. + const result = await migrateLegacySsoClientSecrets(engine as never); + expect(result.found).toBe(1); + expect(result.migrated).toBe(1); + expect(result.failures).toEqual([]); + + const row = await readRowAtRest(engine); + expect(JSON.stringify(row)).not.toContain(SECRET_UNDER_TEST); + expect(String(row[SSO_CLIENT_SECRET_FIELD])).toMatch(/^secret:/); + + // …and it still reads back correctly afterwards. + const after = await ctx.adapter.findOne({ + model: 'ssoProvider', + where: [{ field: 'providerId', value: 'legacy-okta', operator: 'eq', connector: 'AND' }], + }); + expect(JSON.parse(String(after!.oidcConfig)).clientSecret).toBe(SECRET_UNDER_TEST); + + // Idempotent: a second sweep finds nothing left to do. + const again = await migrateLegacySsoClientSecrets(engine as never); + expect(again.found).toBe(0); + }, 60_000); +}); + +describe('[#8009] the write seam in isolation', () => { + it('leaves a partial update alone rather than blanking the stored secret', () => { + // `/sso/update-provider` may send a config with no clientSecret. Blanking + // the credential on such a write would break every later login. + const patch: Record = { + oidc_config: JSON.stringify({ clientId: 'acme-client-id', scopes: ['openid'] }), + }; + expect(liftClientSecretForWrite(SSO_PROVIDER_OBJECT, patch)).toBe(false); + expect(patch[SSO_CLIENT_SECRET_FIELD]).toBeUndefined(); + expect(JSON.parse(String(patch.oidc_config)).clientId).toBe('acme-client-id'); + }); + + it('does not touch rows of other objects', () => { + const row: Record = { + oidc_config: JSON.stringify({ clientSecret: SECRET_UNDER_TEST }), + }; + expect(liftClientSecretForWrite('sys_user', row)).toBe(false); + expect(String(row.oidc_config)).toContain(SECRET_UNDER_TEST); + }); + + it('leaves a malformed blob untouched rather than rewriting it', () => { + const row: Record = { oidc_config: 'not json at all' }; + expect(liftClientSecretForWrite(SSO_PROVIDER_OBJECT, row)).toBe(false); + expect(row.oidc_config).toBe('not json at all'); + expect(row[SSO_CLIENT_SECRET_FIELD]).toBeUndefined(); + }); + + it('declares the column as type secret — the encrypted channel, not a plain column', () => { + const ext = authObjectExtensions.find((e) => e.extend === SSO_PROVIDER_OBJECT); + expect((ext!.fields as Record)[SSO_CLIENT_SECRET_FIELD].type).toBe('secret'); + }); +}); diff --git a/packages/plugins/plugin-auth/src/sso-client-secret.ts b/packages/plugins/plugin-auth/src/sso-client-secret.ts new file mode 100644 index 0000000000..a62601938a --- /dev/null +++ b/packages/plugins/plugin-auth/src/sso-client-secret.ts @@ -0,0 +1,359 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8009] The persistence seam for an OIDC provider's `clientSecret`. + * + * ## The defect (measured, not asserted) + * `sys_sso_provider.oidc_config` is a textarea holding the whole OIDC config as + * one JSON string, `clientSecret` included. A provider registered through the + * real write path stores that secret BYTE-FOR-BYTE IN CLEARTEXT — measured on + * issue #8009 by registering a provider against a real `betterAuth` + real + * `sso()` + this adapter over a real engine, then reading the row back with + * `engine.find` (bypassing the adapter). The object file's own register-action + * helpText claims the value is "stored encrypted by better-auth"; that claim is + * measured FALSE. An OIDC `clientSecret` is what authenticates THIS PLATFORM to + * the IdP, and `sys_sso_provider` is readable through the generic data API + * (`apiMethods: ['get','list']`), so anyone who can read the row can + * impersonate our OIDC client. + * + * better-auth's OWN read endpoints are not the exposure — `sanitizeProvider` + * masks `clientId` and never returns `clientSecret`. The exposure is cleartext + * at rest plus our own generic read path. + * + * ## The seam + * `@better-auth/sso` at the pinned 1.7.0-rc.2 has NO secret-at-rest option, so + * there is no upstream switch to flip: `SSOOptions` has no equivalent of + * `scim({ storeSCIMToken: 'hashed' })`. better-auth owns the writes, so the seam + * sits between better-auth and its adapter — here — and never in a route handler. + * + * register / update-provider + * -> adapter `create` / `update` / `updateMany` + * -> {@link liftClientSecretForWrite}: `clientSecret` is LIFTED OUT of the + * JSON blob into `sys_sso_provider.oidc_client_secret` (`type: 'secret'`) + * -> the engine encrypts it via the registered `ICryptoProvider`, stores + * the ciphertext as a `sys_secret` row, and leaves only an opaque + * `secret:` ref on the column + * -> `oidc_config` persists the same blob MINUS `clientSecret` + * + * /sso/callback (and every other better-auth read of the model) + * -> adapter `findOne` / `findMany` + * -> {@link injectClientSecretOnRead}: the plaintext is recovered through + * `engine.resolveSecretField()` and put BACK into the blob better-auth + * receives, so the IdP token exchange still authenticates. + * + * Decrypt-on-read is MANDATORY, not decorative: `/sso/callback` reads the blob + * back and expects plaintext. Encrypt-on-write alone would break every + * federated login while making the column LOOK protected. + * + * ⚠️ This is NOT the "redact on read" shape that was ruled out on #8009. Redact + * on read leaves cleartext at rest and hides it from readers; this stores + * ciphertext at rest and hands plaintext only to the one privileged server-side + * consumer that must have it. + * + * ## Two things this file deliberately does NOT do + * - It does not invent a second cipher store, and it does not encrypt anything + * itself. The engine owns the `ICryptoProvider` (the host injects it via + * `setCryptoProvider`), so this seam writes cleartext INTO the `secret`-typed + * column exactly once and lets the engine's own write path wrap it. That + * inherits the engine's fail-closed posture for free: no provider ⇒ the write + * THROWS ⇒ registration fails loudly, rather than silently persisting + * cleartext in a column that advertises itself as encrypted. Same reasoning as + * `plugin-webhooks/src/webhook-secret.ts` (#7799), and the same privileged + * accessor (#7823). + * - It does not guess on read. A row whose secret cannot be resolved is handed + * to better-auth WITHOUT a `clientSecret` rather than with a wrong one: the + * IdP rejects the token exchange visibly, which is safer than a silent + * half-authenticated login. + */ + +import type { IDataEngine } from '@objectstack/core'; + +/** Object whose rows carry the OIDC config. */ +export const SSO_PROVIDER_OBJECT = 'sys_sso_provider'; + +/** Column holding the encrypted OIDC client secret (`type: 'secret'`). */ +export const SSO_CLIENT_SECRET_FIELD = 'oidc_client_secret'; + +/** Column holding the rest of the OIDC config, as a JSON string. */ +export const SSO_OIDC_CONFIG_FIELD = 'oidc_config'; + +/** Member lifted out of the blob. */ +const CLIENT_SECRET_KEY = 'clientSecret'; + +/** + * Engine surface this seam needs. `withSystemContext` deliberately exposes only + * the CRUD verbs, so the privileged dereference has to come from the RAW engine + * — `resolveSecretField` is a separately-named privileged verb precisely so it + * cannot be reached from a query string (#7823). + */ +export interface SecretResolvingEngine { + resolveSecretField?( + object: string, recordId: string, field: string, opts?: { tenantId?: string }, + ): Promise; + /** + * [#8022] The engine's crypto-provider registration channel. "No CryptoProvider" + * is not only a misconfiguration — on every host it is also a transient BOOT + * state, because plugins run inside `kernel:ready` while the composition root + * (`serve.ts`) injects the provider only after `runtime.start()` returns. + */ + onCryptoProviderChange?(listener: () => void): () => void; +} + +/** Parsed view of the `oidc_config` column, remembering how it was carried. */ +interface ParsedConfig { + config: Record; + /** True when the column carried a JSON STRING (what better-auth writes). */ + wasString: boolean; +} + +/** + * Parse the `oidc_config` value. better-auth hands the adapter a JSON STRING + * (`JSON.stringify` in its register handler), but the column is also readable as + * an already-parsed object depending on driver/JSON support, so both are + * accepted and the original carrier shape is restored on the way out. Returns + * `null` for anything that is not a usable config — a null column, an empty + * string, or a string that is not JSON — so a malformed row is left untouched + * rather than rewritten into a different malformed shape. + */ +function parseOidcConfig(value: unknown): ParsedConfig | null { + if (value == null) return null; + if (typeof value === 'object' && !Array.isArray(value)) { + return { config: value as Record, wasString: false }; + } + if (typeof value !== 'string' || value.trim() === '') return null; + try { + const parsed: unknown = JSON.parse(value); + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) return null; + return { config: parsed as Record, wasString: true }; + } catch { + return null; + } +} + +/** Re-serialize a config in the same carrier shape it arrived in. */ +function serializeOidcConfig(parsed: ParsedConfig): unknown { + return parsed.wasString ? JSON.stringify(parsed.config) : parsed.config; +} + +/** + * WRITE half. Lift `clientSecret` out of the `oidc_config` blob on `row` and put + * it on the `secret`-typed column, in place. + * + * No-ops (leaving `row` byte-for-byte alone) when the row is for another object, + * carries no `oidc_config`, or carries a blob with no usable `clientSecret`. + * That last case is what makes a PARTIAL update safe: `/sso/update-provider` + * may send a config without the secret, and silently blanking the stored + * credential on such a write would break every subsequent login. + * + * Returns true when a secret was lifted (for logging/tests). + */ +export function liftClientSecretForWrite(objectName: string, row: unknown): boolean { + if (objectName !== SSO_PROVIDER_OBJECT) return false; + if (!row || typeof row !== 'object') return false; + const record = row as Record; + if (!(SSO_OIDC_CONFIG_FIELD in record)) return false; + + const parsed = parseOidcConfig(record[SSO_OIDC_CONFIG_FIELD]); + if (!parsed) return false; + + const secret = parsed.config[CLIENT_SECRET_KEY]; + if (typeof secret !== 'string' || secret === '') return false; + + delete parsed.config[CLIENT_SECRET_KEY]; + record[SSO_OIDC_CONFIG_FIELD] = serializeOidcConfig(parsed); + // Cleartext into the `secret`-typed column exactly once; the ENGINE encrypts. + record[SSO_CLIENT_SECRET_FIELD] = secret; + return true; +} + +/** + * READ half. Put the plaintext `clientSecret` back into the blob better-auth + * receives, in place, and drop the internal secret column from the row. + * + * Three cases, in order: + * 1. the column holds an encrypted ref ⇒ dereference it via the privileged + * accessor and re-inject the plaintext; + * 2. the column is empty but the blob still carries a cleartext `clientSecret` + * ⇒ a row written before this seam existed. Left as-is so federated login + * keeps working on an un-migrated row (see + * {@link migrateLegacySsoClientSecrets} for how such rows are moved + * forward); + * 3. neither ⇒ better-auth gets a config with no `clientSecret`, and the IdP + * rejects the exchange visibly. Deliberate: a wrong secret would fail more + * confusingly, and inventing one is not an option. + * + * The internal column is ALWAYS removed from the returned row, so better-auth + * never sees the read mask as if it were a field of its own model. + */ +export async function injectClientSecretOnRead( + engine: SecretResolvingEngine, + objectName: string, + row: unknown, +): Promise { + if (objectName !== SSO_PROVIDER_OBJECT) return; + if (!row || typeof row !== 'object') return; + const record = row as Record; + + const recordId = record.id; + // The column is masked on every supported read path, so its VALUE here is + // never the ref — the privileged accessor re-reads the row at driver level. + delete record[SSO_CLIENT_SECRET_FIELD]; + + if (typeof recordId !== 'string' || recordId === '') return; + if (typeof engine.resolveSecretField !== 'function') return; + + const parsed = parseOidcConfig(record[SSO_OIDC_CONFIG_FIELD]); + if (!parsed) return; + // Case 2: an un-migrated row already carries its cleartext secret. + if (typeof parsed.config[CLIENT_SECRET_KEY] === 'string') return; + + let plaintext: string | null = null; + try { + plaintext = await engine.resolveSecretField(SSO_PROVIDER_OBJECT, recordId, SSO_CLIENT_SECRET_FIELD); + } catch { + // Case 3: no CryptoProvider, or the sys_secret row is gone. Hand back a + // config with no clientSecret rather than a wrong one. + return; + } + if (typeof plaintext !== 'string' || plaintext === '') return; + + parsed.config[CLIENT_SECRET_KEY] = plaintext; + record[SSO_OIDC_CONFIG_FIELD] = serializeOidcConfig(parsed); +} + +/** Outcome of the one-shot forward migration, for logging and tests. */ +export interface SsoSecretMigrationResult { + /** Rows found still carrying a cleartext `clientSecret` in the blob. */ + found: number; + /** Rows successfully moved into the encrypted channel. */ + migrated: number; + /** Rows that could not be migrated (message per row). */ + failures: string[]; +} + +/** + * MIGRATION half — the disposition for rows written before this seam existed. + * + * Such a row keeps its cleartext `clientSecret` inside `oidc_config`. Leaving it + * there while the object advertises an encrypted column is the "looks protected" + * failure this change exists to remove, so the plugin sweeps them forward once + * at start: every provider row whose blob still carries a cleartext secret is + * re-written through the engine, which lifts the secret into the encrypted + * column and drops it from the blob. + * + * Bounded and idempotent by construction: SSO providers are env-global admin + * config (a handful of rows, not a data table), a migrated row no longer matches + * the "blob still carries a cleartext secret" test, and a row that fails is + * counted and reported rather than retried in a loop. + * + * Fail-SOFT at the call site, fail-CLOSED per row: this never throws into boot + * (an environment with no CryptoProvider must still start), but a row it cannot + * migrate is left exactly as it was — still working, still cleartext, and + * counted in `failures` so the operator sees it. It is never rewritten into a + * half-migrated state. + */ +export async function migrateLegacySsoClientSecrets( + engine: IDataEngine, +): Promise { + const result: SsoSecretMigrationResult = { found: 0, migrated: 0, failures: [] }; + const e = engine as unknown as { + find(object: string, query: unknown): Promise[]>; + update(object: string, data: unknown, options?: unknown): Promise; + }; + + let rows: Record[]; + try { + rows = await e.find(SSO_PROVIDER_OBJECT, { context: { isSystem: true } }); + } catch (err) { + result.failures.push(`could not list providers: ${String((err as Error)?.message ?? err)}`); + return result; + } + + for (const row of rows ?? []) { + const parsed = parseOidcConfig(row?.[SSO_OIDC_CONFIG_FIELD]); + if (!parsed) continue; + if (typeof parsed.config[CLIENT_SECRET_KEY] !== 'string' || parsed.config[CLIENT_SECRET_KEY] === '') continue; + result.found += 1; + + const id = row.id; + if (typeof id !== 'string' || id === '') { + result.failures.push('provider row has no id'); + continue; + } + + const patch: Record = { + id, + [SSO_OIDC_CONFIG_FIELD]: row[SSO_OIDC_CONFIG_FIELD], + }; + // Same seam as the live write path — one implementation, not a second one. + liftClientSecretForWrite(SSO_PROVIDER_OBJECT, patch); + try { + await e.update(SSO_PROVIDER_OBJECT, patch, { context: { isSystem: true } }); + result.migrated += 1; + } catch (err) { + result.failures.push(`${id}: ${String((err as Error)?.message ?? err)}`); + } + } + + return result; +} + +/** Minimal logger surface, so this module needs no logger dependency. */ +interface MigrationLogger { + info(msg: string, meta?: unknown): void; + warn(msg: string, meta?: unknown): void; +} + +/** + * Run {@link migrateLegacySsoClientSecrets} at boot, and again if the host wires + * the CryptoProvider afterwards. + * + * The second half is not belt-and-braces: plugins run inside `kernel:ready`, and + * `serve.ts` injects the provider only after `runtime.start()` RETURNS, so the + * first sweep reliably precedes the capability it needs and would otherwise + * report every legacy row as a failure and leave it cleartext until someone + * re-saved it by hand. Same race, same remedy, as `plugin-webhooks` (#8022). + * + * Returns an unsubscribe function when the engine exposes the channel. + */ +export function scheduleLegacySsoSecretMigration( + engine: IDataEngine, + logger: MigrationLogger, +): (() => void) | undefined { + const report = (result: SsoSecretMigrationResult): void => { + if (result.found === 0 && result.failures.length === 0) return; + if (result.migrated > 0) { + logger.info( + `Auth: migrated ${result.migrated} SSO provider client secret(s) into the encrypted channel (#8009)`, + ); + } + if (result.failures.length > 0) { + logger.warn( + `Auth: ${result.failures.length} SSO provider row(s) still hold a CLEARTEXT OIDC client secret — ` + + 'they keep working, but are not encrypted at rest. Wire a CryptoProvider ' + + '(engine.setCryptoProvider) and restart, or re-save the provider.', + { failures: result.failures }, + ); + } + }; + + const run = (): void => { + void migrateLegacySsoClientSecrets(engine) + .then(report) + .catch((err: unknown) => { + // Never throw into boot: an environment with no CryptoProvider must + // still start. An un-migrated row is visible above, not silent. + logger.warn(`Auth: SSO client-secret migration sweep failed: ${String((err as Error)?.message ?? err)}`); + }); + }; + + // Subscribe BEFORE the first sweep, so a provider that lands mid-sweep is not + // missed — the same ordering `plugin-webhooks` uses for this race. + const observable = engine as unknown as SecretResolvingEngine; + const unsubscribe = typeof observable?.onCryptoProviderChange === 'function' + ? observable.onCryptoProviderChange(() => run()) + : undefined; + + run(); + return unsubscribe; +}