Skip to content

Commit d434481

Browse files
fix(lint): the controlledByParent owner arm prescribes ownerMayEdit only for an owner_id-keyed policy (#26658)
<!-- claude-projects-attribution: {"github_login":"os-project-manager"} --> _Requested by **Leehom** · [project thread](https://claude.ai/code/project/chan_01TenzsRopZj4jMbvjX8uz1X?thread=cmsg_01TenzsRopZj4jMbvjX8uz1XG6dM1MS6FbmME9jADHuk89)_ Fixes #26591 Clause-②: no (lint advice is not a contract face; no accepted metadata shape moves) Metadata-protocol: none ## 维护者速读 - **改了什么**:`os lint` 的 `security-controlled-by-parent-owner-policy-without-opt-in` 只在策略按 `owner_id == current_user.id` 写、且明细表确有 `owner_id` 列时,才建议加 `ownerMayEdit: true`;按别的人员字段(如 hotclm 的 `owner`)写的策略,不论加没加开关都会提示,并改为建议「把负责人放进 `owner_id`」,不再建议这个开关。 - **为什么改**:运行时的写入门只认行上存的 `owner_id`。以前 lint 对任何人员字段都建议加开关,hotclm 照做后 lint 不响了,但负责人照样被 403 拒绝,等于 lint 在误导。 - **风险与回滚**:只改 lint 的提示文字和触发条件,不动运行时权限门,也不动任何元数据契约;回滚就是 revert 本 PR。 - **先看**:`packages/lint/src/validate-security-posture.ts`(新函数 `carriesCbpOwnerColumn`、`pushCbpCallerPolicyOffOwnerColumn` 和规则循环里的 `admittable` 一行);`packages/lint/src/validate-security-posture-owner-may-edit.test.ts`(新的 #26591 用例组);`packages/lint/src/rule-explanations.ts`。 - **两分钟验证**:`pnpm --filter @objectstack/lint exec vitest run src/validate-security-posture-owner-may-edit.test.ts`,预期全部通过,其中「an other-field policy with the opt-in: still flagged」一条证明加了开关的 `owner` 策略仍会被提示。 - **需要你做的**:无,席位落地。 ## What changes `sharing.controlledByParent.ownerMayEdit: true` admits one caller past a `controlled_by_parent` detail's master-write gate. That caller is the user in the stored row's `owner_id` (`assertControlledByParentWrite`), and only on a detail whose registered schema carries that column (`resolveCbpRelation` resolves the opt-in `false` otherwise). The lint arm prescribed the opt-in for any `FIELD == current_user.id` update policy. This PR narrows the advice to the shape the runtime honours. The runtime gate does not change. | detail | predicate keys the caller on | before | after | |---|---|---|---| | carries `owner_id`, no opt-in | `owner_id` (alone or beside another field) | flagged, opt-in prescribed | same | | carries `owner_id`, opted in | `owner_id` (alone or beside another field) | silent | same | | carries `owner_id`, no opt-in | only another user field (`owner`) | flagged, **opt-in prescribed** | flagged, the verdict says the opt-in admits only the stored `owner_id`, and the hint says to key the accountable person on `owner_id`. The hint never names the opt-in | | carries `owner_id`, opted in | only another user field (`owner`) | **silent** | flagged, as above, and the verdict names the declared opt-in | | no `owner_id` (`ownership: 'business_unit' \| 'org' \| 'none'`, no own `owner_id` field) | anything, `owner_id` included | flagged and opt-in prescribed (no opt-in), or silent (opted in) | flagged, the verdict says it is "a column the detail does not carry", and the hint says to give the detail an `owner_id` column. The hint never names the opt-in | - **`packages/lint/src/validate-security-posture.ts`.** `carriesCbpOwnerColumn(obj)` answers whether the detail carries `owner_id` the way the runtime does. It reads the spec's injection plan (`injectedColumnsFor`, `resolveInjectedSystemColumns`) or a field the author declared under that name. The arm keeps `{ optedIn, ownerColumn }` per resolving detail instead of a set of details without the opt-in. A predicate whose keyed fields include `owner_id` on a detail that carries it keeps today's behaviour. Any other caller-keyed predicate goes to the new push site, `pushCbpCallerPolicyOffOwnerColumn`, under the same rule id and the same `warning` tier. `callerIdentityKeyedFields` keeps its shape: it already returns field names, and this arm is its only caller. - **`packages/lint/src/rule-explanations.ts`.** The `os explain` entry now says that the opt-in reads only the stored `owner_id`, only on a detail that carries it. It gives a separate remedy for each case. - **`packages/spec/liveness/object.json`.** The `ownerMayEdit` evidence string described the arm as flagging "an owner-keyed update policy on a detail that has not declared `ownerMayEdit`". After the narrowing that undersells it, so it now describes the narrowed arm, and its `verifiedAt` moves to 2026-10-27. This is an evidence string, not a contract change. The `object.zod.ts` docblock near :3089 ("so the gate and the lint ask one question one way") is still true, because both read the opt-in through `controlledByParentOwnerMayEdit`, so it is unchanged. - **Changeset:** `@objectstack/lint` patch, plus `@objectstack/spec` patch because `liveness/` ships in spec's `files[]`. ## Tests - `validate-security-posture-owner-may-edit.test.ts` adds a `#26591` group on hotclm's shape (a `controlled_by_parent` detail with its own `owner` user field and `using: owner == current_user.id`): - `owner_id` policy without the opt-in: flagged, opt-in prescribed. - `owner_id` policy with it: silent. - `owner` policy without the opt-in: flagged with the `owner_id` remedy. The hint does not contain `ownerMayEdit`. - `owner` policy with the opt-in: still flagged. - Mixed `owner || owner_id` policy: prescribed without the opt-in, silent with it. - `ownership: org | none | business_unit`, with and without the opt-in: flagged with the owner-column remedy and no opt-in. - An author's own `owner_id` field on `ownership: 'none'`: prescribed, then silent. - The fixture is key-legal under `ObjectStackSchema`. - `validate-security-posture.test.ts` meta-tests: - The receiver table excuses `detail` (the arm's own fold) and `sharing` (the opt-in's spelling inside the verdict string). - `cbpDetails` replaces `cbpDetailsWithoutOptIn` in the plumbing list. - The `findings.push` count goes from 18 to 19. - A reachability-corpus row drives the new push site with key-legal metadata. ## Acceptance notes - **ADR-0055 :176 is now partly false.** It says an `update` / `all` policy that keys a field on `current_user.id`, on a detail that has not opted in, "is flagged with the opt-in as the fix". After this PR that holds only for a policy keyed on `owner_id`. The order keeps `docs/adr/**` out of this PR. The report names the line for the seat. - **Overlap with other PRs.** #26599, the only other PR on `validate-security-posture.ts`, has merged since this branch was cut. It edits the OWD-alias docblocks near :101–:170, and `git merge-tree HEAD origin/main` is clean. The open #26337 and #26628 edit other entries of `rule-explanations.ts`. No hunk is shared. ## Verification All readings below are on head `816501243`. <details><summary>Lint package: tests, typecheck, scoped eslint</summary> - `pnpm --filter @objectstack/lint test`: `Test Files 166 passed (166)` · `Tests 7395 passed | 5 skipped (7400)`. This run was on the tree before the corpus row and the `detailName` typecheck fix. The two touched suites re-ran on the final tree: `Test Files 2 passed (2)` · `Tests 210 passed (210)`. - `pnpm --filter @objectstack/lint typecheck` (`tsc --noEmit && pnpm check:test-typecheck`): exit 0. - `eslint --no-inline-config` on the four touched lint files: exit 0. - `pnpm --filter @objectstack/spec exec vitest run` (for the liveness string): `Test Files 810 passed (810)` · `Tests 23353 passed | 1 todo (23354)` · `VERDICT command-exit 0`. </details> <details><summary>Ablation (one, of the narrowing)</summary> `node scripts/ablation-replace.mjs` replaced `const admittable = detail.ownerColumn && fields.includes(CBP_OWNER_COLUMN);` with `const admittable = true || (…); // ABL26591`. That restores the old behaviour: every caller-keyed predicate takes the prescriptive path, and an opted-in detail is silent. - The mutation landed: anchor x1 to x0, replacement x0 to x1, blob `f52007d93faf` to `3b0a2d4cd9cb`. - The two suites went red as expected: `Tests 5 failed | 205 passed (210)`. The 5 failures are both other-field cases (without and with the opt-in) and the three no-`owner_id` cases (org / none / business_unit). Every `owner_id` case stayed green. - The restore was proven: the blob after restore equals HEAD (`f52007d93faf`), and `git diff HEAD` is empty. - The lint suites import `./validate-security-posture.js` from `src/`, so no build sat between the mutation and the run. </details> <details><summary>Derived gates: 65 / 65, reconciled</summary> - `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 65 commands. All 65 ran, every exit code was captured before any pipe, and all 65 exited 0. - `--ran`: `✓ dispatch-gates --ran: 65 derived famil(ies) accounted for — 65 run, 0 NOT-MEASURED`. - Four commands first answered exit 3 (PREREQUISITE NOT MET) and were re-run after their prerequisite. Their second readings are the ones counted: - `check-plugin-teardown-shape.mjs --self-test`, after `git fetch origin 621a487` (the self-test's positive control). - `check:docs-transcript-drift`, `check:lean-entry-closure` and `check:dual-build-cjs-loads`, after `pnpm --filter '@objectstack/lint...' --filter '@objectstack/objectql...' build`. - Dogfood battery: not owed. The title has no `!`, and `Clause-②` has no `(narrowing)` arm. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FEkQ5oX5j1RjbyiKQpdeiF --- _Generated by [Claude Code](https://claude.ai/code/session_01FEkQ5oX5j1RjbyiKQpdeiF)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8222fa8 commit d434481

6 files changed

Lines changed: 243 additions & 25 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
'@objectstack/lint': patch
3+
'@objectstack/spec': patch
4+
---
5+
6+
fix(lint): `security-controlled-by-parent-owner-policy-without-opt-in` prescribes `ownerMayEdit` only for a policy keyed on `owner_id` (#26591)
7+
8+
Clause-②: no (lint advice is not a contract face; no accepted metadata shape moves)
9+
Metadata-protocol: none
10+
11+
`sharing.controlledByParent.ownerMayEdit: true` admits one caller past a `controlled_by_parent` detail's master gate: the user in the stored row's `owner_id`, on a detail that carries that column. The lint arm used to prescribe the opt-in for any `update` / `all` policy of the form `FIELD == current_user.id`. So an app whose accountable person is its own user field (`owner == current_user.id`) declared the opt-in, lint went quiet, and that user was still refused with `403 requires edit access to its master record`.
12+
13+
- The opt-in is still prescribed, as before, for a policy keyed on `owner_id` on a detail that carries it and has not opted in, and the arm stays silent once it has.
14+
- A policy keyed only on another user field is now flagged whether or not the detail opted in. The verdict says the opt-in admits only the stored `owner_id`. The hint never names the opt-in: it says to key the accountable person on `owner_id`, or drop the policy.
15+
- On a detail with no `owner_id` (`ownership: 'business_unit' | 'org' | 'none'` and no `owner_id` field of its own), the opt-in names nobody. A policy keyed on `owner_id` there gets the same treatment, and the hint says to give the detail an `owner_id` column.
16+
- The runtime gate is unchanged. The rule's `os explain` entry covers both cases. The `ownerMayEdit` evidence line in `@objectstack/spec`'s liveness ledger now describes the narrowed arm.

‎packages/lint/src/rule-explanations.ts‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -713,14 +713,20 @@ const SECURITY_CBP_OWNER_POLICY_WITHOUT_OPT_IN_EXPLANATION: RuleExplanation = {
713713
'platform\'s injected per-record owner — updates that row by id with READ access to the master, ' +
714714
'and the detail\'s own policies then decide as authored. Everyone else, and every insert, delete, ' +
715715
'multi-row update and on-behalf-of write, still needs edit access to the master. The opt-in ' +
716-
'reads `owner_id` and nothing else: a policy keyed on another field stays a narrowing even ' +
717-
'after it, so key the policy on `owner_id` or declare what the other field means in prose.',
716+
'reads the stored `owner_id` and nothing else, and only on a detail that carries that column ' +
717+
'(`ownership: \'user\'`, the default; never `\'business_unit\'`, `\'org\'` or `\'none\'`). So the ' +
718+
'rule prescribes the opt-in only for a policy that keys the caller on `owner_id` there. A policy ' +
719+
'keyed only on another user field — `owner == current_user.id`, where `owner` is the detail\'s own ' +
720+
'accountable-person field — stays a narrowing with or without the opt-in, and the rule says so in ' +
721+
'both states instead of prescribing an opt-in that would not admit that user.',
718722
'This is a warning, not an error: a narrowing can be meant (only the owner among the master\'s ' +
719-
'editors may update), and no runtime door refuses the shape. To fix it, declare the opt-in on ' +
720-
'the detail when the owner is meant to write with read on the master, or drop the policy when ' +
721-
'the master gate alone is the intended rule. The opt-in is refused by the parse on an object ' +
722-
'whose `sharingModel` is not `controlled_by_parent`, so it cannot be declared where nothing ' +
723-
'reads it.',
723+
'editors may update), and no runtime door refuses the shape. To fix a policy keyed on `owner_id`, ' +
724+
'declare the opt-in on the detail when the owner is meant to write with read on the master, or ' +
725+
'drop the policy when the master gate alone is the intended rule. To fix a policy keyed on another ' +
726+
'user field, keep the accountable person in `owner_id` (the column the master gate compares the ' +
727+
'caller against) and key the policy on it, or drop the policy. The opt-in is refused by the parse ' +
728+
'on an object whose `sharingModel` is not `controlled_by_parent`, so it cannot be declared where ' +
729+
'nothing reads it.',
724730
],
725731
};
726732

‎packages/lint/src/validate-security-posture-owner-may-edit.test.ts‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,89 @@ describe('security-controlled-by-parent-owner-policy-without-opt-in (#25760)', (
167167
});
168168
});
169169

170+
// [#26591] The opt-in admits ONE column — the stored row's `owner_id`, on a
171+
// detail that carries it — so the arm prescribes it only there. hotclm's
172+
// shape: the accountable person is the detail's own user field `owner`, and
173+
// the requester's update policy keys on it.
174+
describe('the opt-in is prescribed only where it admits (#26591)', () => {
175+
const OWNER_FIELD = { owner: { name: 'owner', type: 'user', reference: 'sys_user', label: 'Owner' } };
176+
/** hotclm's `clm_obligation`: a `controlled_by_parent` detail with its own `owner` user field. */
177+
const hotclmDetail = (extra: Record<string, unknown> = {}) =>
178+
detail({ fields: { ...DETAIL_FIELDS, ...OWNER_FIELD }, ...extra });
179+
const ownerPolicy = policy({ name: 'obligation_requester_own', using: 'owner == current_user.id' });
180+
181+
it('an owner_id-keyed policy without the opt-in: flagged, and the opt-in is the remedy', () => {
182+
const findings = ownerArm(stack([MASTER, hotclmDetail()], policy()));
183+
expect(findings).toHaveLength(1);
184+
expect(findings[0].hint).toContain('Declare sharing: { controlledByParent: { ownerMayEdit: true } }');
185+
});
186+
187+
it('an owner_id-keyed policy with the opt-in: silent', () => {
188+
expect(ownerArm(stack([MASTER, hotclmDetail(OPT_IN)], policy()))).toEqual([]);
189+
});
190+
191+
it('an other-field policy without the opt-in: flagged with the owner_id remedy, never the opt-in', () => {
192+
const findings = ownerArm(stack([MASTER, hotclmDetail()], ownerPolicy));
193+
expect(findings).toHaveLength(1);
194+
expect(findings[0]).toMatchObject({
195+
severity: 'warning',
196+
rule: SECURITY_CBP_OWNER_POLICY_WITHOUT_OPT_IN,
197+
path: 'permissions[0].rowLevelSecurity[0].using',
198+
});
199+
expect(findings[0].message).toContain('(owner == current_user.id)');
200+
expect(findings[0].message).toContain("ownerMayEdit admits only the row's stored owner_id");
201+
expect(findings[0].hint).toContain('Key the accountable person on owner_id');
202+
expect(findings[0].hint).not.toContain('ownerMayEdit');
203+
});
204+
205+
it('an other-field policy with the opt-in: still flagged, the opt-in does not admit that field', () => {
206+
const findings = ownerArm(stack([MASTER, hotclmDetail(OPT_IN)], ownerPolicy));
207+
expect(findings).toHaveLength(1);
208+
expect(findings[0].message).toContain('(owner == current_user.id)');
209+
expect(findings[0].message).toContain("the opt-in it declares (sharing.controlledByParent.ownerMayEdit) admits only the row's stored owner_id");
210+
expect(findings[0].hint).toContain('Key the accountable person on owner_id');
211+
expect(findings[0].hint).not.toContain('ownerMayEdit');
212+
});
213+
214+
it('a predicate keying owner_id beside another field is one the opt-in serves: prescribed without it, silent with it', () => {
215+
const mixed = policy({ using: 'owner == current_user.id || owner_id == current_user.id' });
216+
const findings = ownerArm(stack([MASTER, hotclmDetail()], mixed));
217+
expect(findings).toHaveLength(1);
218+
expect(findings[0].hint).toContain('ownerMayEdit: true');
219+
expect(ownerArm(stack([MASTER, hotclmDetail(OPT_IN)], mixed))).toEqual([]);
220+
});
221+
222+
describe('a detail that carries no owner_id: the opt-in names nobody there', () => {
223+
it.each([
224+
['ownership: org', { ownership: 'org' }],
225+
['ownership: none', { ownership: 'none' }],
226+
['ownership: business_unit', { ownership: 'business_unit' }],
227+
])('%s — an owner_id-keyed policy is flagged with the owner-column remedy, with or without the opt-in', (_label, ownership) => {
228+
for (const extra of [ownership, { ...ownership, ...OPT_IN }]) {
229+
const findings = ownerArm(stack([MASTER, detail(extra)], policy()));
230+
expect(findings, JSON.stringify(extra)).toHaveLength(1);
231+
expect(findings[0].message).toContain('a column "omd_obligation" does not carry');
232+
expect(findings[0].hint).toContain('Give "omd_obligation" an owner_id column');
233+
expect(findings[0].hint).not.toContain('ownerMayEdit');
234+
}
235+
});
236+
237+
it("an author's own owner_id field is the column the runtime reads: the opt-in is prescribed, then silent", () => {
238+
const ownOwner = { ownership: 'none', fields: { ...DETAIL_FIELDS, owner_id: { name: 'owner_id', type: 'user', reference: 'sys_user', label: 'Owner' } } };
239+
expect(ownerArm(stack([MASTER, detail(ownOwner)], policy()))[0]?.hint).toContain('ownerMayEdit: true');
240+
expect(ownerArm(stack([MASTER, detail({ ...ownOwner, ...OPT_IN })], policy()))).toEqual([]);
241+
});
242+
});
243+
244+
it('the hotclm fixture is key-legal: the stack schema refuses no key it carries', () => {
245+
const result = ObjectStackSchema.safeParse({
246+
manifest: { id: 'com.example.omd', name: 'omd', version: '1.0.0', type: 'app' },
247+
...stack([MASTER, hotclmDetail(OPT_IN)], ownerPolicy),
248+
});
249+
expect(result.success ? [] : result.error.issues.filter((i) => i.code === 'unrecognized_keys')).toEqual([]);
250+
});
251+
});
252+
170253
describe('the parsed path reads what the raw path reads', () => {
171254
it('the PARSED detail and set: flagged without the opt-in, silent with it', () => {
172255
const parsedSet = PermissionSetSchema.parse({ name: 'omd_requester', objects: {}, rowLevelSecurity: [policy()] });

‎packages/lint/src/validate-security-posture.test.ts‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1579,6 +1579,14 @@ const NOT_SCHEMA_RECEIVERS: Record<string, string> = {
15791579
// hint text. The scan reads code with its string literals intact, so the
15801580
// spelling presents as a receiver `current_user` reading a key `id`.
15811581
current_user: "the RLS predicate grammar's `current_user.id`, a string literal in the owner arm's message text — not a receiver.",
1582+
// [#26591] The owner arm keeps, per controlled_by_parent detail, whether it
1583+
// opted in and whether it carries `owner_id` — this file's own fold; the
1584+
// authored reads behind it are `controlledByParentOwnerMayEdit(obj)` and
1585+
// `obj.fields`. And `sharing.controlledByParent.ownerMayEdit` is quoted as
1586+
// a string literal in the off-column verdict text, so it presents as a
1587+
// receiver `sharing` reading a key `controlledByParent`.
1588+
detail: "the owner arm's own `{ optedIn, ownerColumn }` fold of one detail, not an authored surface.",
1589+
sharing: "the opt-in's spelling `sharing.controlledByParent.ownerMayEdit`, a string literal in the owner arm's message text — not a receiver.",
15821590
};
15831591

15841592
const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: string; keys: () => string[] }> = [
@@ -1781,7 +1789,7 @@ describe('validateSecurityPosture — reads only keys the spec declares (meta-te
17811789
'bodies', 'entries', 'targets', 'owners', 'extensions', 'byCarrier', 'writers', 'tiedWriters',
17821790
'merged', 'baseFields', 'blamed',
17831791
// #25760: the controlled-by-parent owner arm's own collections and the predicate string.
1784-
'cbpDetailsWithoutOptIn', // the detail names without the opt-in — JS Set methods.
1792+
'cbpDetails', // #26591: the resolving details by name — JS Map methods.
17851793
'policies', // one set's `rowLevelSecurity` records — `.length`, a JS property.
17861794
'predicate', // a policy's `using` / `check` text, a string — `.trim`.
17871795
]);
@@ -2010,6 +2018,29 @@ const REACHABILITY_CORPUS: Array<{ label: string; stack: Record<string, unknown>
20102018
}],
20112019
},
20122020
},
2021+
{
2022+
// [#26591] The same rule's second push site: an opted-in detail whose
2023+
// `update` policy keys the caller on its own user field, which the opt-in
2024+
// (the stored `owner_id` only) does not admit.
2025+
label: 'cbp-caller-policy-off-owner-column',
2026+
stack: {
2027+
objects: [
2028+
objectFixture({ name: 'invoice', sharingModel: 'private' }),
2029+
objectFixture({
2030+
name: 'line_item', sharingModel: 'controlled_by_parent',
2031+
sharing: { controlledByParent: { ownerMayEdit: true } },
2032+
fields: {
2033+
invoice: { type: 'master_detail', label: 'Invoice', reference: 'invoice', required: true },
2034+
assignee: { type: 'user', label: 'Assignee', reference: 'sys_user' },
2035+
},
2036+
}),
2037+
],
2038+
permissions: [{
2039+
name: 'ps', label: 'PS', objects: {},
2040+
rowLevelSecurity: [{ name: 'assigned_lines', object: 'line_item', operation: 'update', using: 'assignee == current_user.id', enabled: true }],
2041+
}],
2042+
},
2043+
},
20132044
{
20142045
label: 'grant-expired-at-authoring',
20152046
stack: { data: [{ object: 'sys_user_position', records: [{ user_id: 'u1', position: 'p', valid_until: '2020-01-01T00:00:00Z' }] }] },
@@ -2033,7 +2064,8 @@ describe('validateSecurityPosture — every branch is reachable without an undec
20332064
// 16 since #23268: the external dial's alias and wider-than-internal sites left with the key.
20342065
// 17 since #25575: allowDelete: false beside modifyAllRecords: true.
20352066
// 18 since #25760: an owner-keyed update policy on a controlled_by_parent detail that has not opted in.
2036-
expect(pushedRuleIds()).toHaveLength(18);
2067+
// 19 since #26591: the same rule id, for a policy keyed only on a column the opt-in does not admit.
2068+
expect(pushedRuleIds()).toHaveLength(19);
20372069
});
20382070

20392071
it('reaches every `findings.push` site from that corpus', () => {

0 commit comments

Comments
 (0)