Skip to content

Commit 72c0152

Browse files
fix(objectql): a secret field's sys_secret row is written only with its record, and released when the column lets go (#27572)
<!-- claude-projects-attribution: {"github_login":"os-sales"} --> _Requested by **Sales** · [project thread](https://claude.ai/code/project/chan_01D5bWFY3rFzgJcytD5gD4Uu?thread=cmsg_01D5bWFY3rFzgJcytD5gD4UuVpEmthvkRbuHGjC6oSQZvd)_ Fixes #27495 Clause-②: no Metadata-protocol: none Skills-docs: none — nothing a metadata author writes changes; the fix is inside the engine's credential write path, and the documented `os secret orphans` report is unchanged. ## 维护者速读 - 改了什么:`secret` 字段的 `sys_secret` 凭据行改为只随记录本身的写入一起落库;写入没完成时收回它已写的凭据行;更新把字段清为 `null` 或换成新值后,释放旧值的凭据行。 - 为什么改:凭据行原先在校验之前就写入,写入被拒也留下无人引用的密文;清空字段也不释放旧行。用户以为没存或已删的密文其实还留在库里。 - 风险与回滚:释放前先核实三件事:同一列没有任何行还引用这个句柄,句柄是这一列自己的,删除跟随记录所在的事务。核实不了就保留该行并记一条 error 日志,这是可恢复的方向。读出的打码、`resolveSecretField`、调用方收到的拒绝都不变;历史遗留的孤儿行不清理,仍由 `os secret orphans` 报告。回滚只需 revert 本 PR。 - 先看:`packages/objectql/src/engine-secret-internal-fields.ts`(风险在 `releaseSecretHandles` 的核实与事务判断),`packages/objectql/src/engine.ts`(insert / update 的写入点与两处释放点),`packages/objectql/src/secret-field-write-commit.test.ts`。 - 两分钟验证:`pnpm --filter @objectstack/objectql exec vitest run src/secret-field-write-commit.test.ts`,预期 21/21 通过。 - 需要你做的:无,席位落地。 ## What changes The credential pass for a `secret` field is split into three steps on the existing secret-fields path (`engine-secret-internal-fields.ts`, whose bodies `engine.ts` calls). - **Seal, before validation, as before.** `encryptSecretFields` still encrypts the value and puts the `secret:` ref on the row, so validation and the post-image permission check judge the stored reference exactly as they did. It no longer writes the `sys_secret` row; it hands the sealed row back. The fail-closed refusals (no crypto provider, no `sys_secret` store, the empty string) are unchanged and still fire at this point. - **Persist, with the record.** `persistSecretRows` writes the sealed rows immediately before the record's own driver statement, after every pass that can refuse the write: on insert, only for the rows `insertMany` keeps; on update, on both the by-id and the predicate branch. - **Release, verified.** `releaseSecretHandles` runs on two occasions. - `refused`: a write that did not complete (validation, an option outside the list, a required field, a hook, the permission check, the driver refusing the statement) takes back the rows it wrote. - `retired`: a committed update that clears a `secret` field with `null`, or replaces its value, releases the handle the field named before. The candidates come from the by-id pre-image, or from the predicate path's prior rows (the D7 memo when it was read, else one narrow `id` + secret-columns read). An echoed ref or an echoed mask releases nothing. It runs after the `afterUpdate` chain, so a refusal that rolls the record back never reaches it. - Each candidate is checked against storage before anything is deleted. No row of that column may still name the handle (a predicate update shares one handle across every row it matched). The `sys_secret` row's `namespace` / `key` must be this object and field. The delete rides the record's transaction: the explicit, still-open handle, else the ambient unit. If the record write rode a unit that does not cover `sys_secret`, the row is kept, because the record could still roll back and name it. - Any failure keeps the row and logs one `error` line that names the column, never the handle id, with the driver fault redacted, and points at `os secret orphans`. The caller's answer never changes. - **Not in this change.** Legacy orphans are not swept; they stay report-only, per #8259. Reads, `resolveSecret` / `resolveSecretField`, the mask round trip and every refusal envelope are unchanged. Deleting a record is not touched. ## Tests - `packages/objectql/src/secret-field-write-commit.test.ts` (new, 21 cases): a real engine over a recording driver with snapshot transactions. - A refused write leaves the `sys_secret` count unchanged: insert refused by a required field, an option outside the list, a `beforeInsert` hook, the post-image permission check, the driver refusing the statement, a batch statement, and a partial `insertMany`; update refused by an option outside the list, the permission check, the driver; a predicate update refused by an option outside the list and by the driver; an `afterInsert` abort. - A committed write still stores, masks and dereferences; an echoed mask releases nothing. - A `null` clear removes the old row; a replacement removes the old row and keeps the new one; a predicate clear removes every matched row's handle; a handle a predicate write shared outlives every row but the last; a handle minted for another column is never released through this one; a release that cannot confirm keeps the row and logs one line naming the column and not the handle. - `packages/qa/dogfood/test/secret-field-orphan-rows.dogfood.test.ts` (new, 5 cases): the same halves at the REST data door of a booted stack with a provider wired, counting `sys_secret` rows at the driver. ## Acceptance notes - The card asks for every refusal class. Three of them (a `before*` hook, the insert-side permission check, an `after*` abort) already wrote no row before this change, because they refuse before the credential pass or roll back with the record. Their cases were green on the unfixed tree and hold that order; the other classes were red there (15 of 21). - Replacing a value is the same mechanism as clearing it (the column lets go of the old handle), so both are released. - The claim's file surface names `secret-fields.ts` and the engine's write and clear steps. Since the engine split, those steps' bodies live in `engine-secret-internal-fields.ts`; `secret-fields.ts` itself needed no change. - This branch is the card thread's harness-pinned `claude/project-thread-r4cp59`; the claim on #27495 names it. ## Verification All readings below were taken at head `c0b6bfc7`, after `origin/main` (`d4ff1a02`) was merged in and the dogfood dependency closure was rebuilt, unless a line says otherwise. <details><summary>Unit, typecheck, lint and the REST door</summary> - Red first, on the unfixed tree: `secret-field-write-commit.test.ts` showed 15 failed / 6 passed (21), each failure on a `sys_secret` count. - `pnpm --filter @objectstack/objectql exec vitest run --maxWorkers=3`: Test Files 487 passed (487), Tests 9143 passed (9143). - `pnpm --filter @objectstack/objectql typecheck`: exit 0; `check:test-typecheck: OK`, 39 files / 233 errors / 64 pinned signatures held, unchanged. - `eslint` on the three changed `packages/objectql` files: exit 0. - `pnpm --filter @objectstack/dogfood exec vitest run --maxWorkers=2 test/secret-field-orphan-rows.dogfood.test.ts`: Tests 5 passed (5). - `pnpm --filter @objectstack/dogfood typecheck`: exit 0. `eslint` on the dogfood file: exit 0. - The full dogfood battery is not derived here: the title has no `!` and `Clause-②` has no `(narrowing)` arm. </details> <details><summary>Ablation (each through scripts/ablation-replace.mjs, restore proven)</summary> Source-resolved, against `secret-field-write-commit.test.ts`; every mutation landed (anchor 1 → 0) and every restore came back to the HEAD blob `163a38d28da2` with `git diff HEAD` empty. - The seal also writes the row (the old timing): 10 failed / 11 passed. Every refused-write case the fix moved went red. - The release's delete made a no-op: 8 failed / 13 passed (the clear, replace, predicate-clear and shared-handle cases, and the four driver-refusal take-backs). - The shared-handle check removed: 1 failed (the shared-handle case). - The column-ownership check removed: 1 failed (the foreign-column case). Dist-resolved, which the dogfood file consumes (taken at `33868e7d`; the one later commit touches only the unit test's double): each mutation carried a marker, `@objectstack/objectql` was rebuilt and `ablation-dist-preflight` found the marker in `dist/`. - The old write timing: 2 failed / 3 passed (both refused-write cases). - The release's delete made a no-op: 2 failed / 3 passed (the clear and replace cases). - Restore: rebuilt, `--absent` clean for both markers, the file 5 passed (5), and the tree clean against HEAD. </details> <details><summary>Local gate subset</summary> - `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` derived 67 commands at `c0b6bfc7`. All 67 were run, each exit code captured before any pipe: 67 × exit 0. `--ran` reconciliation: 67 derived, 67 run, 0 NOT-MEASURED, 0 UNRUN. - On the first pass, at `33868e7d`, two gates did not pass. `check:objectql-double-limit` flagged that the new test's recording double ignored the caller's `limit`; it now applies the bound after its filter (`c0b6bfc7`). `check:dual-build-cjs-loads` answered exit 3 (seven packages had no `dist/`); those were built, and both are exit 0 above. </details> <details><summary>Overlap with open PRs</summary> `git merge-tree` of this head against every open PR whose diff touches these files: #27477, #27541 and #27543 (all on `engine.ts`) merge clean. No other open PR touches them. This head also merges clean with the current `origin/main`. </details> The required CI families run on this PR's check-runs and are not re-run here. ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. (Not touched.) - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes on the changed files. - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: not touched. - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English (the 维护者速读 block is the dispatch's required Chinese summary). - [x] The *Before Submitting* list in CONTRIBUTING.md is done. --- _Generated by [Claude Code](https://claude.ai/code/session_01L6bhgXcN9HaZYSEUdJWEjw)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01L6bhgXcN9HaZYSEUdJWEjw --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f557d90 commit 72c0152

5 files changed

Lines changed: 834 additions & 33 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/objectql': patch
3+
---
4+
5+
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)
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
- **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.
11+
- **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.
12+
- **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‎

Lines changed: 168 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,19 +15,26 @@
1515
* the same ambient-transaction rule for the privileged reads. Each method's
1616
* docblock stays on the method in `engine.ts`, which is still the contract.
1717
*
18+
* [#27495] One later change, on the write side: `encryptSecretFields` seals a
19+
* value without writing its `sys_secret` row and hands the row back, and two
20+
* bodies joined it — `persistSecretRows` writes the sealed rows immediately
21+
* before the record's own driver write, and `releaseSecretHandles` removes a
22+
* handle once a write that did not land, or a committed one that moved the
23+
* column off it, has left it unreferenced.
24+
*
1825
* ## How the moved bodies reach the class
1926
*
2027
* The shape #23050 set for `rest-server.ts` (its `data-action-routes.ts`
2128
* header carries the full reasoning), applied per method as
22-
* `engine-realtime-publish.ts` applies it. The class keeps all nine methods —
29+
* `engine-realtime-publish.ts` applies it. The class keeps all eleven methods —
2330
* the published `.d.ts` lists each one's name and position — and each body is
2431
* one literal of exactly the {@link SecretInternalFieldsHost} members that
25-
* method reads, plus a `.call`. The three fields the bodies read
26-
* (`_registry`, `cryptoProvider`, `txStore`) go as GETTERS, so each read
27-
* happens when the body makes it: `setCryptoProvider` replaces the provider at
28-
* any time, and a body sees whatever is installed at the moment it reads, as
29-
* it did. The five methods they call (`getDriver`,
30-
* `transactionCoversDriverFor`, `omitInternalFields`,
32+
* method reads, plus a `.call`. The four fields the bodies read
33+
* (`_registry`, `cryptoProvider`, `txStore`, `logger`) go as GETTERS, so each
34+
* read happens when the body makes it: `setCryptoProvider` replaces the
35+
* provider at any time, and a body sees whatever is installed at the moment it
36+
* reads, as it did. The six methods they call (`getDriver`,
37+
* `transactionCoversDriverFor`, `isClosedUnitHandle`, `omitInternalFields`,
3138
* `privilegedReadDriverOptions`, `resolveSecret`) go as late-bound closures
3239
* over the instance, so a double or spy set on the engine — tests replace
3340
* `resolveInternalField` and `resolveSecretField` on the instance — is still
@@ -40,8 +47,9 @@
4047
*/
4148

4249
import type { CryptoHandle } from '@objectstack/spec/contracts';
50+
import type { DriverOptions } from '@objectstack/spec/data';
4351
import type { ExecutionContext } from '@objectstack/spec/kernel';
44-
import { redactPropagatedDriverFault } from '@objectstack/types';
52+
import { redactBoundStatement, redactPropagatedDriverFault } from '@objectstack/types';
4553
import {
4654
collectSecretFields,
4755
collectMaskedReadFields,
@@ -68,22 +76,45 @@ export interface SecretInternalFieldsHost {
6876
readonly _registry: ObjectQL['_registry'];
6977
readonly cryptoProvider: ObjectQL['cryptoProvider'];
7078
readonly txStore: ObjectQL['txStore'];
79+
readonly logger: ObjectQL['logger'];
7180
getDriver: ObjectQL['getDriver'];
7281
transactionCoversDriverFor: ObjectQL['transactionCoversDriverFor'];
82+
isClosedUnitHandle: ObjectQL['isClosedUnitHandle'];
7383
omitInternalFields: ObjectQL['omitInternalFields'];
7484
privilegedReadDriverOptions: ObjectQL['privilegedReadDriverOptions'];
7585
resolveSecret: ObjectQL['resolveSecret'];
7686
}
7787

88+
/**
89+
* [#27495] A `sys_secret` row the credential pass sealed for one record write
90+
* and has not written yet: the column it was minted for, the handle id the
91+
* row's ref now names, the row itself, and the driver options the pass was
92+
* handed. {@link persistSecretRows} writes it immediately before the record's
93+
* own driver write, so a write refused before that point leaves nothing behind.
94+
*/
95+
export interface PendingSecretRow {
96+
readonly field: string;
97+
readonly handleId: string;
98+
readonly row: Record<string, unknown>;
99+
readonly driverOptions: unknown;
100+
}
101+
102+
/** [#27495] A handle a write may have left unreferenced, and the column it was minted for. */
103+
export interface SecretHandleSlot {
104+
readonly field: string;
105+
readonly handleId: string;
106+
}
107+
78108
/** The body of `ObjectQL#encryptSecretFields`; the method's docblock there is its contract. */
79109
export async function encryptSecretFields(
80110
this: Pick<SecretInternalFieldsHost, '_registry' | 'cryptoProvider' | 'getDriver'>,
81111
object: string,
82112
row: Record<string, unknown>,
83113
context: ExecutionContext | undefined,
84114
driverOptions: unknown,
85-
): Promise<void> {
86-
if (!row || typeof row !== 'object') return;
115+
): Promise<PendingSecretRow[]> {
116+
const pending: PendingSecretRow[] = [];
117+
if (!row || typeof row !== 'object') return pending;
87118
const schema = this._registry.getObject(object);
88119

89120
// Echoed-mask drop for every field the read path masks (secret + generic
@@ -97,7 +128,7 @@ export async function encryptSecretFields(
97128
}
98129

99130
const secretFields = collectSecretFields(schema);
100-
if (secretFields.length === 0) return;
131+
if (secretFields.length === 0) return pending;
101132

102133
for (const field of secretFields) {
103134
if (!(field in row)) continue;
@@ -129,19 +160,25 @@ export async function encryptSecretFields(
129160
tenantId: context?.tenantId,
130161
});
131162

132-
let secretDriver;
163+
// Fail-closed on a missing store HERE, before validation, as before: the
164+
// row is only written later (#27495), but whether it could be is a verdict
165+
// on the composition, not on the record.
133166
try {
134-
secretDriver = this.getDriver('sys_secret');
167+
this.getDriver('sys_secret');
135168
} catch {
136169
throw new Error(
137170
`Cannot persist secret field "${object}.${field}": the sys_secret store is not available. `
138171
+ 'Ensure the platform-objects (sys_secret) are registered before writing secret fields (fail-closed).',
139172
);
140173
}
141174

142-
await secretDriver.create(
143-
'sys_secret',
144-
{
175+
// [#27495] Sealed, not written: the ref goes on the row now, so validation
176+
// and the post-image seam judge the stored reference as they always have,
177+
// and the row reaches `sys_secret` only with the record that names it.
178+
pending.push({
179+
field,
180+
handleId: handle.id,
181+
row: {
145182
id: handle.id,
146183
namespace: object,
147184
key: field,
@@ -151,11 +188,124 @@ export async function encryptSecretFields(
151188
ciphertext: handle.ciphertext,
152189
created_at: new Date().toISOString(),
153190
},
154-
driverOptions as any,
155-
);
191+
driverOptions,
192+
});
156193

157194
row[field] = makeSecretRef(handle.id);
158195
}
196+
return pending;
197+
}
198+
199+
/** [#27495] The `secret` fields a write payload carries a key for — the columns the write can move. */
200+
export function secretFieldsWritten(
201+
schema: Parameters<typeof collectSecretFields>[0],
202+
payload: unknown,
203+
): string[] {
204+
if (!payload || typeof payload !== 'object') return [];
205+
return collectSecretFields(schema).filter((field) => field in (payload as Record<string, unknown>));
206+
}
207+
208+
/**
209+
* [#27495] The handles a committed update moved its columns off: for each
210+
* `secret` field the payload carries, every prior row's handle the payload
211+
* does not keep — a `null` clear or a new value, never an echoed ref or a
212+
* dropped echoed mask. A candidate list, not a verdict: `releaseSecretHandles`
213+
* verifies each one against storage before anything is removed.
214+
*/
215+
export function retiredSecretSlots(
216+
schema: Parameters<typeof collectSecretFields>[0],
217+
priorRows: readonly (Record<string, unknown> | null | undefined)[],
218+
payload: unknown,
219+
): SecretHandleSlot[] {
220+
const out: SecretHandleSlot[] = [];
221+
for (const field of secretFieldsWritten(schema, payload)) {
222+
const next = (payload as Record<string, unknown>)[field];
223+
for (const prior of priorRows) {
224+
const handleId = parseSecretRef(prior?.[field]);
225+
if (handleId && next !== makeSecretRef(handleId)) out.push({ field, handleId });
226+
}
227+
}
228+
return out;
229+
}
230+
231+
/** The body of `ObjectQL#persistSecretRows`; the method's docblock there is its contract. */
232+
export async function persistSecretRows(
233+
this: Pick<SecretInternalFieldsHost, 'getDriver'>,
234+
pending: readonly PendingSecretRow[],
235+
written: PendingSecretRow[],
236+
): Promise<void> {
237+
for (const one of pending) {
238+
await this.getDriver('sys_secret').create('sys_secret', one.row, one.driverOptions as any);
239+
written.push(one);
240+
}
241+
}
242+
243+
/** The body of `ObjectQL#releaseSecretHandles`; the method's docblock there is its contract. */
244+
export async function releaseSecretHandles(
245+
this: Pick<SecretInternalFieldsHost, 'getDriver' | 'txStore' | 'transactionCoversDriverFor' | 'isClosedUnitHandle' | 'logger'>,
246+
object: string,
247+
slots: readonly SecretHandleSlot[],
248+
context: ExecutionContext | undefined,
249+
occasion: 'refused' | 'retired',
250+
): Promise<void> {
251+
if (slots.length === 0) return;
252+
const kept = (slot: SecretHandleSlot, why: string, error?: unknown): void => {
253+
// The column, never the handle id: a handle is what a stored ref names.
254+
const what = occasion === 'refused'
255+
? `the sys_secret row sealed for a write on "${object}.${slot.field}" that did not complete`
256+
: `the sys_secret row that "${object}.${slot.field}" named before this write`;
257+
const redacted = error === undefined ? undefined : redactBoundStatement(error);
258+
this.logger.error(
259+
`Credential row kept: ${what} was not removed, because ${why}. The write's own answer is unchanged, `
260+
+ 'but that ciphertext is still stored and decryptable with nothing pointing at it. '
261+
+ '`os secret orphans` reports it.',
262+
redacted instanceof Error ? redacted : undefined,
263+
{ object, field: slot.field },
264+
);
265+
};
266+
// The unit the record write rode, chosen as `buildDriverOptions` chooses it:
267+
// an explicit, still-open handle first, then the ambient one.
268+
const explicit = context?.transaction;
269+
const tx = explicit !== undefined && !this.isClosedUnitHandle(explicit)
270+
? explicit
271+
: this.txStore.getStore()?.transaction;
272+
const recordInUnit = tx !== undefined && this.transactionCoversDriverFor(object, tx);
273+
const storeInUnit = tx !== undefined && this.transactionCoversDriverFor('sys_secret', tx);
274+
const seen = new Set<string>();
275+
for (const slot of slots) {
276+
const dedupe = `${slot.field}:${slot.handleId}`;
277+
if (seen.has(dedupe)) continue;
278+
seen.add(dedupe);
279+
// A record write inside a unit the store is not part of can still roll
280+
// back after this delete, and would then name a row that is gone: keep
281+
// the orphan, the recoverable direction.
282+
if (recordInUnit && !storeInUnit) {
283+
kept(slot, 'the record write ran in a transaction that does not cover the sys_secret store, so the delete could outlive a rollback');
284+
continue;
285+
}
286+
try {
287+
// Verify, never infer: no row of the column may still name the handle —
288+
// a predicate write shares one handle across every row it matched.
289+
const recordOptions: DriverOptions | undefined = recordInUnit ? { transaction: tx } : undefined;
290+
const refs = await this.getDriver(object).count(
291+
object,
292+
{ where: { [slot.field]: makeSecretRef(slot.handleId) } },
293+
recordOptions,
294+
);
295+
if (refs > 0) continue;
296+
const storeOptions: DriverOptions | undefined = storeInUnit ? { transaction: tx } : undefined;
297+
const secretDriver = this.getDriver('sys_secret');
298+
const found = await secretDriver.find('sys_secret', { where: { id: slot.handleId } }, storeOptions);
299+
const secret: any = Array.isArray(found) ? found[0] : found;
300+
if (!secret) continue;
301+
// Only this column's own handle: a ref a system writer copied onto
302+
// another column says nothing about the column the handle was minted for.
303+
if (secret.namespace !== object || secret.key !== slot.field) continue;
304+
await secretDriver.delete('sys_secret', slot.handleId, storeOptions);
305+
} catch (e) {
306+
kept(slot, 'storage could not confirm the column let go of it, or could not delete it', e);
307+
}
308+
}
159309
}
160310

161311
/** The body of `ObjectQL#refuseEmptyPasswordFields`; the method's docblock there is its contract. */

0 commit comments

Comments
 (0)