Skip to content

Commit dc6abfd

Browse files
baozhoutaoclaude
andauthored
fix(plugin-security): report only materialized capability names, so a refused declaration falls back to the derived placeholder (#4967 Part 1/3) (#5875)
bootstrapDeclaredCapabilities filled its returned name list BEFORE the upsert decided anything, so all three refusal paths reported a name they never wrote a row for. The caller uses that list to tell bootstrapSystemCapabilities to skip deriving the back-compat placeholder, so a declaration refused for want of an owning package suppressed the placeholder too and the capability then existed in no sys_capability row at all -- writing the declaration was strictly worse than omitting it. The list (renamed materializedNames) now reports only names this pass CONFIRMED have a row: written here (seeded/updated/claimed), or an existing row that must not be clobbered (admin-authored, another package's, or a curated platform name the curated pass owns). The unowned path reports its name only when a row already exists, and otherwise falls through to the derivation. Adds the skippedUnowned counter that path never had, so every named declaration lands in exactly one counter and the list reconciles with them. Part 3: the unowned-refusal diagnostic stays a warn (#4632 -- functional degradation, not durability) and now names the permission set(s) that GRANT the capability plus the actual consequence, threaded in as an argument from the bootstrap permission sets rather than any new global state. Part 2 of #4967 (stack.capabilities -> registry _packageId) is out of scope here and tracked as #5870. Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv Co-authored-by: Claude <noreply@anthropic.com>
1 parent 51a587d commit dc6abfd

6 files changed

Lines changed: 444 additions & 40 deletions
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
---
2+
"@objectstack/plugin-security": patch
3+
---
4+
5+
fix(plugin-security): 被拒收的 capability 声明不再连派生占位一起压掉 (#4967 Part 1/3)
6+
7+
`SecurityPlugin` 分两遍种 `sys_capability`:第一遍落包声明的 capability
8+
(`managed_by:'package'` + `package_id`),第二遍种平台 curated 集合 + 从
9+
permission set 的 `systemPermissions[]` **派生**的 back-compat 占位,并**跳过**
10+
第一遍报上来的名字,以免占位把已写好的声明覆盖掉。
11+
12+
问题在于第一遍报的是「读到的每个名字」,而不是「真正落了行的名字」:
13+
`bootstrapDeclaredCapabilities` 在 upsert 作出任何决定**之前**就把
14+
`cap.name` 推进了返回列表。而 upsert 有三条**拒收**路径,一行都不写。其中
15+
「声明没有归属包」这一条既没写行、又占住了名字,于是派生占位也被跳过——
16+
capability **在任何一行里都不存在**。净效果是:**写下这条声明,比不写还糟**
17+
(不写至少还有派生占位)。这正是 showcase 的
18+
`showcase.export_data` 只留下一条 `warn` 的成因。
19+
20+
修法是把「上报」与「读到」拆开:一个名字进入上报列表(现更名为
21+
`materializedNames`)的条件,是本遍**确认它有行**——本遍写成了
22+
(seeded / updated / claimed),或找到一行不能被覆盖的既有行(admin 自建、他包
23+
所有、curated 平台名)。三条拒收路径按「派生是否会覆盖既有 authored 行」分别
24+
处置,理由写在代码里:
25+
26+
- **curated 平台名**:仍然上报。curated 那一遍无条件种这些名字,行必然存在;
27+
且派生路径本来就够不到 curated 名(它已在 curated 表里)。
28+
- **他包所有 / admin 自建**:仍然上报。行存在且 label/description 是**作者写
29+
的**,派生会把它们刷成 humanize 出来的占位——压掉派生正是这份列表的用途。
30+
- **没有归属包**:仅当已存在一行时才上报。没有行时回落到派生占位,和「从未
31+
写过这条声明」时一样。
32+
33+
同时补上这条路径此前缺失的计数器 `skippedUnowned`,于是每条具名声明恰好落在
34+
一个计数器里,列表与计数器可以对账。
35+
36+
**行为变化(升级须知)**:一条被拒收(无归属包)且被某个 permission set 授权
37+
的 capability,此前在 `sys_capability` 里**没有任何行**,现在会出现一行
38+
`managed_by:'platform'` 的派生占位——即它在 Setup 的能力列表里可见、可解析、
39+
带 humanize 出来的 label。注意这不改变**运行时判定**:权限求值一直是按
40+
`systemPermissions[]` 里的字符串取并集的,从不查 `sys_capability`;恢复的是
41+
注册表一侧的 declared = enforced(能力有定义记录、可见、可管理、有 provenance),
42+
不是把一个原本不生效的授权变成生效。若某个部署依赖「那条能力在能力列表里查不
43+
到」,升级后它会出现。
44+
45+
诊断消息同时按 #4632 改进(级别仍为 `warn` —— 功能性降级,非持久性失败):
46+
拒收时点名**授权它的 permission set**,并写明真实后果,例如
47+
`[security] declared capability "showcase.export_data" has no owning package (granted by showcase_ops): falls back to the back-compat derived placeholder …`。
48+
无人授权、或已有行的情形各有对应措辞。

‎packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts‎

Lines changed: 221 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ describe('bootstrapDeclaredCapabilities (ADR-0066 D1 package declaration)', () =
4141
]);
4242
const out = await bootstrapDeclaredCapabilities(ql, null);
4343
expect(out.seeded).toBe(1);
44-
expect(out.declaredNames).toEqual(['export_data']);
44+
expect(out.materializedNames).toEqual(['export_data']);
4545
const row = ql.rows.find((r) => r.name === 'export_data');
4646
expect(row).toMatchObject({
4747
name: 'export_data',
@@ -88,6 +88,7 @@ describe('bootstrapDeclaredCapabilities (ADR-0066 D1 package declaration)', () =
8888
const ql = makeQl([{ name: 'orphan_cap', label: 'Orphan' }]);
8989
const out = await bootstrapDeclaredCapabilities(ql, null);
9090
expect(out.seeded).toBe(0);
91+
expect(out.skippedUnowned).toBe(1);
9192
expect(ql.rows.find((r) => r.name === 'orphan_cap')).toBeUndefined();
9293
});
9394

@@ -123,11 +124,11 @@ describe('bootstrapDeclaredCapabilities (ADR-0066 D1 package declaration)', () =
123124
});
124125

125126
it('declared name suppresses the implicit derived placeholder (no clobber)', async () => {
126-
// Full boot order: declared first, then system with declaredNames.
127+
// Full boot order: declared first, then system with materializedNames.
127128
const ql = makeQl([{ name: 'export_data', label: 'Export Data', scope: 'org', _packageId: 'com.acme.reports' }]);
128129
const cap = await bootstrapDeclaredCapabilities(ql, null);
129130
await bootstrapSystemCapabilities(ql, [{ systemPermissions: ['export_data'] }], {
130-
declaredCapabilityNames: cap.declaredNames,
131+
materializedCapabilityNames: cap.materializedNames,
131132
});
132133
const row = ql.rows.find((r) => r.name === 'export_data');
133134
// The package row is untouched — no humanized placeholder overwrote it.
@@ -138,6 +139,222 @@ describe('bootstrapDeclaredCapabilities (ADR-0066 D1 package declaration)', () =
138139
it('returns an empty outcome when nothing is declared', async () => {
139140
const ql = makeQl([]);
140141
const out = await bootstrapDeclaredCapabilities(ql, null);
141-
expect(out).toMatchObject({ seeded: 0, updated: 0, claimed: 0, declaredNames: [] });
142+
expect(out).toMatchObject({ seeded: 0, updated: 0, claimed: 0, skippedUnowned: 0, materializedNames: [] });
143+
});
144+
});
145+
146+
// ───────────────────────────────────────────────────────────────────────────
147+
// [#4967 Part 1] A REFUSED declaration must not suppress the back-compat
148+
// derivation. `materializedNames` reports the names this pass CONFIRMED have a
149+
// row — the three refusal paths land on different sides of that line, for
150+
// different reasons, so each gets its own pin (and, where the direction is not
151+
// obvious, the reverse case that shows what the other answer would cost).
152+
// ───────────────────────────────────────────────────────────────────────────
153+
describe('refused declarations vs. the derived placeholder (#4967 Part 1)', () => {
154+
const OPS_SETS = [{ name: 'showcase_ops', systemPermissions: ['setup.access', 'showcase.export_data'] }];
155+
156+
it('NO OWNING PACKAGE + no row: falls through, so the derivation materializes it', async () => {
157+
// The showcase repro: `showcase.export_data` declared without a resolvable
158+
// owner, granted by `showcase_ops`.
159+
const ql = makeQl([{ name: 'showcase.export_data', label: 'Export Data' }]);
160+
const out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: OPS_SETS });
161+
162+
// The declaration is refused — no package row, and (the fix) the name is
163+
// NOT reported as materialized.
164+
expect(out.seeded).toBe(0);
165+
expect(out.skippedUnowned).toBe(1);
166+
expect(out.materializedNames).toEqual([]);
167+
168+
// Second pass — the capability now exists, as the back-compat placeholder
169+
// it would have had if the declaration had never been written.
170+
await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: out.materializedNames });
171+
expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({
172+
name: 'showcase.export_data', managed_by: 'platform', active: true,
173+
});
174+
// …and it RESOLVES by name, which is what the granting permission set needs
175+
// from the registry (Setup listing, provenance, ADR-0066 ⑨ lint sources).
176+
expect(await ql.find('sys_capability', { where: { name: 'showcase.export_data' } })).toHaveLength(1);
177+
});
178+
179+
it('REVERSE: the pre-#4967 list (every declared name) leaves the capability in no row at all', async () => {
180+
// Same fixture, same second pass — only the skip list is the old one, which
181+
// reported a name the first pass refused to write. Nothing derives it and
182+
// nothing declared it: the hole.
183+
const ql = makeQl([{ name: 'showcase.export_data', label: 'Export Data' }]);
184+
await bootstrapDeclaredCapabilities(ql, null, { permissionSets: OPS_SETS });
185+
await bootstrapSystemCapabilities(ql, OPS_SETS, {
186+
materializedCapabilityNames: ['showcase.export_data'], // ← the old `declaredNames`
187+
});
188+
expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toBeUndefined();
189+
});
190+
191+
it('is stable across boots: the refusal re-derives nothing and duplicates nothing', async () => {
192+
const ql = makeQl([{ name: 'showcase.export_data', label: 'Export Data' }]);
193+
for (let boot = 0; boot < 2; boot += 1) {
194+
const out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: OPS_SETS });
195+
await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: out.materializedNames });
196+
}
197+
expect(ql.rows.filter((r) => r.name === 'showcase.export_data')).toHaveLength(1);
198+
// Boot 2 finds the placeholder, so the refusal now reports the name — the
199+
// row exists and must not be re-derived over.
200+
const out2 = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: OPS_SETS });
201+
expect(out2.skippedUnowned).toBe(1);
202+
expect(out2.materializedNames).toEqual(['showcase.export_data']);
203+
});
204+
205+
it('NO OWNING PACKAGE + an admin row: still suppresses, so the placeholder cannot clobber it', async () => {
206+
const ql = makeQl([{ name: 'showcase.export_data', label: 'Declared Label' }]);
207+
ql.rows.push({ id: 'cap_admin', name: 'showcase.export_data', label: 'Admin Made', description: 'Admin wrote this.', managed_by: 'admin' });
208+
const out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: OPS_SETS });
209+
expect(out.skippedUnowned).toBe(1);
210+
expect(out.materializedNames).toEqual(['showcase.export_data']);
211+
await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: out.materializedNames });
212+
expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({
213+
label: 'Admin Made', description: 'Admin wrote this.', managed_by: 'admin',
214+
});
215+
});
216+
217+
it('REVERSE: dropping that name from the list lets the derivation overwrite the admin row', async () => {
218+
// Why the unowned path checks for an EXISTING row instead of always
219+
// falling through: the derived defaults refresh label/description on any
220+
// row they find.
221+
const ql = makeQl([]);
222+
ql.rows.push({ id: 'cap_admin', name: 'showcase.export_data', label: 'Admin Made', description: 'Admin wrote this.', managed_by: 'admin' });
223+
await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: [] });
224+
expect(ql.rows.find((r) => r.name === 'showcase.export_data')?.label).toBe('Showcase Export Data');
225+
});
226+
227+
it('FOREIGN owner: suppresses, because the other package authored that row', async () => {
228+
const sets = [{ name: 'ops', systemPermissions: ['shared_cap'] }];
229+
const ql = makeQl([{ name: 'shared_cap', label: 'Mine', _packageId: 'com.b' }]);
230+
ql.rows.push({ id: 'cap_x', name: 'shared_cap', label: 'Owner Label', description: 'Owner wrote this.', managed_by: 'package', package_id: 'com.a' });
231+
const out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: sets });
232+
expect(out.skippedForeign).toBe(1);
233+
expect(out.materializedNames).toEqual(['shared_cap']);
234+
await bootstrapSystemCapabilities(ql, sets, { materializedCapabilityNames: out.materializedNames });
235+
expect(ql.rows.find((r) => r.name === 'shared_cap')).toMatchObject({
236+
label: 'Owner Label', description: 'Owner wrote this.', package_id: 'com.a',
237+
});
238+
});
239+
240+
it('CURATED platform name: suppresses, and the curated pass seeds the row regardless', async () => {
241+
// A no-op for the skip list (the derived path never reaches a curated name
242+
// — it is already in the curated map), but a truthful answer: the row
243+
// exists after the second pass either way.
244+
const sets = [{ name: 'ops', systemPermissions: ['manage_users'] }];
245+
const ql = makeQl([{ name: 'manage_users', label: 'Evil', _packageId: 'com.acme.evil' }]);
246+
const out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: sets });
247+
expect(out.skippedPlatform).toBe(1);
248+
expect(out.materializedNames).toEqual(['manage_users']);
249+
await bootstrapSystemCapabilities(ql, sets, { materializedCapabilityNames: out.materializedNames });
250+
expect(ql.rows.find((r) => r.name === 'manage_users')).toMatchObject({
251+
label: 'Manage Users', managed_by: 'platform', // curated definition, not the package's
252+
});
253+
});
254+
255+
it('materializedNames reconciles with the outcome counters', async () => {
256+
const ql = makeQl([
257+
{ name: 'a.new', _packageId: 'com.a' }, // → seeded
258+
{ name: 'a.own', label: 'Fresh', _packageId: 'com.a' }, // → updated
259+
{ name: 'a.derived', _packageId: 'com.a' }, // → claimed
260+
{ name: 'a.admin', _packageId: 'com.a' }, // → skippedAdmin
261+
{ name: 'a.foreign', _packageId: 'com.a' }, // → skippedForeign
262+
{ name: 'manage_users', _packageId: 'com.a' }, // → skippedPlatform
263+
{ name: 'a.orphan' }, // → skippedUnowned, NO row
264+
]);
265+
ql.rows.push({ id: 'c1', name: 'a.own', managed_by: 'package', package_id: 'com.a' });
266+
ql.rows.push({ id: 'c2', name: 'a.derived', managed_by: 'platform' });
267+
ql.rows.push({ id: 'c3', name: 'a.admin', managed_by: 'admin' });
268+
ql.rows.push({ id: 'c4', name: 'a.foreign', managed_by: 'package', package_id: 'com.z' });
269+
270+
const out = await bootstrapDeclaredCapabilities(ql, null);
271+
272+
expect(out).toMatchObject({
273+
seeded: 1, updated: 1, claimed: 1,
274+
skippedAdmin: 1, skippedForeign: 1, skippedPlatform: 1, skippedUnowned: 1,
275+
});
276+
// Every named declaration lands in exactly one counter…
277+
const counted = out.seeded + out.updated + out.claimed
278+
+ out.skippedAdmin + out.skippedForeign + out.skippedPlatform + out.skippedUnowned;
279+
expect(counted).toBe(7);
280+
// …and `materializedNames` is that set minus the refusal that found no row.
281+
expect(out.materializedNames).toEqual(['a.new', 'a.own', 'a.derived', 'a.admin', 'a.foreign', 'manage_users']);
282+
expect(out.materializedNames).toHaveLength(counted - out.skippedUnowned);
283+
expect(out.materializedNames).not.toContain('a.orphan');
284+
});
285+
});
286+
287+
// ───────────────────────────────────────────────────────────────────────────
288+
// [#4967 Part 3] The refusal diagnostic names the GRANTOR permission set(s)
289+
// and the actual consequence. Level stays `warn` per #4632 (functional
290+
// degradation, not a durability failure).
291+
// ───────────────────────────────────────────────────────────────────────────
292+
describe('unowned-declaration diagnostic (#4967 Part 3)', () => {
293+
function spyLogger() {
294+
const warns: Array<{ msg: string; meta?: Record<string, any> }> = [];
295+
const errors: string[] = [];
296+
return {
297+
warns,
298+
errors,
299+
logger: {
300+
warn: (msg: string, meta?: Record<string, any>) => { warns.push({ msg, meta }); },
301+
error: (msg: string) => { errors.push(msg); },
302+
},
303+
};
304+
}
305+
306+
it('names every permission set that grants the capability, and stays a warn', async () => {
307+
const { warns, errors, logger } = spyLogger();
308+
const ql = makeQl([{ name: 'showcase.export_data', label: 'Export Data' }]);
309+
await bootstrapDeclaredCapabilities(ql, null, {
310+
logger,
311+
permissionSets: [
312+
{ name: 'showcase_ops', systemPermissions: ['setup.access', 'showcase.export_data'] },
313+
{ name: 'showcase_admin', systemPermissions: ['showcase.export_data'] },
314+
{ name: 'unrelated', systemPermissions: ['setup.access'] },
315+
],
316+
});
317+
const w = warns.find((x) => x.msg.includes('showcase.export_data'));
318+
expect(w).toBeDefined();
319+
expect(w!.msg).toContain('has no owning package');
320+
expect(w!.msg).toContain('showcase_ops');
321+
expect(w!.msg).toContain('showcase_admin');
322+
expect(w!.msg).not.toContain('unrelated');
323+
// The consequence, not just the name: it still exists, without provenance.
324+
expect(w!.msg).toContain('derived placeholder');
325+
expect(w!.meta?.grantedBy).toEqual(['showcase_ops', 'showcase_admin']);
326+
// [#4632] functional degradation → warn, never error.
327+
expect(errors).toEqual([]);
328+
});
329+
330+
it('says so plainly when NOTHING grants the capability (it exists nowhere)', async () => {
331+
const { warns, logger } = spyLogger();
332+
const ql = makeQl([{ name: 'never_granted' }]);
333+
await bootstrapDeclaredCapabilities(ql, null, { logger, permissionSets: [{ name: 'ops', systemPermissions: ['setup.access'] }] });
334+
const w = warns.find((x) => x.msg.includes('never_granted'));
335+
expect(w!.msg).toContain('granted by no bootstrap permission set');
336+
expect(w!.msg).toContain('materialized nowhere');
337+
expect(w!.meta?.grantedBy).toEqual([]);
338+
});
339+
340+
it('reports the existing row when one already resolves the name', async () => {
341+
const { warns, logger } = spyLogger();
342+
const ql = makeQl([{ name: 'showcase.export_data' }]);
343+
ql.rows.push({ id: 'cap_p', name: 'showcase.export_data', managed_by: 'platform' });
344+
await bootstrapDeclaredCapabilities(ql, null, {
345+
logger,
346+
permissionSets: [{ name: 'showcase_ops', systemPermissions: ['showcase.export_data'] }],
347+
});
348+
const w = warns.find((x) => x.msg.includes('showcase.export_data'));
349+
expect(w!.msg).toContain('left as-is');
350+
expect(w!.meta?.grantedBy).toEqual(['showcase_ops']);
351+
});
352+
353+
it('falls back to a placeholder label for an unnamed permission set', async () => {
354+
const { warns, logger } = spyLogger();
355+
const ql = makeQl([{ name: 'orphan_cap' }]);
356+
await bootstrapDeclaredCapabilities(ql, null, { logger, permissionSets: [{ systemPermissions: ['orphan_cap'] }] });
357+
const w = warns.find((x) => x.msg.includes('orphan_cap'));
358+
expect(w!.meta?.grantedBy).toEqual(['(unnamed permission set)']);
142359
});
143360
});

0 commit comments

Comments
 (0)