Skip to content

fix(plugin-security): a by-id write's row re-read propagates a store fault, and judges the platform's own predicates without the caller-predicate guard - #30307

Draft
objectstack-fleet[bot] wants to merge 6 commits into
mainfrom
claude/sec-hardening-r3
Draft

objectstack-fleet[bot] wants to merge 6 commits into
mainfrom
claude/sec-hardening-r3

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Clause-②: yes (widening)
Metadata-protocol: none

维护者速读

  • 改了什么:按 id 的更新/删除在写前复读该行时,存储故障按原样抛出;平台自己拼进复读的租户墙与行级写谓词,不再被"调用方谓词守卫"当成调用方的过滤条件来审;主从(controlled_by_parent)写对主记录的复读同样处理。
  • 为什么改:原先复读的任何失败都答成记录级 403,数据源宕机被说成无权访问;字段规则隐藏了 organization_id 的调用方,在 group/isolated 姿态下对本组织记录的按 id 写被一律拒绝。
  • 风险与回滚:放宽面只限于"平台拼出的谓词不再被守卫审";调用方自己的谓词照审,读不到的行与不存在的 id 答法与其他调用方一致;squash 落地后单 commit 可整体 revert。
  • 先看:security-plugin-authz-middleware.ts(step 2.7 复读处)、predicate-guard.ts(collectConditionFields 跳过 policy 子树那一行)、security-plugin-cbp-write-gate.ts(主记录复读)。
  • 两分钟验证:pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2 src/by-id-write-row-reread.posture.test.ts,预期 19 passed。
  • 需要你做的:无,席位落地。

What changes

Before an update or delete by id, the security middleware re-reads the addressed row under the caller's context together with the platform's write predicates for that row (the tenancy wall and the row-level write predicates). Two rules now hold for that re-read (step 2.7), and the second also for the controlled-by-parent master read a detail write asks (step 2.8):

  1. A store fault propagates as raised; only a declared refusal keeps the record-level answer. The re-read's outcome is classified by readUnlessRefused, the one classification the write doors already use for a caller-context read: a row or no row as before; a declared 4xx keeps the record-level 403 PERMISSION_DENIED, one answer whatever the id names; anything else is the write's answer, unchanged (for ERR_DATASOURCE_UNAVAILABLE the transports answer 503). The step 2.8 master read keeps its 403 for every failure (its own comment records why); this PR does not change that catch.
  2. The platform's own predicates are not the caller's predicate. The write predicates ride into the re-read under one fresh $and stamped 'policy' with the spec's filter-subtree provenance mark (markFilterSubtreeProvenance, the mark step 3 already stamps on every read scope it injects), and the field-level predicate guard (collectConditionFields) does not judge a subtree so marked. Unmarked or any other mark is judged as before, and the mark does not survive a copy, a serialization or the wire. No second guard and no second read: the one re-read is asked once, and the one guard reads the existing mark.

Unchanged: a caller's own filter on a field it cannot read is refused (read and predicate write); a row the caller cannot read and an id naming no row answer as they do for every other caller; a row the caller reads but may not write keeps its 403; a system write is not the gate's; the predicate write's read question still guards the caller's verbatim predicate.

Tests

  • by-id-write-row-reread.posture.test.ts (new; real ObjectQL over SQLite, real SecurityPlugin, postures group and isolated, single as control):
    • a store fault raised by the re-read is the write's answer, the same error object, nothing written; a declared 4xx keeps the record-level 403, identical for a row and for no row; with the store answering, the same writes land.
    • a caller whose field rules hide the organization column updates and deletes a row of its organization by id; a row of another organization and an id naming no row answer that caller exactly as they answer a caller who reads the column (404 RECORD_NOT_FOUND), nothing written; the caller's own predicate on the column is still refused on a read and on a predicate write; a system write lands.
    • a controlled-by-parent detail insert by a caller whose field rules hide the master's organization column is admitted under a master of its organization; a master of another organization answers it what it answers a caller who reads the column.
    • an organization administrator whose field rules hide both assignment tables' organization column removes an in-boundary assignment by id (sys_user_position, sys_user_permission_set), admitted as for an administrator who reads the column.
  • predicate-guard.test.ts: a 'policy' subtree is not collected nor anything beneath it; the caller's own predicate beside it is still refused; an unmarked subtree, an 'author' subtree, a spread copy and a JSON copy of a 'policy' subtree are all judged.
  • authz-matrix-gate.test.ts: the Layer 0 matrix helper reads the write predicates from the re-read's platform subtree (the re-read is now { $and: [{ id }, PLATFORM] }); every matrix expectation is unchanged.

Acceptance notes

Verification

All readings below are on head b6b340a3db (after merging main) unless named otherwise.

Full plugin-security suite

pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2 under the verify lock: Test Files 243 passed (243), Tests 5349 passed | 45 skipped (5394), VERDICT command-exit 0. pnpm --filter @objectstack/plugin-security typecheck: exit 0 (tsc both configs and check:test-typecheck OK).

One-shot ablations (each restored, blob equal to HEAD, `git diff HEAD` empty)
  • Rule 1: the re-read's classification replaced by a catch-all: 2 failed (the store-fault case, group and isolated: the write answered 403 instead of raising the fault).
  • Rule 2, step 2.7 mark removed: 4 failed (own-organization update and delete, and the not-found equality, in both walled postures).
  • Rule 2, step 2.8 mark removed: 2 failed (the detail under a master of its organization answered 403).
  • Rule 2, guard skip removed: 7 failed (the unit pin plus every posture pin above).
  • Rule 2, step 2.7 mark removed, the organization administrator's pin (after the merge of main): 2 failed (group, isolated: the removal answered 403).
Gates

node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack derived 60 commands; all 60 run on b6b340a3db, every exit code captured before any pipe, every one 0; --ran reconciliation: 60 derived, 60 run, 0 NOT-MEASURED (a derived zero). An earlier sweep on the pre-merge head first read check:i18n and check:dual-build-cjs-loads as exit 3 (no built output yet) and check:query-options-erasure as exit 1 (the new test's option casts); the options were typed in this PR and all three read 0.

Dogfood

No ! in the title and no narrowing arm, so not the full battery: the ten dogfood files that drive by-id and predicate writes (write-door-unreadable-is-not-found, update-by-id-refusal-order, occ-stale-token-organization-wall, owner-anchor-and-bulk-writes, showcase-invoice-cbp, authored-row-write-scope, showcase-fls-read-mask-strip, delegated-admin-assignment-write-unreadable-row, predicate-write-unreadable-not-matched, cbp-parent-attachment-comment-gates), closure built, under the verify lock: Test Files 10 passed (10), Tests 107 passed (107), VERDICT command-exit 0.


Generated by Claude Code

…fault, and judges the platform's own predicates without the caller-predicate guard

The re-read classifies its outcome through readUnlessRefused: a declared
4xx refusal keeps the record-level 403, anything else propagates as raised.
The write predicates the platform composes ride into the re-read under one
fresh subtree marked 'policy' with the spec's filter-subtree provenance mark,
and the field-level predicate guard does not judge a subtree so marked. The
controlled-by-parent master read carries its master write predicates the
same way.

Claude-Session: https://claude.ai/code/session_01QSeKYn5CmmdShCJifm6WYj
Co-authored-by: Claude <noreply@anthropic.com>
…ates from the re-read's platform subtree

Claude-Session: https://claude.ai/code/session_01QSeKYn5CmmdShCJifm6WYj
Co-authored-by: Claude <noreply@anthropic.com>
…s hide the organization column removes an in-boundary assignment by id

Claude-Session: https://claude.ai/code/session_01QSeKYn5CmmdShCJifm6WYj
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-security, touching 3 documentable anchor(s).

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

  • content/docs/permissions/system-context.mdx (via registerAuthzMiddleware (symbol, a top-level function))
What this run could not see
  • 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); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 103 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.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 17 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 c1240a38ac2c23c8d526e476a20edeabbbefd6f6 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 6c78fb227a6fc1843c7b76a9fbdc85c0ab306fc7 — the merge of head cc1fbac302c6bc1f9b1a4929b28512285b8619ab into base c1240a38ac2c23c8d526e476a20edeabbbefd6f6, 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 6c78fb227a6fc1843c7b76a9fbdc85c0ab306fc7 && git checkout 6c78fb227a6fc1843c7b76a9fbdc85c0ab306fc7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin c1240a38ac2c23c8d526e476a20edeabbbefd6f6 cc1fbac302c6bc1f9b1a4929b28512285b8619ab && git checkout -B drift-repro c1240a38ac2c23c8d526e476a20edeabbbefd6f6 && git merge --no-ff cc1fbac302c6bc1f9b1a4929b28512285b8619ab

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

⚠️ 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 c1240a38ac2c23c8d526e476a20edeabbbefd6f6 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

security#1 · os-justin · session_01QSeKYn5CmmdShCJifm6WYj

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: b6b340a3db9bf6254c5941f56c410bc3fe85e74b
Local-runs: none
Scope: packages/plugins/plugin-security/src/security-plugin-authz-middleware.ts, packages/plugins/plugin-security/src/security-plugin-cbp-write-gate.ts, packages/plugins/plugin-security/src/predicate-guard.ts, packages/plugins/plugin-security/src/by-id-write-row-reread.posture.test.ts, packages/plugins/plugin-security/src/predicate-guard.test.ts, packages/plugins/plugin-security/src/authz-matrix-gate.test.ts + packages/spec/src/data/filter-subtree-provenance.ts, packages/spec/src/data/filter-lowering.ts, packages/spec/src/data/readable-ids.ts, packages/plugins/plugin-security/src/security-plugin.ts (readUnlessRefused, callerCannotReadAddressedRow, copyOfCallerPredicate), packages/plugins/plugin-security/src/security-plugin-addressed-writes.ts, packages/plugins/plugin-security/src/rls-compiler.ts, packages/plugins/plugin-security/src/rls-check-stored-form.ts, packages/plugins/plugin-sharing/src/sharing-plugin.ts, packages/services/service-analytics/src/strategies/objectql-strategy.ts, packages/objectql/src/engine.ts (findOne), packages/objectql/src/engine-find.ts, packages/objectql/src/engine-readable-ids-permission-formula.ts, packages/objectql/src/external-driver-floor.ts, packages/objectql/src/filter-comparand-shape.ts, packages/core/src/utils/readable-ids-driver-refusal.ts, packages/drivers/driver-sql/src/sql-driver.ts, packages/drivers/driver-sql/src/sql-driver-read-exit-fault.ts, packages/types/src/data-error-classification.ts (declaredHttpStatus)

Inputs read: PR #30307 body and file list, the net diff against main at the head above (7 files, +550/-19), the head's check-runs, and the cited code at the head. Check-runs as read at 2026-11-05 05:54 UTC: every concluded run is success or skipped (Build Core, TypeScript Type Check, Lint & Repo Gates and all Lint/Type Check lanes, Governed Surface Queue Guard, Test UI, Live E2E UI, Dogfood Regression Gate, Check Changeset, Check PR Size, PR-body guards); Test Core shards 3, 5, 6, 7 success, shards 1, 2, 4, 8 still in_progress, so the required Test Core context had not concluded — not a pass yet.

① Derived judgments

  1. Rule (1), step 2.7 — correct. The caller-context re-read's outcome is classified by readUnlessRefused, the classification the write doors already use (callerCannotReadAddressedRow, the predicate write's read question): a declared 4xx keeps the record-level 403 and skips the not-found question, one answer for a row and for no row; anything undeclared or 5xx propagates as raised, before anything is written. declaredHttpStatus reads status/statusCode in 400–599 only, so an undeclared store fault is never mistaken for a refusal.
  2. Rule (2), step 2.7 and 2.8 — correct. The write predicates ride under one fresh $and node built at the call site and marked 'policy'; a frozen or shared member cannot leave the node unmarked (the mark is first-mark-wins and no-ops on frozen input, so marking the fresh wrapper rather than the members is the right choice). collectConditionFields stops at a 'policy' node and everything beneath it; an unmarked node, an 'author' node, a spread copy, a JSON copy and a structured clone are judged (the mark is a non-enumerable symbol key; none of those carries it). The mark reaches the guard on the real engine: findOne spreads the query into its AST without rebuilding where, and the token/lowering rewrites copy a replaced node's mark onto its replacement (filter-lowering.ts).
  3. Rule (3) — holds. A row the caller cannot read and an id naming no row still reach callerCannotReadAddressedRow and answer not-found as for every caller; a row the caller reads but may not write answers 403; a caller the read door refuses answers 403 for every id; a system write is not the gate's; the predicate write's read question still copies and judges the caller's verbatim predicate. Pinned on a real engine under group and isolated with single as control (19 pins), plus the organization administrator's assignment removal.
  4. No existence signal is added by rule (1): the re-read is asked for every id alike, and the propagated fault is the read door's own answer to this caller for a direct read of the same object, so the write door discloses nothing the read door does not.
  5. The Layer 0 matrix helper now reads the platform subtree's members; every matrix expectation is unchanged — correct.
  6. Public surface: collectConditionFields / assertReadableQueryFields are exported from @objectstack/plugin-security; their semantics widen (a marked platform subtree is no longer judged). Clause-②: yes (widening) is the right declaration.

② Semver level

@objectstack/plugin-security: patch, Clause-②: yes (widening), Metadata-protocol: none — consistent. The widening is the admitted writes; the changed failure status is the write door following the contract the read door and readUnlessRefused already declare, not a narrowing of a published acceptance set, and the error the caller now receives is the one a direct read already gives it, so no (narrowing) arm is owed. Level follows the kind: none ⇒ patch. The changeset's wording must change (③ must-fix below); its declarations and level stay.

③ Boundary flags

  • Must-fix — public text below rule level. The order's disclosure rule (security-findings §6: title, body and changeset carry the hardening rule only) is breached in three places: the changeset's two sentences that state what the doors answered before this change; the body's 维护者速读 second bullet (为什么改); and the ablations block, which narrates the doors' answers with each rule removed. Each must be reduced to the rule form (the "now holds" sentences already present are at rule level and suffice); the ablations block may keep pin counts only. The changeset ships in CHANGELOG, so it is the first to fix. This is a FAIL.
  • Open question in the body (step 2.8's catch-all): answered below under ④; it is a ruling request for the seat, not a change for this PR.
  • Acceptance note on fix(plugin-security): an organization administrator's assignment removal is judged on the stored row, and its refusal names only values the caller can read #30248: verified — d0da8802a0 is on origin/main and merged into the branch at f3d93267d6.
  • Prose checked true of the tree: the mark is the one step 3 already stamps on every injected read scope; no second guard and no second read; the mark does not survive a copy, a serialization or the wire; ERR_DATASOURCE_UNAVAILABLE maps to 503 at the transports; 19 pins in the new file; the predicate write's read question judges the caller's verbatim predicate.
  • Should-fix — rule (2)'s declared reach is narrower than its real reach. The guard's skip applies to every 'policy'-marked subtree that reaches step 2.9 inside a caller-context read's where, and the tree already marks such subtrees outside the two doors this PR names: the analytics read-scope merge (withReadScope and its FK expand hand a marked scope to aggregate under the caller's context) and a sibling middleware's sharing scope when it composes before this one. All are platform predicates — the rule's own direction, nothing of the caller's is admitted — but the changeset and body should name the reach, and a pin on the analytics face is owed (a follow-up card is acceptable).
  • Author is objectstack-fleet[bot]; no login note.

④ Design judgment

  • predicate-guard.ts: the skip is node-level and fail-closed — null and 'author' are judged, only a positive 'policy' is skipped — so the tree's "unmarked ⇒ caller's" direction holds. Attempts to break rule (2) from the wire fail by construction: JSON carries no symbol key; a spread copies enumerable keys only; structuredClone drops symbols; a caller's $and/$or wrapping arrives unmarked and is judged; first-mark-wins prevents a later boundary re-vouching a policy node as the caller's. The platform never copies a caller-authored predicate into a marked subtree: every rewrite (filter-lowering, filter-comparand-shape, rls-compiler, rls-check-stored-form, readable-ids-driver-refusal) copies the replaced node's own mark, so a caller node yields an unmarked or 'author' rewrite; the sharing, RLS and analytics scopes are built from policy and shares, never from the caller's where; the predicate write's read-door scope is an id list; view filters arrive over the wire unmarked. In-process code running under the caller's context (a hook, a plugin) can import the mark, but that code can equally set a system context — not a boundary this guard draws. Holds.
  • security-plugin-authz-middleware.ts (step 2.7): the fresh wrapper is the right place for the mark. Step 3's vouch on the re-read is unchanged (the root where keeps its identity, so it is still vouched as the caller's), and the inner 'policy' node now wins positionally inside it, which also decides what a driver-side refusal raised from inside the platform's predicates may name — the direction is withholding, a tightening the PR does not claim; mechanism in the reviewer's note. The store-fault propagation adds no per-id answer (① 4). Holds.
  • security-plugin-cbp-write-gate.ts (step 2.8): the mark holds as at 2.7. The catch-all stays by the recorded ruling the comment names and is consistent with rule (3) as the seat set it; after this PR it differs from step 2.7 for a store fault only, so a detail write under an unreachable store is answered as a denial where the master's own door would now raise the fault. That asymmetry needs a ruling, not a code change here — the seat should put it to the maintainer; recorded here as should-fix.
  • Tests: the posture file pins both rules and the rule (3) invariants on a real engine with the real plugin, including the row/no-row equality under a declared refusal and the equality of the hidden-column caller's answers with the plain caller's; the unit pins cover the copies. The analytics reach (③ should-fix) is unpinned.

Implemented-by: claude/sec-hardening-r3
Reviewed-by: session_01QSeKYn5CmmdShCJifm6WYj

VERDICT: FAIL

…les only, and name every platform scope the predicate guard does not judge

The changeset states the two rules without narrating earlier answers, and
names where the provenance rule reaches: the by-id write re-read, the
controlled-by-parent master read, the analytics read-scope merge handed to an
aggregate under the caller's context, and a sharing read scope composed
before the security middleware. A REST-level pin covers the analytics face.

Claude-Session: https://claude.ai/code/session_01QSeKYn5CmmdShCJifm6WYj
Co-authored-by: Claude <noreply@anthropic.com>

This branch has not been deployed

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

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant