Skip to content

Commit e1d773e

Browse files
claude[bot]claude
andauthored
fix(security): measure the unscoped existence page cap instead of trusting it (#11518) (#11962)
`buildExistingByName`'s UNSCOPED page was capped at `limit: names.length`, exact only while one row can exist per name. Since #8461 / ADR-0120 D1 the identity tables are unique PER ORGANIZATION and ADR-0066 D1 encourages admins to EXTEND the registry inside their own organization, so one name legitimately carries a row per organization plus the platform's. The rows that fall off a full page are the highest ids under #4363's `ORDER BY id ASC`, so whole names vanish — and a vanished name reads as `absent`, which INSERTS. No constant multiplier is correct (the bound is the organization count), so the cap is now a measurement: the read asks for one row MORE than it will hold, and a page carrying that extra row is a PREFIX of the answer. It joins the module's existing "could not answer" causes and degrades to the per-item read — the fallback already there for a driver without `$in` — with its own diagnostic. ⚠️ Behaviour change on two shipped seeders in the truncating case: from a silent wrong answer to a loud slow one. Non-truncating installs are unchanged, one read and no warning. The SCOPED arm keeps #10103's `names.length * 2` — there the number is a proven bound, not a budget — and gains the same probe, so a scoped page that overflows it (an absent unique index) degrades loudly instead of truncating silently. Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 08d6d5e commit e1d773e

6 files changed

Lines changed: 521 additions & 46 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(security): stop reading a truncated existence page as "absent" — the unscoped page cap is now measured, not trusted (#11518)
6+
7+
`buildExistingByName` (`seed-name-lookup.ts`) is the batched existence oracle the
8+
identity seeders consult in place of a per-item read. Its UNSCOPED page was
9+
capped at `limit: names.length`, which is exact only while one row can exist per
10+
name. Since #8461 / ADR-0120 D1 `sys_capability.name` and
11+
`sys_permission_set.name` are unique **per organization**, and ADR-0066 D1
12+
explicitly encourages admins to EXTEND the registry inside their own
13+
organization — so one name legitimately carries a row per organization plus the
14+
platform's, and an unscoped page of N names can match far more than N rows.
15+
16+
The rows that fall off a full page are the highest `id`s under #4363's
17+
`ORDER BY id ASC` tie-breaker, so **whole names vanish from the page** — and a
18+
name missing from the page reads as `absent`, which routes its caller to the
19+
**INSERT** branch. #10103 had already found and repaired exactly this on the
20+
SCOPED arm; the unscoped arm never got the repair, and two seeders on `main`
21+
read unscoped (`bootstrapDeclaredCapabilities`, `permission-set-projection`'s
22+
env-overlay pass).
23+
24+
`names.length * 2` would have been the same defect with a larger constant:
25+
rows-per-name is bounded only by the number of organizations, so no constant
26+
multiplier is correct. Instead the cap stopped being a promise and became a
27+
**measurement** — the read asks for one row MORE than it is willing to hold, and
28+
a page that comes back carrying that extra row is a PREFIX of the answer rather
29+
than the answer. It then joins the module's existing "could not answer" causes
30+
and degrades to the per-item read, the fallback already there for a driver
31+
without `$in`. Both directions are exact: no complete page is ever mistaken for
32+
a truncated one, and no truncated page for a complete one.
33+
34+
**Behaviour change, stated rather than slipped in.** In the truncating case the
35+
two unscoped seeders go from a **silent wrong answer to a loud slow one**: names
36+
that used to be reported `absent` (and re-inserted, or refused by the unique key
37+
as a collision naming a row nobody ever saw) are now answered correctly, at the
38+
cost of one read per name plus a warning naming the object and the budget it
39+
could not fit inside. An install that does not overflow the budget — every stock
40+
one, where a name carries a single row — issues exactly the same single read it
41+
issued before and says nothing.
42+
43+
The SCOPED arm keeps #10103's cap exactly (`names.length * 2`), because there the
44+
number is a proven bound rather than a budget: `applyTenantScope` returns this
45+
organization's rows plus organization-less ones, and the declared name index is
46+
unique per organization. It gains the same probe, which turns a scoped page that
47+
overflows that bound — reachable only where the unique index is absent or not yet
48+
created — into the same loud degradation instead of a silent truncation.

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

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ function makeQl(declared: any[] = []) {
3636
// of these names exist" — so every provenance case below would silently
3737
// become a first-boot insert while the suite reported green. That is the
3838
// double's limits masquerading as the seeder's behaviour.
39-
return rows.filter((r) =>
39+
const matched = rows.filter((r) =>
4040
Object.entries(where).every(([k, v]) => {
4141
if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`);
4242
if (v && typeof v === 'object' && !Array.isArray(v)) {
@@ -47,6 +47,16 @@ function makeQl(declared: any[] = []) {
4747
return (v === null ? r[k] == null : r[k] === v);
4848
}),
4949
);
50+
// [#11518] `limit` is HONOURED, and a paged read is ordered by `id`
51+
// ascending (#4363's pagination tie-breaker). Both are properties of the
52+
// shipped drivers, measured for the sibling double in
53+
// `bootstrap-system-capabilities.test.ts`; this one ignored `limit`
54+
// entirely, which made the whole class of page-cap defect INEXPRESSIBLE
55+
// here — including #11518's, whose consequence lands on THIS seeder.
56+
if (q?.limit === undefined) return matched;
57+
return [...matched]
58+
.sort((a, b) => (String(a.id) < String(b.id) ? -1 : String(a.id) > String(b.id) ? 1 : 0))
59+
.slice(0, q.limit);
5060
},
5161
async insert(object: string, data: any) {
5262
if (object !== 'sys_capability') return null;
@@ -409,3 +419,86 @@ describe('unowned-declaration diagnostic (#4967 Part 3)', () => {
409419
expect(w!.meta?.grantedBy).toEqual(['(unnamed permission set)']);
410420
});
411421
});
422+
423+
/**
424+
* [#11518] THE CONSEQUENCE THIS SEEDER PAYS FOR A TRUNCATED EXISTENCE PAGE.
425+
*
426+
* This is one of the two callers on `main` that read UNSCOPED (the other is
427+
* `permission-set-projection`'s overlay pass), and `seed-name-lookup.ts` capped
428+
* an unscoped page at `names.length` — exact only while one row can exist per
429+
* name. Since #8461 / ADR-0120 D1 `sys_capability.name` is unique PER
430+
* ORGANIZATION and ADR-0066 D1 encourages admins to EXTEND the registry inside
431+
* their own organization, so a healthy install carries a row per organization
432+
* plus the package's. The rows that fall off a full page are the highest `id`s
433+
* (#4363's `ORDER BY id ASC`), so whole names vanish — and a vanished name reads
434+
* as `absent`, which sends THIS loop to its INSERT branch.
435+
*
436+
* That is the whole severity of the card: not a read that under-reports, but a
437+
* read that under-reports and then WRITES. Measured here on the seeder rather
438+
* than argued, which needs the double above to honour `limit` — it did not, and
439+
* that is why nothing in this file could see the defect.
440+
*/
441+
describe('#11518 — a truncated existence page must never route this seeder to INSERT', () => {
442+
const NAMES = Array.from({ length: 8 }, (_, i) => `acme.cap_${i}`);
443+
const PACKAGE_ID = 'com.acme.suite';
444+
const declared = NAMES.map((name, i) => ({
445+
name, label: `Cap ${i}`, description: `Capability ${i}.`, scope: 'org', _packageId: PACKAGE_ID,
446+
}));
447+
448+
/**
449+
* A REBUILD on a healthy install: every declared name already has this
450+
* package's row, and two organizations have extended the first one. Their ids
451+
* sort first, so they take the head of the page and the package's own rows are
452+
* what falls off the end.
453+
*/
454+
function fixture() {
455+
const ql = makeQl(declared);
456+
ql.rows.push(
457+
{ id: 'aaa_org_jia', organization_id: 'org_jia', name: NAMES[0], label: 'Jia copy', description: 'jia', scope: 'org', managed_by: 'admin', active: true },
458+
{ id: 'aab_org_yi', organization_id: 'org_yi', name: NAMES[0], label: 'Yi copy', description: 'yi', scope: 'org', managed_by: 'admin', active: true },
459+
);
460+
NAMES.forEach((name, i) => ql.rows.push({
461+
id: `cap_${i}`, name, label: `Cap ${i}`, description: `Capability ${i}.`, scope: 'org',
462+
managed_by: 'package', package_id: PACKAGE_ID, organization_id: null, active: true,
463+
}));
464+
return ql;
465+
}
466+
467+
it('POSITIVE CONTROL: at the cap that was live before this fix, two declared names have no row on the page', async () => {
468+
// Pins the DOUBLE, so it holds before and after the repair — without it,
469+
// "the seeder wrote nothing" could be green because the trap was never set.
470+
const ql = fixture();
471+
const page: any[] = await ql.find('sys_capability', {
472+
where: { name: { $in: NAMES } },
473+
limit: NAMES.length, // ← the UNSCOPED cap this card repairs
474+
});
475+
const onThePage = new Set(page.map((r) => r.name));
476+
expect(page).toHaveLength(NAMES.length);
477+
expect([...onThePage].sort()).toEqual(NAMES.slice(0, 6).sort());
478+
for (const lost of [NAMES[6], NAMES[7]]) {
479+
expect(onThePage.has(lost), `${lost} fell off a full page`).toBe(false);
480+
expect(ql.rows.some((r: any) => r.name === lost && r.managed_by === 'package')).toBe(true);
481+
}
482+
});
483+
484+
it('re-seeds a healthy multi-organization install WITHOUT writing anything', async () => {
485+
const ql = fixture();
486+
const rowsBefore = ql.rows.length;
487+
const out = await bootstrapDeclaredCapabilities(ql, null);
488+
489+
// ⛔ The load-bearing assertion. Before the repair this was `seeded: 2` and
490+
// two DUPLICATE rows for names whose package rows were sitting in the table
491+
// — every boot, on an install that is simply using ADR-0066 D1.
492+
expect(out.seeded).toBe(0);
493+
expect(ql.rows).toHaveLength(rowsBefore);
494+
// Nothing else was written either: seven of the names resolve to this
495+
// package's own unchanged row…
496+
expect(out.updated).toBe(0);
497+
expect(out.unchanged).toBe(NAMES.length - 1);
498+
// …and the eighth resolves to an organization's authored copy, which this
499+
// seeder never clobbers. Unscoped, the first row by id is the row — the
500+
// pre-#10946 per-item answer, unchanged by this repair.
501+
expect(out.skippedAdmin).toBe(1);
502+
expect(out.unreadable).toBe(0);
503+
});
504+
});

0 commit comments

Comments
 (0)