fix(objectql): a secret field's sys_secret row is written only with its record, and released when the column lets go

.changeset/27495-secret-field-orphan-rows.md

@@ -0,0 +1,12 @@
+---
+'@objectstack/objectql': patch
+---
+
+fix(objectql): a `secret` field's `sys_secret` row is written only with the record that names it, and released when that record's column lets go of it (#27495)
+
+Clause-②: no
+Metadata-protocol: none
+
+- **Written with the record.** The credential pass still seals the value before validation, so the row carries its `secret:` ref when validation and the post-image permission check judge it. The `sys_secret` row itself is now written immediately before the record's own driver write, after every pass that can refuse the write. A create or update refused for any reason (validation, an option outside the list, a required field, a `before*` hook, the permission check) writes no credential row, a row `insertMany` culls writes none, and a statement the driver refuses takes back the row it was given.
+- **Released when the column lets go.** A committed update that clears a `secret` field with `null`, or replaces its value, removes the `sys_secret` row the field named before. The removal is verified first: no row of that column may still name the handle (a predicate update shares one handle across every row it matched), the handle must be that column's own, and the delete rides the record's transaction where one is open. A removal that cannot be confirmed keeps the row and logs one `error` line naming the column; `os secret orphans` reports such rows.
+- **Unchanged.** What a read returns (the mask), what `resolveSecret` / `resolveSecretField` return for a stored value, an echoed mask (still a no-op), and the refusal a caller receives. Credential rows left behind before this release are not swept; they stay report-only through `os secret orphans`.

packages/objectql/src/engine-secret-internal-fields.ts

@@ -15,19 +15,26 @@
  * the same ambient-transaction rule for the privileged reads. Each method's
  * docblock stays on the method in `engine.ts`, which is still the contract.
  *
+ * [#27495] One later change, on the write side: `encryptSecretFields` seals a
+ * value without writing its `sys_secret` row and hands the row back, and two
+ * bodies joined it — `persistSecretRows` writes the sealed rows immediately
+ * before the record's own driver write, and `releaseSecretHandles` removes a
+ * handle once a write that did not land, or a committed one that moved the
+ * column off it, has left it unreferenced.
+ *
  * ## How the moved bodies reach the class
  *
  * The shape #23050 set for `rest-server.ts` (its `data-action-routes.ts`
  * header carries the full reasoning), applied per method as
- * `engine-realtime-publish.ts` applies it. The class keeps all nine methods —
+ * `engine-realtime-publish.ts` applies it. The class keeps all eleven methods —
  * the published `.d.ts` lists each one's name and position — and each body is
  * one literal of exactly the {@link SecretInternalFieldsHost} members that
- * method reads, plus a `.call`. The three fields the bodies read
- * (`_registry`, `cryptoProvider`, `txStore`) go as GETTERS, so each read
- * happens when the body makes it: `setCryptoProvider` replaces the provider at
- * any time, and a body sees whatever is installed at the moment it reads, as
- * it did. The five methods they call (`getDriver`,
- * `transactionCoversDriverFor`, `omitInternalFields`,
+ * method reads, plus a `.call`. The four fields the bodies read
+ * (`_registry`, `cryptoProvider`, `txStore`, `logger`) go as GETTERS, so each
+ * read happens when the body makes it: `setCryptoProvider` replaces the
+ * provider at any time, and a body sees whatever is installed at the moment it
+ * reads, as it did. The six methods they call (`getDriver`,
+ * `transactionCoversDriverFor`, `isClosedUnitHandle`, `omitInternalFields`,
  * `privilegedReadDriverOptions`, `resolveSecret`) go as late-bound closures
  * over the instance, so a double or spy set on the engine — tests replace
  * `resolveInternalField` and `resolveSecretField` on the instance — is still
@@ -40,8 +47,9 @@
  */
 
 import type { CryptoHandle } from '@objectstack/spec/contracts';
+import type { DriverOptions } from '@objectstack/spec/data';
 import type { ExecutionContext } from '@objectstack/spec/kernel';
-import { redactPropagatedDriverFault } from '@objectstack/types';
+import { redactBoundStatement, redactPropagatedDriverFault } from '@objectstack/types';
 import {
   collectSecretFields,
   collectMaskedReadFields,
@@ -68,22 +76,45 @@ export interface SecretInternalFieldsHost {
   readonly _registry: ObjectQL['_registry'];
   readonly cryptoProvider: ObjectQL['cryptoProvider'];
   readonly txStore: ObjectQL['txStore'];
+  readonly logger: ObjectQL['logger'];
   getDriver: ObjectQL['getDriver'];
   transactionCoversDriverFor: ObjectQL['transactionCoversDriverFor'];
+  isClosedUnitHandle: ObjectQL['isClosedUnitHandle'];
   omitInternalFields: ObjectQL['omitInternalFields'];
   privilegedReadDriverOptions: ObjectQL['privilegedReadDriverOptions'];
   resolveSecret: ObjectQL['resolveSecret'];
 }
 
+/**
+ * [#27495] A `sys_secret` row the credential pass sealed for one record write
+ * and has not written yet: the column it was minted for, the handle id the
+ * row's ref now names, the row itself, and the driver options the pass was
+ * handed. {@link persistSecretRows} writes it immediately before the record's
+ * own driver write, so a write refused before that point leaves nothing behind.
+ */
+export interface PendingSecretRow {
+  readonly field: string;
+  readonly handleId: string;
+  readonly row: Record<string, unknown>;
+  readonly driverOptions: unknown;
+}
+
+/** [#27495] A handle a write may have left unreferenced, and the column it was minted for. */
+export interface SecretHandleSlot {
+  readonly field: string;
+  readonly handleId: string;
+}
+
 /** The body of `ObjectQL#encryptSecretFields`; the method's docblock there is its contract. */
 export async function encryptSecretFields(
   this: Pick<SecretInternalFieldsHost, '_registry' | 'cryptoProvider' | 'getDriver'>,
   object: string,
   row: Record<string, unknown>,
   context: ExecutionContext | undefined,
   driverOptions: unknown,
-): Promise<void> {
-  if (!row || typeof row !== 'object') return;
+): Promise<PendingSecretRow[]> {
+  const pending: PendingSecretRow[] = [];
+  if (!row || typeof row !== 'object') return pending;
   const schema = this._registry.getObject(object);
 
   // Echoed-mask drop for every field the read path masks (secret + generic
@@ -97,7 +128,7 @@ export async function encryptSecretFields(
   }
 
   const secretFields = collectSecretFields(schema);
-  if (secretFields.length === 0) return;
+  if (secretFields.length === 0) return pending;
 
   for (const field of secretFields) {
     if (!(field in row)) continue;
@@ -129,19 +160,25 @@ export async function encryptSecretFields(
       tenantId: context?.tenantId,
     });
 
-    let secretDriver;
+    // Fail-closed on a missing store HERE, before validation, as before: the
+    // row is only written later (#27495), but whether it could be is a verdict
+    // on the composition, not on the record.
     try {
-      secretDriver = this.getDriver('sys_secret');
+      this.getDriver('sys_secret');
     } catch {
       throw new Error(
         `Cannot persist secret field "${object}.${field}": the sys_secret store is not available. `
           + 'Ensure the platform-objects (sys_secret) are registered before writing secret fields (fail-closed).',
       );
     }
 
-    await secretDriver.create(
-      'sys_secret',
-      {
+    // [#27495] Sealed, not written: the ref goes on the row now, so validation
+    // and the post-image seam judge the stored reference as they always have,
+    // and the row reaches `sys_secret` only with the record that names it.
+    pending.push({
+      field,
+      handleId: handle.id,
+      row: {
         id: handle.id,
         namespace: object,
         key: field,
@@ -151,11 +188,124 @@ export async function encryptSecretFields(
         ciphertext: handle.ciphertext,
         created_at: new Date().toISOString(),
       },
-      driverOptions as any,
-    );
+      driverOptions,
+    });
 
     row[field] = makeSecretRef(handle.id);
   }
+  return pending;
+}
+
+/** [#27495] The `secret` fields a write payload carries a key for — the columns the write can move. */
+export function secretFieldsWritten(
+  schema: Parameters<typeof collectSecretFields>[0],
+  payload: unknown,
+): string[] {
+  if (!payload || typeof payload !== 'object') return [];
+  return collectSecretFields(schema).filter((field) => field in (payload as Record<string, unknown>));
+}
+
+/**
+ * [#27495] The handles a committed update moved its columns off: for each
+ * `secret` field the payload carries, every prior row's handle the payload
+ * does not keep — a `null` clear or a new value, never an echoed ref or a
+ * dropped echoed mask. A candidate list, not a verdict: `releaseSecretHandles`
+ * verifies each one against storage before anything is removed.
+ */
+export function retiredSecretSlots(
+  schema: Parameters<typeof collectSecretFields>[0],
+  priorRows: readonly (Record<string, unknown> | null | undefined)[],
+  payload: unknown,
+): SecretHandleSlot[] {
+  const out: SecretHandleSlot[] = [];
+  for (const field of secretFieldsWritten(schema, payload)) {
+    const next = (payload as Record<string, unknown>)[field];
+    for (const prior of priorRows) {
+      const handleId = parseSecretRef(prior?.[field]);
+      if (handleId && next !== makeSecretRef(handleId)) out.push({ field, handleId });
+    }
+  }
+  return out;
+}
+
+/** The body of `ObjectQL#persistSecretRows`; the method's docblock there is its contract. */
+export async function persistSecretRows(
+  this: Pick<SecretInternalFieldsHost, 'getDriver'>,
+  pending: readonly PendingSecretRow[],
+  written: PendingSecretRow[],
+): Promise<void> {
+  for (const one of pending) {
+    await this.getDriver('sys_secret').create('sys_secret', one.row, one.driverOptions as any);
+    written.push(one);
+  }
+}
+
+/** The body of `ObjectQL#releaseSecretHandles`; the method's docblock there is its contract. */
+export async function releaseSecretHandles(
+  this: Pick<SecretInternalFieldsHost, 'getDriver' | 'txStore' | 'transactionCoversDriverFor' | 'isClosedUnitHandle' | 'logger'>,
+  object: string,
+  slots: readonly SecretHandleSlot[],
+  context: ExecutionContext | undefined,
+  occasion: 'refused' | 'retired',
+): Promise<void> {
+  if (slots.length === 0) return;
+  const kept = (slot: SecretHandleSlot, why: string, error?: unknown): void => {
+    // The column, never the handle id: a handle is what a stored ref names.
+    const what = occasion === 'refused'
+      ? `the sys_secret row sealed for a write on "${object}.${slot.field}" that did not complete`
+      : `the sys_secret row that "${object}.${slot.field}" named before this write`;
+    const redacted = error === undefined ? undefined : redactBoundStatement(error);
+    this.logger.error(
+      `Credential row kept: ${what} was not removed, because ${why}. The write's own answer is unchanged, `
+        + 'but that ciphertext is still stored and decryptable with nothing pointing at it. '
+        + '`os secret orphans` reports it.',
+      redacted instanceof Error ? redacted : undefined,
+      { object, field: slot.field },
+    );
+  };
+  // The unit the record write rode, chosen as `buildDriverOptions` chooses it:
+  // an explicit, still-open handle first, then the ambient one.
+  const explicit = context?.transaction;
+  const tx = explicit !== undefined && !this.isClosedUnitHandle(explicit)
+    ? explicit
+    : this.txStore.getStore()?.transaction;
+  const recordInUnit = tx !== undefined && this.transactionCoversDriverFor(object, tx);
+  const storeInUnit = tx !== undefined && this.transactionCoversDriverFor('sys_secret', tx);
+  const seen = new Set<string>();
+  for (const slot of slots) {
+    const dedupe = `${slot.field}:${slot.handleId}`;
+    if (seen.has(dedupe)) continue;
+    seen.add(dedupe);
+    // A record write inside a unit the store is not part of can still roll
+    // back after this delete, and would then name a row that is gone: keep
+    // the orphan, the recoverable direction.
+    if (recordInUnit && !storeInUnit) {
+      kept(slot, 'the record write ran in a transaction that does not cover the sys_secret store, so the delete could outlive a rollback');
+      continue;
+    }
+    try {
+      // Verify, never infer: no row of the column may still name the handle —
+      // a predicate write shares one handle across every row it matched.
+      const recordOptions: DriverOptions | undefined = recordInUnit ? { transaction: tx } : undefined;
+      const refs = await this.getDriver(object).count(
+        object,
+        { where: { [slot.field]: makeSecretRef(slot.handleId) } },
+        recordOptions,
+      );
+      if (refs > 0) continue;
+      const storeOptions: DriverOptions | undefined = storeInUnit ? { transaction: tx } : undefined;
+      const secretDriver = this.getDriver('sys_secret');
+      const found = await secretDriver.find('sys_secret', { where: { id: slot.handleId } }, storeOptions);
+      const secret: any = Array.isArray(found) ? found[0] : found;
+      if (!secret) continue;
+      // Only this column's own handle: a ref a system writer copied onto
+      // another column says nothing about the column the handle was minted for.
+      if (secret.namespace !== object || secret.key !== slot.field) continue;
+      await secretDriver.delete('sys_secret', slot.handleId, storeOptions);
+    } catch (e) {
+      kept(slot, 'storage could not confirm the column let go of it, or could not delete it', e);
+    }
+  }
 }
 
 /** The body of `ObjectQL#refuseEmptyPasswordFields`; the method's docblock there is its contract. */