Skip to content

Commit fc55892

Browse files
fix(plugin-security): explain reports a Modify All-superseded row-conditioned field lock as binding on no row (#29637)
<!-- claude-projects-attribution: {"github_login":"os-steve"} --> _Requested by **Steve** · [project thread](https://claude.ai/code/project/chan_01FdL1XeUyvVNxLreh3F4Qi9?thread=cmsg_01FdL1XeUyvVNxLreh3F4Qi96ws2DLQgZkin2u7GFC6oDF)_ Fixes #29610 Clause-②: yes (widening) Metadata-protocol: none ## 维护者速读 - 改了什么:`os explain` 的字段级说明层,对持有 Modify All Data 的调用者,不再把带 `when` 行条件的字段锁说成"按行判、其余处一律锁",改为单列一句"已被 Modify All 字段票覆盖,锁在任何行都不生效",并点名持票的权限集。 - 为什么改:裁决 6573563853 字母 A(#29387 已落地)后,Modify All 的字段票在逐条票之后、任何 `when` 判定下都把每个字段投为可编辑;说明文字却还在讲锁,管理员排查字段权限时会被误导。 - 风险与回滚:只改说明文字,执行面零改动;回滚即 revert 本 PR。View All 与无票调用者的句子与今天一致。 - 先看:`packages/plugins/plugin-security/src/permission-evaluator.ts`(`conditionedFieldEntries` 读同一个 `superuserFieldVote`)、`explain-engine.ts` 的 `conditionedSentence`、`explain-engine.test.ts` 三条 `[#29610]` 用例。 - 两分钟验证:`pnpm --filter @objectstack/plugin-security exec vitest run src/explain-engine.test.ts -t 29610`,预期 3 条全绿。 - 需要你做的:无,席位落地。 ## What changes The FLS layer of the access explanation listed every field permission that carries a `when` as "judged per row by the write gate, applied as the lock everywhere else". For a caller whose sets hold Modify All Data on the object that was untrue: since ruling of record 6573563853 letter A (PR #29387), `getFieldPermissions` applies the super-user field vote after the per-entry votes, under every `when` verdict, so such a lock binds on no row. Measured through the public explain route (see Verification): the write gate admits the field for that caller while the report called it locked. - **Seam (no second copy of the rule).** `PermissionEvaluator.conditionedFieldEntries` now names, per entry, the sets whose Modify All vote supersedes it (`supersededBy`). It reads `superuserFieldVote` from `@objectstack/core`, the same function `getFieldPermissions` applies, and attributes the sets through `superuserBypassSets` (the one `holdsSuperuserBypass` predicate). No extraction was needed: the vote already lives in core and both readers now call it. The explain engine only words the result. - **Wording: annotate, not drop.** Superseded entries move into their own sentence, in the "Superseded by the View/Modify All Data bypass [...]" form the record-level sharing attribution already uses: `N row-conditioned field permission(s) are superseded by the Modify All Data field vote [SETS], which votes every field editable on every row, so the lock binds on no row: [...]`. Entries the vote does not supersede keep the existing sentence, counted on their own. - **View All supersedes nothing.** It votes only the read bit, which a conditioned entry already casts (the spec refuses `readable: false` beside a `when`), so the edit lock still binds per row. Measured: the write gate refuses the field on a matching row for a View All holder. - **D10.** Each principal's entries are judged against its own sets. The agent and delegator masks intersect, so a delegator's lock still binds when only the agent holds the vote. - The hidden/partial mask is untouched. Enforcement is unchanged. Forced test edit: `security-plugin.test.ts` `[#25866] conditionedFieldEntries lists the row-conditioned entries of the object, as authored` pins the entry shape with `toEqual`, so both expected entries gain `supersededBy: []`. ## Tests `explain-engine.test.ts`, next to the `[#25866]` row-condition test: - `[#29610] a Modify All set supersedes a row-conditioned lock` — the entry is listed as superseded by the vote, names the set, and is not called "judged per row". - `[#29610] a View All set does NOT supersede it` — read-side control: the row-conditioned sentence stays. - `[#29610] a caller without the vote keeps the row-conditioned sentence` — control. ## Acceptance notes - `ConditionedFieldEntry` gains a required output member `supersededBy`. It is reachable only as the return type of the exported `PermissionEvaluator.conditionedFieldEntries`; nothing in the repo builds one besides the evaluator. The seat ruled this public return-shape enlargement a widening (contract review 6591028293, claim amended in 6591039615), so the PR body and the changeset declare `Clause-②: yes (widening)`; `Metadata-protocol: none` and the patch level stand. - Related and not in scope: #29544 (lint for the super-user bit beside a mask), #29543 (`when` description wording). ## Verification Head `8ec800e4` (fix `ca1b6e6a`, the round-2 changeset declaration commit, and merges of `origin/main`; the latest merge brought `plugin-security` files from main, so the package runs and gates below were repeated on this head). <details><summary>Measurement through the public explain route, before and after (scratch dogfood boot, not committed)</summary> A throwaway dogfood boot measured three callers that each hold a `when`-conditioned field entry on one object: one without the vote (control), one with View All, one with Modify All. Each read the explain route's FLS layer, and the write gate's answer for that field on a row the condition names was read beside it. - Write gate: control refused, View All refused, Modify All admitted (the ruled behaviour). - Before (main `5bd615ef`): all three were told the entry is "judged per row by the write gate, applied as the lock everywhere else", Modify All included. - After (`ca1b6e6a`): control and View All unchanged; Modify All reads "2 row-conditioned field permission(s) are superseded by the Modify All Data field vote [the MA set], which votes every field editable on every row, so the lock binds on no row". </details> <details><summary>Red first on main, ablation</summary> - Red on main (tests added, fix absent): `Tests 1 failed | 2 passed` — the Modify All case: `expected 'No field-level masking applies. 1 fie…' not to contain 'judged per row by the write gate'`. - Ablation via `scripts/ablation-replace.mjs` on `permission-evaluator.ts`: the vote read was disabled with a string-literal guard. Anchor 1 to 0, blob f0269b4 to 57f704a0. Same failure, `Tests 1 failed | 2 passed`. Restored: blob equals HEAD f0269b4, `git diff HEAD` empty. The test imports the evaluator from `src` by relative path, so no dist leg applies. </details> <details><summary>Package runs and gates</summary> - `pnpm --filter @objectstack/plugin-security typecheck` exit 0; `pnpm --filter @objectstack/plugin-security test`: Test Files 241 passed (241), Tests 5205 passed, 45 skipped (5250). Lock VERDICT command-exit 0, on `ca1b6e6a`, `1fcfe8f5` and `8ec800e4` (5206 passed on the last, one test more from main). - Lint: root eslint `--no-inline-config` on `packages/plugins/plugin-security` exit 0 (the package has no lint script). - `dispatch-gates --commands` on `8ec800e4` (and before it on `1fcfe8f5`): 62 derived, 62 run, all exit 0; `--ran` reconciliation: 62 run, 0 NOT-MEASURED, 0 UNRUN. On `1fcfe8f5` three gates first exited on a missing prerequisite (`check-plugin-teardown-shape --self-test` exit 3 needed the pinned fixture commit fetched; `check:dts-closure` exit 1 and `check:dual-build-cjs-loads` exit 3 needed a declaration build of every package) and were re-run green after it. - Dogfood battery: not owed (no `!` in the title, no `(narrowing)` arm). </details> <!-- signed: domain:server#2 · os-steve · session_01ADHkmSGbDCYYvHbU6am1V2 --> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c318490 commit fc55892

5 files changed

Lines changed: 122 additions & 10 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
---
4+
5+
fix(plugin-security): `os explain` no longer tells a Modify All holder that a row-conditioned field lock binds
6+
7+
Clause-②: yes (widening)
8+
Metadata-protocol: none
9+
10+
The access explanation's field-level layer listed every field permission that carries a `when` as "judged per row by the write gate, applied as the lock everywhere else". For a caller whose permission sets hold Modify All Data on the object, that was untrue: the field merge applies the Modify All vote after the per-entry votes, under every `when` verdict, so such a lock binds on no row (ruling of record 6573563853, letter A). The explanation now lists those entries separately, as superseded by the Modify All Data field vote and naming the sets that hold it. A View All holder, and a caller who holds neither, keeps the row-conditioned sentence as before, because their edit lock still binds per row.
11+
12+
Enforcement is unchanged. `PermissionEvaluator.conditionedFieldEntries` now names each entry's superseding sets in a new `supersededBy` member, read from the same `superuserFieldVote` that `getFieldPermissions` applies.

‎packages/plugins/plugin-security/src/explain-engine.test.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,69 @@ describe('explainAccess — fls reports partial masking (#9127)', () => {
308308
expect(fls.detail).toContain(`amount (locked when status == 'approved', set "hr_user")`);
309309
});
310310

311+
// [#29610 / ruling of record 6573563853, letter A] A set holding Modify All
312+
// on the object votes editable on every field after the per-entry vote, under
313+
// every `when` verdict, so for that caller a conditioned lock binds on no row.
314+
// The report reads the SAME vote (`superuserFieldVote` through
315+
// `conditionedFieldEntries`), never a second copy of it.
316+
const CONDITIONED_AMOUNT = {
317+
'leave_request.amount': { readable: true, editable: false, when: "status == 'approved'" },
318+
};
319+
320+
it('[#29610] a Modify All set supersedes a row-conditioned lock: the entry is reported as binding on no row', async () => {
321+
const fls = await flsOf({
322+
sets: [
323+
{
324+
name: 'hr_user',
325+
objects: { leave_request: { allowRead: true, allowEdit: true } },
326+
fields: CONDITIONED_AMOUNT,
327+
},
328+
{
329+
name: 'hr_admin',
330+
objects: { leave_request: { allowRead: true, allowEdit: true, viewAllRecords: true, modifyAllRecords: true } },
331+
},
332+
] as any,
333+
});
334+
expect(fls.verdict).toBe('not_applicable');
335+
expect(fls.detail).not.toContain('judged per row by the write gate');
336+
expect(fls.detail).toContain('1 row-conditioned field permission(s) are superseded by the Modify All Data field vote [hr_admin]');
337+
expect(fls.detail).toContain('binds on no row');
338+
expect(fls.detail).toContain(`amount (locked when status == 'approved', set "hr_user")`);
339+
});
340+
341+
it('[#29610] a View All set does NOT supersede it: the edit lock still binds per row (read-side control)', async () => {
342+
const fls = await flsOf({
343+
sets: [
344+
{
345+
name: 'hr_user',
346+
objects: { leave_request: { allowRead: true, allowEdit: true } },
347+
fields: CONDITIONED_AMOUNT,
348+
},
349+
{
350+
name: 'hr_auditor',
351+
objects: { leave_request: { allowRead: true, viewAllRecords: true } },
352+
},
353+
] as any,
354+
});
355+
expect(fls.detail).toContain('1 field permission(s) carry a row condition — judged per row by the write gate');
356+
expect(fls.detail).toContain(`amount (locked when status == 'approved', set "hr_user")`);
357+
expect(fls.detail).not.toContain('superseded');
358+
});
359+
360+
it('[#29610] a caller without the vote keeps the row-conditioned sentence as before (control)', async () => {
361+
const fls = await flsOf({
362+
sets: [
363+
{
364+
name: 'hr_user',
365+
objects: { leave_request: { allowRead: true, allowEdit: true } },
366+
fields: CONDITIONED_AMOUNT,
367+
},
368+
] as any,
369+
});
370+
expect(fls.detail).toContain('1 field permission(s) carry a row condition — judged per row by the write gate');
371+
expect(fls.detail).not.toContain('superseded');
372+
});
373+
311374
it('[#25866] an entry without a `when` adds no row-condition sentence', async () => {
312375
const fls = await flsOf({
313376
sets: [

‎packages/plugins/plugin-security/src/explain-engine.ts‎

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2959,18 +2959,35 @@ async function explainObjectPass(deps: ExplainEngineDeps, input: ExplainInput, a
29592959
// stored row before an update); this report holds no row, so each such entry
29602960
// is reported AS a row-conditioned lock — which is also how every row-blind
29612961
// reader applies it — rather than folded silently into the mask above.
2962+
//
2963+
// [#29610 / ruling of record 6573563853, letter A] …unless the principal's
2964+
// Modify All vote supersedes it: `getFieldPermissions` applies that vote after
2965+
// the per-entry votes under every `when` verdict, so such an entry's lock
2966+
// binds on no row. The evaluator names the superseding sets from the SAME
2967+
// vote (`ConditionedFieldEntry.supersededBy`); this layer only words it, in
2968+
// the "Superseded by …" form the sharing attribution already uses. Each
2969+
// principal's entries are judged against its own sets: under D10 the masks
2970+
// intersect, so a delegator's lock still binds when only the agent holds the
2971+
// vote.
29622972
const conditioned = [
29632973
...deps.evaluator.conditionedFieldEntries(object, sets),
29642974
...(delegatorSets ? deps.evaluator.conditionedFieldEntries(object, delegatorSets) : []),
29652975
];
2976+
const describeConditioned = (e: (typeof conditioned)[number]): string =>
2977+
`${e.field} (${e.editable ? 'editable' : 'locked'} when ${e.when}, set "${e.set}")`;
2978+
const binding = conditioned.filter((e) => e.supersededBy.length === 0);
2979+
const superseded = conditioned.filter((e) => e.supersededBy.length > 0);
2980+
const supersedingSets = [...new Set(superseded.flatMap((e) => e.supersededBy))];
29662981
const conditionedSentence =
2967-
conditioned.length > 0
2968-
? ` ${conditioned.length} field permission(s) carry a row condition — judged per row by the write gate, ` +
2969-
`applied as the lock everywhere else: ${listFields(
2970-
conditioned.map((e) => `${e.field} (${e.editable ? 'editable' : 'locked'} when ${e.when}, set "${e.set}")`),
2971-
(f) => f,
2972-
)}.`
2973-
: '';
2982+
(binding.length > 0
2983+
? ` ${binding.length} field permission(s) carry a row condition — judged per row by the write gate, ` +
2984+
`applied as the lock everywhere else: ${listFields(binding.map(describeConditioned), (f) => f)}.`
2985+
: '') +
2986+
(superseded.length > 0
2987+
? ` ${superseded.length} row-conditioned field permission(s) are superseded by the Modify All Data field vote ` +
2988+
`[${supersedingSets.join(', ')}], which votes every field editable on every row, so the lock binds on no row: ` +
2989+
`${listFields(superseded.map(describeConditioned), (f) => f)}.`
2990+
: '');
29742991
layers.push({
29752992
layer: 'fls',
29762993
verdict: maskNarrows ? 'narrows' : 'not_applicable',

‎packages/plugins/plugin-security/src/permission-evaluator.ts‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,18 @@ export interface ConditionedFieldEntry {
3535
when: string;
3636
/** The entry's edit vote on a matching row. */
3737
editable: boolean;
38+
/**
39+
* [#29610 / ruling of record 6573563853, letter A] The sets whose Modify All
40+
* vote supersedes the entry — empty when none does. Read from
41+
* `superuserFieldVote` (`@objectstack/core`), the vote
42+
* {@link PermissionEvaluator.getFieldPermissions} applies after the
43+
* per-entry votes under every `when` verdict: when it votes `editable`, every
44+
* field of the object is readable and editable on every row, so the entry's
45+
* lock binds on no row for this principal. A View All vote supersedes
46+
* nothing here: it votes only the read bit, which a conditioned entry already
47+
* casts (the spec refuses `readable: false` beside a `when`).
48+
*/
49+
supersededBy: string[];
3850
}
3951

4052
/**
@@ -525,13 +537,20 @@ export class PermissionEvaluator {
525537
* [#25866] The row-conditioned entries the sets declare on `objectName`,
526538
* for a reader that holds no row (`os explain`): each is reported AS a
527539
* row-conditioned lock — judged per row by the write gate, applied as its
528-
* lock everywhere else — rather than folded silently into the mask.
540+
* lock everywhere else — rather than folded silently into the mask. Each
541+
* entry names the sets whose Modify All vote supersedes it (`supersededBy`,
542+
* #29610), so the report never calls a lock binding that the merge overrides.
529543
*/
530544
conditionedFieldEntries(
531545
objectName: string,
532546
permissionSets: PermissionSet[],
533547
): ConditionedFieldEntry[] {
534548
const out: ConditionedFieldEntry[] = [];
549+
// [#29610] The same super-user vote `getFieldPermissions` applies — never a
550+
// second reading of the bits. An `editable` vote overrides every entry.
551+
const supersededBy = superuserFieldVote(permissionSets, objectName)?.editable
552+
? this.superuserBypassSets(objectName, permissionSets, { bit: 'modify' })
553+
: [];
535554
for (const ps of permissionSets) {
536555
if (!ps.fields) continue;
537556
for (const [key, perm] of Object.entries(ps.fields)) {
@@ -543,6 +562,7 @@ export class PermissionEvaluator {
543562
set: ps.name,
544563
when,
545564
editable: perm.editable === true,
565+
supersededBy: [...supersededBy],
546566
});
547567
}
548568
}

‎packages/plugins/plugin-security/src/security-plugin.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2839,10 +2839,10 @@ describe('PermissionEvaluator', () => {
28392839
'task.title': { readable: true, editable: true, when: 'done == false' } as any,
28402840
});
28412841
expect(evaluator.conditionedFieldEntries('contact', [ps])).toEqual([
2842-
{ field: 'email', set: 'hr', when: "status == 'approved'", editable: false },
2842+
{ field: 'email', set: 'hr', when: "status == 'approved'", editable: false, supersededBy: [] },
28432843
]);
28442844
expect(evaluator.conditionedFieldEntries('task', [ps])).toEqual([
2845-
{ field: 'title', set: 'hr', when: 'done == false', editable: true },
2845+
{ field: 'title', set: 'hr', when: 'done == false', editable: true, supersededBy: [] },
28462846
]);
28472847
});
28482848

0 commit comments

Comments
 (0)