Skip to content

Commit cad764d

Browse files
fix(objectql)!: an update compares a value the caller sends only against the stored row as the caller reads it (#31063)
security#1 · os-tesla · session_01L9qibvynRbCtMndW8EHNGc Clause-②: yes (narrowing) — a non-system update that sends a `secret:` reference is refused even when the record already holds it, and a non-system update naming the master-detail FK of a detail with a parent-scoped `requiredWhen`, in a FK column it may edit but not read, no longer lets a row that already lacks a required value rest under the header it names. Metadata-protocol: none ## 维护者速读 - 改了什么:update 时把调用方送来的值与存储行比较的两处(secret 引用保留、master-detail 改挂时的前置检查读取旧表头),都改成只按调用方自己的读门所见来比较;系统写不变。 - 为什么改:对一个调用方能写不能读的列,update 的答复不应随该列的存储值而变;此前这两处直接比较未投影的存储行。 - 风险与回滚:收窄。非系统写送回 `secret:` 引用一律 400(含记录自己的引用);FK 不可读者点名表头时,缺必填值的遗留行不再“留存”。回滚即还原两处 engine 代码。 - 先看:`packages/objectql/src/engine.ts` 按 id 与批量两处 `priorMasterIdsCallerReads` 调用;`packages/objectql/src/engine-secret-internal-fields.ts` 的保留分支;`packages/objectql/src/engine-master-detail-related.ts` 新增的 `priorMasterIdsCallerReads` 与预览 `previousParents` 的同步。 - 两分钟验证:`pnpm --filter @objectstack/plugin-security exec vitest run src/reference-forgiveness-readable-column.test.ts`,预期 44 个全绿。 - 需要你做的:无,席位落地。 ## What changes An update compares a value the caller sends with the stored row in two more places than the reference check, and both now read the stored row only as the caller's own read door serves it (`storedReferencesCallerReads`, the one projection the reference check already uses), by id and in bulk. A system write is unchanged at both. - **`secret` reference keep** (`encryptSecretFields`). A non-system write keeps a `secret:` reference only where every row it lands on serves exactly that reference to the caller's read door. The read path serves a `secret` column masked to every caller, so no reference is kept: every non-system reference is refused `400 VALIDATION_ERROR`, one answer whatever it names and whatever the column holds. Before, the record's own stored reference was accepted and kept. The echoed mask, a new value and `null` behave as before; the system principal keeps the pass-through. The refusal sentence now says a reference is kept only where the caller reads it in the column, and names omitting the field or sending back the mask as the way to leave it unchanged. - **Master-detail repoint pre-check** (`update`, by id and bulk). On a detail with a parent-scoped `requiredWhen`, when the write names the FK, the header the ADR-0113 pre-check reads as `previousParent` comes from the stored FK only as the caller's read door serves it (`priorMasterIdsCallerReads`). A FK the caller cannot read as stored leaves the prior header unknown, bound as the unresolved header, so whether a second header is read, and which, does not depend on the stored value. A FK the caller reads is judged as before, and a write that does not name the FK reads nothing more. - **The validate preview** (`resolvePreviewParents`, reached only from `validate()`, whose stored rows are already projected through `servedAsStored`) now binds the prior header the way the write does: when the patch names the FK over a stored FK the caller was not served as stored, `previousParents[i]` is the unresolved header (`null`). Preview and write now give one verdict for that caller, whatever header the row hung off. - `content/docs/permissions/system-context.mdx` row 23e states the new answer. ## Tests - `packages/plugins/plugin-security/src/reference-forgiveness-readable-column.test.ts`, two new top-level describes on the real security plugin: - pre-check: with the FK `{ readable: false, editable: true }`, naming a header answers identically whether the line hangs off that header or another, by id and in bulk, and nothing is written; controls: an edit that does not name the FK lands, a caller who reads the FK keeps the legacy rest and the repoint refusal, a system write is judged as before. - secret reference: the row's own reference, another row's and one naming nothing get one answer, by id and in bulk, for a caller who can and who cannot read the column; controls: the mask echo and a new value, and the system pass-through. - `packages/objectql/src/secret-field-own-ref-write.test.ts` pinned the old keep of a record's own reference. It now pins the rule above: the record's own reference is refused exactly as another's, by id and on a predicate update, and a `beforeUpdate` hook that spreads the stored row back is judged on the reference it carries (one that omits the `secret` column lands). - `packages/qa/dogfood/test/secret-field-own-ref-write.dogfood.test.ts` pinned the same keep over REST and now pins the same refusal. - `packages/objectql/src/validate-preview-parent.test.ts`, a new top-level describe: for a caller not served the FK, preview and write give the same `required` verdict whatever header the line hangs off; control: a caller served the FK keeps the legacy rest and the repoint refusal, in both. ## Acceptance notes - No shipped flow, seed, example or console path sends a `secret:` reference from a non-system write: every in-repo writer of a `secret` column (webhooks, SSO client secret, connector connections and credentials, flow credentials) writes under a system context, the CLI ownership apply works below the engine, and no shipped hook writes `ctx.previous` back into an update payload. The one in-repo reliance was the two suites above. - A third-party `beforeUpdate` hook that spreads `ctx.previous` into the payload on an object with a `secret` field is now refused for a non-system caller; the changeset names the fix (omit the `secret` columns). - The controlled-by-parent write gate (plugin-security, ADR-0055 §3) authorizes a detail write against the master its stored row hangs off, read as stored. That is the authorization model itself, not a comparison of a sent value with the stored row, so this PR does not change it. - On a driver that leaves an unset FK out of the stored row entirely, a non-system caller who names the FK now has the prior header bound as unresolved, as for a hidden FK; a driver that stores the FK as `null` is judged as before. ## Verification <details><summary>Ablations (each committed first, mutated through scripts/ablation-replace.mjs, restored to HEAD and verified empty)</summary> - Secret keep compared against the stored rows as read: objectql own-reference suite 4 failed / 16 passed; security suite 4 failed / 40 passed (by id and bulk, for both grants). Restored: blob equals HEAD, `git diff HEAD` empty. - By-id prior FK read off the stored row: security suite 1 failed / 43 passed (the by-id hidden-FK pin: the line resting under the stored header was written). Restored as above. - Bulk prior FK read off the stored rows: security suite 1 failed / 43 passed (the bulk hidden-FK pin). Restored as above. - Preview prior header bound as before (no unknown arm): preview pin file 1 failed / 23 passed (the preview admitted what the write refused). Restored as above. </details> <details><summary>Suites and gates</summary> Code readings are on `f49cf38154`; the final head `82a9a6bbda` adds only one changeset sentence, and the changeset gates were re-run on it. - `pnpm --filter @objectstack/objectql test`: Test Files 563 passed (563), Tests 11361 passed (11361). `typecheck` and `check:test-typecheck` OK. - `pnpm --filter @objectstack/plugin-security test`: Test Files 248 passed (248), Tests 5773 passed, 45 skipped. `typecheck` and `check:test-typecheck` OK. - Full dogfood battery, `pnpm --filter @objectstack/dogfood test` under the verify lock: VERDICT command-exit 0, Test Files 430 passed, 3 skipped (433); Tests 3572 passed, 41 skipped (3613). - Derived gates (`dispatch-gates --commands`, 10 paths): 97 derived, 95 exit 0. `check:type-check-debt` first exited 3 (prerequisite) and passed on a rerun after its own full build. Two stay NOT MEASURED locally, exit 3, prerequisite not met (a full build): `spec check:skill-examples` and `check:dual-build-cjs-loads`; CI runs both. `dispatch-gates --ran` with exit codes: 97 accounted, 0 unrun. - Re-run on `82a9a6bbda`: `check-adr-0087-registration` (the changeset reads as breaking, clause-② narrowing, not-required no-migration-prescription), `check-changeset-no-major`, `check-empty-changeset`, `check:nul-bytes`, `check:doc-authoring`, all exit 0. </details> --- _Generated by [Claude Code](https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 23cf11a commit cad764d

10 files changed

Lines changed: 508 additions & 68 deletions
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
'@objectstack/objectql': patch
3+
---
4+
5+
fix(objectql)!: an update compares a value the caller sends only against the stored row as the caller's own read door serves it — a `secret` reference and a master-detail repoint
6+
7+
Clause-②: yes (narrowing) — a non-system update that sends a `secret:` reference is refused even when the record already holds it, and a non-system update naming the master-detail FK of a detail with a parent-scoped `requiredWhen`, in a FK column it may edit but not read, no longer lets a row that already lacks a required value rest under the header it names.
8+
Metadata-protocol: none
9+
10+
<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped, and no export or type changes. Both refusals are answers the engine already gives — VALIDATION_ERROR for a secret reference, the field's required finding for requiredWhen — so there is nothing for a migration to rewrite and no tombstone. -->
11+
12+
**BREAKING** accept-set narrowing at two update seams of `@objectstack/objectql`, served by every write door, by id and in bulk alike. A system write is unchanged at both.
13+
14+
**A `secret` reference.** A non-system update keeps a `secret:` reference only where every record it lands on serves that reference, in that column, to the caller's own read door. The read path serves a `secret` column masked to every caller, so after the upgrade every non-system update that sends a reference is refused with `400 VALIDATION_ERROR` and nothing is written, with one answer whatever the reference names and whatever the column holds — the record's own reference included, which was accepted and kept before. The same holds for a reference a `beforeUpdate` hook writes onto the payload, such as one that spreads the stored row (`ctx.previous`) back into it. To leave a secret unchanged, omit the field or send back the read mask; a hook that writes the stored row back omits its `secret` columns. A new value is still sealed, `null` still clears, and the system principal keeps the pass-through.
15+
16+
**A master-detail repoint.** On a detail whose fields declare a parent-scoped `requiredWhen`, the ADR-0113 pre-check of an update that names the master-detail FK reads the header the row hung off from the stored FK only as the caller's own read door serves it. Where the caller may edit the FK but not read it (a field permission `{ readable: false, editable: true }`), the prior header is unknown to it and is bound as an unresolved one, so the pre-check cannot prove a violation was already stored: a row that already lacks a value its requirement asks for, and that such a caller names a header for, is refused with the field's `required` finding whatever header the row hung off — including the header it already hangs off, which used to rest. A caller who can read the FK is judged as before, and an update that does not name the FK is unchanged. The `validate()` preview of such an update binds the prior header the same way, so it predicts the same `required` refusal instead of admitting the row.

‎content/docs/permissions/system-context.mdx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ bugs — it is the protection or the side effect that silently does not happen.
138138
| 23b | **Position-catalog check skipped** — a `sys_user_position` write whose `position` names no position in the security catalog is not refused | plugin-security | Get: a system writer can store an assignment naming no position — the seed loader writes `stack.data` from `AppPlugin.start()` under a system context, as invitation acceptance and the platform's own bootstraps do. Lose: such a row grants nothing and nothing says so. A non-system insert, or a non-system update that changes `position`, is refused `400 VALIDATION_FAILED` / `reference_not_found` instead when the security catalog holds no position of that name: the environment registry (ADR-0131 D3), one per deployment, which takes no organization, so a name only a `sys_position` row carries — any organization's — is refused like one nobody carries. It is the same stand-down as the engine's referential-integrity check for a lookup column | `assertPositionNamesCatalogRow` |
139139
| 23c | **Grant-name catalog check skipped** — a `sys_user_permission_set` write whose `permission_set` names no permission set in the security catalog is not refused | plugin-security | Get: a system writer can store a grant naming a set not yet registered — seed replay and boot provisioning may write the grant before the definition it names is registered. Lose: such a row grants nothing and nothing says so. A non-system insert, or a non-system update that changes `permission_set`, is refused `400 VALIDATION_FAILED` / `reference_not_found` instead when the security catalog holds no set by that name; a grant that names no set at all is refused `required` for every caller. The grant names its set by name alone, and nothing derives the name from `permission_set_id` (ADR-0131 D4, D10) | `packages/plugins/plugin-security/src/grant-permission-set-name.ts#onInsert`, `packages/plugins/plugin-security/src/grant-permission-set-name.ts#onUpdate` |
140140
| 23d | **Grant-holder membership check skipped** — a `sys_user_position` or `sys_user_permission_set` write naming a user with no `sys_member` row in the row's organization is not refused | plugin-security | Get: a system writer can store such a grant — invitation acceptance writes the membership and its placement together, and seed replay, the organization-admin reconcile and the platform bootstraps write grants as the system. Lose: such a row takes effect once its user is admitted to the organization. A non-system insert, or a non-system update that moves `user_id` or `organization_id`, is refused `400 VALIDATION_FAILED` / `reference_not_found` at `user_id` instead, for every caller, a platform administrator included; the membership is read as `{ isSystem: true, tenantId }` with `tenantId` the row's own organization, never another one. A row stored with no organization is not judged. Under `single`, 18.0 stamps a new grant with the Default Organization, so such a row is one an earlier release wrote | `packages/plugins/plugin-security/src/grant-holder-membership-refusal.ts#onInsert`, `packages/plugins/plugin-security/src/grant-holder-membership-refusal.ts#onUpdate` |
141-
| 23e | **A `secret` field takes a stored reference as given** — a value already spelled `secret:` is stored unchanged | objectql | Get: a system writer may copy a `secret:` reference onto a record as it stands (seed replay, a migration). Every other caller may only seal a new value or keep its own record's ref: on a create any such value is refused, and on an update it is accepted only when every record the write lands on already holds exactly that reference in that column, which is then left as it is, as an echoed read mask is. Any other reference is refused `400 VALIDATION_ERROR` before anything is written, with one answer whatever it names. Lose: the check that the reference is the record's own — a system writer that copies one is trusted to mean it. Inbound HTTP never carries the flag | `encryptSecretFields` |
141+
| 23e | **A `secret` field takes a stored reference as given** — a value already spelled `secret:` is stored unchanged | objectql | Get: a system writer may copy a `secret:` reference onto a record as it stands (seed replay, a migration). Every other caller may only seal a new value or keep its own record's ref, and its own is the value it reads in that column: on a create any such value is refused, and on an update it would be kept only where every record the write lands on serves exactly that reference to this caller's own read door. The read path serves a `secret` column masked to every caller, so every reference such a caller sends is refused `400 VALIDATION_ERROR` before anything is written, with one answer whatever it names and whatever the column holds; an echoed read mask still leaves the column as it is. Lose: the check that the reference is the record's own — a system writer that copies one is trusted to mean it. Inbound HTTP never carries the flag | `encryptSecretFields` |
142142
| 23f | **Agent ceiling grant check skipped** — a `sys_user_permission_set` write naming an MCP agent ceiling set is not refused | plugin-security | Get: a system writer is not judged. The platform-admin promotion and the organization-admin reconcile name their sets in code, and neither is a ceiling; the self-registration grant writes the configured `audience.selfRegistrationPermissionSet`, which the audience validation (`assertAudienceConfig`, plugin-auth) refuses when it names an agent ceiling set. Lose: seed replay and the development-only demo-staff seed write the sets their stack declares, unjudged, so a seed that names an agent ceiling set stores it. A non-system insert, or a non-system update whose resulting row names one, is refused `400 VALIDATION_FAILED` / `invalid_value` at `permission_set`, for every caller, a platform administrator included; a delete is not judged (ADR-0090 D10) | `packages/plugins/plugin-security/src/agent-ceiling-grant-refusal.ts#onInsert`, `packages/plugins/plugin-security/src/agent-ceiling-grant-refusal.ts#onUpdate` |
143143
| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `buildDriverOptions` |
144144
| 25 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `packages/plugins/plugin-security/src/system-write-guard.ts#isUserContextWrite`, `packages/plugins/plugin-security/src/system-write-guard.ts#assertEngineOwnedWriteAllowed` |

‎packages/objectql/src/engine-master-detail-related.ts‎

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,11 @@
5454
* onto its function (`--hand-docs` of `scripts/codemod-split-engine.mjs`),
5555
* and the class keeps none for it; a public or protected method's docblock
5656
* stays on the class, the contract, and its function keeps the placeholder.
57+
*
58+
* One body was written here rather than moved: `priorMasterIdsCallerReads`,
59+
* the stored master an update's pre-check reads, as the caller's read door
60+
* serves the FK. Its private method on the class is a delegation of the same
61+
* shape.
5762
*/
5863

5964
import { referenceTargetOf } from '@objectstack/spec/data';
@@ -70,6 +75,7 @@ import type { RelatedFieldBinding, RelatedRecordBinding } from './validation/rul
7075
import { resolveMasterDetailRelation } from './master-detail.js';
7176
import { isFederatedObject } from './federated-object.js';
7277
import { masterIdOf, servedAsStored, staticReadonlyStoredView, undeclaredRelatedFields } from './engine.js';
78+
import { storedReferencesCallerReads } from './engine-referential-integrity.js';
7379
import type { ObjectQL } from './engine.js';
7480

7581
/**
@@ -248,6 +254,14 @@ export async function settleMasterDetailLanding<H>(
248254
* merged with the patch on an update, so the patch's FK wins over the stored
249255
* one; `storedRows[i]` names the header a repoint leaves.
250256
*
257+
* ⛔ `storedRows[i]` holds only the columns the read door served this caller
258+
* as stored, so a stored FK it was not served is absent there. When the patch
259+
* names the FK over such a row, the header the row leaves is UNKNOWN to this
260+
* caller, and `previousParents[i]` is `null`, the unresolved header — the one
261+
* fixed binding the write gives it ({@link priorMasterIdsCallerReads}), so the
262+
* pre-check's verdict is the write's and never depends on what the column
263+
* holds. A patch that does not name the FK reads no second header.
264+
*
251265
* ## Read under the caller's access, not elevated
252266
*
253267
* A header read elevated here would answer a predicate over a row the caller
@@ -290,6 +304,13 @@ export async function resolvePreviewParents(
290304
const priorIds = storedRows.map((stored) => (rel && stored ? masterIdOf(rel.fk, null, stored) : undefined));
291305
// The write resolves a second header only for a repoint (`repointsMaster`).
292306
const repoints = priorIds.map((prior, i) => prior != null && landingIds[i] !== prior);
307+
// The patch names the FK over a stored row whose FK this caller was not
308+
// served as stored: the prior header is unknown, as the write binds it.
309+
const priorUnknown = storedRows.map((stored, i) => {
310+
const view = views[i];
311+
return rel != null && stored != null && !Object.prototype.hasOwnProperty.call(stored, rel.fk)
312+
&& view != null && typeof view === 'object' && rel.fk in view;
313+
});
293314
const ids = new Set<string>();
294315
landingIds.forEach((id) => { if (id != null) ids.add(String(id)); });
295316
priorIds.forEach((id, i) => { if (repoints[i]) ids.add(String(id)); });
@@ -334,10 +355,50 @@ export async function resolvePreviewParents(
334355
const headerOf = (id: string | number | undefined) => (id == null ? null : (headers.get(String(id)) ?? null));
335356
return {
336357
parents: landingIds.map(headerOf),
337-
previousParents: priorIds.map((id, i) => (repoints[i] ? headerOf(id) : undefined)),
358+
previousParents: priorIds.map((id, i) => (priorUnknown[i] ? null : repoints[i] ? headerOf(id) : undefined)),
338359
};
339360
}
340361

362+
/**
363+
* The master each of an update's stored rows hangs off, as THIS caller's own
364+
* read door serves the master-detail FK: the header the ADR-0113 pre-check of
365+
* a parent-scoped `requiredWhen` reads as `previousParent` when the write
366+
* names the FK.
367+
*
368+
* `{ id }` for a row whose FK the read door serves this caller exactly as
369+
* stored ({@link storedReferencesCallerReads}, the update's one projection of
370+
* stored rows onto the caller's read): `id` is the stored master, or
371+
* `undefined` for a row that hangs off none. `null` for a row whose FK it does
372+
* not — hidden, or served transformed, or a row the door does not return: the
373+
* prior master is UNKNOWN to this caller, and the write binds one fixed header
374+
* for it, the unresolved one, so neither whether the pre-check reads a second
375+
* header nor which one it reads depends on what the column holds. A SYSTEM
376+
* caller is answered from the rows as stored, as before.
377+
*
378+
* A stored row that carries no FK key at all is read as holding no master, as
379+
* `masterIdOf` reads it: the key is spelled out as empty before the
380+
* projection, so a system caller's answer for it is `{ id: undefined }`.
381+
*/
382+
export async function priorMasterIdsCallerReads(
383+
this: Pick<MasterDetailRelatedHost, 'find' | 'logger'>,
384+
schema: unknown,
385+
rows: ReadonlyArray<Record<string, unknown> | null | undefined>,
386+
context: ExecutionContext | undefined,
387+
): Promise<Array<{ readonly id: string | number | undefined } | null>> {
388+
const rel = resolveMasterDetailRelation(schema as any);
389+
if (!rel) return rows.map(() => null);
390+
const stored = rows.map((row) => {
391+
const own = row && typeof row === 'object' ? row : {};
392+
return Object.prototype.hasOwnProperty.call(own, rel.fk) ? own : { ...own, [rel.fk]: undefined };
393+
});
394+
const served = await storedReferencesCallerReads.call(
395+
this, (schema as { name?: unknown } | null | undefined)?.name, stored, [rel.fk], context,
396+
);
397+
return served.map((view) => (
398+
view && Object.prototype.hasOwnProperty.call(view, rel.fk) ? { id: masterIdOf(rel.fk, null, view) } : null
399+
));
400+
}
401+
341402
/**
342403
* [#18682] Resolve the related rows this object's PREDICATE rules read one
343404
* hop through a reference field, for a whole batch of rows at once.

0 commit comments

Comments
 (0)