diff --git a/.changeset/settings-rotation-repoint-secret-handle.md b/.changeset/settings-rotation-repoint-secret-handle.md new file mode 100644 index 0000000000..fee938f173 --- /dev/null +++ b/.changeset/settings-rotation-repoint-secret-handle.md @@ -0,0 +1,48 @@ +--- +"@objectstack/service-settings": patch +--- + +fix(service-settings): rotating an encrypted setting now actually rotates it — the secret handle is repointed, and the retired ciphertext is destroyed (#8030) + +A **second** `PUT` of a new value for an encrypted setting key answered **200** +with a correctly redacted body, advanced `updated_at`, wrote an audit row and +inserted a genuinely new `sys_secret` row holding the new plaintext — and left +`sys_setting.value_enc` pointing at the **first** handle. The effective secret +never changed. + +Nothing an administrator can see said so. Rotating a leaked SMTP password or +provider API key looked exactly like a rotation that worked, while the leaked +credential stayed the one in force. The **first** write of any secret was +correct, so the defect was invisible until the second. + +**Cause.** `sys_setting.value_enc` (and `updated_by`) are declared +`readonly: true`, and the engine strips author-declared read-only columns from a +**non-system** caller's UPDATE payload. `SettingsService` persisted its rows +through a plain, un-elevated `engine.update`, so the handle could never be +repointed. The INSERT path is deliberately exempt from that strip — which is +precisely why the first write landed and every later one did not. + +**Fix.** `SettingsService` performs its own row update as a **system** write. +It is a privileged writer — the manifest capability gate, the env/upper-scope +lock pre-flight and value validation have all already run by then, and these are +columns it owns rather than ones a caller forged. The `IDataEngine` adapter +forwards that execution context on both of its branches. + +⚠️ `value_enc` **stays `readonly: true`**. The elevation is scoped to this one +write, so an external caller reaching `sys_setting` through the data layer still +cannot repoint a secret handle — that flag is a security control, and removing +it would have been the wrong direction on this defect. There is a test that +fails if someone removes it. + +**Orphans are reaped, not accepted.** A rotation used to leave the previous +`sys_secret` row behind — one more decryptable copy of the credential the admin +just retired, accumulating per rotation (7 → 8 → 9 across three writes). The +row a rotated-away handle named is now deleted once the repoint has committed. +The delete is best-effort and reported if it fails: the new secret is already in +force at that point, and a failed cleanup must not turn a successful rotation +into an error. + +Unaffected: the first-write path is byte-for-byte the same; the `••••••••` +mask-echo no-op still leaves the stored ciphertext untouched; env-locked secrets +still refuse writes with `409 SETTINGS_LOCKED`; and a secret store without the +new optional `delete` keeps working, simply accepting the orphans. diff --git a/packages/services/service-settings/package.json b/packages/services/service-settings/package.json index bd336a4538..7d73f85f41 100644 --- a/packages/services/service-settings/package.json +++ b/packages/services/service-settings/package.json @@ -26,6 +26,7 @@ "@objectstack/types": "workspace:*" }, "devDependencies": { + "@objectstack/objectql": "workspace:*", "@types/node": "^26.1.2", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/services/service-settings/src/settings-secret-rotation.test.ts b/packages/services/service-settings/src/settings-secret-rotation.test.ts new file mode 100644 index 0000000000..2e9301ccfd --- /dev/null +++ b/packages/services/service-settings/src/settings-secret-rotation.test.ts @@ -0,0 +1,459 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8030 — **rotating an encrypted setting must actually rotate it.** + * + * ## The defect + * + * `sys_setting.value_enc` is declared `readonly: true` + * (`packages/platform-objects/src/system/sys-setting.object.ts`), and the + * engine strips author-declared read-only columns from a **non-system** + * caller's UPDATE payload (`stripReadonlyFields`, gated on + * `if (!opCtx.context?.isSystem)` in `packages/objectql/src/engine.ts`). The + * INSERT path is deliberately exempt (#3413). + * + * `SettingsService` wrote its rows through a plain, un-elevated + * `engine.update`, so: + * + * - the **first** write of a secret INSERTed, and was correct; + * - every **later** write UPDATEd, had `value_enc` stripped out from under + * it, and left the handle pointing at the ORIGINAL ciphertext. + * + * What made it a P0 rather than a lost write is that **nothing visible said + * so**: the PUT answered 200 with a correctly redacted body, `updated_at` + * advanced, an audit row was written, and a genuinely new `sys_secret` row + * holding the new plaintext was inserted. An admin rotating a leaked SMTP + * password or provider API key had every reason to believe the leak was + * closed. It was not — the old credential was still the one in force, and one + * more decryptable copy of it had just been added to the database (the filer + * measured `sys_secret` going 7 → 8 → 9 across three writes). + * + * ## Why these run against the REAL engine + * + * The defect IS the engine's strip rule meeting the platform's own field + * declaration. A hand-written fake that models "drop read-only keys" proves + * only that the fake was written to match the fix. So this file boots a real + * `ObjectQL` over the real `SysSetting` / `SysSecret` schemas and drives the + * real `SettingsService` through the real `IDataEngine` adapter — the same + * four pieces the running server bolts together. `vitest.config.ts` aliases + * `@objectstack/objectql` to its SOURCE so the verdict is about the checkout + * and not about a prebuilt `dist`. + * + * ## What each case measures + * + * 1. **the rotation** — a second AND a third PUT of a new value repoint + * `value_enc`, and a read-after-write through the service resolves the new + * plaintext. Red on `origin/main` at case 1's second write. + * 2. **the first write is unchanged** — one write still INSERTs one row, one + * handle, one `sys_secret` row that decrypts to the value written. + * 3. **`readonly` still means readonly for everyone else** — the field + * declaration is a security control and the fix must not have removed it: + * an ordinary (non-system) caller reaching `sys_setting` directly still + * cannot repoint a secret handle. This is the case that fails if someone + * "fixes" #8030 by deleting `readonly: true`. + * 4. **the mask-echo no-op** (#7522 / PR #7554) — a PUT carrying `••••••••` + * leaves the stored ciphertext BYTE-IDENTICAL. Verified sound by the filer + * and explicitly not the cause here, so it is pinned against regression. + * 5. **no orphans** — three rotations leave exactly one `sys_secret` row, and + * the retired ciphertexts are GONE rather than merely unreferenced. + * 6. **reaping is best-effort** — a store whose `delete` throws still leaves + * the rotation landed. The new secret being in force is the property that + * matters; the failed cleanup is reported, not raised. + * 7. **the adapter forwards `context`** — on BOTH of its branches. The + * settings row write takes the `multi` one (its `where` is the composite + * key, never an `id`), so the by-id branch is the half that would rot + * unnoticed. + */ + +import { describe, expect, it, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SysSecret, SysSetting } from '@objectstack/platform-objects/system'; +import type { SettingsManifest } from '@objectstack/spec/system'; +import { SettingsService } from './settings-service.js'; +import { wrapEngineAsSettingsEngine } from './settings-service-plugin.js'; +import { LocalCryptoProvider } from './local-crypto-provider.js'; +import { dropEchoedSecretMasks, SETTINGS_SECRET_MASK } from './settings-secret-redaction.js'; +import type { SettingsSecretStore } from './settings-service.types.js'; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +const OWNER_PACKAGE = 'com.objectstack.test.settings-secret-rotation'; + +/** + * One encrypted key and one plain one. Deliberately minimal: the shipped + * manifests carry `visible` expressions and cross-field required rules, and + * this file is about the storage handle, not about `validatePatch`. + */ +const smsManifest: SettingsManifest = { + namespace: 'sms', + version: 1, + label: 'SMS', + scope: 'tenant', + readPermission: 'setup.access', + writePermission: 'setup.write', + specifiers: [ + { type: 'text', key: 'twilio_account_sid', label: 'Account SID', required: false }, + { type: 'password', key: 'twilio_auth_token', label: 'Auth token', required: false, encrypted: true }, + ], +}; + +type Store = Map>>; + +/** A driver over plain Maps — enough of `IDataDriver` for the settings write path. */ +function makeMemoryDriver() { + const store: Store = new Map(); + let nextId = 0; + const copy = (r: Record) => ({ ...r }); + const rowsOf = (object: string) => { + let s = store.get(object); + if (!s) { s = new Map(); store.set(object, s); } + return s; + }; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]) => (row[k] ?? null) === (v ?? null)); + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + return [...rowsOf(object).values()].filter((r) => matches(r, ast?.where)).map(copy); + }, + async findOne(object: string, ast: any) { + for (const r of rowsOf(object).values()) if (matches(r, ast?.where)) return copy(r); + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `row_${nextId}`; + const row = { ...data, id }; + rowsOf(object).set(id, row); + return copy(row); + }, + async update(object: string, id: string, data: Record) { + const s = rowsOf(object); + const cur = s.get(id); + if (!cur) return null; + const next = { ...cur, ...data, id }; + s.set(id, next); + return copy(next); + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + return id && rowsOf(object).has(id) ? this.update(object, id, data) : this.create(object, data); + }, + async delete(object: string, id: string) { return rowsOf(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + const s = rowsOf(object); + for (const r of rows) s.set(r.id as string, { ...s.get(r.id as string), ...data, id: r.id }); + return rows.length; + }, + async deleteMany(object: string, ast: any) { + const rows = await this.find(object, ast); + for (const r of rows) rowsOf(object).delete(r.id as string); + return rows.length; + }, + async syncSchema() {}, async dropTable() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, rowsOf }; +} + +/** + * The four pieces the running server bolts together: a real engine over the + * real system objects, the real `IDataEngine → SettingsEngine` adapter, the + * real `sys_secret` store the plugin builds, and the real service. + * + * `secretStoreOverrides` lets one case break `delete` without touching the + * others; `withDelete: false` reproduces a store that cannot reap at all. + */ +async function boot(opts: { + secretStoreOverrides?: Partial; + withDelete?: boolean; +} = {}) { + const engine = new ObjectQL(); + const { driver, rowsOf } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + for (const o of [SysSetting, SysSecret]) { + engine.registry.registerObject(o as any, OWNER_PACKAGE); + } + + const eng: any = engine; + const baseStore: SettingsSecretStore = { + async insert(row) { + await eng.insert('sys_secret', row, { bypassTenantAudit: true }); + return { id: row.id }; + }, + async get(id) { + const rows = await eng.find('sys_secret', { where: { id }, limit: 1, bypassTenantAudit: true }); + return (Array.isArray(rows) ? rows[0] : null) ?? null; + }, + async update(id, patch) { + await eng.update('sys_secret', { id, ...patch }, { bypassTenantAudit: true }); + }, + ...(opts.withDelete === false ? {} : { + async delete(id: string) { + await eng.delete('sys_secret', { + where: { id }, bypassTenantAudit: true, context: { isSystem: true }, + }); + }, + }), + }; + + const logged: string[] = []; + const svc = new SettingsService({ + env: {}, + engine: wrapEngineAsSettingsEngine(engine as any), + cryptoProvider: new LocalCryptoProvider(), + secretStore: { ...baseStore, ...(opts.secretStoreOverrides ?? {}) }, + logger: { error: (m) => { logged.push(m); } }, + }); + svc.registerManifest(smsManifest); + + /** The stored row for `sms.twilio_auth_token`, straight off the driver. */ + const settingRow = () => + [...rowsOf('sys_setting').values()].find((r) => r.key === 'twilio_auth_token') as + Record | undefined; + const secretRows = () => [...rowsOf('sys_secret').values()]; + + return { engine, svc, settingRow, secretRows, logged }; +} + +// --------------------------------------------------------------------------- +// 1. The rotation +// --------------------------------------------------------------------------- + +describe('#8030 — rotating an encrypted setting repoints sys_setting.value_enc', () => { + it('a SECOND and THIRD write of a new value take effect', async () => { + const { svc, settingRow, secretRows } = await boot(); + + // Write 1 — the INSERT. Correct on `origin/main` too; this is the baseline + // the defect hides behind. + await svc.set('sms', 'twilio_auth_token', 'alpha'); + const handleA = settingRow()?.value_enc as string; + expect(handleA).toMatch(/^sec_/); + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('alpha'); + + // Write 2 — the UPDATE. On `origin/main` `value_enc` is still `handleA` + // here (the read-only strip took it), a new `sys_secret` row holding + // `beta` exists, and the effective secret is STILL `alpha`. + await svc.set('sms', 'twilio_auth_token', 'beta'); + const handleB = settingRow()?.value_enc as string; + expect(handleB).not.toBe(handleA); + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('beta'); + + // Write 3 — a rotation off a row that was itself written by an UPDATE. + await svc.set('sms', 'twilio_auth_token', 'gamma'); + const handleC = settingRow()?.value_enc as string; + expect(handleC).not.toBe(handleB); + expect(handleC).not.toBe(handleA); + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('gamma'); + + // The handle in force names the ciphertext that decrypts to the newest + // value — checked against storage, not against the service's own read. + const live = secretRows().find((r) => r.id === handleC); + expect(live).toBeDefined(); + expect(String(live!.ciphertext)).not.toContain('gamma'); + }); + + it('resetting to null clears the handle rather than leaving the old secret live', async () => { + const { svc, settingRow } = await boot(); + await svc.set('sms', 'twilio_auth_token', 'alpha'); + expect(settingRow()?.value_enc).toMatch(/^sec_/); + + await svc.set('sms', 'twilio_auth_token', null); + expect(settingRow()?.value_enc ?? null).toBeNull(); + expect((await svc.get('sms', 'twilio_auth_token')).value ?? null).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// 2. The first write is unchanged +// --------------------------------------------------------------------------- + +describe('#8030 — the first-write path is untouched', () => { + it('one write inserts one row, one handle and one decryptable sys_secret row', async () => { + const { svc, settingRow, secretRows } = await boot(); + + await svc.set('sms', 'twilio_auth_token', 'alpha'); + + const row = settingRow(); + expect(row).toBeDefined(); + expect(row!.encrypted).toBe(true); + expect(row!.value ?? null).toBeNull(); + expect(secretRows()).toHaveLength(1); + expect(secretRows()[0].id).toBe(row!.value_enc); + expect(String(secretRows()[0].ciphertext)).not.toContain('alpha'); + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('alpha'); + }); + + it('a non-encrypted key in the same namespace still round-trips as a plain value', async () => { + const { svc, secretRows } = await boot(); + await svc.set('sms', 'twilio_account_sid', 'AC1'); + await svc.set('sms', 'twilio_account_sid', 'AC2'); + expect((await svc.get('sms', 'twilio_account_sid')).value).toBe('AC2'); + expect(secretRows()).toHaveLength(0); + }); +}); + +// --------------------------------------------------------------------------- +// 3. `readonly` is still a security control for everybody else +// --------------------------------------------------------------------------- + +describe('#8030 — value_enc stays readonly for non-system callers', () => { + it('an ordinary caller cannot repoint a secret handle through the data layer', async () => { + const { engine, svc, settingRow } = await boot(); + await svc.set('sms', 'twilio_auth_token', 'alpha'); + const handleA = settingRow()?.value_enc as string; + const rowId = settingRow()?.id as string; + + // The attack the `readonly: true` flag exists to stop: point the setting + // at a ciphertext of the attacker's choosing. A NON-system update must + // still have `value_enc` stripped — the elevation is scoped to + // SettingsService's own write, not baked into the field declaration. + await (engine as any).update( + 'sys_setting', + { id: rowId, value_enc: 'sec_attacker_controlled' }, + { context: { isSystem: false, userId: 'u1' } }, + ); + + expect(settingRow()?.value_enc).toBe(handleA); + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('alpha'); + }); +}); + +// --------------------------------------------------------------------------- +// 4. The #7522 mask-echo no-op +// --------------------------------------------------------------------------- + +describe('#8030 — the mask-echo no-op (#7522 / PR #7554) still holds', () => { + it('a PUT carrying the mask leaves the stored ciphertext byte-identical', async () => { + const { svc, settingRow, secretRows } = await boot(); + await svc.set('sms', 'twilio_auth_token', 'alpha'); + const before = { ...settingRow()! }; + const cipherBefore = secretRows().map((r) => String(r.ciphertext)); + + // Exactly what the route does: the echoed mask is dropped from the patch + // BEFORE it reaches the service, so no write happens at all. + const patch = dropEchoedSecretMasks( + { twilio_auth_token: SETTINGS_SECRET_MASK, twilio_account_sid: 'AC1' }, + svc.secretKeysOf('sms'), + ); + expect(patch).not.toHaveProperty('twilio_auth_token'); + await svc.setMany('sms', patch); + + expect(settingRow()!.value_enc).toBe(before.value_enc); + expect(secretRows().map((r) => String(r.ciphertext))).toEqual(cipherBefore); + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('alpha'); + // …and the non-secret key in the same body still landed. + expect((await svc.get('sms', 'twilio_account_sid')).value).toBe('AC1'); + }); +}); + +// --------------------------------------------------------------------------- +// 5 & 6. Orphan reaping +// --------------------------------------------------------------------------- + +describe('#8030 — a rotation leaves no orphan sys_secret rows', () => { + it('three rotations leave exactly one row, and the retired ciphertexts are gone', async () => { + const { svc, settingRow, secretRows } = await boot(); + + await svc.set('sms', 'twilio_auth_token', 'alpha'); + const handleA = settingRow()?.value_enc as string; + expect(secretRows()).toHaveLength(1); + + await svc.set('sms', 'twilio_auth_token', 'beta'); + expect(secretRows()).toHaveLength(1); + expect(secretRows().some((r) => r.id === handleA)).toBe(false); + + await svc.set('sms', 'twilio_auth_token', 'gamma'); + expect(secretRows()).toHaveLength(1); + expect(secretRows()[0].id).toBe(settingRow()?.value_enc); + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('gamma'); + }); + + it('a reset clears the handle AND reaps the ciphertext it named', async () => { + const { svc, secretRows } = await boot(); + await svc.set('sms', 'twilio_auth_token', 'alpha'); + expect(secretRows()).toHaveLength(1); + await svc.set('sms', 'twilio_auth_token', null); + expect(secretRows()).toHaveLength(0); + }); + + it('a store with no delete keeps working — the orphans are simply accepted', async () => { + const { svc, settingRow, secretRows } = await boot({ withDelete: false }); + await svc.set('sms', 'twilio_auth_token', 'alpha'); + await svc.set('sms', 'twilio_auth_token', 'beta'); + // The rotation itself is the invariant; reaping is the optional half. + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('beta'); + expect(settingRow()?.value_enc).toBe(secretRows().find((r) => r.id === settingRow()?.value_enc)?.id); + expect(secretRows()).toHaveLength(2); + }); + + it('a delete that THROWS does not turn a successful rotation into an error', async () => { + const { svc, settingRow, logged } = await boot({ + secretStoreOverrides: { delete: vi.fn(async () => { throw new Error('storage offline'); }) }, + }); + await svc.set('sms', 'twilio_auth_token', 'alpha'); + const handleA = settingRow()?.value_enc as string; + + await expect(svc.set('sms', 'twilio_auth_token', 'beta')).resolves.toBeDefined(); + expect(settingRow()?.value_enc).not.toBe(handleA); + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('beta'); + // Loud, because "the old credential is gone" is exactly what is not true here. + expect(logged.join('\n')).toMatch(/could not be deleted from sys_secret/); + expect(logged.join('\n')).toMatch(/storage offline/); + }); +}); + +// --------------------------------------------------------------------------- +// 7. The adapter +// --------------------------------------------------------------------------- + +describe('#8030 — wrapEngineAsSettingsEngine forwards the execution context', () => { + it('on the multi branch (the one the settings row write actually takes)', async () => { + const calls: any[] = []; + const wrapped = wrapEngineAsSettingsEngine({ + update: async (...args: any[]) => { calls.push(args); }, + } as any); + + await wrapped.update('sys_setting', { + where: { namespace: 'sms', key: 'twilio_auth_token', scope: 'tenant', user_id: null }, + data: { value_enc: 'sec_new' }, + context: { isSystem: true }, + }); + + expect(calls).toHaveLength(1); + expect(calls[0][2]).toMatchObject({ multi: true, context: { isSystem: true } }); + }); + + it('and on the by-id branch, which no settings write exercises today', async () => { + const calls: any[] = []; + const wrapped = wrapEngineAsSettingsEngine({ + update: async (...args: any[]) => { calls.push(args); }, + } as any); + + await wrapped.update('sys_setting', { + where: { id: 'row_1' }, + data: { value_enc: 'sec_new' }, + bypassTenantAudit: true, + context: { isSystem: true }, + }); + + expect(calls).toHaveLength(1); + expect(calls[0][1]).toMatchObject({ id: 'row_1', value_enc: 'sec_new' }); + expect(calls[0][2]).toMatchObject({ bypassTenantAudit: true, context: { isSystem: true } }); + }); +}); diff --git a/packages/services/service-settings/src/settings-service-plugin.ts b/packages/services/service-settings/src/settings-service-plugin.ts index 90826d0553..30b54fc46b 100644 --- a/packages/services/service-settings/src/settings-service-plugin.ts +++ b/packages/services/service-settings/src/settings-service-plugin.ts @@ -304,6 +304,21 @@ export class SettingsServicePlugin implements Plugin { { bypassTenantAudit: true }, ); }, + async delete(id) { + // [#8030] The rotated-away ciphertext. `sys_setting.value_enc` has + // already been repointed by the time this runs, so the row is + // unreferenced — see `SettingsService.reapRotatedSecret` for why + // leaving it is a security problem rather than untidiness. + // + // System-elevated for the same reason the settings row update is: + // `sys_secret` is a platform-owned table and this is the platform + // deleting its own row, after the caller's write already passed the + // settings service's capability and lock gates. + await eng.delete( + 'sys_secret', + { where: { id }, bypassTenantAudit: true, context: { isSystem: true } }, + ); + }, }; } @@ -354,8 +369,25 @@ export class SettingsServicePlugin implements Plugin { * supplies a non-id where clause (composite-key tables), we fall back * to `multi: true` so the engine routes through `driver.updateMany` * instead of throwing. + * + * ### `context` is load-bearing, on BOTH branches (#8030) + * + * `SettingsService` sends `{ isSystem: true }` with its row writes because + * `sys_setting.value_enc` / `updated_by` are declared `readonly: true` and the + * engine strips author-declared read-only columns from a NON-system caller's + * UPDATE payload. This adapter is the only thing between that declaration and + * the engine, so a branch that forgets to forward `options.context` restores + * the defect in full: the rotation writes a new `sys_secret` row, answers 200 + * with a redacted echo, advances `updated_at` — and leaves `value_enc` on the + * OLD handle, so the credential the admin just rotated away is still live. + * + * Both branches forward it, and the settings row write in practice takes the + * `multi` one (its `where` is the composite `(namespace, key, scope, user_id)`, + * never an `id`) — so the by-id branch is the one that would rot unnoticed. + * Exported for that reason: `settings-secret-rotation.test.ts` drives the real + * engine through this adapter on both. */ -function wrapEngineAsSettingsEngine(engine: IDataEngine): SettingsEngine { +export function wrapEngineAsSettingsEngine(engine: IDataEngine): SettingsEngine { const eng: any = engine; return { async find(objectName, opts) { @@ -365,12 +397,16 @@ function wrapEngineAsSettingsEngine(engine: IDataEngine): SettingsEngine { return eng.insert(objectName, data, opts); }, async update(objectName, opts) { - const { where, data, bypassTenantAudit } = opts as { + const { where, data, bypassTenantAudit, context } = opts as { where: Record; data: Record; bypassTenantAudit?: boolean; + context?: Record; + }; + const driverOpts = { + ...(bypassTenantAudit ? { bypassTenantAudit: true } : {}), + ...(context ? { context } : {}), }; - const driverOpts = bypassTenantAudit ? { bypassTenantAudit: true } : undefined; const id = (where as any)?.id; if (id !== undefined && id !== null) { return eng.update(objectName, { id, ...data }, driverOpts); @@ -378,7 +414,7 @@ function wrapEngineAsSettingsEngine(engine: IDataEngine): SettingsEngine { return eng.update(objectName, data, { where, multi: true, - ...(driverOpts ?? {}), + ...driverOpts, }); }, }; diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index eb29079332..1704c42de7 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -44,6 +44,20 @@ import { const DEFAULT_OBJECT = 'sys_setting'; +/** + * The execution context `SettingsService`'s own row writes run under (#8030). + * + * `sys_setting` is a platform-owned table with platform-owned columns + * (`value_enc`, `updated_by` are declared `readonly: true`), and this service + * is the only writer of them — after its own capability, lock and validation + * gates. See {@link SettingsService.upsertRow} for the full argument and for + * why the field stays `readonly` for everybody else. + * + * Frozen so a downstream engine adapter cannot mutate the service's posture by + * writing into the bag it was handed. + */ +const SETTINGS_SYSTEM_WRITE_CONTEXT = Object.freeze({ isSystem: true as const }); + /** * Value-bearing specifier types — drives which entries we expect to * find in the K/V store. Keeps the resolver in sync with the spec @@ -1338,7 +1352,7 @@ export class SettingsService { } } - await this.upsertRow({ + const previousEnc = await this.upsertRow({ namespace, key, scope, @@ -1350,6 +1364,13 @@ export class SettingsService { updated_by: ctx.userId ?? null, }); + // The handle the row USED to point at is now unreferenced — destroy the + // ciphertext it names (#8030). Ordered after the repoint on purpose, and + // never allowed to fail the write; see `reapRotatedSecret`. Not gated on + // `isEncrypted`: a key that STOPS being encrypted (a manifest edit) orphans + // its handle in exactly the same way, and the helper is self-guarding. + await this.reapRotatedSecret(previousEnc, storedEnc); + if (this.audit) { await this.audit.record({ namespace, @@ -1854,7 +1875,41 @@ export class SettingsService { ); } - private async upsertRow(row: SettingsRow): Promise { + /** + * Write one settings row, INSERT-or-UPDATE, and report the `value_enc` the + * row held **before** the write (`null` when there was no row, or the row + * carried no handle). + * + * ### Why the update is a SYSTEM write (#8030) + * + * `sys_setting.value_enc` and `sys_setting.updated_by` are declared + * `readonly: true` (`packages/platform-objects/src/system/sys-setting.object.ts`), + * and the engine STRIPS author-declared read-only columns from a + * **non-system** caller's UPDATE payload (`stripReadonlyFields`, gated on + * `if (!opCtx.context?.isSystem)` in `packages/objectql/src/engine.ts`). The + * INSERT path is deliberately exempt from that strip (#3413) — which is + * exactly why the FIRST write of a secret landed correctly and every later + * one silently did not: a rotation inserted a fresh `sys_secret` row, got its + * 200 with a redacted echo and an advanced `updated_at`, and left + * `value_enc` pointing at the ORIGINAL handle. The leaked credential an + * admin had just "rotated" was still the one in force. + * + * `SettingsService` is a privileged writer: it has already run the + * manifest's read/write capability gate (`assertPermitted`), the env-lock + * and upper-scope-lock pre-flight, and `validatePatch` before anything + * reaches here, and the columns in question are ones IT owns rather than + * ones a caller forged. So the write is elevated — the same posture, and for + * the same measured reason, as the roll-up recompute's elevation in + * `ObjectQL.recomputeSummaries` (#7673): a platform-owned read-only column + * whose only writer is the platform must be written as the platform. + * + * ⚠️ The elevation is deliberately scoped to THIS call and NOT to the field + * declaration: `value_enc` stays `readonly: true`, so an external caller + * reaching `sys_setting` directly still cannot repoint a secret handle. That + * flag is a security control, and removing it is the wrong direction on this + * defect. + */ + private async upsertRow(row: SettingsRow): Promise { if (this.engine) { const where: Record = { namespace: row.namespace, @@ -1872,15 +1927,17 @@ export class SettingsService { ...bypass, } as any); if (existing[0]) { + const previousEnc = (existing[0] as { value_enc?: unknown }).value_enc; await this.engine.update(this.objectName, { where, data: { ...row }, + context: SETTINGS_SYSTEM_WRITE_CONTEXT, ...bypass, } as any); - } else { - await this.engine.insert(this.objectName, { ...row }, bypass as any); + return typeof previousEnc === 'string' && previousEnc !== '' ? previousEnc : null; } - return; + await this.engine.insert(this.objectName, { ...row }, bypass as any); + return null; } const idx = this.memory.findIndex( (r) => @@ -1889,8 +1946,59 @@ export class SettingsService { r.scope === row.scope && (r.user_id ?? null) === (row.user_id ?? null), ); - if (idx >= 0) this.memory[idx] = row; - else this.memory.push(row); + if (idx >= 0) { + const previousEnc = this.memory[idx].value_enc; + this.memory[idx] = row; + return typeof previousEnc === 'string' && previousEnc !== '' ? previousEnc : null; + } + this.memory.push(row); + return null; + } + + /** + * Delete the `sys_secret` row a rotated-away handle pointed at (#8030). + * + * Reaping rather than accepting the orphans is the security answer, not a + * tidiness one: the whole point of rotating a leaked SMTP password or + * provider API key is that the old value stops existing. An orphan row is a + * decryptable copy of the credential the admin just retired, sitting in + * `sys_secret` under the same data key, reachable by anyone who can read the + * table — and it accumulates one row per rotation forever (the filer measured + * 7 → 8 → 9 across three writes), so the exposure grows with exactly the + * hygiene we ask operators to practise. + * + * Nothing else can reference the handle: ids are minted per `encrypt()` call, + * `sys_setting.value_enc` is the only column that holds one, and the audit + * trail records digests (`sha256:…`) rather than handles — so it stays + * readable after the ciphertext is gone. + * + * **Best-effort, and deliberately after the repoint.** The write has already + * committed by the time this runs; a store that cannot delete (the port's + * `delete` is optional, so pre-existing fakes and the legacy inline-crypto + * path simply have none) or a delete that throws must never turn a + * SUCCESSFUL rotation into an error — the new secret is already in force, + * which is the property that matters. + */ + private async reapRotatedSecret(previousEnc: string | null, nextEnc: string | null): Promise { + if (!previousEnc || previousEnc === nextEnc) return; + // Handles only. The legacy inline-crypto path stores the ciphertext ITSELF + // in `value_enc`, and there is no `sys_secret` row to reap for it. + if (!previousEnc.startsWith('sec_')) return; + const del = this.secretStore?.delete; + if (!del) return; + try { + await del.call(this.secretStore, previousEnc); + } catch (err: any) { + // Loud, because the operator's mental model after a rotation is "the old + // credential is gone" and this is the one branch where it is not. + const message = + `[SettingsService] rotated secret '${previousEnc}' could not be deleted from ` + + `sys_secret — the rotation itself SUCCEEDED (the new value is in force), but the ` + + `previous ciphertext is still stored and remains decryptable. ` + + `Reason: ${err?.message ?? err}`; + if (this.logger?.error) this.logger.error(message); + else console.error(message); + } } private async materialiseRow(row: SettingsRow): Promise { diff --git a/packages/services/service-settings/src/settings-service.types.ts b/packages/services/service-settings/src/settings-service.types.ts index d9c09bb1e6..7fe763fe7f 100644 --- a/packages/services/service-settings/src/settings-service.types.ts +++ b/packages/services/service-settings/src/settings-service.types.ts @@ -92,6 +92,24 @@ export interface SettingsEngine { where: Record; data: Record; bypassTenantAudit?: boolean; + /** + * Execution context for the write, forwarded VERBATIM to the data + * engine's `options.context` (#8030). + * + * `SettingsService` sends `{ isSystem: true }` here for its own row + * writes, because `sys_setting.value_enc` and `updated_by` are declared + * `readonly: true` and the engine strips author-declared read-only + * columns from a NON-system caller's UPDATE payload + * (`stripReadonlyFields`). Without it a secret rotation inserts the new + * ciphertext, answers 200 with a correctly redacted body, and leaves + * `value_enc` pointing at the OLD handle — the rotated-away credential + * stays in force. + * + * ⛔ An adapter over `IDataEngine` MUST forward this. Dropping it + * restores the defect silently, with every visible signal still saying + * the write landed. + */ + context?: Record; }, ): Promise; delete?(objectName: string, opts: { where: Record }): Promise; @@ -147,6 +165,21 @@ export interface SettingsSecretStore { version?: number; ciphertext?: string; }): Promise; + /** + * Destroy the row a rotated-away handle names (#8030) — OPTIONAL. + * + * Called after a write has repointed `sys_setting.value_enc`, so the row + * being deleted is unreferenced by construction. The point is security, not + * housekeeping: a rotation exists to make the previous credential stop + * existing, and an orphan `sys_secret` row is a decryptable copy of exactly + * the value the admin retired — one more per rotation, forever. + * + * Optional so that a store which genuinely cannot delete (and every + * pre-existing test double) keeps working: absence means the orphans are + * accepted, and the rotation itself still lands. A throw is swallowed and + * reported — it must never turn a successful rotation into an error. + */ + delete?(id: string): Promise; } /** diff --git a/packages/services/service-settings/vitest.config.ts b/packages/services/service-settings/vitest.config.ts new file mode 100644 index 0000000000..ab6533eadf --- /dev/null +++ b/packages/services/service-settings/vitest.config.ts @@ -0,0 +1,48 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +// #8030 brought the first REAL-ENGINE test into this package +// (`settings-secret-rotation.test.ts`), and a real-engine test resolved +// through `exports` would read `@objectstack/objectql`'s **dist** — a build +// artifact. The whole point of that test is the engine's read-only strip +// (`stripReadonlyFields`, gated on `context.isSystem`); asserting it against a +// prebuilt copy would report on build state rather than on the source in the +// checkout, and the failure mode is a GREEN test that proves nothing +// (`scripts/check-test-source-alias.mjs` for the measured history). +// +// `@objectstack/core` is aliased for the same reason and by the same change: +// that test is the first one in this package to reach +// `settings-service-plugin.ts` (for the real `IDataEngine → SettingsEngine` +// adapter), which pulls `resolveAuthzContext` from core — so core became +// newly reachable from the package's tests and the gate named it. +// +// Only those two are aliased, deliberately. The package's other workspace +// imports (`@objectstack/platform-objects`, `@objectstack/spec`, +// `@objectstack/types`) are its registered set in that gate's +// `KNOWN_UNALIASED_TEST_IMPORTS`; the registry is audited for set EQUALITY in +// both directions, so aliasing them here is a separate, deliberate shrink and +// not a rider on a P0 security fix. +import { defineConfig } from 'vitest/config'; +import path from 'path'; + +export default defineConfig({ + test: { + globals: true, + environment: 'node', + }, + resolve: { + // Array form with an ANCHORED pattern, per the trap the gate documents: + // the object form matches by PREFIX, so a bare key whose replacement is a + // FILE also swallows every subpath and resolves it to `…/index.ts/` + // (`ENOTDIR`, at run time, in a config that looks right). + alias: [ + { + find: /^@objectstack\/objectql$/, + replacement: path.resolve(__dirname, '../../objectql/src/index.ts'), + }, + { + find: /^@objectstack\/core$/, + replacement: path.resolve(__dirname, '../../core/src/index.ts'), + }, + ], + }, +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a66e8677ea..54494d9c47 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2470,6 +2470,9 @@ importers: specifier: workspace:* version: link:../../types devDependencies: + '@objectstack/objectql': + specifier: workspace:* + version: link:../../objectql '@types/node': specifier: ^26.1.2 version: 26.1.2