diff --git a/.changeset/seed-name-lookup-unscoped-page-budget.md b/.changeset/seed-name-lookup-unscoped-page-budget.md new file mode 100644 index 0000000000..64cc912f7c --- /dev/null +++ b/.changeset/seed-name-lookup-unscoped-page-budget.md @@ -0,0 +1,48 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(security): stop reading a truncated existence page as "absent" — the unscoped page cap is now measured, not trusted (#11518) + +`buildExistingByName` (`seed-name-lookup.ts`) is the batched existence oracle the +identity seeders consult in place of a per-item read. Its UNSCOPED page was +capped at `limit: names.length`, which is exact only while one row can exist per +name. Since #8461 / ADR-0120 D1 `sys_capability.name` and +`sys_permission_set.name` are unique **per organization**, and ADR-0066 D1 +explicitly encourages admins to EXTEND the registry inside their own +organization — so one name legitimately carries a row per organization plus the +platform's, and an unscoped page of N names can match far more than N rows. + +The rows that fall off a full page are the highest `id`s under #4363's +`ORDER BY id ASC` tie-breaker, so **whole names vanish from the page** — and a +name missing from the page reads as `absent`, which routes its caller to the +**INSERT** branch. #10103 had already found and repaired exactly this on the +SCOPED arm; the unscoped arm never got the repair, and two seeders on `main` +read unscoped (`bootstrapDeclaredCapabilities`, `permission-set-projection`'s +env-overlay pass). + +⛔ `names.length * 2` would have been the same defect with a larger constant: +rows-per-name is bounded only by the number of organizations, so no constant +multiplier is correct. Instead the cap stopped being a promise and became a +**measurement** — the read asks for one row MORE than it is willing to hold, and +a page that comes back carrying that extra row is a PREFIX of the answer rather +than the answer. It then 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`. Both directions are exact: no complete page is ever mistaken for +a truncated one, and no truncated page for a complete one. + +**Behaviour change, stated rather than slipped in.** In the truncating case the +two unscoped seeders go from a **silent wrong answer to a loud slow one**: names +that used to be reported `absent` (and re-inserted, or refused by the unique key +as a collision naming a row nobody ever saw) are now answered correctly, at the +cost of one read per name plus a warning naming the object and the budget it +could not fit inside. An install that does not overflow the budget — every stock +one, where a name carries a single row — issues exactly the same single read it +issued before and says nothing. + +The SCOPED arm keeps #10103's cap exactly (`names.length * 2`), because there the +number is a proven bound rather than a budget: `applyTenantScope` returns this +organization's rows plus organization-less ones, and the declared name index is +unique per organization. It gains the same probe, which turns a scoped page that +overflows that bound — reachable only where the unique index is absent or not yet +created — into the same loud degradation instead of a silent truncation. diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts index 4978a0b93d..01b16402b7 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts @@ -36,7 +36,7 @@ function makeQl(declared: any[] = []) { // of these names exist" — so every provenance case below would silently // become a first-boot insert while the suite reported green. That is the // double's limits masquerading as the seeder's behaviour. - return rows.filter((r) => + const matched = rows.filter((r) => Object.entries(where).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); if (v && typeof v === 'object' && !Array.isArray(v)) { @@ -47,6 +47,16 @@ function makeQl(declared: any[] = []) { return (v === null ? r[k] == null : r[k] === v); }), ); + // [#11518] `limit` is HONOURED, and a paged read is ordered by `id` + // ascending (#4363's pagination tie-breaker). Both are properties of the + // shipped drivers, measured for the sibling double in + // `bootstrap-system-capabilities.test.ts`; this one ignored `limit` + // entirely, which made the whole class of page-cap defect INEXPRESSIBLE + // here — including #11518's, whose consequence lands on THIS seeder. + if (q?.limit === undefined) return matched; + return [...matched] + .sort((a, b) => (String(a.id) < String(b.id) ? -1 : String(a.id) > String(b.id) ? 1 : 0)) + .slice(0, q.limit); }, async insert(object: string, data: any) { if (object !== 'sys_capability') return null; @@ -409,3 +419,86 @@ describe('unowned-declaration diagnostic (#4967 Part 3)', () => { expect(w!.meta?.grantedBy).toEqual(['(unnamed permission set)']); }); }); + +/** + * [#11518] THE CONSEQUENCE THIS SEEDER PAYS FOR A TRUNCATED EXISTENCE PAGE. + * + * This is one of the two callers on `main` that read UNSCOPED (the other is + * `permission-set-projection`'s overlay pass), and `seed-name-lookup.ts` capped + * an unscoped page at `names.length` — exact only while one row can exist per + * name. Since #8461 / ADR-0120 D1 `sys_capability.name` is unique PER + * ORGANIZATION and ADR-0066 D1 encourages admins to EXTEND the registry inside + * their own organization, so a healthy install carries a row per organization + * plus the package's. The rows that fall off a full page are the highest `id`s + * (#4363's `ORDER BY id ASC`), so whole names vanish — and a vanished name reads + * as `absent`, which sends THIS loop to its INSERT branch. + * + * That is the whole severity of the card: not a read that under-reports, but a + * read that under-reports and then WRITES. Measured here on the seeder rather + * than argued, which needs the double above to honour `limit` — it did not, and + * that is why nothing in this file could see the defect. + */ +describe('#11518 — a truncated existence page must never route this seeder to INSERT', () => { + const NAMES = Array.from({ length: 8 }, (_, i) => `acme.cap_${i}`); + const PACKAGE_ID = 'com.acme.suite'; + const declared = NAMES.map((name, i) => ({ + name, label: `Cap ${i}`, description: `Capability ${i}.`, scope: 'org', _packageId: PACKAGE_ID, + })); + + /** + * A REBUILD on a healthy install: every declared name already has this + * package's row, and two organizations have extended the first one. Their ids + * sort first, so they take the head of the page and the package's own rows are + * what falls off the end. + */ + function fixture() { + const ql = makeQl(declared); + ql.rows.push( + { id: 'aaa_org_jia', organization_id: 'org_jia', name: NAMES[0], label: 'Jia copy', description: 'jia', scope: 'org', managed_by: 'admin', active: true }, + { id: 'aab_org_yi', organization_id: 'org_yi', name: NAMES[0], label: 'Yi copy', description: 'yi', scope: 'org', managed_by: 'admin', active: true }, + ); + NAMES.forEach((name, i) => ql.rows.push({ + id: `cap_${i}`, name, label: `Cap ${i}`, description: `Capability ${i}.`, scope: 'org', + managed_by: 'package', package_id: PACKAGE_ID, organization_id: null, active: true, + })); + return ql; + } + + it('POSITIVE CONTROL: at the cap that was live before this fix, two declared names have no row on the page', async () => { + // Pins the DOUBLE, so it holds before and after the repair — without it, + // "the seeder wrote nothing" could be green because the trap was never set. + const ql = fixture(); + const page: any[] = await ql.find('sys_capability', { + where: { name: { $in: NAMES } }, + limit: NAMES.length, // ← the UNSCOPED cap this card repairs + }); + const onThePage = new Set(page.map((r) => r.name)); + expect(page).toHaveLength(NAMES.length); + expect([...onThePage].sort()).toEqual(NAMES.slice(0, 6).sort()); + for (const lost of [NAMES[6], NAMES[7]]) { + expect(onThePage.has(lost), `${lost} fell off a full page`).toBe(false); + expect(ql.rows.some((r: any) => r.name === lost && r.managed_by === 'package')).toBe(true); + } + }); + + it('re-seeds a healthy multi-organization install WITHOUT writing anything', async () => { + const ql = fixture(); + const rowsBefore = ql.rows.length; + const out = await bootstrapDeclaredCapabilities(ql, null); + + // ⛔ The load-bearing assertion. Before the repair this was `seeded: 2` and + // two DUPLICATE rows for names whose package rows were sitting in the table + // — every boot, on an install that is simply using ADR-0066 D1. + expect(out.seeded).toBe(0); + expect(ql.rows).toHaveLength(rowsBefore); + // Nothing else was written either: seven of the names resolve to this + // package's own unchanged row… + expect(out.updated).toBe(0); + expect(out.unchanged).toBe(NAMES.length - 1); + // …and the eighth resolves to an organization's authored copy, which this + // seeder never clobbers. Unscoped, the first row by id is the row — the + // pre-#10946 per-item answer, unchanged by this repair. + expect(out.skippedAdmin).toBe(1); + expect(out.unreadable).toBe(0); + }); +}); diff --git a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts index 0f72478f4b..45ace035e8 100644 --- a/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts +++ b/packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts @@ -1013,27 +1013,34 @@ describe('#11451 — the batched curated read must carry its predicate, not filt expect((found as { row: any }).row.managed_by).toBe('admin'); }); - it('WITHOUT the predicate the page also TRUNCATES, and a truncated page reads as "absent"', async () => { + it('WITHOUT the predicate the page hauls every organization\'s copy — and the wrong row answers', async () => { const ql = fixture(); - // 10 rows carry the 8 requested names; the unscoped page is capped at 8, so - // the two highest-id rows fall off — and their names are platform rows that - // demonstrably exist. + // ⚠️ [#11518] THIS TEST USED TO PIN THE OPPOSITE OUTCOME, and the change is + // the repair rather than a weakened assertion. The unscoped page was capped + // at one row per requested name, so on this same 10-row fixture the two + // highest-id platform rows fell off the 8-row page and their names read as + // `absent` — which routes the caller to its INSERT branch. That was #11518's + // measurement, taken here; the cap is now measured rather than trusted + // (`seed-name-lookup.ts`), so the page carries all ten rows and loses nobody. const index = await buildExistingByName(ql, 'sys_capability', CURATED_NAMES); - const missing: string[] = []; for (const name of CURATED_NAMES) { - if ((await index.get(name)).status === 'absent') missing.push(name); - } - expect(missing).toEqual([CURATED_NAMES[6], CURATED_NAMES[7]]); - // Their rows are right there. "Absent" would send the curated half to its - // insert branch, where the unique key refuses the write and the seeder - // reports a `blockedCurated` collision — every boot, on a healthy install. - for (const name of missing) { - expect(ql.rows.some((r) => r.name === name && r.managed_by === 'platform')).toBe(true); + expect((await index.get(name)).status, name).toBe('present'); } - // …and the predicated read over the same fixture loses nobody. + // What #11518 does NOT repair — and cannot — is WHICH row answers. The + // unpredicated read still hauls both organizations' copies into the page, + // and unscoped the first row is the row, so an organization's authored copy + // answers for the platform's own definition. That is #11451 exactly, and no + // page budget reaches it: only the predicate does. + const shared = await index.get(SHARED); + expect((shared as { row: any }).row.id).toBe('aaa_org_jia'); + expect((shared as { row: any }).row.managed_by).toBe('admin'); + + // …while the predicated read over the same fixture answers with the + // platform's row for every name, and never sees the copies at all. const scoped = await buildExistingByName(ql, 'sys_capability', CURATED_NAMES, undefined, undefined, CURATED_LOOKUP); for (const name of CURATED_NAMES) expect((await scoped.get(name)).status).toBe('present'); + expect(((await scoped.get(SHARED)) as { row: any }).row.id).toBe('cap_0'); }); it('the seeder itself is unharmed by the fixture that breaks the unpredicated read', async () => { @@ -1061,3 +1068,189 @@ describe('#11451 — the batched curated read must carry its predicate, not filt expect(warn.mock.calls.map((c) => c[0]).some((m) => String(m).includes('could not be read'))).toBe(true); }); }); + +/** + * [#11518] THE UNSCOPED PAGE CAP IS A MEASUREMENT, NOT A PROMISE. + * + * `readNamePage` used to cap an unscoped page at `names.length`, which is 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 — and the rows that fall off a full + * page are the highest `id`s under #4363's `ORDER BY id ASC`, so WHOLE NAMES + * vanish. A vanished name reads as `absent`, and `absent` INSERTS. + * + * ⛔ `names.length * 2` would have been the same defect with a larger constant: + * rows-per-name is bounded only by the number of organizations. So the cap is + * not widened to a "safe" number — the read asks for ONE ROW MORE than it is + * willing to hold, and a page that comes back with 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, which is the fallback that was already there + * for a driver without `$in`. + * + * ⚠️ These tests use the `makeQl` double at the top of this file precisely + * because it HONOURS `limit` and orders by `id` ascending — the two shipped + * driver behaviours the defect is made of. A double that ignored `limit` could + * not express any of this. + */ +describe('#11518 — a page that could not fit the answer must not report "absent"', () => { + const CURATED_NAMES = KNOWN_CAPABILITIES.map((c) => c.name); + /** The name organizations also hold — ADR-0066 D1 "admins EXTEND". */ + const SHARED = CURATED_NAMES[0]; + + /** Record every query the double is asked, in order. */ + function recordFinds(ql: any): any[] { + const seen: any[] = []; + const inner = ql.find; + ql.find = async (object: string, q: any, ctx?: any) => { + seen.push({ object, ...q }); + return inner(object, q, ctx); + }; + return seen; + } + + /** Every curated name in the platform bucket, plus `copies` organizations' rows for one of them. */ + function fixture(copies = 2) { + const ql = makeQl(); + // Organization ids sort BEFORE the platform's, so they take the head of any + // page and it is the PLATFORM rows that fall off the end. + for (let i = 0; i < copies; i++) { + ql.rows.push({ + id: `aaa_org_${String(i).padStart(3, '0')}`, organization_id: `org_${i}`, name: SHARED, + label: `Copy ${i}`, description: `copy ${i}`, scope: 'org', managed_by: 'admin', active: true, + }); + } + KNOWN_CAPABILITIES.forEach((c, i) => ql.rows.push({ + id: `cap_${i}`, name: c.name, label: c.label, description: c.description, + scope: c.scope, managed_by: 'platform', organization_id: null, active: true, + })); + return ql; + } + + it('POSITIVE CONTROL: the cap that was live before this fix truncates on this fixture', async () => { + // Harness-level and stable — it pins the DOUBLE, so it holds before and + // after the repair, and it is what makes the next test meaningful. Without + // it, "no name reads absent" could be green because the trap was never set. + const ql = fixture(); + const page: any[] = await ql.find('sys_capability', { + where: { name: { $in: CURATED_NAMES } }, + limit: CURATED_NAMES.length, // ← the UNSCOPED cap this card repairs + }); + expect(ql.rows).toHaveLength(CURATED_NAMES.length + 2); + expect(page).toHaveLength(CURATED_NAMES.length); + const namesOnThePage = new Set(page.map((r) => r.name)); + expect(namesOnThePage.size, 'two names are missing from a page that is full').toBe(CURATED_NAMES.length - 2); + for (const lost of [CURATED_NAMES[6], CURATED_NAMES[7]]) { + expect(namesOnThePage.has(lost)).toBe(false); + expect(ql.rows.some((r: any) => r.name === lost && r.managed_by === 'platform')).toBe(true); + } + }); + + it('reads every name whose row exists, in ONE round trip', async () => { + const ql = fixture(); + const finds = recordFinds(ql); + const warn = vi.fn(); + const index = await buildExistingByName(ql, 'sys_capability', CURATED_NAMES, { warn }); + + const absent: string[] = []; + for (const name of CURATED_NAMES) { + if ((await index.get(name)).status === 'absent') absent.push(name); + } + // ⛔ The two names the page used to lose. Their rows are right there. + expect(absent).toEqual([]); + // The whole point of #10946 survives: an install that does not overflow the + // budget still pays exactly ONE read, and says nothing. + expect(finds).toHaveLength(1); + expect(finds[0].limit).toBeGreaterThan(CURATED_NAMES.length); + expect(warn).not.toHaveBeenCalled(); + }); + + it('an overflowing page degrades to the per-item read — LOUDLY, and never to "absent"', async () => { + // 40 organizations have extended one curated name (ADR-0066 D1). No page + // budget covers this, which is the point: the bound is the organization + // count, so the read has to notice rather than guess. + const ql = fixture(40); + const finds = recordFinds(ql); + const warn = vi.fn(); + const index = await buildExistingByName(ql, 'sys_capability', CURATED_NAMES, { warn }); + + for (const name of CURATED_NAMES) { + expect((await index.get(name)).status, name).toBe('present'); + } + + // The degradation is structural, not incidental: one batched read that could + // not answer, then one read per name — `limit: 1`, exactly the shape the + // loops used before the hoist. + expect(finds).toHaveLength(1 + CURATED_NAMES.length); + expect(finds[0].where).toEqual({ name: { $in: CURATED_NAMES } }); + expect(finds[0].limit).toBeGreaterThan(CURATED_NAMES.length); + expect(finds.slice(1).every((q) => q.limit === 1)).toBe(true); + + // …and the answer is the one the pre-#10946 per-item read gave: unscoped, + // the first row by id is the row. + expect(((await index.get(CURATED_NAMES[7])) as { row: any }).row.id).toBe('cap_7'); + expect(((await index.get(SHARED)) as { row: any }).row.id).toBe('aaa_org_000'); + + // Loud: ONE line, naming the object and the budget it could not fit inside. + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain('TRUNCATED'); + expect(String(warn.mock.calls[0][0])).toContain('falling back to one read per item'); + expect(warn.mock.calls[0][1]).toMatchObject({ object: 'sys_capability', names: CURATED_NAMES.length }); + expect(typeof warn.mock.calls[0][1].rowBudget).toBe('number'); + }); + + it('an unreadable page is still reported as a FAILED read, not as a truncated one', async () => { + // The two causes share a consequence and must not share a diagnostic: an + // outage and a wide catalog take opposite remedies. + const ql = Object.assign(fixture(), { async find() { throw new Error('fake driver: read unavailable'); } }); + const warn = vi.fn(); + await buildExistingByName(ql, 'sys_capability', CURATED_NAMES, { warn }); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain('batched seed existence read failed'); + expect(String(warn.mock.calls[0][0])).not.toContain('TRUNCATED'); + }); + + it('the SCOPED arm keeps #10103\'s exact bound — its probe never fires on a healthy catalog', async () => { + // Scoped, the bound is PROVEN rather than budgeted: `applyTenantScope` + // returns `organization_id = :tenant OR organization_id IS NULL` and the + // declared name index is unique per organization, so two rows per name is + // the ceiling. #10103's cap stays exactly that. + const ql = makeQl(); + ql.rows.push( + { id: 'a_residue', organization_id: null, name: 'manage_users', label: 'Residue', description: 'pre-fix', scope: 'org', managed_by: 'platform', active: true }, + { id: 'b_own', organization_id: 'org_jia', name: 'manage_users', label: 'Own', description: 'ours', scope: 'org', managed_by: 'platform', active: true }, + ); + const finds = recordFinds(ql); + const warn = vi.fn(); + const index = await buildExistingByName(ql, 'sys_capability', ['manage_users'], { warn }, 'org_jia'); + + const found = await index.get('manage_users'); + expect(found.status).toBe('present'); + expect((found as { row: any }).row.id).toBe('b_own'); + expect(finds).toHaveLength(1); + expect(warn).not.toHaveBeenCalled(); + }); + + it('a SCOPED page that overflows that bound degrades too — the uniqueness it rests on is not holding', async () => { + // Three rows for one name under one organization's scope is only reachable + // where the declared unique index is absent or not yet created — the same + // deployment the module header calls out as the one the pre-#10946 shape + // duplicated rows on. It is now a loud degradation instead of a silent + // truncation. + const ql = makeQl(); + ql.rows.push( + { id: 'a_residue', organization_id: null, name: 'manage_users', label: 'Residue', description: 'pre-fix', scope: 'org', managed_by: 'platform', active: true }, + { id: 'b_dup', organization_id: 'org_jia', name: 'manage_users', label: 'Dup', description: 'dup', scope: 'org', managed_by: 'platform', active: true }, + { id: 'c_own', organization_id: 'org_jia', name: 'manage_users', label: 'Own', description: 'ours', scope: 'org', managed_by: 'platform', active: true }, + ); + const warn = vi.fn(); + const index = await buildExistingByName(ql, 'sys_capability', ['manage_users'], { warn }, 'org_jia'); + + const found = await index.get('manage_users'); + expect(found.status).toBe('present'); + expect((found as { row: any }).row.organization_id).toBe('org_jia'); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain('TRUNCATED'); + expect(warn.mock.calls[0][1]).toMatchObject({ organization: 'org_jia' }); + }); +}); diff --git a/packages/plugins/plugin-security/src/packaged-permission-set-lock.test.ts b/packages/plugins/plugin-security/src/packaged-permission-set-lock.test.ts index 00c10b1e9d..dfcc6f4a76 100644 --- a/packages/plugins/plugin-security/src/packaged-permission-set-lock.test.ts +++ b/packages/plugins/plugin-security/src/packaged-permission-set-lock.test.ts @@ -41,12 +41,22 @@ * ## ⭐ The fail-open this must not inherit (#11518) * * `buildExistingByName`'s UNSCOPED page cap (`limit: names.length`, - * `seed-name-lookup.ts`) truncates as soon as one name can carry more than one - * row, and a truncated page reads as "absent". Asking THAT oracle "is this set + * `seed-name-lookup.ts`) truncated as soon as one name could carry more than one + * row, and a truncated page read as "absent". Asking THAT oracle "is this set * package-declared?" would turn a truncation into "not package-declared" — and * the save this ruling exists to refuse would be accepted. A silent fork * produced by the code written to stop silent forks. * + * #11518 has since repaired the cap itself — an overflowing page is now detected + * and degrades to the per-item read rather than answering — so the controls + * below no longer guard against THAT truncation reaching this verdict. They are + * kept, and they still pass, because what they actually pin is structural and + * outlives the repair: this verdict is not decided by a name-keyed page read at + * all. Unscoped, `buildExistingByName` still answers with the FIRST row by id + * for a name, so on a name several organizations hold it answers with somebody + * else's row — the same question, a different wrong answer, and one no page + * budget reaches. + * * So the provenance question is decided from the engine's SchemaRegistry — the * same source `readDeclared` / `permission-set-overlay-discard.ts` already use, * an in-memory array with no page, no cap and no `$in`. Two controls prove the @@ -812,7 +822,7 @@ describe('control A — #11518 shape: a name carrying MORE THAN ONE row still re const names = ['ehr_quality_inspector']; const capped = await ql.find('sys_permission_set', { where: { name: { $in: names } }, - limit: names.length, // ← the UNSCOPED cap live on main at seed-name-lookup.ts + limit: names.length, // ← the UNSCOPED cap seed-name-lookup.ts carried before #11518 }); expect(ql.permRows.filter((r: any) => r.name === 'ehr_quality_inspector')).toHaveLength(2); expect(capped, 'the page is truncated — half the rows for this name are invisible').toHaveLength(1); diff --git a/packages/plugins/plugin-security/src/packaged-permission-set-lock.ts b/packages/plugins/plugin-security/src/packaged-permission-set-lock.ts index 40b5b2c09a..5e7d88a058 100644 --- a/packages/plugins/plugin-security/src/packaged-permission-set-lock.ts +++ b/packages/plugins/plugin-security/src/packaged-permission-set-lock.ts @@ -33,11 +33,17 @@ * * ⛔ So the answer is NOT taken from a name-keyed page over * `sys_permission_set`. The batched existence oracle - * (`seed-name-lookup.ts`'s `buildExistingByName`) caps its UNSCOPED page at + * (`seed-name-lookup.ts`'s `buildExistingByName`) capped its UNSCOPED page at * `limit: names.length`, which truncates the moment one name can carry more - * than one row — and a truncated page reads as `absent` (#11518, open at the - * time of writing; ⛔ not fixed here, it belongs to whoever takes it). Under - * this lock that `absent` would read as "not package-declared". + * than one row — and a truncated page read as `absent`. #11518 has since + * repaired that: the page budget is measured (one row more than it will hold is + * requested, so overflow is DETECTED) and an overflowing page degrades to the + * per-item read instead of answering. ⚠️ That does not make this oracle safe to + * ask HERE, and the reason is worth stating rather than re-deriving: unscoped, + * `buildExistingByName` answers with the FIRST row by id for a name, so on a + * name several organizations hold it can answer with somebody else's row — a + * different wrong answer to the same question. This lock's read must have no + * page in it at all. * * ⭐ The answer comes from the engine's SchemaRegistry instead — the same * source `bootstrapDeclaredPermissions`' {@link readDeclared} and diff --git a/packages/plugins/plugin-security/src/seed-name-lookup.ts b/packages/plugins/plugin-security/src/seed-name-lookup.ts index ce86471f2a..0775e020d9 100644 --- a/packages/plugins/plugin-security/src/seed-name-lookup.ts +++ b/packages/plugins/plugin-security/src/seed-name-lookup.ts @@ -36,6 +36,8 @@ * - a thrown read → could not answer * - a response that is neither an array * nor `{ records: [...] }` → could not answer + * - a page carrying MORE rows than it + * budgeted for (#11518, below) → could not answer * - `[]` → ANSWERED: none of these names exist * * "Could not answer" degrades — loudly warned — to the per-item read the loops @@ -55,6 +57,32 @@ * Directive #12): the batched and per-item reads ask the driver the same * question, and the answer has one meaning. * + * ## The page budget, and why it is measured rather than trusted (#11518) + * + * A `LIMIT` cannot express "one row per name", so the page needs a cap and no + * cap is CORRECT. `limit: names.length` was exact only while one row could exist + * per name; #10103 found that and repaired the SCOPED arm, where the honest + * bound really is two rows per name. The unscoped arm kept the old cap, and its + * bound is not two — it is the number of organizations, because + * `sys_capability.name` and `sys_permission_set.name` are unique PER + * ORGANIZATION (#8461 / ADR-0120 D1) and ADR-0066 D1 encourages admins to EXTEND + * the registry inside their own organization. `names.length * 2` would have been + * the same defect with a larger constant. + * + * So the cap stopped being a promise and became a MEASUREMENT: the read asks for + * one row MORE than it is willing to hold, and a page that comes back with that + * extra row is a prefix of the answer rather than the answer. It joins the two + * causes above — could not answer — and degrades to the same per-item read. + * + * ⚠️ That is a BEHAVIOUR CHANGE for the two callers that read unscoped + * (`bootstrapDeclaredCapabilities`, `permission-set-projection`'s overlay pass), + * and the direction is deliberate: in the truncating case they used to get a + * SILENT WRONG answer — names reported `absent` whose rows were sitting in the + * table, sending each to its insert branch — and now get a LOUD SLOW one, N + * round trips and a warning naming the budget. An install that does not truncate + * (every stock one: one row per name) reads exactly the same page it read + * before, in exactly one round trip. + * * ## Chunking * * `$in` binds one parameter per name, and SQLite builds cap bound parameters @@ -150,8 +178,80 @@ export interface ExistingByNameIndex { } /** - * Read one page of names. Returns `null` — distinct from `[]` — when the driver - * did not return a result set at all. + * [#11518] Rows per requested name an UNSCOPED page is willing to hold before + * it stops trying to answer in one read. + * + * ⚠️ A BUDGET, not a bound — the distinction is the whole of #11518. Nothing + * bounds rows-per-name here: `sys_capability.name` and `sys_permission_set.name` + * are unique PER ORGANIZATION (#8461 / ADR-0120 D1) 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. Any constant + * multiplier is therefore the same defect with a larger number, which is why + * this one buys nothing except SPEED: {@link readNamePage} asks for one row more + * than it, so exceeding it is DETECTED rather than silently truncated. + * + * The shape (`names.length * 4`, floored) matches the sibling generous cap on + * the same table in `security-plugin.ts`'s permission-set loader. + */ +const UNSCOPED_ROWS_PER_NAME = 4; + +/** Floor for the unscoped budget, so a one-name read is not budgeted at four. */ +const UNSCOPED_PAGE_FLOOR = 20; + +/** + * [#10103] Rows per requested name a SCOPED page must hold — and here the + * number IS a proven bound rather than a budget. `applyTenantScope` returns + * `organization_id = :tenant OR organization_id IS NULL`, and the declared name + * index is unique per organization, so each name has at most this organization's + * row plus one organization-less leftover. Kept exact deliberately: a scoped + * page that overflows it means the uniqueness the catalog is built on is not + * holding, and #11518's probe turns that into a loud degradation instead of a + * silent truncation. + */ +const SCOPED_ROWS_PER_NAME = 2; + +/** How many rows this page is willing to hold. See the two constants above. */ +function pageRowBudget(names: string[], organizationId?: string): number { + return organizationId + ? names.length * SCOPED_ROWS_PER_NAME + : Math.max(names.length * UNSCOPED_ROWS_PER_NAME, UNSCOPED_PAGE_FLOOR); +} + +/** + * One page, or WHY there is none — the two `cause`s are different events with + * the same consequence (see {@link readNamePage}). + */ +type NamePage = + | { readonly ok: true; readonly rows: any[] } + | { readonly ok: false; readonly cause: 'unreadable' | 'truncated'; readonly budget: number }; + +/** + * Read one page of names — `ok: false`, distinct from an empty page, when this + * read cannot answer. + * + * ## [#11518] Truncation is "could not answer", not "none of them exist" + * + * A `LIMIT` cannot say "one row per name", so any cap this read picks can be + * exceeded by a healthy install (see {@link UNSCOPED_ROWS_PER_NAME}). The rows + * that fall off are the highest `id`s under #4363's `ORDER BY id ASC` tie- + * breaker, so WHOLE NAMES vanish from the page — and a name missing from the + * page reads as `absent`, which routes its caller to the INSERT branch. That is + * the same conflation the module header refuses for a failed read, arriving + * through a page that succeeded. + * + * So the cap is turned into a MEASUREMENT: ask for `budget + 1` rows and + * compare. + * + * - `page.length <= budget` — the driver had no `budget + 1`-th row to give, + * so this page is provably the COMPLETE set for these names; + * - `page.length > budget` — there is at least one more row than this read is + * willing to hold, so the page is a PREFIX of the answer and cannot be read + * as one. `ok: false`, and the caller degrades to the per-item read exactly + * as it does for a driver without `$in`. + * + * Both directions are exact, which is what makes the budget a free choice: no + * legitimate page is ever mistaken for a truncated one, and no truncated page is + * ever mistaken for a complete one. */ async function readNamePage( ql: any, @@ -159,7 +259,8 @@ async function readNamePage( names: string[], organizationId?: string, equals?: Readonly>, -): Promise { +): Promise { + const budget = pageRowBudget(names, organizationId); let rows: any; try { rows = await ql.find( @@ -170,23 +271,26 @@ async function readNamePage( // emitted before — not the same keys plus `undefined`-valued ones, // which `toEqual` would have quietly accepted. where: { name: { $in: names }, ...(equals ?? {}) }, - // [#10103] `names.length` was exactly right while one row existed per - // name. Once the catalog is per organization the driver returns this - // organization's rows AND any organization-less ones, so that cap - // TRUNCATES — and a truncated page reads as "absent", which inserts. - // Bounded, just wide enough to admit both. - limit: organizationId ? names.length * 2 : names.length, + // [#11518] ONE MORE than the budget, always — the extra row is the + // probe, and reading it back is how truncation is told from a page that + // merely happens to be full. + limit: budget + 1, }, { context: lookupCtx(organizationId) }, ); } catch { - return null; + return { ok: false, cause: 'unreadable', budget }; } - if (Array.isArray(rows)) return rows; // Some drivers wrap the page (`{ records }`) — a wrapped array is still an // answer. Anything else (undefined/null/a scalar) is not. - if (Array.isArray(rows?.records)) return rows.records as any[]; - return null; + const page: any[] | null = Array.isArray(rows) + ? rows + : Array.isArray(rows?.records) + ? (rows.records as any[]) + : null; + if (page === null) return { ok: false, cause: 'unreadable', budget }; + if (page.length > budget) return { ok: false, cause: 'truncated', budget }; + return { ok: true, rows: page }; } /** @@ -275,16 +379,23 @@ export async function buildExistingByName( * are different questions: `sys_capability.name` is unique per ORGANIZATION, * so one name can have a row per organization plus the platform's. * - * ⚠️ PRECONDITION, and it is the caller's to discharge: the predicate must - * keep the result a SINGLETON per name. `readNamePage` caps an unscoped page - * at `names.length`, so a question that can return more than one row per name - * truncates — and a truncated page reads as `absent`, which INSERTS. The - * curated predicate discharges this by construction: the declared unique key - * is `(COALESCE(organization_id, '__global__'), name)` (ADR-0120 D3), so the + * ⚠️ The predicate should keep the result a SINGLETON per name, and the + * curated one discharges that by construction: the declared unique key is + * `(COALESCE(organization_id, '__global__'), name)` (ADR-0120 D3), so the * NULL-organization bucket admits at most one row per name — "exactly the * bucket this key part keeps a singleton", as `sys-capability.object.ts` puts * it. Narrowing can only SHRINK a page, so passing a predicate never makes * truncation likelier than the unpredicated read it replaces. + * + * [#11518] That used to be a CORRECTNESS precondition the caller had to + * discharge — an unscoped page was capped at `names.length`, so a + * non-singleton question truncated, and a truncated page read as `absent`, + * which INSERTS. {@link readNamePage} now measures its own truncation, so a + * caller that breaks the singleton property gets the per-item read (slower, + * and it says so) rather than a wrong answer. What the predicate still buys is + * WHICH row answers: unscoped, the first row of the page is the row, so a + * question wide enough to match somebody else's copy resolves to it — the + * separate harm #11451 exists for, and one no page budget can repair. */ equals?: Readonly>, ): Promise { @@ -316,17 +427,31 @@ export async function buildExistingByName( if (wanted.length === 0) return fromIndex; for (let i = 0; i < wanted.length; i += NAME_CHUNK_SIZE) { - const page = await readNamePage(ql, object, wanted.slice(i, i + NAME_CHUNK_SIZE), organizationId, equals); - if (page === null) { + const outcome = await readNamePage(ql, object, wanted.slice(i, i + NAME_CHUNK_SIZE), organizationId, equals); + if (!outcome.ok) { // ⛔ NOT "none of them exist" — see the module header. Fall back to the // per-item read so behaviour is exactly what it was before the hoist. + // + // [#11518] TWO events, ONE consequence. A truncated page is not a broken + // driver — the read worked and the answer is simply wider than one page — + // so it is named separately, because the remedies differ: an unreadable + // database is an outage, while a truncated page is an install whose + // catalog carries more rows per name than this read budgets for, and the + // only cost is the round trips the batching removed. logger?.warn?.( - '[security] batched seed existence read failed — falling back to one read per item', - { object, names: wanted.length, ...(organizationId ? { organization: organizationId } : {}) }, + outcome.cause === 'truncated' + ? '[security] batched seed existence read TRUNCATED — more rows carry these names than one page holds, so the page cannot answer; falling back to one read per item' + : '[security] batched seed existence read failed — falling back to one read per item', + { + object, + names: wanted.length, + ...(outcome.cause === 'truncated' ? { rowBudget: outcome.budget } : {}), + ...(organizationId ? { organization: organizationId } : {}), + }, ); return perItemIndex(ql, object, organizationId, equals); } - for (const row of page) { + for (const row of outcome.rows) { const name = row?.name; if (name == null) continue; // [#10103] EVERY row is kept, not just the first. A scoped page can carry