Skip to content

Commit 68a2a21

Browse files
fix(metadata-protocol): an update by id answers the same refusal whether or not the id exists, as delete does (#28615)
domain:devx#1 · sunhaolin · session_017xRBcVy4VXmkQjn2MGeaWk Clause-②: no Metadata-protocol: none Skills-docs: none — no file under `skills/**` teaches what a by-id update answers for an id that names no record (a `git grep` of `skills/` for `RECORD_NOT_FOUND` names only the approvals record lock), so no skill line changes. The Update Record section of `content/docs/protocol/kernel/http-protocol.mdx` states the order. ## 维护者速读 - **改了什么**:按 id 更新记录时,先判断"这次更新允不允许",再查"记录在不在"——与按 id 删除的顺序一致。覆盖单条 `PATCH`、批量 `updateMany`、`batch`(update)三个入口,以及经它们更新的所有入口(dispatcher `/data`、`/mcp` HTTP 工具、声明式更新动作)。 - **为什么改**:QA 第三轮发现的安全加固(细节按披露裁决不公开) - **风险与回滚**:不允许更新该对象的调用方(无编辑权限,或外部数据源只读)对不存在的 id 现在收到与存在的 id 相同的 403,原来是 404;有权限的调用方、成功更新、`405`、乐观锁(`If-Match`)、upsert 与系统上下文写入均不变。回滚 = revert 本 PR。 - **先看**:① `packages/metadata-protocol/src/protocol.ts` 的 `updateData`——删掉写前的存在性探测,改为引擎返回 `null` 后再查存在;② `packages/metadata-protocol/src/protocol-batch-data.ts` 的两处 update 分支(同一改法);③ `packages/qa/dogfood/test/update-by-id-refusal-order.dogfood.test.ts`。 - **两分钟验证**:`pnpm --filter @objectstack/dogfood exec vitest run test/update-by-id-refusal-order.dogfood.test.ts` —— 预期 16/16 通过(四类对象上 PATCH、批量更新、DELETE 对"存在/不存在"的 id 给出同一个回答,另有编辑权限、行级范围、管理员三组对照)。 - **需要你做的**:无,席位落地。 ## What changes An update by id now answers the same refusal whether or not the id exists, as delete does. - **`updateData`** (`PATCH /api/v1/data/:object/:id`, and every door that updates through it: the runtime dispatcher's `/data`, the `/mcp` HTTP tools, `callData`, declarative update actions) no longer reads the record before calling `engine.update`. The engine judges the write first — the security middleware's object- and row-level verdicts, the external datasource's write guard — and then its own by-id not-found gate (inside the middleware chain, ahead of every `before*` hook) answers `404 RECORD_NOT_FOUND` to a caller the write is open to. The protocol asks existence (the system-context probe) only when the engine resolved a `null` readback: that keeps the `404` for an engine that resolves `null` for a missing row, and keeps `200 { record: null }` for an update that moved its row out of the caller's row scope. - **Optimistic concurrency** on `updateData` is asked the way `deleteData` asks it: `assertVersionMatch`, a probe only when the caller sent a token. - **`updateManyData` and `batchData`'s `update` operation**: the same — no probe ahead of the engine, the existence check after a `null` readback, per row. - **Docs**: the Update Record section of `content/docs/protocol/kernel/http-protocol.mdx` states the order. Unchanged, and already one answer for both ids for a caller refused the operation: `DELETE` on every door and the cross-object `POST /api/v1/batch` (the engine judges first), and clone and the record field-access read, which load the record through the read door first (each measured at the HTTP door on the stock showcase). The action-by-id record load, the MCP stdio bridge's `update_record` / `delete_record` and `callData`'s no-protocol fallback likewise read the record as the caller, through the read door, before any write, on their update and delete arms alike (read in code). ## Tests - **HTTP door (dogfood, stock showcase)** — `packages/qa/dogfood/test/update-by-id-refusal-order.dogfood.test.ts`, 16 cases. A plain member (baseline only) on `showcase_project_membership`, `sys_user_permission_set`, `showcase_ext_customer` and `showcase_ext_order`: `PATCH`, `updateMany` and `batch` (update) answer the same status and body for an id that names a row and one that names none; `DELETE` keeps its one answer; a stale `If-Match` keeps one status and code. Controls: a member holding the edit grant updates (200) and gets `404 RECORD_NOT_FOUND` for no row; a member with edit and a row-level restriction (`showcase_private_note`) gets one not-found answer for a foreign row and for no row and still updates its own; the administrator updates (200), gets `404` for no row on an ordinary object, and gets `EXTERNAL_WRITE_FORBIDDEN` for both ids on a read-only federated object, as its `DELETE` does; refused writes leave the row untouched. - **Engine** — `packages/objectql/src/engine-write-not-found-gate.test.ts` gains section 5: a refusal the middleware chain answers comes before the by-id not-found gate, for update and delete, and nothing reaches the driver. - **Protocol** — `protocol.record-not-found.test.ts`: a refused write answers the same for a real and a missing id on `updateData` and `deleteData` with no read ahead of the engine; the post-write existence check is asked as the system and keeps the `200` of an update that moved its row out of scope. `protocol.bulk-record-not-found.test.ts`: the same parity on `updateManyData` and `batchData`. ## Verification All readings below were taken on head `0acc8a19b` (this branch, `origin/main` merged at `258fb1e94`) unless a line names another commit (the metadata-protocol, runtime and dogfood rows were read on `d0d3d0be0`; `0acc8a19b` changes only `packages/objectql/src/protocol-data.test.ts`); wall times are from a shared container. CI on `0acc8a19b`: 50 check-runs, 38 success, 12 skipped, none failed — all eight `Test Core` shards, `TypeScript Type Check`, `Lint · ESLint` and `Check Changeset` among them. <details><summary>Touched packages — tests and typecheck</summary> ``` pnpm --filter @objectstack/metadata-protocol test Test Files 289 passed | 4 skipped (293) Tests 21898 passed | 26 skipped (21924) pnpm --filter @objectstack/objectql test (on 0acc8a1) Test Files 510 passed (510) Tests 9879 passed (9879) pnpm --filter @objectstack/runtime exec vitest run src/domains/data-conflict-caller-visibility.test.ts src/plugin-driver-external-object.spike.test.ts Test Files 2 passed (2) Tests 28 passed (28) pnpm --filter @objectstack/dogfood exec vitest run test/update-by-id-refusal-order.dogfood.test.ts Test Files 1 passed (1) Tests 16 passed (16) pnpm --filter @objectstack/dogfood exec vitest run test/showcase-anonymous-deny-surfaces.dogfood.test.ts Test Files 1 passed (1) Tests 75 passed (75) ``` Other suites that drive `updateData` through a real protocol, on `0acc8a19b`: client `client.test.ts` 219, organizations `create-explicit-organization-wall.test.ts` 45, plugin-approvals `record-lock-batch-row-status.integration.test.ts` 4, http-conformance `hono-declared-envelope.conformance.test.ts` 20, service-automation `write-nodes-stored-metadata-family-refusal.integration.test.ts` 17, service-messaging (two notification-template files) 38 — all passed. Consumer suites, on `5a4fb7a95` (the change itself is the same there; later commits touch tests and one equivalent spelling of the post-write check): `pnpm --filter @objectstack/runtime test` 499 files / 8092 tests passed; `pnpm --filter @objectstack/rest test` 362 files / 6904 tests passed. `pnpm --filter PKG typecheck` exit 0 for `metadata-protocol`, `objectql`, `runtime` and `dogfood`; `eslint --no-inline-config` over the 10 touched `.ts` files exit 0. </details> <details><summary>Ablation of the reorder — one-shot, restored</summary> Mutation (`scripts/ablation-replace.mjs`, anchor hit 1 → 0, blob `78abbab6ef4b` → `c387c962240a`): an existence probe put back ahead of `engine.update` in `updateData`. `ablation-dist-preflight` found the marker in `dist/index.js` and `dist/index.cjs` after the rebuild. - dogfood pin under the mutation: 6 of 16 red — the four `PATCH` families, the stale `If-Match` case and the administrator's federated-object case (the bulk doors stay green: they were not mutated); - `protocol.record-not-found.test.ts` under the mutation: 4 of 18 red. Restore: blob back to the `HEAD` blob `78abbab6ef4b`, `git diff HEAD` empty, whole-tree `git status --porcelain` empty, rebuilt; `ablation-dist-preflight --absent` marker absent from all 24 built files; dogfood pin 16/16 and the unit file 18/18 green. </details> <details><summary>Gates — dispatch-gates.mjs --commands on 0acc8a1</summary> 93 derived families, run locally, every one exit 0 (`pnpm check:type-check-debt` under the verify lock). `dispatch-gates.mjs --ran` with every exit code recorded: `93 derived, 93 run, 0 NOT-MEASURED, 0 UNRUN`. Among them: ``` check-system-context-census: OK — 163 elevation read sites in 22 packages across 90 files … check-doc-anchors: 851 internal #fragment link(s) across 481 source file(s) all resolve to a real heading check-engine-double-contract: OK — 1149 pinned, 120 in the DEBT ledger, 3 exempt. check-nul-bytes: OK (scanned 23283 text file(s) … no raw ASCII control bytes) check-changeset-no-major: This diff introduces no `major` bump. check-adr-0087-registration: this PR adds no declared-breaking changeset (1 non-breaking changeset(s) seen). ``` </details> <details><summary>Full dogfood battery, under the verify lock</summary> On `924d21987`: 363 files passed, 3 skipped, 1 failed — `showcase-anonymous-deny-surfaces.dogfood.test.ts` › "the SAME answer the REST data surface gives for that same id", which compared the action door's ghost-id answer with a member's `PATCH` on `showcase_task`, an object that member may not edit. That is this change's consequence: the comparison now uses the data read (the surface the action's record load goes through) and asserts the member's `PATCH` is the write refusal. The repaired file is 75/75 on `d0d3d0be0`. </details> ## Acceptance notes - **Fixture triage.** The bulk and batch-conformance doubles answered a missing id with an unclassified throw (the write pipeline's old answer); they now refuse it the way ObjectQL's by-id gate does — after the write is judged, before any hook — and "the write pipeline never ran for that row" is asserted on a `pipeline` spy instead of "the engine was never called". The engine-layer guarantee is pinned by the engine's own not-found gate tests. `update-path-id-wins` and two runtime tests change their read counts: a tokenless `PATCH`, like a tokenless `DELETE`, reads nothing ahead of the engine, and a refused `PATCH` on a plugin driver costs the remote no read. - **`packages/objectql/src/protocol-data.test.ts` (OCC cases).** A PATCH with no token or a blank one now asserts zero reads ahead of the engine; a missing id is answered `RECORD_NOT_FOUND` by the engine's not-found gate (the double rejects with the shared envelope) or, for an engine that resolves a `null` readback, by the system-context existence check after the write — in both, a token never turns the missing row into a `409`. - **Admin on a read-only federated object.** An unknown id now answers the write guard's `403 EXTERNAL_WRITE_FORBIDDEN`, the answer its `DELETE` already gave, where it answered `404`. --- _Generated by [Claude Code](https://claude.ai/code/session_017xRBcVy4VXmkQjn2MGeaWk)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f3f7b62 commit 68a2a21

14 files changed

Lines changed: 693 additions & 181 deletions
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
harden(data): an update by id answers the same refusal whether or not the id exists, as delete does
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
Skills-docs: none — no file under `skills/**` teaches what a by-id update answers for an id that names no record (a `git grep` of `skills/` for `RECORD_NOT_FOUND` names only the approvals record lock), so no skill line changes. The Update Record section of `content/docs/protocol/kernel/http-protocol.mdx` states the order.
10+
11+
The by-id update doors now judge the update before they look the record up — the order the by-id delete door already has.
12+
13+
- **What changes.** A caller the update is refused to — no edit permission on the object, or an external data source that refuses writes — now gets that refusal (`403 PERMISSION_DENIED`, `403 EXTERNAL_WRITE_FORBIDDEN`) for an id that names no record too, where it got `404 RECORD_NOT_FOUND`. This holds on `PATCH /api/v1/data/:object/:id`, on each row of `POST /api/v1/data/:object/updateMany` and of `POST /api/v1/data/:object/batch` with `operation: "update"`, and on every door that updates through them (the runtime dispatcher's `/data`, the `/mcp` HTTP tools, declarative update actions).
14+
- **What does not change.** A caller who may update the object still gets `404 RECORD_NOT_FOUND` for an id that names no record, and for a record its row-level access hides. Successful updates, the `405` for an operation the object does not expose, optimistic concurrency (`If-Match` / `expectedVersion`), upsert and system-context writes answer as before.
15+
- **Embedders calling the protocol directly.** `updateData`, `updateManyData` and `batchData`'s `update` operation no longer read the record before calling `engine.update`. They ask whether it exists only after the engine resolves a `null` readback, so an engine that resolves `null` for an id that names no record still gets `404 RECORD_NOT_FOUND`, and an update that moved its record out of the caller's row scope still answers `200` with `record: null`.

‎content/docs/protocol/kernel/http-protocol.mdx‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -713,6 +713,14 @@ field is left unchanged.
713713
> lacks edit permission on is *rejected* with `403 PermissionDeniedError`, not
714714
> stripped.
715715
716+
**Refusals come before the record lookup:**
717+
The update is judged before the record is looked up, as a delete is. A caller who may
718+
not update the object (no edit permission, or an external data source that refuses
719+
writes) gets the same `403` whether or not `{record_id}` names a record, and so does
720+
each row of `updateMany` and of `batch` with `operation: "update"`. A caller who may
721+
update the object gets `404 RECORD_NOT_FOUND` for an id that names no record, and for
722+
a record its row-level access hides.
723+
716724
### Delete Record
717725

718726
Delete a record by ID.

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

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -311,15 +311,16 @@ export async function runBatchDataLoop(
311311
}
312312
case 'update': {
313313
if (!record.id) throw rowRequiredIdError('update');
314-
// [#5088] Same existence gate as `updateMany` and the
315-
// single-record PATCH — a row naming no record must not
316-
// enter the write pipeline, where #4770's stored ⊕
317-
// payload merge has no stored side and #4775 blames the
318-
// hook for the resulting gap.
319-
await this.assertRecordExists(object, record.id);
320314
// [#3455] Collect the engine's LEGAL write strips per row.
321315
const dropped: DroppedFieldsEvent[] = [];
322316
const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); }, ...ctxOpt } as any));
317+
// [#5088] Same existence answer as `updateMany` and the
318+
// single-record PATCH, and in the same place: AFTER the
319+
// engine has judged the write (its refusals, then its
320+
// not-found gate ahead of every hook), so a refused row
321+
// reads the same whether or not its id names a record —
322+
// see `assertRecordExists`.
323+
if (updated == null) await this.assertRecordExists(object, record.id);
323324
omitInternalFieldsFromWriteResponse(batchSchema, updated); // [#7823]
324325
results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) });
325326
succeeded++;
@@ -335,8 +336,8 @@ export async function runBatchDataLoop(
335336
// duplicate-keyed — an authorization/update
336337
// scenario reported as a key collision — or, on a
337338
// store without a unique id constraint, wrote a
338-
// second row. Same probe as the update branch above
339-
// and the single-record path (#4620): it asks the
339+
// second row. The same `probeRecord` the update branch
340+
// above asks after its write (#4620): it asks the
340341
// database a fact, and whether the caller may WRITE
341342
// the row it proves stays #1994's decision inside
342343
// `engine.update`.
@@ -739,17 +740,19 @@ export async function runUpdateManyLoop(
739740
// handling — at best a 404 with `undefined` interpolated into
740741
// the message, at worst a where-clause with no id at all.
741742
if (!record.id) throw rowRequiredIdError('update');
742-
// [#5088] Third gap, the same shape: no existence gate. A row
743-
// naming no record went straight into `engine.update`, so the
744-
// hook pipeline ran over a payload-only record and the row came
745-
// back `INTERNAL_ERROR` from #4775's condition abort — blaming
746-
// the hook for a caller's stale id. Probe first, per row, so
747-
// this face answers what the single-record PATCH answers.
748-
await this.assertRecordExists(object, record.id);
749743
const dropped: DroppedFieldsEvent[] = [];
750744
const opts: any = { where: { id: record.id }, onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } };
751745
if (context !== undefined) opts.context = context;
752746
const updated = byIdUpdateRecord(await this.engine.update(object, record.data || {}, opts));
747+
// [#5088] Third gap, the same shape: no existence answer. A row
748+
// naming no record came back `INTERNAL_ERROR` from #4775's
749+
// condition abort — blaming the hook for a caller's stale id. It
750+
// answers what the single-record PATCH answers now, asked where
751+
// that door asks it: AFTER the engine has judged the write (its
752+
// refusals, then its not-found gate ahead of every hook), so a
753+
// refused row reads the same whether or not its id names a
754+
// record — see `assertRecordExists`.
755+
if (updated == null) await this.assertRecordExists(object, record.id);
753756
omitInternalFieldsFromWriteResponse(updateManySchema, updated); // [#7823]
754757
results.push({ id: record.id, success: true, data: updated, index, ...(dropped.length > 0 ? { droppedFields: dropped } : {}) });
755758
succeeded++;

‎packages/metadata-protocol/src/protocol.batch-row-conformance.test.ts‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import { describe, it, expect, vi } from 'vitest';
2828
import { BatchOperationResultSchema, BatchUpdateResponseSchema } from '@objectstack/spec/api';
2929
import { ObjectStackProtocolImplementation } from './protocol.js';
3030
import { assertEngineUpdateDispatch, assertEngineDeleteDispatch, assertEngineFindOnePredicate } from '@objectstack/metadata-core';
31+
import { recordNotFoundError } from '@objectstack/core';
3132

3233
const SCHEMA = { name: 'invoice', fields: { title: { name: 'title', type: 'text' } } };
3334

@@ -56,11 +57,13 @@ function makeStoreEngine() {
5657
rows.set(rec.id, rec);
5758
return rec;
5859
}),
59-
update: vi.fn(async (_object: string, data: any, options?: any) => {
60+
update: vi.fn(async (object: string, data: any, options?: any) => {
6061
assertEngineUpdateDispatch(data, options);
6162
const id = options?.where?.id;
6263
const current = rows.get(id);
63-
if (!current) throw new Error(`no such record: ${id}`);
64+
// ObjectQL's by-id not-found gate: the engine refuses a row that is
65+
// not there itself, after the write is judged and before any hook.
66+
if (!current) throw recordNotFoundError(object, id);
6467
if (data?.title === POISON) throw new Error('update exploded');
6568
const next = { ...current, ...data };
6669
rows.set(id, next);
@@ -156,10 +159,10 @@ describe('batchData rows conform to BatchOperationResultSchema (#4793)', () => {
156159
expectConformantResponse(res, 2);
157160
expect(res.results[0].data).toBeUndefined();
158161
// [#5088] Was an unclassified engine throw (`no such record`, rendered
159-
// INTERNAL_ERROR) because the row reached `engine.update` at all. The
160-
// update branch now runs the same existence probe as the single-record
161-
// PATCH, so a missing id is refused BEFORE the write with the catalogued
162-
// 404 — see `protocol.bulk-record-not-found.test.ts`.
162+
// INTERNAL_ERROR). The engine now refuses a missing id itself with the
163+
// catalogued 404 — after it has judged the write, which is why the
164+
// update branch asks nothing about existence ahead of it — see
165+
// `protocol.bulk-record-not-found.test.ts`.
163166
expect(res.results[1].errors[0]).toMatchObject({ code: 'RECORD_NOT_FOUND', httpStatus: 404 });
164167
expect(res.results[1].errors[0].message).toMatch(/not found/);
165168
});

0 commit comments

Comments
 (0)