From db96543ca04ae2f324ddef1cf0a543d0bbf90529 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 14:06:44 +0000 Subject: [PATCH] fix(service-settings): repoint sys_setting.value_enc on a secret rotation, and reap the retired ciphertext (#8030) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, so a leaked credential survived the rotation meant to retire it, with every visible signal saying the rotation had landed. The first write of any secret was correct, so it 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 (`stripReadonlyFields`, gated on `!opCtx.context?.isSystem`). `SettingsService.upsertRow` persisted through a plain, un-elevated `engine.update`, so the handle could never be repointed. The INSERT path is deliberately exempt from that strip (#3413) — which is exactly why the first write landed and every later one did not. Fix, on the service side only: - `upsertRow` performs its UPDATE as a system write. `SettingsService` is a privileged writer — the manifest capability gate, the env/upper-scope lock pre-flight and `validatePatch` have all run by then, and these are columns it owns rather than ones a caller forged. Same posture, same reason, as `ObjectQL.recomputeSummaries`' elevation (#7673). - `SettingsEngine.update` gains a `context` member and the `IDataEngine` adapter forwards it on BOTH 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. - `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. A test fails if that flag is removed. Orphans are reaped rather than accepted: the `sys_secret` row a rotated-away handle named is deleted once the repoint has committed. An orphan is a decryptable copy of the credential the admin just retired, one more per rotation. The store's `delete` is optional and the call is best-effort — the new secret is already in force by then, and a failed cleanup is reported, never raised. Tests boot a real ObjectQL over the real `SysSetting` / `SysSecret` schemas and drive the real service through the real adapter, so the verdict is about the engine's actual strip rule rather than about a fake written to match the fix. `vitest.config.ts` aliases `@objectstack/objectql` and `@objectstack/core` to source, per `check-test-source-alias`. Fixes #8030 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AFizXytCQYaGCNvewL7TaE --- ...settings-rotation-repoint-secret-handle.md | 48 ++ .../services/service-settings/package.json | 1 + .../src/settings-secret-rotation.test.ts | 459 ++++++++++++++++++ .../src/settings-service-plugin.ts | 44 +- .../service-settings/src/settings-service.ts | 122 ++++- .../src/settings-service.types.ts | 33 ++ .../service-settings/vitest.config.ts | 48 ++ pnpm-lock.yaml | 3 + 8 files changed, 747 insertions(+), 11 deletions(-) create mode 100644 .changeset/settings-rotation-repoint-secret-handle.md create mode 100644 packages/services/service-settings/src/settings-secret-rotation.test.ts create mode 100644 packages/services/service-settings/vitest.config.ts 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