fix(service-settings): repoint sys_setting.value_enc on a secret rotation, and reap the retired ciphertext (#8030)

.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.

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"

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,20 +397,24 @@ 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<string, unknown>;
  data: Record<string, unknown>;
  bypassTenantAudit?: boolean;
+ context?: Record<string, unknown>;
+ };
+ 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);
  }
  return eng.update(objectName, data, {
  where,
  multi: true,
- ...(driverOpts ?? {}),
+ ...driverOpts,
  });
  },
  };

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<void> {
+ /**
+ * 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<string | null> {
  if (this.engine) {
  const where: Record<string, unknown> = {
  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<void> {
+ 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<unknown> {

packages/services/service-settings/src/settings-service.types.ts

@@ -92,6 +92,24 @@ export interface SettingsEngine {
  where: Record<string, unknown>;
  data: Record<string, unknown>;
  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<string, unknown>;
  },
  ): Promise<any>;
  delete?(objectName: string, opts: { where: Record<string, unknown> }): Promise<any>;
@@ -147,6 +165,21 @@ export interface SettingsSecretStore {
  version?: number;
  ciphertext?: string;
  }): Promise<void>;
+ /**
+ * 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<void>;
 }
 
 /**

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/<sub>`
+ // (`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'),
+ },
+ ],
+ },
+});