Repository navigation
fix(plugin-security): explain's delete and transfer verdicts on a controlled_by_parent record come from the master-detail write check - #22549
Conversation
…ed_by_parent record, beside each verb's door Enumerates every operation security/explain answers against step 2.8 of the write path, at three layers: the engine over a deps bag, the registered service beside the middleware's by-id write of each verb, and the REST explain route beside each verb's REST door (PATCH, DELETE, and the PATCH that writes owner_id) for the master's editor and a non-editor. Red on this commit for delete and transfer; the fix follows. Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude <noreply@anthropic.com>
…trolled_by_parent record come from the master-detail write check The write path runs the master-detail write check (step 2.8) on every by-id write of a controlled_by_parent record and hands the record's ownership floor over to it (step 2.7). explain asked the check for an update only, so a delete kept its owner_only_deletes floor and a transfer read the sharing gate's abstention as writable. explain now asks the served member for every by-id write step 2.8 runs on. The check judges edit access to the master whatever the record's verb, so the update answer it computes is each verb's answer. The explain wiring carries step 2.7's coverage vouch for a delete as it did for an update. No second copy of the check, no spec change. Claude-Session: https://claude.ai/code/session_01WYYhVJ78u7PhwFViWo1EmQ Co-authored-by: Claude <noreply@anthropic.com>
…plain-write-verb-parity
📓 Docs Drift CheckThis PR changes 1 package(s): 1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 16 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin f30d3df8954f2da5d73b70121fded8a655d4d5e4 && git checkout f30d3df8954f2da5d73b70121fded8a655d4d5e4
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f782f1764410dcf7ac80108c3526eab4043032d7 0b582c6d913e9e583cd4574a0f3c88b716803b57 && git checkout -B drift-repro f782f1764410dcf7ac80108c3526eab4043032d7 && git merge --no-ff 0b582c6d913e9e583cd4574a0f3c88b716803b57
node scripts/docs-audit/affected-docs.mjs --json f782f1764410dcf7ac80108c3526eab4043032d7
|
Fixes #22530
Clause-②: no
POST /api/v1/security/explainnow answers every by-id write of acontrolled_by_parentrecord the way that verb's own door answers it. PR #22529 did this forupdate. This PR does it fordeleteandtransfer, and listsrestoreandpurgethe same way. The record verdict comes from the master-detail write check (ADR-0055) that the write path runs at step 2.8. explain asks it through the served membercheckControlledByParentWrite, the same call PR #22529 wired. There is no second copy of the check and nopackages/specchange.Step 1: the measurement that decides whether a spec stage is needed
The unlock asked for this first: can
deleteandtransferreach the door's verdict through a declared member? They can. No spec change is needed. Read onorigin/mainfaf634850. Line numbers are at this PR's head.security-plugin.ts:3321) callsassertControlledByParentWrite(sets, object, opCtx.operation, ...)forinsert,update,delete,transfer,restoreandpurge, then again for the delegator. The verb reaches only two places inside that call:insertbranch (:9342, which reads the master id from the request body) and the insert stand-down (:9462);assertMasterRowEditable(:9568) never reads the verb. All three of its legs askupdateof the master.controlledByParentWriteOutcomeOf(:544) classifies by error class and the leg WeakMap, never by prose. So for every by-id verb, the outcome step 2.8 reaches equals the outcome the served member computes for an update (:9174,:9191).masterGateCoversOperation(platform-ownership-policies.ts:353) states it for the floor hand-over: "a detail DELETE is covered by the same master check an UPDATE is".ExplainEngineDeps.checkControlledByParentWrite, wired to plugin-security's own private method (security-plugin.ts:5559). The spec member does not need to take a verb, and no new member or outcome type is needed. PM assumption 3 holds.transferoperation: its middleware vocabulary is pinned inobjectql'sengine-middleware-operation-vocabulary.test.ts, andengine.ts:3097lists seven operations. A transfer reaches a record as thePATCHthat writesowner_id. That is anupdate, so it runs step 2.7 and step 2.8, plus step 3.5's transfer grant (:3472,:3553). On that door, step 2.8's answer is the member's update answer.PM assumption 1 holds as stated. PM assumption 2 holds, refined by the verb table below.
The verb enumeration: each write verb explain answers × step 2.8
createinsert:3321), on the insert branch: the master comes from the request body (:9342)updateupdatePATCHdeletedeleteDELETEtransfertransferPATCHwritingowner_id(an update,:3472). The pre-wiredtransfermiddleware op also runs it (controlled-by-parent-detail-write-authority.test.ts§1)restorerestore:3321), but the object gate refuses it to every principal first (permission-evaluator.ts:53,DESTRUCTIVE_OPERATIONS, its grant retired). No route dispatches itpurgepurgerestoreread,exportfindReproduction (before)
The new dogfood pin, run against
plugin-securitybuilt fromfaf634850(test commit48e239fe4,dist/from that source, confirmed by grep). The steward holds every write verb oncpg_contractand holdsorg_member, so both floors bind. Each child was inserted by the system, so the steward did not create it.recordbeforeDELETE403PERMISSION_DENIED, names the master's row-level securityvisible: false,decidedBy: 'rls'(the floor; no word about the master)DELETE200visible: false(the floor)PATCH {owner_id}403PERMISSION_DENIED, names the master's row-level securityvisible: true,decidedBy: 'object_crud'PATCH {owner_id}200visible: true(already the door's answer)The transfer row is new. The card named transfer as a read-only inference, and this measures it: explain told a principal who may not edit the master that they may transfer the record, beside a 403.
What changed
explain-engine.ts:MASTER_CHECKED_BY_ID_WRITESlists the verbs step 2.8 runs on that address one existing record by id:update,delete,transfer,restore,purge. The engine asks the member for each of them where it asked forupdatealone.security-plugin.ts, theexplainwiring only. The record write path's step 2.7 coverage vouch is now!actsOnBehalfOf(c)forupdateanddeletealike, which is step 2.7's own!delegatorSets. The floor itself still comes off only wheremasterGateCoversOperationcovers the verb. The vouch knob's docblock and the two wiring comments now describe this..changeset/22530-explain-cbp-write-verb-parity.md:patchfor@objectstack/plugin-security. No key is added toExplainEngineDepsand no signature changes, soClause-②: no.Pins
ExplainOperationSchema.optionsequals the door table's keys, so a new verb with no row fails. For each verb with a by-id door (update,delete,transfer), the master's non-editor gets the door's 403 (naming the master's row-level security) and explainvisible: false,decidedBy: 'sharing', naming the verb and leg. The master's editor (not the creator) gets the door's 200 and explainvisible: true.restore/purgegetobject_crudfor both principalspackages/qa/dogfood/test/cbp-explain-master-write.dogfood.test.ts(new nested block; the boot adds acpe_contract_stewardset withassertArmedonorg_memberand the set)update,deleteandtransfer: four refused legs (record_sharing,row_level_security,object_permission,master_chain), each beside the middleware's by-id write of that verb refusing on that leg. Four admitted records, each beside the same write being admitted.restore/purge: the middleware refuses before step 2.8, and explain saysobject_crudcontrolled-by-parent-write-member.test.tsallow/not_applicableare byte-identical (toEqual) to the report without the member. Asked once with the explained context.restore/purgeare asked, andobject_cruddecides.read/export/createare never asked, nor is an object-level request or a missing record. The classification is totalexplain-controlled-by-parent-write.test.tsAblations
Each leg went through
scripts/ablation-replace.mjsin wrap mode (anchor hit 1 → 0, blob changed), with an outertrapthat restores toHEADon EXIT, INT and TERM. Thenpnpm --filter @objectstack/plugin-security build, thenscripts/ablation-dist-preflight.mjswith the marker (--source-markerfor the single-quoted source spelling), and only then the runs. Before the mutation the marker had 0 hits in the pristinedist/. On the restore leg, the blob equalsHEAD's andgit diff HEADis empty. The restore leg rebuilt, and the preflight with--absentread the marker absent from all 6 built files with the tree clean.MASTER_CHECKED_BY_ID_WRITES.has(engineOp)→engineOp === 'update'visible: true,decidedBy: 'object_crud'beside the 403), 11 greenmasterGateCoversThisWrite = !actsOnBehalfOf(c)→engineOp === 'update' && !actsOnBehalfOf(c)decidedBy: 'rls'; the editor getsvisible: falsebesideDELETE200), 11 green. Plugin suites: 74/74 greenAblation A's first attempt was void. Its dist preflight passed the
dist/reading but refused the tree reading, because the source spells the marker with single quotes. The inner script stopped before any test ran (exit 90). It was re-run with--source-marker, and the table reports that run. Ablation A also shows why the vouch and the check go together: with the vouch and no master check, the non-editor's delete readsvisible: true.Local verification (HEAD
0b582c6d9, which mergesorigin/maince78ff7bc, a CI-only change)pnpm --filter @objectstack/plugin-security test: 192 files, 4033 passed, 45 skipped, exit 0.pnpm --filter @objectstack/plugin-security typecheckandpnpm --filter @objectstack/dogfood typecheck: exit 0.--listFilesshows the two edited plugin tests in the test-layer program (tsconfig.test.json) and the dogfood file in its program.security/explainorcontrolled_by_parentpassed (186 tests), includingcbp-explain-master-write(13),cbp-parent-attachment-comment-gates,owd-public-read-write-write-floor(explain delete on a non-controlled_by_parentobject, unchanged) andshowcase-invoice-cbp. The full dogfood suite (three CI shards) is declared to CI.node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstackderived 71 commands at0b582c6d9; all 71 ran, each exit code recorded.--ranreads "71 derived, 71 run, 0 NOT-MEASURED, 0 UNRUN" (a derived zero).pnpm check:dual-build-cjs-loadsfirst answeredPREREQUISITE NOT MET(exit 3: eight unrelated packages had nodist/); after building those eight (turbo, all cache hits) it answered exit 0, which is the reading recorded.pnpm lintowns the full run):eslint.config.mjslints**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}, so the five changed.tsfiles are the touched population.--format jsonread 5 files, 0 errors, 0 warnings.parserOptions.project, no typed rules, as the config states), and this diff edits no lint config. So it cannot move any untouched file's verdict.Acceptance notes
security-service.tsstill describes an update only. That stays true: explain asks it the update question, and plugin-security uses the equality it already declares atmasterGateCoversOperation. The pins hold the equality at the door for each verb.createis out of the domain. Step 2.8 judges an insert by the master id in the request body. An explain request carries no body, and a record-grainedcreateaddresses no existing record, so there is no door answer to be at parity with. The enumeration pins classify it explicitly.restore/purgeare asked the master check because step 2.8 lists them. Today the object gate refuses them to everyone and decides first. On those verbs the sharing layer can name a master leg below anobject_crudverdict, as an update already does when CRUD refuses.record.visiblenow shows it to a master editor who did not create the child, and hides it from a non-editor, naming the master leg.Out-of-lane findings (for the seat to file)
explaintransferoutsidecontrolled_by_parent: its record-level row-level security is computed for the rawtransferoperation.rls-compiler.tsmapOperationToRLS(:1238) sends any other verb toselect, andcomputeLayeredRlsFilter(:8050) treats it as a read. So no update-class policy and no ownership floor ever reach a transfer's verdict. The transfer door is the update that writesowner_id, and it meets both. Step 2.7 mapstransfertoupdate(:3173).cpgfixture, then deleted). Apublic_read_writecpg_boardrow is excluded by an app-authored update policy (name == 'open', the row isclosed) for a member holding edit and transfer. explainupdateanswersvisible: false,decidedBy: 'rls'. explaintransferanswersvisible: true,decidedBy: 'object_crud', with the rls layer reading "No business RLS policy applies to this record". The transfer door,PATCH /api/v1/data/cpg_board/IDwithowner_id, answers 403PERMISSION_DENIED.runtime:explain-engine.ts applyRecordAttribution(computeLayeredRlsFilter(sets, object, engineOp, …)withengineOptransfer) →runtime:rls-compiler.ts mapOperationToRLS(defaultselect); the door maps it atsecurity-plugin.ts:3173.explain transfer rls select mapOperationToRLS,explain transfer update policy owner_id door,transfer record verdict row-level security.Generated by Claude Code