Skip to content

fix(plugin-security)!: evaluate the insert-side RLS check on the row that will be stored, after beforeInsert - #16805

Draft
os-trump wants to merge 5 commits into
mainfrom
claude/issue-16608-insert-check-post-image
Draft

fix(plugin-security)!: evaluate the insert-side RLS check on the row that will be stored, after beforeInsert#16805
os-trump wants to merge 5 commits into
mainfrom
claude/issue-16608-insert-check-post-image

Conversation

@os-trump

@os-trump os-trump commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16608

The insert-side RLS check is evaluated on the row that will be stored — after beforeInsert — instead of on the caller's raw payload, so insert and update judge the same thing.

Clause-②: yes — the security middleware's accept / refuse behaviour changes. needs:contract-review is carried on this PR and on the card. The contract-review tier is unavailable, so this stays in draft; the ruling permits the label as compensation, and nothing below was relaxed on that account.


The ruling this implements

Maintainer, 2026-09-07, director seat, summon #17, decision batch #3, verbatim: 「同意」 (issue comment 5572345610).

The security middleware's insert post-image becomes the hook-mutated payload — the row that will be stored — so insert and update judge the same thing.

⛔ Option 2 (keep the order and write the contract that a checked field must arrive from the caller, plus an os validate rule) is refused, not deferred: it institutionalises the contradiction — the caller sending the value the hook exists to make un-sendable — and adds a permanent rule to keep it. Nothing here drifts back to it.

The two measurements the ruling required first

Both were taken on origin/main @ 941232040 before the evaluation point moved, and posted in full on the card (comment 5579761278). Summary:

① The bypass case — IT REPRODUCES. The ruling flagged this as reasoning, not measured. It is now measured, on a real engine with a real SecurityPlugin and a real driver. A caller holding org_a sends employer_org: org_a on a child whose parent belongs to org_b; the app's runAs: 'system' stamp reads the parent outside RLS and overwrites the field.

payload before this PR stored row (read off the driver's table)
{ employer: 'emp_b', employer_org: 'org_a' } ADMITTED { employer: 'emp_b', employer_org: 'org_b' }
{ employer: 'emp_a' } — left to the hook (the card's row 2) 403 PERMISSION_DENIED nothing stored
{ employer: 'emp_a', employer_org: 'org_a' } (the card's row 1) ADMITTED employer_org: 'org_a'

Today's order admits a row whose stored scope the caller does not hold. That is a cross-organization write, and it is what regrades the card to priority:p1. After this PR: row 1 is refused with nothing stored, row 2 is admitted and stores org_a, row 3 is unchanged.

② The census — non-zero, and large: 29 hooks. Reported as a fork, with the list, on the card. Every beforeInsert hook whose derivation reads a caller-supplied field: 4 in this repo (pinyin's __search companion, plugin-auth's sys_member.role canonicalisation, showcase's title normaliser, app-crm's stage to probability), 7 in ats @ c3c6526 (all of them, all runAs: 'system'), 18 in hotcrm @ d47e37a. The per-hook table is on the card.

⛔ Not a silent fallback to option 2. What this PR installs instead of 29 per-hook declarations is one invariant, which is exactly the guard the census was asking for:

After this change, a stored row always satisfies the insert check, whatever the caller sent.

That is what makes a hook's input face stop being a security surface. The census's worry was laundering — caller sends X, hook derives checked-Y from X, Y reaches the store unjudged. After this change Y is precisely what is judged. And relative to today it is a strict narrowing in the direction that matters: today the caller can satisfy the insert check by sending any in-scope value in the checked field, because the check reads the payload and the payload is entirely the caller's. Measurement ① is that fact exercised. The insert check currently offers no guarantee about the stored row; after this it offers exactly that one.

Whether the 29 input faces still warrant per-hook declaration work as a follow-up is a PM/maintainer call and is not blocking this PR. My recommendation is on the card.


What changed

@objectstack/objectqlOperationContext.postHookWriteImageCheck. An optional judgement an enforcement layer installs and ObjectQL.insert runs once the beforeInsert chain has produced the row. It sits immediately after the post-hook declared-field door (#13657) and before every producer with a side effectresolveSystemInsertOrganization, encryptSecretFields (which writes a sys_secret row), applyAutonumbers (which CONSUMES a sequence number), validation, the statement — so a refusal still costs nothing, the same rule #8682 wrote for the door. Rows the declared-field door culled from a partial batch are skipped: they will not be written. honoured is set before evaluate, so a throwing check still reads as honoured — the flag answers "did the seam run", never "did the write pass".

@objectstack/plugin-security — step 3.6 installs instead of matching. For insert the compiled check filter goes on the operation context; for update nothing changes (that path already merges its caller pre-image, and the ADR-0090 D10 delegator half rides along unchanged). The filter is still compiled in the middleware, where the caller's permission sets, the delegator's, the staged membership and this request's context are all resolved. (That staging is what landed for #16607 in PR #16722; it is untouched here.) Only the IMAGE is deferred; deferring the compilation would move authorization inputs into the engine's timeline for no gain. Both verbs share one refusal closure, so a caller cannot tell which side judged.

Fail-closed on a seam that never runs. A middleware that installed the judgement and finds honoured unset refuses the write and logs at ERROR, with a developer message that says the check was not evaluated rather than that it failed — different facts, and an operator debugging one must not be handed the other. ⛔ Deliberately not softened to a warning: a middleware that cannot say a write was checked must not report that it was.

Scope kept, not widened. Only objects governed by a permission set that EXPLICITLY declares check, single-row inserts, non-system caller — the gate's existing scope. Batch inserts were never post-image validated here and still are not.

The conformance case

packages/plugins/plugin-security/src/insert-check-post-image.test.ts — one proposition, the scoping field's landing decides, written once and asserted for both verbs on both driver families (driver-sql better-sqlite3 :memory:, and driver-sqlite-wasm):

  • lands IN scope, admitted, and the stored row carries that value;
  • lands OUT of scope, refused on the ADR-0112 envelope (PERMISSION_DENIED / 403 / the catalog sentence / the developer line), and nothing moved — refusal and non-landing are asserted as separate facts, because a gate that refuses after the row lands is not a gate.

The insert arm's out-of-scope cell differs from its in-scope twin by the parent alone: both payloads carry an in-scope employer_org, so they are indistinguishable to the pre-hook image and no green there can come from it. Plus the card's two rows (bare payload admitted; duplicated stamp still admitted), and the fail-closed leg against an engine double that ignores the seam. 13 cells, all green.

Ablation (mutation on disk proven by anchor grep counts plus git hash-object against the HEAD blob; restore leg proven byte-identical, git diff HEAD empty; both packages resolve to SOURCE in this package's vitest config, so no rebuild stands between the edit and the run). Mutating the installed judgement to read opCtx.data instead of the rows the engine hands it — the defect put back, minimally, with the seam still in place so the fail-closed leg is not what turns the colour:

predicted: RED on exactly the four insert cells whose verdict differs between the two images
measured:  Tests  4 failed | 9 passed (13)
           x lands OUT of scope (driver-sql)      x bare payload (driver-sql)
           x lands OUT of scope (sqlite-wasm)     x bare payload (sqlite-wasm)

Tests and gates

Measured on the final commit of this branch, cd09d3b99.

pnpm --filter @objectstack/plugin-security test 103 files / 1915 tests passed
pnpm --filter @objectstack/objectql test 285 files / 4808 tests passed
typecheck, both packages green; plugin-security's test layer compiles at 0 errors / 0 ledger entries
dispatch-gates.mjs --ran reconciliation 83 derived, 83 run, 0 NOT-MEASURED, 0 UNRUN

Four gate families the diff derives were red and are fixed in it: check:adr-0087-registration (the changeset now carries not-required (no-migration-prescription) — an enforcement-ORDER change moves no authorable key, spelling or stored shape), check:engine-double-contract (the fail-closed leg's double routes delete/update/findOne through the real dispatch predicates; the three new pinned seams are recorded), check:test-source-alias plus check:type-source-resolution (the second driver family is read from the producer's SOURCE on both axes — a vitest alias and a bare-key tsconfig paths rule; measured: the paths route adds zero diagnostics from other packages here, so PR #12570's cost did not recur), and check:doc-authoring (the tracker id is out of the runtime string and stays in the adjacent comment).

Three gates report exit 3 — PREREQUISITE NOT MET, i.e. NOT MEASURED, neither green nor red: check:i18n, check:type-check-debt and check:dual-build-cjs-loads each need the whole workspace built first. That is a CI-owned run; declared here rather than silently skipped.

pnpm lint — a proven narrowing, not a skipped run. Run over the six source files this diff touches: --format json reports 6 files linted, 0 errors, 0 warnings. The narrowing is sound because this repo runs one eslint.config.mjs which never enables type-aware linting for any file (no parserOptions.project, no typed @typescript-eslint rules — stated and measured with a positive control in that file's own comment at the QUERY_OPTIONS_TEST_GLOBS block), so nothing in this diff can move the verdict on a file it does not touch. The repo-wide sweep remains CI's.

Reviewer's fast path

The whole change is two hunks. In packages/objectql/src/engine.ts, insert() calls one new optional callback after the beforeInsert dispatch and before the first producer. In packages/plugins/plugin-security/src/security-plugin.ts step 3.6, the insert branch installs that callback where it used to match a filter against opCtx.data; the update branch is the old code, unmoved. Everything else is the conformance file, the doubles learning to model the engine, and gate bookkeeping.

验收备注

  • The update path has the identical hole, and this PR does not fix it. Measured on this branch, both drivers: an update that repoints the parent so the beforeUpdate stamp rewrites the checked field after the middleware merged its pre-image is ADMITTED, and the row is stored in an organization the caller does not hold ({ employer: 'emp_b', employer_org: 'org_b' } for a caller holding only org_a). Filed as plugin-security: the UPDATE-side RLS check post-image is pre-image + change set evaluated before beforeUpdate, so a hook-stamped scoping field can move a row into an organization the caller does not hold #16790. Not fixed here on purpose: the ruling names the insert side and treats the update path as the correct reference, so extending the evaluation point to a second verb is a second accept/refuse change that wants its own decision rather than riding in as a bounded in-place fix. The mechanism this PR adds is directly reusable for it. The conformance file states out loud that it does not claim that route, rather than pinning today's answer there — a test asserting the defective behaviour would advertise a guarantee the runtime does not deliver.
  • check-clause2-carriers --pair 16805 reports exit 4 — NOT clause-② legible — and the reason is the claim comment's SHAPE, not a missing declaration. The line is written, correctly and in the fixed spelling, in the PM's claim comment on plugin-security: the insert-side RLS check post-image is the raw caller payload evaluated before beforeInsert, so a field a hook stamps can never satisfy an insert check — the caller must send the value the hook will overwrite anyway #16608 — but that comment opens ## Claim: …, and CLAIM_COMMENT_MARKER (/^\s*>?\s*Claim(?:ed)?\s*:/mi) does not accept a heading-style claim, which the checker's own doc-block says in so many words. So the predicate finds the declaration "on the thread" and not "in the claim comment". Positive control, same document with the two # characters removed from that one line: exit 0, "the clause-② declaration is readable in the fixed spelling and both carriers agree." ⛔ Not corrected from here — the claim comment belongs to the claiming seat, and the checker itself says the declaration IS the judgement and must not be filled in on that seat's behalf. One ## removed on that comment makes the pair legible.
  • noted, not filed: the check gate has never post-image-validated batch inserts (!Array.isArray(opCtx.data) in its own guard) or predicate (multi: true) updates. Both are pre-existing, both are documented in the gate, and neither moves here.
  • noted, not filed: this change gives engine doubles a new obligation — an executor that stands in for ObjectQL.insert must run the installed judgement or the middleware refuses. Three call sites in two existing test files were updated. check:engine-double-contract covers delete/update/findOne but not this seam, so a future double will learn about it from a red test rather than from that gate.

Authored by Claude Code in session session_012zTkyNHJ7TkuN2oXtP5x37.

…ill be stored

The security middleware runs before the engine's operation, so for an insert
its post-image was the caller's payload as it arrived — ahead of every
`beforeInsert` hook. A denormalised scoping field is what an RLS predicate
compares (ADR-0055) and what an app stamps server-side so a caller cannot
choose it, so the gate judged a value that never lands and ignored the one
that does. Measured both ways on 17.3.0: a payload leaving the field to the
hook was refused while the identical payload carrying it was admitted, and an
insert naming an in-scope organization on a parent in another organization was
admitted with the parent's organization stored on it.

`OperationContext` gains `postHookWriteImageCheck`, a judgement an enforcement
layer installs and `insert()` runs once the hook chain has produced the row —
after the post-hook declared-field door, before every producer with a side
effect. plugin-security installs its compiled check filter there; the update
path, which already merges its pre-image, is unchanged. A seam that was
installed and never run refuses the write rather than vouching for it.

One conformance cell, both verbs, both drivers: the scoping field's landing
decides. Refs #16608, ruling 2026-09-07.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
The insert-side RLS check is installed on the operation context and run by
`ObjectQL.insert`; a double whose executor is a bare `async () => {}` models
an engine that carries a write past a gate that never ran, which the
middleware refuses fail-closed. The doubles in `security-plugin.test.ts` and
`rls-check-membership-staging.test.ts` now run the judgement the way the
engine does — flag first, then evaluate — so they model the engine instead of
a looser approximation of it.

Also fixes the fail-closed log call to the `error(message, error?, meta?)`
contract arg order (#5637).

Refs #16608.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
- the changeset carries its ADR-0087 disposition: an enforcement-ORDER change
  moves no authorable key, spelling or stored shape, so no conversion entry
  and nothing for an upgrader to hand-edit (check:adr-0087-registration);
- the fail-closed leg's engine double routes delete/update/findOne through the
  real dispatch predicates, so it cannot be looser than ObjectQL
  (check:engine-double-contract);
- `@objectstack/driver-sqlite-wasm` — the conformance cell's second driver
  family — is read from the producer's SOURCE on both axes: a vitest alias
  (check:test-source-alias) and a bare-key tsconfig `paths` rule
  (check:type-source-resolution). Measured: the paths route adds zero
  diagnostics from other packages here; the test layer still compiles at 0.

Refs #16608.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
…untime string

`check:engine-double-contract --write` records the three newly-pinned seams in
the #16608 conformance file's engine double — new pinned coverage the ledger
had not learned about yet.

The fail-closed developer message no longer carries the tracker id: a runtime
string reaches authors and operators, none of whom can resolve `#NNNN`
(check:doc-authoring). The id stays in the adjacent comment, where the reader
who can resolve it is already looking.

Refs #16608.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/objectql, @objectstack/plugin-security, touching 5 documentable anchor(s). ⚠️ 2 changed file(s) yielded no anchor (packages/plugins/plugin-security/tsconfig.json, packages/plugins/plugin-security/vitest.config.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/data-flow.mdx (via OperationContext (symbol, a top-level interface))
  • content/docs/permissions/system-context.mdx (via OperationContext (symbol, a top-level interface))
What this run could not see
  • 2 changed file(s) yielded no anchor (packages/plugins/plugin-security/tsconfig.json, packages/plugins/plugin-security/vitest.config.ts) — pages documenting those are invisible to this run
  • 4 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 26 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json dd2184ae255dde66b66a042da020dcc5e54fa478packageMentionDocs.

Which tree this was computed on

This run read content/docs from 4d0db65b0eb7b1ab62135f6ef506a397c9d0d03c — the merge of head cd09d3b99f603e9bfada1ae6fd53cf9ef2fd41a7 into base dd2184ae255dde66b66a042da020dcc5e54fa478, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 4d0db65b0eb7b1ab62135f6ef506a397c9d0d03c && git checkout 4d0db65b0eb7b1ab62135f6ef506a397c9d0d03c
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin dd2184ae255dde66b66a042da020dcc5e54fa478 cd09d3b99f603e9bfada1ae6fd53cf9ef2fd41a7 && git checkout -B drift-repro dd2184ae255dde66b66a042da020dcc5e54fa478 && git merge --no-ff cd09d3b99f603e9bfada1ae6fd53cf9ef2fd41a7

node scripts/docs-audit/affected-docs.mjs --json dd2184ae255dde66b66a042da020dcc5e54fa478

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs dd2184ae255dde66b66a042da020dcc5e54fa478 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation tests tooling labels Sep 8, 2026

Copy link
Copy Markdown
Contributor

Contract review (CONTRACT_REVIEW_TIER, isolated seat) — PR #16805 @ cd09d3b99

Verdict: CHANGES REQUIRED — one residual path where the row the seam judges is not the row that is stored (F1). Everything else the ruling asked for is present and verified; the remedy for F1 is small (a reorder or a narrowed claim plus a pin), and nothing found here reopens the refused option 2.

Ruling implemented: YES, exactly — the insert post-image is the hook-mutated row; the update path is untouched; option 2 is not present in any form. Nothing in the PR body was taken on trust: every claim below was re-read on refs/review/16805 (head cd09d3b99, merge-base b38821d1c) against origin/main (094b8fd9c).

The ruling (issue #16608, comment 5572345610, quoted verbatim)

Ruling recorded — 1: the insert-side RLS check is evaluated on the row as it will be written, after beforeInsert, the way the update path already judges its merged image — two measurements first (director seat, summon #17, decision batch #3, 2026-09-07)

Provenance (who / verbatim / where): maintainer, live PM chat with the director seat (session_01XesLUWmuhjuRwmU618AZ1M), 2026-09-07T14:5xZ, batch #3 presented as 1(1) · 2(1) · 3A · 4C · 5A with this card as item 1 recommending 1 (triage's four-facet block 5572151910, with its hard premise); reply, verbatim: 「同意」.

Ruled. The security middleware's insert post-image becomes the hook-mutated payload — the row that will be stored — so insert and update judge the same thing. Option 2 (keep the order and write the contract that a checked field must arrive from the caller, plus an os validate rule) is refused: it institutionalises the contradiction the card names, the caller sending the value the hook exists to make un-sendable, and adds a permanent rule to keep it.

Whose ruling: the comment is authored by the director seat (hotlong, MEMBER) and records a maintainer decision by verbatim quote (「同意」) to the seat's option 1 recommendation. So: a seat-recorded maintainer ruling, not a seat's own ruling. The two preconditions it set (bypass measurement, hook census) were both posted on the card before the evaluation point moved (comment 5579761278); the bypass reproduced (regrade to p1 executed in 5580367981), the census came back 29 and was reported as the fork the ruling asked for, not as a fallback.

Numbered verification

  1. Diff vs merge-base (git diff --stat b38821d1c..cd09d3b99, 11 files, +893/−83): .changeset/insert-check-post-image.md · packages/objectql/src/engine.ts · packages/plugins/plugin-security/{package.json,tsconfig.json,vitest.config.ts} · packages/plugins/plugin-security/src/{security-plugin.ts,security-plugin.test.ts,rls-check-membership-staging.test.ts,insert-check-post-image.test.ts} · pnpm-lock.yaml · scripts/engine-double-contract.pinned.json. Governed paths: NO — none of docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md, content/docs/releases/** is touched.

  2. Evaluation order, read on both refs. origin/main step 3.6 builds postImage = { ...opCtx.data } for insert and matches the compiled check there, before next() — i.e. before the engine's beforeInsert dispatch (security-plugin.ts:2773-2816 on main). Head: insert installs opCtx.postHookWriteImageCheck = { evaluate } (security-plugin.ts:2890-2903) and the engine calls it at engine.ts:10375-10384 — after triggerHooks('beforeInsert') (10257), after the seal (10275-10279) and the post-hook declared-field door (10339-10349), and before resolveSystemInsertOrganization (10408), encryptSecretFields (10456), the strips, validation, applyAutonumbers (10691) and the driver. executeWithMiddleware (3720-3741) hands the executor the same opCtx object the middleware saw, so the installed object and the honoured flag are one reference. The rows judged are rowHookContexts[i].input.data, the same objects rows is built from at 10440. Verified: the hook-mutated row is what the check sees, with no double evaluation (one evaluate per operation, over all live rows).

    • (a) No window for a hook-derived value: confirmed for every value a beforeInsert hook can write. Culled rows (partial batches) are skipped and never written.
    • (b) Tenant column: if the policy's check names the tenant column, the seam sees the hook-stamped value. But the Layer 0 write wall (security-plugin.ts:2990-3060) still judges opCtx.data pre-hook, and driver-sql's injectTenantOnInsert (12602-12614) fills the tenant column only when it is absent/empty — an explicit value written by a hook is stored as-is. So a hook that writes organization_id decides the stored tenant with no post-hook Layer 0 judgement. Pre-existing, outside the ruling's wording, and the seam this PR adds is the fix's obvious host — F2.
    • (c) Fail-closed: an unevaluable/malformed check compiles to RLS_DENY_FILTER (rls-compiler.ts:352-358, 460-462), which matchesFilterCondition cannot satisfy, so evaluate throws denyCheck(); an evaluate that throws propagates out of engine.insert (the seam sits outside the try at 10430) and the write is refused. honoured is set before evaluate so a throwing check reads as honoured. A seam that never runs is refused after next() (security-plugin.ts:3309-3327) on the same ADR-0112 envelope, logged at ERROR. Both directions closed.
    • (d) Update ordering: unchanged — the else branch at security-plugin.ts:2905-2919 is the pre-PR code (extractSingleIdgetCallerPreImage{ ...pre, ...opCtx.data }satisfiesCheck), still evaluated before next(). The beforeUpdate twin of this defect is measured and filed as plugin-security: the UPDATE-side RLS check post-image is pre-image + change set evaluated before beforeUpdate, so a hook-stamped scoping field can move a row into an organization the caller does not hold #16790; not pinned as today's answer — correct under PD chore: version packages #10.
    • (e) Bulk paths: insertManyinsert(array, { __partialRowErrors }) (10918-10920) reaches the seam, and the seam receives every live row. But the middleware's own guard !Array.isArray(opCtx.data) (2839) never installs the judgement for an array payload, so no array insert is check-gated — on main or on head. That path is caller-reachable: @objectstack/rest import-runner.ts batches CREATE rows through createManyData/insertManyData, and metadata-protocol createManyData (12295-12307) calls engine.insert(records, { context }). Pre-existing and noted in the PR body as "not filed" — F3.
    • Residual: one path where the checked row ≠ the stored row on a checked field — F1. After the seam, for a non-system caller, stripReadonlyFields (rule-validator.ts, invoked at engine.ts:10502-10520) deletes a caller-supplied value on a static-readonly field when it is not hook-written, and the engine then re-defaults the taken keys via applyFieldDefaults. A check over a readonly scoping field (the natural shape for a server-stamped field) therefore judges the caller's in-scope value and the store receives the defaultValue — or NULL if there is none — whenever the hook did not write the key (e.g. its early return when the parent key is absent). The seam doc's sentence "Nothing between here and the driver adds a value the caller could have steered" is true; the changeset's invariant "a stored row always satisfies the insert check, whatever the caller sent" is not. Not a cross-org write (the substituted value is metadata-owned), pre-existing under the old order too, but it is exactly the shape the ruling was written against, one layer down.
  3. Clause-②. Body carries Clause-②: yes; agreed — the middleware's accept/refuse set moves. The changeset states FROM/TO per case: (i) payload leaving the stamped field to the hook: FROM 403 TO admitted; (ii) payload naming an in-scope value on a parent in another org: FROM admitted (stored with the parent's org) TO refused, nothing stored; (iii) payload duplicating the stamp in-scope: unchanged; (iv) hosts/engine doubles that execute a checked insert without running the seam: FROM admitted TO refused (403, ERROR log) — present in the changeset text. Scope unchanged (explicit check, single row, non-system caller). Agrees with the body.

  4. Changeset. @objectstack/plugin-security: minor, @objectstack/objectql: minor; both are in the fixed group. node scripts/check-changeset-no-major.mjs --base origin/main --head refs/review/16805 → exit 0; check-adr-0087-registration.mjs[BREAKING+bang] not-required (no-migration-prescription) parsed with its reason. Level graded by hand against batch [WIP] Add query enhancements and advanced validation features #35 "WHICH LEVEL" (pr-automation.yml:667-682): objectql adds a new exported interface PostHookWriteImageCheck and a new OperationContext member → additive widening → minor ✓; plugin-security is an accept-set narrowing with no new public surface → during the launch window minor + BREAKING banner + ADR-0087 disposition ✓ (major refused by the guard). [finding] The changeset LEVEL axis is blind to every NESTED package: packages/*/src/** matches one segment, so 51 of 74 workspace packages (all drivers/services/adapters) can pair Clause-②: yes with patch and stay green #16713 blind spot applies: packages/plugins/plugin-security/src/** is nested, so the LEVEL axis records no package for it and its green is "did not look"; my own grade above stands in for it.

  5. Tests. insert-check-post-image.test.ts: the proposition is written once and run for both verbs on both driver families (13 cells). Pins that redden on revert: the insert out-of-scope cell differs from its in-scope twin by the parent alone (both payloads carry in-scope employer_org), so the pre-hook image cannot turn it green; the bare-payload cell pins "the caller need not send the stamped value"; the author's ablation (judge opCtx.data instead of the engine rows) reports exactly those 4 insert cells red, 9 green — consistent with what the cells assert, not independently re-run from this seat (no checkout). Negative controls: the duplicated-stamp cell (unchanged behaviour) and the update arm (unchanged code). Refusal AND non-landing are asserted separately off the driver's table. The fail-closed leg pins the unhonoured seam on the ADR-0112 envelope with the "not evaluated" developer text and an ERROR log. Engine doubles in two existing files now run the seam (runEngineWriteBody) and the three new pinned doubles are recorded. Typecheck: package.json typecheck runs tsconfig.test.json via check:test-typecheck, so the test layer compiles; driver-sqlite-wasm gets a source alias on both axes. No .skip/.only/.todo in the diff. Gaps: F4.

  6. CI on head cd09d3b99. 41 check runs: 33 success, 6 skipped, 2 failure — both are Part-of PR must not also close its card (check:partof-closing-keyword): four of the five commits carry Refs #16608 while the body says Fixes #16608; the gate is advisory (not a required context) and its own log says no author action clears it on a pushed branch. Everything else green, including Test Core 1-6, Type Check (source/workspace/consumer/debt), Lint & Repo Gates, Temporal Conformance, Governed Surface Queue Guard, Check Changeset. mergeable_state: unstable (draft; the advisory red). Branch is 19 commits behind origin/main (094b8fd9c), merge-base b38821d1c; not a conflict, but a re-merge before landing is the queue's call.

Findings

  • F1 — (blocking) the post-seam static-readonly strip + re-default can store a value the seam did not judge. engine.ts:10502-10520 runs after the seam at 10375. Expectation: either (preferred) move the two side-effect-free strips (stripRuntimeOwnedFields, stripReadonlyFields + its re-default) ahead of the seam — they read only suppliedPerRow, rowHookWrittenKeys, schemaForValidation and options, all resolved before it — so the seam judges the row after every value-changing pass and before every side-effecting one; and add one cell (readonly scoping field, caller sends in-scope value, hook does not write, no/foreign default) asserting the stored row satisfies the check or the insert is refused. If the reorder is judged riskier than it looks, the acceptable alternative is to narrow the changeset's and the seam doc's invariant to name the engine-owned passes that still run after the seam (tenant fill of an absent column with the caller's own org, readonly strip/re-default, autonumber, secret refs, audit stamps) and pin the readonly case as a documented boundary — but the current wording must not ship as written.
  • F2 — (not blocking, file it) the Layer 0 tenant write wall still judges the pre-hook image, and injectTenantOnInsert preserves an explicit tenant value, so a beforeInsert hook that writes the tenant column decides the stored tenant unjudged. Expectation: a card (sibling of plugin-security: the UPDATE-side RLS check post-image is pre-image + change set evaluated before beforeUpdate, so a hook-stamped scoping field can move a row into an organization the caller does not hold #16790) to install the Layer 0 write filter on the same seam; the seam already hands over the rows.
  • F3 — (not blocking, file it) array inserts are never check-gated (!Array.isArray(opCtx.data) at security-plugin.ts:2839), and createManyData/insertManyData reach engine.insert(array, { context }) from the REST import runner. On head the seam already receives all live rows, so lifting the insert half of that guard is now a small change. Expectation: file rather than "noted, not filed"; a caller-reachable route around a p1 gate should have a number.
  • F4 — (tests) two claims without a pin through the seam: (i) an unevaluable check (RLS_DENY_FILTER) refuses the insert via evaluate; (ii) "a refusal costs nothing" — no cell asserts that a refused insert consumed no autonumber and minted no sys_secret row. Expectation: one cell each, or cite the existing pin that covers it.
  • F5 — (process) check:partof-closing-keyword red on both runs: Refs #16608 in commits 674914d0d, 9f8402f64, 787e278b2, cd09d3b99; body Fixes #16608. Advisory, not clearable without a rewrite (forbidden). Expectation: maintainer weighs at landing; the squash message will carry Refs, the body's Fixes is what closes the card.
  • F6 — (hygiene) pnpm-lock.yaml carries more than the one devDependency add: 7 esbuild@0.28.1 peer-resolution entries collapse (11 → 4 references vs merge-base). Install/Build/Validate Package Dependencies are green, so it is consistent, but it is unrelated normalisation riding along. Expectation: note it in the body or drop it.
  • F7 — (stale body line) the check-clause2-carriers exit-4 note describes the ## Claim: heading; the PM has since posted a column-0 Claim: … · Clause-②: yes line (5580367981). Expectation: re-run --pair 16805 and update the body line so the next reader is not sent to fix a carrier that is already fixed.

Maintainer-only merge: yesfix(…)! breaking, needs:contract-review on both carriers, Clause-②: yes, p1 security narrowing, draft; and F1 is a change to the security seam itself. This seat did not approve, request changes, label, edit or push anything.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

3 participants