Skip to content

Commit 7b62f0a

Browse files
fix(metadata-protocol): batchData's by-id upsert chooses its arm by what the caller can read (#28721)
domain:devx#1 · sunhaolin · session_017xRBcVy4VXmkQjn2MGeaWk Clause-②: no Metadata-protocol: none Skills-docs: none — no file under `skills/**` teaches how the batch `upsert` chooses between update and create (a `git grep -i upsert` of `skills/` names the import and seed upserts and the generated-route table only), so no skill line changes. ## 维护者速读 - 改了什么:`POST /data/:object/batch` 的 `upsert`(protocol 的 `batchData`)里带 `id` 的行,改为按调用方能读到什么来选「更新」还是「新建」。调用方读得到的记录照旧走更新,仍由引擎判写权限;读不到的 id 按新建处理,答复与同 id 的 create 完全一致。只改 `protocol-batch-data.ts` 这一处分叉。 - 为什么改:QA 第三轮发现的安全加固(细节按披露裁决不公开) - 风险与回滚:只换答复、不改接受集。调用方读不到的那一行,现在收到同 id 的 create 会收到的答复;读得到的行、无 id 的行、原子批的回滚、无上下文的进程内调用都不变。回滚即 revert 本 PR,不涉及数据迁移。 - 先看:`packages/metadata-protocol/src/protocol-batch-data.ts` 的 `case 'upsert'` 分叉(风险 hunk:`objectReadable` 与调用方 `findOne` 两行)和新函数 `callerMayReadObject`;其次 `protocol.upsert-existence.test.ts`。 - 两分钟验证:`pnpm --filter @objectstack/dogfood exec vitest run --maxWorkers=1 test/upsert-by-id-caller-read.dogfood.test.ts`,预期 28/28 通过(单租户 showcase 19 个,两组织隔离启动 9 个)。 - 需要你做的:无,席位落地。 ## What changes `batchData`'s by-id `upsert` (`packages/metadata-protocol/src/protocol-batch-data.ts`, the `case 'upsert'` fork of `runBatchDataLoop`) now chooses its arm by what the **caller** can read, under the context the arm's write runs with: 1. the object-level read admission, asked once per loop of the `security` service's `canReadObject` (the engine middleware's own read gate, `callerMayReadObject`); a context-less call, or a host whose security service lacks the method, is not asked, and the row read below still enforces there; 2. the caller's own `findOne` by id, where RLS, sharing and the organization wall apply. Inside the atomic arm that context carries the transaction. A row the caller can read keeps the update arm, where `engine.update` still judges the write. Any other id takes the create arm, the same `engine.insert` call and row builder that `case 'create'` uses, so the row answers exactly as a create naming that id answers on the same door. No new code, field, detail or sentence; nothing names another organization. There is still no fallback insert after an update failure. The fork's code comment records why this reverses the earlier existence fork, citing ruling 5985885287 (#21771) and the 2026-11-01 maintainer ruling that extends ruling 5934879010 (#21185, rule 2). `BatchDataHost` trades `probeRecord` (no batch body reads it any more) for `getServicesRegistry`; the OCC path and `probeRecord`'s own readers are untouched. The read's options are typed (`EngineQueryOptionsParsed`), not erased. ## Tests - `packages/metadata-protocol/src/protocol.upsert-existence.test.ts`, rewritten (11 cases): an unreadable row takes the insert arm and its row refusal deep-equals a batch create's for the same id, store untouched; the fork reads with the caller's context, never the system's; an object the security service refuses sends every row to the insert arm without a row read, asked once per loop; an admitted object still forks on the row read; a throwing admission fails the row and writes nothing; a context-less call is not asked; no fallback insert after an update failure; missing id inserts; readable row updates with the caller's context; atomic rollback with the causal row's create refusal and both reads on the transaction; mixed batch. - `packages/qa/dogfood/test/upsert-by-id-caller-read.dogfood.test.ts`, new (28 cases), at the HTTP door, atomic and non-atomic. Stock showcase: create without edit on a private row, create and edit on a private row, a create-only principal with no read; each answers the batch create's body for the same id (row `id` echo aside), with the plain create's `409 UNIQUE_VIOLATION` code and status, row byte-identical, and an unknown id is created. A readable row with edit is updated; a readable row without edit keeps the update arm's refusal; a refused-both control answers one refusal for an existing and an unknown id. Walled boot (`multiTenant: true`, `isolated`, invitation-only, two organizations created over `/auth/organization/create`): an editor and a creator in organization B on organization A's row get the create's answer, which names neither organization A's id, name nor slug; the editor updates its own organization's row and the creator gets the update arm's refusal. - `protocol.readonly-insert.test.ts`: one comment names the fork's read. ## Acceptance notes - Clause-② `no`: the accept set does not move. A row the caller can read takes the same arm as before. A row it cannot read never reaches the update arm: it takes the create arm and answers as a create naming that id. Only the refusal that row receives changes, to the create's (the precedent is the order change that landed as `no` in #28615). No `(narrowing)` arm was found. - On `driver-memory`, which declares primary-key uniqueness out of its scope, a create naming an existing id lands a second row; the upsert's create arm now does what the create does there. The fork's comment states it. No store enforcing the key (SQL, sqlite-wasm, Turso) is affected. ## Verification Readings below were taken at `adf4e0cb4`, before `origin/main` was merged (`12dd59a7e`, delta still exactly these six files). The merged head's run (closure rebuild, package suite, focused pins, one-shot ablation with restore proof, `check:type-check-debt`, the gate union and the full dogfood battery) is in flight under the verify lock; its readings are below (written by the seat). **Final-head readings.** The run started at `12dd59a7e` (merge of `origin/main`) and ended at `9fb84ac12`, whose only addition is a `packages/core` pin file. Every step ran under the verify lock and reported VERDICT command-exit 0. - Closure build: exit 0. - metadata-protocol full suite: Test Files 292 passed | 4 skipped (296), Tests 21958 passed | 26 skipped. - metadata-protocol typecheck and dogfood typecheck: exit 0. - `upsert-by-id-caller-read.dogfood.test.ts`: 28/28 (stock showcase 19; walled two-org 9). - `protocol.upsert-existence.test.ts`: 11/11. - `check:type-check-debt`: exit 0. - Full dogfood battery `pnpm --filter @objectstack/dogfood exec vitest run --maxWorkers=1`: Test Files 372 passed | 3 skipped (375), Tests 3022 passed | 28 skipped (3050). - One-shot ablation, with the fork reverted to the system-context probe: - unit 8 failed | 3 passed; - dogfood 10 failed | 18 passed (exactly the hidden-row and cross-org cases); - restore proven (blob == HEAD, preflight `--absent` exit 0). - Gate union at `9fb84ac12`: 67 derived, 67 run, 0 NOT-MEASURED. - CI at `9fb84ac12`: 55 success, 9 skipped, 0 failure. The earlier `Test Core (4/8)` red at `12dd59a7e` came from the core row-serving-door pin, which did not yet classify the new `canReadObject` caller; `9fb84ac12` fixes it. <details><summary>Touched packages: tests and typecheck at adf4e0c (verify lock VERDICT command-exit 0)</summary> ```text pnpm --filter @objectstack/metadata-protocol build EXIT=0 dogfood upsert-by-id-caller-read.dogfood.test.ts (--maxWorkers=1) EXIT=0 Test Files 1 passed (1) · Tests 28 passed (28) pnpm --filter @objectstack/metadata-protocol exec vitest run EXIT=0 Test Files 291 passed | 4 skipped (295) · Tests 21948 passed | 26 skipped (21974) pnpm --filter @objectstack/metadata-protocol typecheck EXIT=0 pnpm --filter @objectstack/dogfood typecheck EXIT=0 pnpm check:type-check-debt EXIT=0 0 raw tsc error(s) total, none above its recorded number focused unit files (upsert-existence + readonly-insert) EXIT=0 Tests 22 passed (22) ``` </details> <details><summary>Gates: dispatch-gates.mjs --commands (65, identical at 12dd59a)</summary> ```text 62 of 65 EXIT=0 at bf59c1e. pnpm check:query-options-erasure EXIT=1 -> the caller read was typed; EXIT=0 at adf4e0c ("none new") pnpm check:dual-build-cjs-loads EXIT=3 PREREQUISITE NOT MET (unbuilt packages) -> EXIT=0 after the closure build pnpm check:type-check-debt EXIT=3 PREREQUISITE NOT MET (lock) -> EXIT=0 under the lock (above) node scripts/check-platform-checklist.mjs --changed-from origin/main EXIT=0 (32 anchors judged, 0 lost) ``` </details> --- _Generated by [Claude Code](https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 706ac52 commit 7b62f0a

7 files changed

Lines changed: 679 additions & 98 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
fix(metadata-protocol): the by-id `upsert` of `batchData` chooses its arm by what the caller can read
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
Security hardening of `POST /data/:object/batch` with `operation: 'upsert'` (the protocol's `batchData`). A row that carries an `id` now chooses between its update and create arms by what the calling principal can read, the rule the other by-id write doors already follow: a record the caller cannot read is treated as one that does not exist.
11+
12+
- **Unchanged:** a record the caller can read is updated, under the caller's own write permissions, exactly as before; a row without an `id` is created; atomic batches roll back on the first failing row; system and context-less calls fork as their reads run.
13+
- **Changed:** any other `id` takes the create arm, so the row answers exactly as a `create` naming that `id` answers on the same door.
14+
- **Your fix:** none. A client reconciling batch results reads each row's `errors[0].code` as before.

‎packages/core/src/security/row-serving-door-exposure.pin.test.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,14 @@ const CALLERS: Record<string, Classification> = {
109109
+ 'that only narrows the read the caller addressed, whose own door already admitted it; no row and no field of the '
110110
+ 'parent is served, and a refused parent answers `$in: []`',
111111
},
112+
'packages/metadata-protocol/src/protocol-batch-data.ts': {
113+
kind: 'not-a-door',
114+
reason:
115+
"the by-id upsert fork of `batchData` (`callerMayReadObject`): it asks the admission only to choose between the "
116+
+ 'update and create arms of a row the caller addressed on the batch data route, whose own door already judged the '
117+
+ "object's exposure; `true` still reads the row as the caller, through the engine middleware, and it serves no row "
118+
+ 'of its own',
119+
},
112120
// [#25522 cut 11] File-field hydration's `sys_file` admission, moved out of engine.ts.
113121
'packages/objectql/src/engine-file-reference-hydration.ts': {
114122
kind: 'not-a-door',

‎packages/metadata-protocol/src/protocol-batch-data.ts‎

Lines changed: 114 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,9 @@
3232
* path → `runAtomicBatch` → the loop → the builders) do so through the
3333
* protocol, exactly as before.
3434
*
35-
* The one field the bodies read, `engine`, goes as a GETTER, so each read
36-
* happens when the body makes it: tests replace it on a constructed protocol.
35+
* The two fields the bodies read, `engine` and `getServicesRegistry`, go as
36+
* GETTERS, so each read happens when the body makes it: tests replace them on a
37+
* constructed protocol.
3738
*
3839
* The other module-level declarations the bodies read stay in `protocol.ts`,
3940
* where code that did not move reads them too, and are imported from there:
@@ -47,7 +48,8 @@
4748
import type { ObjectStackProtocolImplementation, BatchDataLoopOutcome, BatchDataRowResult } from './protocol.js';
4849
import { byIdUpdateRecord, rowRequiredIdError, toRowApiError } from './protocol.js';
4950
import type { BatchUpdateRequest, BatchUpdateResponse, DeleteManyDataRequest, UpdateManyDataRequest } from '@objectstack/spec/api';
50-
import type { DroppedFieldsEvent } from '@objectstack/spec/data';
51+
import type { DroppedFieldsEvent, EngineQueryOptionsParsed } from '@objectstack/spec/data';
52+
import type { ISecurityService } from '@objectstack/spec/contracts';
5153
import { engineCanRollBack, recordNotFoundError } from '@objectstack/core';
5254
import { omitInternalFieldsFromWriteResponse } from './write-response-internal-fields.js';
5355

@@ -107,6 +109,40 @@ function rowOperationFailureFallback(operation: string): string {
107109
return `The ${operation} of this record failed. The reason is in the server log.`;
108110
}
109111

112+
/**
113+
* The OBJECT-level half of the question `batchData`'s by-id upsert forks on —
114+
* may the CALLER read `object` at all? — asked of the `security` service's
115+
* `canReadObject`, the engine middleware's own read gate arm for arm
116+
* (`ISecurityService.canReadObject`), with the context the loop's writes run
117+
* under. The ROW-level half is the caller's own `findOne` by id, at the fork.
118+
*
119+
* Asked separately because the middleware answers a caller with no read grant
120+
* by REFUSING the `findOne` (`403`), not with `null`: without this half, a
121+
* caller who may create but not read would have every by-id upsert row refused
122+
* outright, where a create naming the same id is accepted. A `false` here is
123+
* "this caller can read no row of the object", so every row takes the insert
124+
* arm, which `engine.insert` still judges in full.
125+
*
126+
* `true` means only "not refused at this layer" and never widens anything: the
127+
* `findOne` that follows is still the middleware's, row scope included. So a
128+
* context-less in-process call (the read `findOne` makes without a context is
129+
* the one its writes make) is not asked about, and neither is a host whose
130+
* security service is absent or predates the method — `findOne` still
131+
* enforces there, so the absence can only make the answer stricter (a refusal
132+
* fails the row), as `searchAll` reasons for the same method. A
133+
* `canReadObject` that throws fails the row rather than choosing an arm.
134+
*/
135+
async function callerMayReadObject(
136+
this: Pick<BatchDataHost, 'getServicesRegistry'>,
137+
object: string,
138+
context: unknown,
139+
): Promise<boolean> {
140+
if (context === undefined) return true;
141+
const security = this.getServicesRegistry?.().get('security') as Partial<ISecurityService> | undefined;
142+
if (typeof security?.canReadObject !== 'function') return true;
143+
return security.canReadObject(object, context as Parameters<NonNullable<ISecurityService['canReadObject']>>[1]);
144+
}
145+
110146
/**
111147
* The `ObjectStackProtocolImplementation` members the functions below read —
112148
* every one of them, and no other; each function's `this` is the `Pick` of the
@@ -123,8 +159,8 @@ export interface BatchDataHost {
123159
buildRolledBackBatchResponse: ObjectStackProtocolImplementation['buildRolledBackBatchResponse'];
124160
buildUpdateManyResponse: ObjectStackProtocolImplementation['buildUpdateManyResponse'];
125161
readonly engine: ObjectStackProtocolImplementation['engine'];
162+
readonly getServicesRegistry: ObjectStackProtocolImplementation['getServicesRegistry'];
126163
locateBatchCause: ObjectStackProtocolImplementation['locateBatchCause'];
127-
probeRecord: ObjectStackProtocolImplementation['probeRecord'];
128164
reconcileStoppedBatch: ObjectStackProtocolImplementation['reconcileStoppedBatch'];
129165
runAtomicBatch: ObjectStackProtocolImplementation['runAtomicBatch'];
130166
runAtomicBatchData: ObjectStackProtocolImplementation['runAtomicBatchData'];
@@ -262,7 +298,7 @@ export async function runAtomicBatch(
262298

263299
/** The body of `ObjectStackProtocolImplementation#runBatchDataLoop`; the method's docblock there is its contract. */
264300
export async function runBatchDataLoop(
265-
this: Pick<BatchDataHost, 'assertRecordExists' | 'engine' | 'probeRecord'>,
301+
this: Pick<BatchDataHost, 'assertRecordExists' | 'engine' | 'getServicesRegistry'>,
266302
args: {
267303
object: string;
268304
operation: BatchUpdateRequest['operation'];
@@ -287,6 +323,11 @@ export async function runBatchDataLoop(
287323
// drop — the upsert arms did exactly that until the patch round.
288324
const ctxOpt = context !== undefined ? { context } : {};
289325

326+
// The object-level half of the upsert fork's question (see the fork and
327+
// `callerMayReadObject`), asked at most once per loop: the object and the
328+
// context are the loop's, so every row gets the same answer.
329+
let objectReadable: Promise<boolean> | undefined;
330+
290331
// [#4793] `index` is the row's position in the REQUEST array — the
291332
// correlation a caller needs for failure rows that carry no id.
292333
for (const [index, record] of records.entries()) {
@@ -328,28 +369,75 @@ export async function runBatchDataLoop(
328369
}
329370
case 'upsert': {
330371
if (record.id) {
331-
// [#5099] The update-or-insert fork asks EXISTENCE,
332-
// not visibility. `findOne` under the CALLER's
333-
// context is the read RLS/sharing narrows (#3455),
334-
// so an existing row outside the caller's scope
335-
// answered null, took the insert arm, and either
336-
// duplicate-keyed — an authorization/update
337-
// scenario reported as a key collision — or, on a
338-
// store without a unique id constraint, wrote a
339-
// second row. The same `probeRecord` the update branch
340-
// above asks after its write (#4620): it asks the
341-
// database a fact, and whether the caller may WRITE
342-
// the row it proves stays #1994's decision inside
343-
// `engine.update`.
372+
// The update-or-insert fork asks what the CALLER can
373+
// read. On the write doors a row the caller cannot
374+
// read is a row that does not exist (ruling
375+
// 5985885287 on #21771, the governing text for the
376+
// write doors), so such a row takes the INSERT arm —
377+
// what a create naming the same id does — and that
378+
// insert meets the deployment-global primary key and
379+
// answers the `409 UNIQUE_VIOLATION` a plain create
380+
// with that id answers. A row the caller CAN read
381+
// keeps the UPDATE arm, where `engine.update` still
382+
// judges whether the caller may write it.
383+
//
384+
// "Can read" is the read door's question in both its
385+
// halves, under the context the arm's write runs
386+
// with (`ctxOpt`): the object-level admission
387+
// (`callerMayReadObject`, once per loop) and then the
388+
// caller's own `findOne` by id, where RLS, sharing
389+
// and the organization wall all apply. Inside the
390+
// atomic arm that context is the transaction's, so
391+
// the read sees the batch's own uncommitted rows.
392+
//
393+
// ⚠️ This REVERSES #5099 / #5187, by ruling. #5187
394+
// moved this fork FROM the caller's read TO the
395+
// system-context `probeRecord`, because forking on
396+
// visibility reported an authorization/update
397+
// scenario as a key collision. That trade is now
398+
// ruled the other way, for two reasons:
399+
//
400+
// - forking on existence answered a row the caller
401+
// cannot read differently from an id that names
402+
// nothing — the update arm's refusal (`403`, or the
403+
// by-id pre-image check's `404`) beside a CREATED
404+
// row — so a caller holding create learned that a
405+
// row it cannot read exists, which ruling
406+
// 5985885287 closed on the by-id update and delete
407+
// doors;
408+
// - the key collision #5187 called a misdirection is
409+
// the answer the maintainer ratified on 2026-11-01
410+
// as the accepted cost of a deployment-global key,
411+
// extending ruling 5934879010 on #21185 (rule 2),
412+
// which `SqlDriver.upsert` already follows for a
413+
// conflict in another organization: it tells the
414+
// caller exactly what a create with that id tells
415+
// it, and nothing more.
416+
//
417+
// ⛔ So a hidden collision gets NO answer of its own:
418+
// no new code, field, detail or sentence, and nothing
419+
// that names another organization. The collision is
420+
// `engine.insert`'s own refusal, reported through the
421+
// same row builder `case 'create'` uses.
422+
//
423+
// #5099's second observation stays true, as a fact
424+
// about the store rather than this fork: on a store
425+
// that does not enforce the primary key
426+
// (`driver-memory` declares it out of scope) the
427+
// insert lands a second row with that id, exactly as
428+
// a create naming that id already does there.
344429
//
345-
// The old fallback (update threw → blind insert)
346-
// is gone with the fork's visibility read: with
347-
// existence decided BEFORE the write, a fallback
348-
// insert could only bury a real update failure
349-
// under the duplicate-key error of inserting a row
350-
// just proven to exist — the same masking ADR-0119
351-
// D4 forbade inside the atomic arm.
352-
const existing = await this.probeRecord(object, record.id);
430+
// #5099's other half is kept unchanged: there is NO
431+
// fallback. The arm is chosen before any write, so an
432+
// update failure surfaces itself and is never buried
433+
// under an insert's error — the masking ADR-0119 D4
434+
// forbade inside the atomic arm.
435+
objectReadable ??= callerMayReadObject.call(this, object, context);
436+
const callerRead: EngineQueryOptionsParsed = { where: { id: record.id } };
437+
if (context !== undefined) callerRead.context = context;
438+
const existing = (await objectReadable)
439+
? await this.engine.findOne(object, callerRead)
440+
: null;
353441
if (existing) {
354442
const dropped: DroppedFieldsEvent[] = [];
355443
const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any));

‎packages/metadata-protocol/src/protocol.readonly-insert.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ function makeProtocol(schema: any = SCHEMA) {
9696
findOne: vi.fn(async (object: string, query?: EngineFindOneQueryInput) => {
9797
assertEngineFindOnePredicate(object, query);
9898
// The clone source exists; any other id names no row, which is what sends
99-
// `batchData`'s upsert fork (`probeRecord`, #5099) down its CREATE arm.
99+
// `batchData`'s upsert fork (the caller's `findOne`) down its CREATE arm.
100100
const id = (query as any)?.where?.id;
101101
return id === 'src-1' ? { id: 'src-1', title: 'Source', approval_status: 'approved' } : null;
102102
}),

‎packages/metadata-protocol/src/protocol.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15370,9 +15370,10 @@ export class ObjectStackProtocolImplementation implements
1537015370
* `atomic=false`). It used to change a second — forbidding the upsert
1537115371
* fallback insert, whose failure inside an aborted transaction could only
1537215372
* mask the real cause — until #5099 removed that fallback from BOTH arms:
15373-
* the upsert fork is decided by an existence probe before any write, so a
15374-
* fallback insert could only bury a real update failure under the
15375-
* duplicate-key error of inserting a row just proven to exist.
15373+
* the upsert fork is decided before any write (by what the caller can
15374+
* read, since the fork's own note says why), so a fallback insert could
15375+
* only bury a real update failure under the duplicate-key error of
15376+
* inserting a row the caller was just shown to read.
1537615377
*/
1537715378
private runBatchDataLoop(args: {
1537815379
object: string;
@@ -15385,10 +15386,11 @@ export class ObjectStackProtocolImplementation implements
1538515386
}): Promise<BatchDataLoopOutcome> {
1538615387
// [#25467 cut 1] The body moved to `protocol-batch-data.ts` (see `BatchDataHost`).
1538715388
const readEngine = () => this.engine;
15389+
const readGetServicesRegistry = () => this.getServicesRegistry;
1538815390
const host: ThisParameterType<typeof runBatchDataLoop> = {
1538915391
assertRecordExists: (...a) => this.assertRecordExists(...a),
1539015392
get engine() { return readEngine(); },
15391-
probeRecord: (...a) => this.probeRecord(...a),
15393+
get getServicesRegistry() { return readGetServicesRegistry(); },
1539215394
};
1539315395
return runBatchDataLoop.call(host, args);
1539415396
}

0 commit comments

Comments
 (0)