From e140b0b0b2b653dd89ca3e4765ab47fc9b1ce0dd Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 12:57:38 +0000 Subject: [PATCH 1/2] fix(settings): verify the repoint before reaping a rotated secret (#8262) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `reapRotatedSecret` deleted the handle `upsertRow` reported as `previousEnc` and inferred that the repoint had taken effect from `previousEnc !== nextEnc`. That inference fails on a `SettingsEngine` adapter that drops `context`: the readonly `value_enc` is stripped from the non-system UPDATE, the row keeps naming the old handle, and the reaper destroyed the ciphertext STILL IN FORCE — leaving a dangling `value_enc` that reads as empty, unrecoverably. Re-read the row and delete only once storage confirms it no longer names the handle. Refusals leave an orphan (recoverable) and are logged loudly, so a non-forwarding adapter now announces itself instead of silently losing values. The verification read sits behind every cheap guard and inside the reaper's existing "never fail the write" guarantee. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01MX1qcBzfwZb5wkRrJTNbhH --- ...reaper-verifies-repoint-before-deleting.md | 45 ++++ .../src/settings-secret-rotation.test.ts | 210 +++++++++++++++++- .../src/settings-service-plugin.ts | 10 +- .../service-settings/src/settings-service.ts | 209 ++++++++++++++--- .../src/settings-service.types.ts | 18 +- 5 files changed, 451 insertions(+), 41 deletions(-) create mode 100644 .changeset/reaper-verifies-repoint-before-deleting.md diff --git a/.changeset/reaper-verifies-repoint-before-deleting.md b/.changeset/reaper-verifies-repoint-before-deleting.md new file mode 100644 index 0000000000..5efbd0869a --- /dev/null +++ b/.changeset/reaper-verifies-repoint-before-deleting.md @@ -0,0 +1,45 @@ +--- +"@objectstack/service-settings": patch +--- + +fix(settings): the rotated-secret reaper verifies the repoint instead of inferring it (#8262) + +`SettingsService.reapRotatedSecret` deleted the `sys_secret` row that +`upsertRow` reported as `previousEnc`, and inferred that the repoint it was +cleaning up after had taken effect from `previousEnc !== nextEnc`. That +inference holds for the shipped adapter, which forwards +`context: { isSystem: true }`. It does not hold for an adapter that drops +`context` — the reader `SettingsEngine`'s own doc comment contemplates, and a +documented extension point rather than a mistake nobody makes. + +With `context` dropped, `sys_setting.value_enc` is `readonly: true` so the +UPDATE has it stripped, the row keeps naming the OLD handle, and the reaper +then deleted **the ciphertext still in force**: `materialiseRow` dereferenced a +dangling handle, got nothing, and the setting silently read as empty. That is +unrecoverable — the audit trail records digests, never handles or ciphertext, +so nothing can even name what was destroyed. Measured on the real engine over +the real `SysSetting` / `SysSecret` schemas, three writes gave `sys_secret` +`1 → 1 → 2` with `value_enc` pinned to a row that no longer existed. + +The reaper now re-reads the row after the write and deletes `previousEnc` only +once storage confirms the row no longer names it. The criterion is +`current !== previousEnc` rather than the narrower `current === nextEnc`: +under a concurrent rotation the row may already have moved on to a third +handle, where `previousEnc` is genuinely unreferenced and the narrower test +would leak the orphan the reaping exists to prevent. Both refuse the case that +matters. + +Every refusal branch (unreadable row, failed read, row still naming the +handle) leaves an orphan and logs — the recoverable direction, and the one an +orphan sweep can clean up; there is no recoverable direction on the other +side. The added read sits behind every cheap guard, so it is paid only where a +destructive delete would otherwise follow, and it is inside the same +best-effort guarantee as the delete: a rotation is never failed by it. + +Latent rather than live: no shipped path reaches this, because the shipped +adapter forwards `context`. The population at risk is third-party and custom +`SettingsEngine` adapter authors — who also had no discovery path, since the +warning on `SettingsEngine.update` still described only the pre-#8063 +consequence ("the rotated-away credential stays in force"). That warning now +states the real consequence, and a non-forwarding adapter announces itself in +the log instead of failing silently. diff --git a/packages/services/service-settings/src/settings-secret-rotation.test.ts b/packages/services/service-settings/src/settings-secret-rotation.test.ts index 00db9d9c35..9b65058353 100644 --- a/packages/services/service-settings/src/settings-secret-rotation.test.ts +++ b/packages/services/service-settings/src/settings-secret-rotation.test.ts @@ -66,14 +66,19 @@ */ import { describe, expect, it, vi } from 'vitest'; -import { ObjectQL } from '@objectstack/objectql'; +// `assertEngineUpdateDispatch` re-exports the shared producer-side predicate +// (`@objectstack/metadata-core` since #5619) through objectql, which is already +// a devDependency here and already aliased to SOURCE by `vitest.config.ts` — so +// the #8262 doubles below stay pinned to the real dispatch contract without +// adding a dependency or an alias. +import { assertEngineUpdateDispatch, 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'; +import type { SettingsEngine, SettingsSecretStore } from './settings-service.types.js'; // --------------------------------------------------------------------------- // Fixtures @@ -173,17 +178,87 @@ function makeMemoryDriver() { return { driver, rowsOf }; } +/** + * [#8262] Reproduces the adapter the `SettingsEngine` doc comment warns about: + * identical to `wrapEngineAsSettingsEngine` except that it DROPS `context` on + * the way to the engine. + * + * That single omission is the whole hazard — `sys_setting.value_enc` is + * declared `readonly: true` and the engine strips author-declared read-only + * columns from a NON-system caller's UPDATE (`stripReadonlyFields`, gated on + * `context.isSystem`), while the INSERT path is exempt (#3413). It is a + * documented extension point, so the population that reaches it is real: + * third-party adapter authors, who have no other discovery path. + */ +function wrapEngineDroppingContext(engine: any): SettingsEngine { + const real = wrapEngineAsSettingsEngine(engine); + return { + find: real.find.bind(real), + insert: real.insert.bind(real), + async update(objectName, opts) { + // Loose in exactly ONE dimension — `context` — and conformant in every + // other, or the row states it produces stop being evidence about the + // real engine. `multi: true` is passed unconditionally because that IS + // the settings adapter's contract (a scalar `where.id` outranks `multi` + // in the shared predicate, and the settings row write never has one). + assertEngineUpdateDispatch((opts as any)?.data ?? {}, { + where: (opts as any)?.where, + multi: true, + }); + const { context: _dropped, ...withoutContext } = opts as any; + return real.update(objectName, withoutContext); + }, + }; +} + +/** + * [#8262] Forwards `context` correctly, but makes the FIRST read after any + * update throw — which is exactly the reaper's post-write verification read. + * + * The constraint this exists to pin: `reapRotatedSecret` runs after the write + * has committed and is "never allowed to fail the write" (its call site says + * so). Adding a read to it must not turn a transient read failure into a + * failed rotation. + */ +function wrapEngineFailingPostUpdateReads(engine: any): SettingsEngine { + const real = wrapEngineAsSettingsEngine(engine); + let armed = false; + return { + async find(objectName, opts) { + if (armed) { + armed = false; + throw new Error('read replica offline'); + } + return real.find(objectName, opts); + }, + insert: real.insert.bind(real), + async update(objectName, opts) { + assertEngineUpdateDispatch((opts as any)?.data ?? {}, { + where: (opts as any)?.where, + multi: true, + }); + const res = await real.update(objectName, opts); + armed = true; + return res; + }, + }; +} + /** * 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. + * others; `withDelete: false` reproduces a store that cannot reap at all; + * `forwardContext: false` / `failVerificationRead` swap in the two #8262 + * adapters above. */ async function boot(opts: { secretStoreOverrides?: Partial; withDelete?: boolean; + forwardContext?: boolean; + failVerificationRead?: boolean; } = {}) { const engine = new ObjectQL(); const { driver, rowsOf } = makeMemoryDriver(); @@ -218,7 +293,11 @@ async function boot(opts: { const logged: string[] = []; const svc = new SettingsService({ env: {}, - engine: wrapEngineAsSettingsEngine(engine as any), + engine: opts.failVerificationRead + ? wrapEngineFailingPostUpdateReads(engine) + : opts.forwardContext === false + ? wrapEngineDroppingContext(engine) + : wrapEngineAsSettingsEngine(engine as any), cryptoProvider: new LocalCryptoProvider(), secretStore: { ...baseStore, ...(opts.secretStoreOverrides ?? {}) }, logger: { error: (m) => { logged.push(m); } }, @@ -460,3 +539,126 @@ describe('#8030 — wrapEngineAsSettingsEngine forwards the execution context', expect(calls[0][2]).toMatchObject({ bypassTenantAudit: true, context: { isSystem: true } }); }); }); + +// --------------------------------------------------------------------------- +// 8. #8262 — the reaper VERIFIES the repoint instead of inferring it +// --------------------------------------------------------------------------- + +/** + * #8262 — `reapRotatedSecret` deleted the handle `upsertRow` reported as + * `previousEnc` without ever confirming the repoint it was cleaning up after + * had taken effect; it inferred that from `previousEnc !== nextEnc`. + * + * That inference holds for the shipped adapter (which forwards + * `context: { isSystem: true }`) and fails for one that drops it — the reader + * `SettingsEngine`'s own doc comment contemplates. With `context` dropped the + * UPDATE has `value_enc` stripped, so the row still names `previousEnc`, and + * the reaper deleted **the ciphertext still in force**: `materialiseRow` + * dereferences a dangling handle, gets nothing, and the setting silently reads + * empty. Unrecoverable — the audit trail records digests, never handles. + * + * Before the reaper existed the same adapter bug was non-destructive (the + * rotated-away credential merely stayed in force). These cases pin that the + * failure mode is back to recoverable, and now LOUD rather than silent. + * + * ⚠️ Direction of the counterfactual: on the pre-fix source, `three writes` + * below reads `[1, 1, 2]` and the value reads back `null`. The fix does not + * make a context-dropping adapter correct — nothing at this layer can, the + * repoint is stripped one layer down — it makes the failure survivable. + * Sections 5 and 6 are the other half of the pin: with `context` forwarded, + * the verification passes and reaping still happens on every rotation, so a + * fix that simply stopped reaping would go red there. + */ +describe('#8262 — the reaper never deletes the ciphertext that is still in force', () => { + it('a context-dropping adapter keeps the in-force ciphertext, and the setting still reads', async () => { + const { svc, settingRow, secretRows } = await boot({ forwardContext: false }); + + await svc.set('sms', 'twilio_auth_token', 'alpha'); + const handleA = settingRow()?.value_enc as string; + expect(handleA).toMatch(/^sec_/); + expect(secretRows()).toHaveLength(1); + + // The second write is where the defect lived: the repoint is stripped, so + // `value_enc` still names `handleA`, while `upsertRow` reports it as the + // handle rotated AWAY from. + await svc.set('sms', 'twilio_auth_token', 'beta'); + + // The strip itself is NOT this card's subject and is unchanged: the row + // still names the old handle. What must never happen is the deletion. + expect(settingRow()?.value_enc).toBe(handleA); + + // ⛔ THE assertion. Pre-fix this row was gone and `value_enc` dangled. + expect(secretRows().some((r) => r.id === handleA)).toBe(true); + + // …and the consequence that makes it data loss rather than a stale value: + // pre-fix this read returned `null` with the credential unrecoverable. + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('alpha'); + }); + + it('three writes leave the recoverable pre-reaper shape (1→2→3), not the destructive one (1→1→2)', async () => { + const { svc, settingRow, secretRows } = await boot({ forwardContext: false }); + + await svc.set('sms', 'twilio_auth_token', 'tok-1'); + const pinned = settingRow()?.value_enc as string; + const afterFirst = secretRows().length; + + await svc.set('sms', 'twilio_auth_token', 'tok-2'); + const afterSecond = secretRows().length; + + await svc.set('sms', 'twilio_auth_token', 'tok-3'); + const afterThird = secretRows().length; + + // The card's table, inverted. `[1, 1, 2]` is the destructive shape: the + // second write deleted the row the setting pointed at. + expect([afterFirst, afterSecond, afterThird]).toEqual([1, 2, 3]); + expect(settingRow()?.value_enc).toBe(pinned); + expect(secretRows().some((r) => r.id === pinned)).toBe(true); + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('tok-1'); + }); + + it('says so LOUDLY — the failure the adapter doc calls silent now names itself', async () => { + const { svc, logged } = await boot({ forwardContext: false }); + await svc.set('sms', 'twilio_auth_token', 'alpha'); + await svc.set('sms', 'twilio_auth_token', 'beta'); + + const out = logged.join('\n'); + // The operator's question is "did my rotation happen?", so the message has + // to answer that, name the row, and name the cause. + expect(out).toMatch(/did NOT take effect/); + expect(out).toMatch(/sms\.twilio_auth_token/); + expect(out).toMatch(/context/); + // It must be a refusal, not a report of something already destroyed. + expect(out).toMatch(/REFUSED to delete/); + }); + + it('a verification read that THROWS still leaves the rotation landed (never fails the write)', async () => { + const { svc, settingRow, secretRows, logged } = await boot({ failVerificationRead: true }); + + await svc.set('sms', 'twilio_auth_token', 'alpha'); + const handleA = settingRow()?.value_enc as string; + + // The write itself must survive a reaper that cannot verify — the call + // site's "never allowed to fail the write" constraint covers the read the + // fix added, not just the delete. + await expect(svc.set('sms', 'twilio_auth_token', 'beta')).resolves.toBeDefined(); + + // The rotation landed (context IS forwarded here) … + const handleB = settingRow()?.value_enc as string; + expect(handleB).not.toBe(handleA); + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('beta'); + + // … and the unverifiable handle was left alone rather than destroyed on a + // guess. An orphan is recoverable; a deleted in-force ciphertext is not. + expect(secretRows().some((r) => r.id === handleA)).toBe(true); + expect(logged.join('\n')).toMatch(/could not confirm/); + }); + + it('a store with no delete is unaffected — no verification read is issued at all', async () => { + // The verification read costs one I/O and must only be paid where a + // destructive delete would otherwise follow. + const { svc, secretRows } = await boot({ withDelete: false, forwardContext: false }); + await svc.set('sms', 'twilio_auth_token', 'alpha'); + await svc.set('sms', 'twilio_auth_token', 'beta'); + expect(secretRows()).toHaveLength(2); + }); +}); diff --git a/packages/services/service-settings/src/settings-service-plugin.ts b/packages/services/service-settings/src/settings-service-plugin.ts index 1102afb4d5..5a0b715a31 100644 --- a/packages/services/service-settings/src/settings-service-plugin.ts +++ b/packages/services/service-settings/src/settings-service-plugin.ts @@ -318,10 +318,12 @@ export class SettingsServicePlugin implements Plugin { ); }, 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. + // [#8030] The rotated-away ciphertext. By the time this runs the + // caller has re-read `sys_setting.value_enc` and CONFIRMED it no + // longer names this handle ([#8262] — it used to infer that), so the + // row is genuinely unreferenced — see `SettingsService.reapRotatedSecret` + // for why leaving it is a security problem rather than untidiness, and + // why the confirmation is not optional. // // System-elevated for the same reason the settings row update is: // `sys_secret` is a platform-owned table and this is the platform diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index fb851625e6..30e8fc44fb 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -1431,7 +1431,9 @@ export class SettingsService { } } - const previousEnc = await this.upsertRow({ + // Hoisted so the reap below can re-read the SAME row this write targeted + // (#8262) — the composite key is what makes the verification meaningful. + const rowForKey: SettingsRow = { namespace, key, scope, @@ -1441,14 +1443,18 @@ export class SettingsService { encrypted: isEncrypted, updated_at: new Date().toISOString(), 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); + }; + const previousEnc = await this.upsertRow(rowForKey); + + // Destroy the ciphertext the row USED to name (#8030) — but only once a + // re-read confirms the row really stopped naming it (#8262); "a new + // ciphertext was written" is not evidence that the repoint landed. + // Ordered after the repoint on purpose, and never allowed to fail the + // write — a guarantee that now covers the verification read as well as + // the delete; 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(rowForKey, previousEnc, storedEnc); if (this.audit) { try { @@ -2001,18 +2007,57 @@ export class SettingsService { * 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 = { + /** + * The composite key identifying ONE settings row, in the shape the engine + * path needs. + * + * Extracted rather than repeated (#8262): `reapRotatedSecret`'s verification + * read has to target **exactly** the row `upsertRow` just wrote. A re-read + * aimed at a slightly different row would answer a different question while + * looking correct — and the answer decides whether a ciphertext is deleted. + */ + private rowIdentity(row: SettingsRow): { + where: Record; + bypass: Record; + } { + return { + where: { namespace: row.namespace, key: row.key, scope: row.scope, user_id: row.user_id ?? null, - }; + }, // global rows are platform-wide — bypass the tenant audit warning // (we intentionally write tenant_id=null). tenant/user rows still // benefit from the warning when ctx.tenantId is missing. - const bypass = row.scope === 'global' ? { bypassTenantAudit: true } : {}; + bypass: row.scope === 'global' ? { bypassTenantAudit: true } : {}, + }; + } + + /** The in-memory store's index for the same composite key. */ + private memoryIndexOf(row: SettingsRow): number { + return this.memory.findIndex( + (r) => + r.namespace === row.namespace && + r.key === row.key && + r.scope === row.scope && + (r.user_id ?? null) === (row.user_id ?? null), + ); + } + + /** + * `value_enc` as a handle-or-nothing. One normalisation for both the value + * `upsertRow` REPORTS and the value `reapRotatedSecret` COMPARES it against + * (#8262) — two spellings of "no handle" (`''` vs `null`) diverging across + * those two call sites would make the comparison decide wrongly. + */ + private static handleOf(value: unknown): string | null { + return typeof value === 'string' && value !== '' ? value : null; + } + + private async upsertRow(row: SettingsRow): Promise { + if (this.engine) { + const { where, bypass } = this.rowIdentity(row); const existing = await this.engine.find(this.objectName, { where, limit: 1, @@ -2026,22 +2071,16 @@ export class SettingsService { context: SETTINGS_SYSTEM_WRITE_CONTEXT, ...bypass, } as any); - return typeof previousEnc === 'string' && previousEnc !== '' ? previousEnc : null; + return SettingsService.handleOf(previousEnc); } await this.engine.insert(this.objectName, { ...row }, bypass as any); return null; } - const idx = this.memory.findIndex( - (r) => - r.namespace === row.namespace && - r.key === row.key && - r.scope === row.scope && - (r.user_id ?? null) === (row.user_id ?? null), - ); + const idx = this.memoryIndexOf(row); if (idx >= 0) { const previousEnc = this.memory[idx].value_enc; this.memory[idx] = row; - return typeof previousEnc === 'string' && previousEnc !== '' ? previousEnc : null; + return SettingsService.handleOf(previousEnc); } this.memory.push(row); return null; @@ -2070,27 +2109,137 @@ export class SettingsService { * 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. + * + * ## VERIFY the repoint; never infer it (#8262) + * + * `previousEnc !== nextEnc` says a new ciphertext was written. It does NOT + * say the row stopped pointing at the old one — and the two come apart on a + * `SettingsEngine` adapter that drops `context`: `value_enc` is + * `readonly: true`, so a non-system UPDATE has it stripped + * (`stripReadonlyFields`), the row keeps naming `previousEnc`, and deleting + * on the inference destroys **the ciphertext still in force**. + * `materialiseRow` then dereferences a dangling handle, gets nothing, and + * the setting silently reads empty — unrecoverably, because the audit trail + * records digests rather than handles, so nothing can even name what was + * lost. That adapter is a documented extension point (see the ⛔ note on + * `SettingsEngine.update`), which is exactly why the reaper cannot assume + * the ideal one. + * + * So: re-read the row and delete only once storage confirms it no longer + * names the handle. The criterion is `current !== previousEnc` rather than + * the narrower `current === nextEnc`, deliberately — under a concurrent + * rotation the row may already have moved on to a THIRD handle, in which + * case `previousEnc` is genuinely unreferenced and the narrower test would + * leak the orphan #8030 exists to prevent. Both refuse the case that + * matters, where the row still names `previousEnc`. + * + * ⚠️ Every refusal branch leaves an ORPHAN, which is the recoverable + * direction and the one #8103 sweeps. There is no recoverable direction on + * the other side. And the added read is inside the same best-effort + * guarantee as the delete: it runs after all cheap guards, only where a + * destructive delete would otherwise follow, and can never fail the write. */ - private async reapRotatedSecret(previousEnc: string | null, nextEnc: string | null): Promise { + private async reapRotatedSecret( + row: SettingsRow, + 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; + + // ── Verification read. Ordered after every cheap guard above so a rotation + // that cannot reap anything never pays for it. ──────────────────────────── + let current: { found: boolean; handle: string | null }; + try { + current = await this.readStoredHandle(row); + } catch (err: any) { + this.reportReapRefusal( + `[SettingsService] could not confirm the secret rotation of '${row.namespace}.${row.key}' ` + + `took effect, so the previous ciphertext '${previousEnc}' was LEFT IN PLACE rather than ` + + `deleted. The rotation itself SUCCEEDED; that ciphertext is still stored and remains ` + + `decryptable. Reason: ${err?.message ?? err}`, + ); + return; + } + if (!current.found) { + this.reportReapRefusal( + `[SettingsService] could not confirm the secret rotation of '${row.namespace}.${row.key}' ` + + `took effect — the row could not be read back after the write — so the previous ` + + `ciphertext '${previousEnc}' was LEFT IN PLACE rather than deleted. It is still stored ` + + `and remains decryptable.`, + ); + return; + } + if (current.handle === previousEnc) { + this.reportReapRefusal( + `[SettingsService] REFUSED to delete rotated secret '${previousEnc}' for ` + + `'${row.namespace}.${row.key}': the rotation did NOT take effect. The stored row still ` + + `names that handle, so it is the ciphertext currently IN FORCE and deleting it would ` + + `destroy the value. The newly written ciphertext (${nextEnc ?? 'none'}) is unreferenced. ` + + `Cause: the SettingsEngine adapter in use is not forwarding the execution context — ` + + `sys_setting.value_enc is declared readonly, and the engine strips it from a NON-system ` + + `UPDATE. Fix the adapter to forward context verbatim, then re-apply the value: as far ` + + `as storage is concerned this rotation never happened.`, + ); + 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 = + this.reportReapRefusal( `[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); + `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}`, + ); + } + } + + /** + * Re-read the row's CURRENT `value_enc` straight from storage (#8262). + * + * `found: false` (the row could not be read back at all) is deliberately + * distinct from `handle: null` (the row exists and holds no handle): the two + * lead to OPPOSITE decisions in `reapRotatedSecret` — a reset-to-null must + * still reap, an unreadable row must not. + */ + private async readStoredHandle( + row: SettingsRow, + ): Promise<{ found: boolean; handle: string | null }> { + if (this.engine) { + const { where, bypass } = this.rowIdentity(row); + const rows = await this.engine.find(this.objectName, { + where, + limit: 1, + ...bypass, + } as any); + const current = Array.isArray(rows) ? rows[0] : undefined; + if (!current) return { found: false, handle: null }; + return { + found: true, + handle: SettingsService.handleOf((current as { value_enc?: unknown }).value_enc), + }; } + const idx = this.memoryIndexOf(row); + if (idx < 0) return { found: false, handle: null }; + return { found: true, handle: SettingsService.handleOf(this.memory[idx].value_enc) }; + } + + /** + * One channel for every branch in which a retired ciphertext outlives the + * rotation. Loud on purpose: the operator's mental model afterwards is "the + * old credential is gone", and these are the branches where it is not. + */ + private reportReapRefusal(message: string): void { + 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 f4aad13e0a..b5ec27b54e 100644 --- a/packages/services/service-settings/src/settings-service.types.ts +++ b/packages/services/service-settings/src/settings-service.types.ts @@ -118,11 +118,23 @@ export interface SettingsEngine { * (`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. + * stays in force, and the ciphertext just written is orphaned. * * ⛔ An adapter over `IDataEngine` MUST forward this. Dropping it - * restores the defect silently, with every visible signal still saying - * the write landed. + * restores the defect, and every response-visible signal still says the + * write landed: the ONLY thing that reports it is a server-side error + * from `SettingsService.reapRotatedSecret`, which re-reads the row, + * sees it still naming the old handle, and refuses to reap (#8262). + * + * ⚠️ Between #8063 and #8262 the consequence was worse than the + * paragraph above, and an adapter written against that window's docs is + * exposed to it: the reaper INFERRED the repoint from + * `previousEnc !== nextEnc` and deleted the handle the row still named + * — destroying the credential in force and leaving a dangling + * `value_enc` that reads as empty, unrecoverably (the audit trail + * records digests, never handles or ciphertext). The reaper verifies + * rather than infers now, so this failure is recoverable again. That is + * not a licence to drop `context`: your rotations still do not happen. */ context?: Record; }, From 9038c12443dd7f204de1f2700fc939b46e6cfe7f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 13:02:50 +0000 Subject: [PATCH 2/2] refactor(settings): keep the verification read on the declared options type (#8262) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rowIdentity`'s `bypass` is typed `{ bypassTenantAudit?: true }` rather than `Record`, so the reaper's re-read spreads into a `SettingsEngine.find` options object without an `as any`. The query-options-erasure ratchet caught the erasure as a new site — correctly: the verification read's whole value is that it goes through the declared contract. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01MX1qcBzfwZb5wkRrJTNbhH --- .../services/service-settings/src/settings-service.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index 30e8fc44fb..5998e8a1be 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -2018,7 +2018,10 @@ export class SettingsService { */ private rowIdentity(row: SettingsRow): { where: Record; - bypass: Record; + // Narrow on purpose: spread into a `SettingsEngine.find` options object it + // has to stay assignable to the DECLARED option type, so the verification + // read below needs no `as any` erasure of the contract it depends on. + bypass: { bypassTenantAudit?: true }; } { return { where: { @@ -2215,11 +2218,7 @@ export class SettingsService { ): Promise<{ found: boolean; handle: string | null }> { if (this.engine) { const { where, bypass } = this.rowIdentity(row); - const rows = await this.engine.find(this.objectName, { - where, - limit: 1, - ...bypass, - } as any); + const rows = await this.engine.find(this.objectName, { where, limit: 1, ...bypass }); const current = Array.isArray(rows) ? rows[0] : undefined; if (!current) return { found: false, handle: null }; return {