diff --git a/.changeset/sharing-declared-field-binder-converge.md b/.changeset/sharing-declared-field-binder-converge.md new file mode 100644 index 0000000000..d0129fc270 --- /dev/null +++ b/.changeset/sharing-declared-field-binder-converge.md @@ -0,0 +1,45 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +fix(sharing): `publicSharing.eligibility` binds declared fields through the canonical `materializeDeclaredFields` instead of a local copy (#8489) + +`share-link-service.ts` carried its own `bindDeclaredFields` — a hand-written +mirror of `@objectstack/objectql`'s `materializeDeclaredFields`, named as a copy +in its own doc comment. It is retired; `assertEligible` now imports the +canonical helper from `@objectstack/objectql/core` (already a runtime dependency +of this package), with a spread at the call site because the canonical +materialises in place. + +**This changes eligibility verdicts on exactly one row shape**, and the change +was accepted knowingly (maintainer ruling, 2026-08-16). The retired mirror bound +a declared field by key PRESENCE (`!(name in record)`); the canonical binds by +VALUE (`record[name] === undefined`). They agree on every other input class, +including a missing or malformed `fields` map, where both return the record +untouched. Where they differ is a declared field held as an own key whose value +is `undefined` — a shape `InMemoryDriver` measurably produces (an explicit +`undefined` on `create` survives to `find`) and `SqlDriver` structurally cannot +(a SQL NULL arrives as `null`). + +On that shape only, with a declared `status`: + +| eligibility predicate | before | after | +|:------------------------------|:-------------------------------|:-------------------------| +| `record.status == null` | 422 `ELIGIBILITY_UNEVALUABLE` | **link is minted** | +| `has(record.status)` | 422 `RECORD_NOT_ELIGIBLE` | **link is minted** | +| `!has(record.status)` | **link was minted** | 422 `RECORD_NOT_ELIGIBLE` | +| `record.status == 'published'`| 422 `ELIGIBILITY_UNEVALUABLE` | 422 `RECORD_NOT_ELIGIBLE` | + +The first two rows widen acceptance: the predicate is now *answered* rather than +faulting on a key CEL reads as absent, and on this fail-closed gate a fault was a +refusal. The third row is the one that mattered for the decision — it **closes an +over-acceptance**. `has()` guards an UNDECLARED key and never an empty value once +bindings are materialised, so `!has(record.)` is false; the +mirror was minting share links there that every other server-side surface +refuses. The fourth row keeps its direction and changes only its ADR-0112 `code`. + +The eligibility pin is rewritten to discriminate (#9085): its previous +declared-field case passed with the binder fully ablated, because every seeded +row carried the field it claimed was absent. The replacements use a declared +field the stored row genuinely does not carry, and fail in opposite directions +under ablation. diff --git a/packages/plugins/plugin-sharing/src/share-link-eligibility.test.ts b/packages/plugins/plugin-sharing/src/share-link-eligibility.test.ts index d751051107..bb7d979946 100644 --- a/packages/plugins/plugin-sharing/src/share-link-eligibility.test.ts +++ b/packages/plugins/plugin-sharing/src/share-link-eligibility.test.ts @@ -94,6 +94,23 @@ afterEach(async () => { } }); +interface BootOptions { + /** + * The object definition the DRIVER is initialised from, when it must differ + * from the one the engine reports through `getSchema`. Defaulting to the same + * object is the ordinary case; the two differ only where a test needs a + * declared field the stored row genuinely does not carry (see the #9085 + * block), which is the shape the binder exists for. + */ + ddl?: any; + /** + * Reshape each `article` row on its way out of the driver. Used only to + * reproduce a row shape `SqlDriver` structurally cannot express — see the + * own-key-`undefined` block for what is reproduced and why it is real. + */ + shapeRow?: (row: any) => any; +} + /** * A real backend behind the real service. * @@ -101,20 +118,24 @@ afterEach(async () => { * the in-memory registry read the engine performs, so the policy the service * reads is the object's declared one. */ -async function boot(article: any = ARTICLE) { +async function boot(article: any = ARTICLE, options: BootOptions = {}) { const driver = new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, }); openDrivers.push(driver); - await driver.initObjects([SysShareLink as any, article]); + await driver.initObjects([SysShareLink as any, options.ddl ?? article]); for (const row of ROWS) await driver.create('article', row); const schemas: Record = { article, sys_share_link: SysShareLink }; const engine = { getSchema: (name: string) => schemas[name], - find: (object: string, query: any) => driver.find(object, query), + find: async (object: string, query: any) => { + const rows = await driver.find(object, query); + if (!options.shapeRow || object !== 'article' || !Array.isArray(rows)) return rows; + return rows.map(options.shapeRow); + }, findOne: (object: string, query: any) => driver.findOne(object, query), insert: (object: string, data: any) => driver.create(object, data), // Opened with ObjectQL's OWN dispatch predicate rather than a hand-mirrored @@ -278,14 +299,168 @@ describe('[#7861] publicSharing.eligibility is enforced at createLink', () => { expect(await mintedLinks(driver)).toHaveLength(0); }); - it('a DECLARED field the row left empty is judged, not faulted', async () => { - // Several drivers omit NULL columns. Binding declared-and-absent to - // `null` is what keeps such a row a `false` verdict (refused on the - // merits) rather than an `ELIGIBILITY_UNEVALUABLE` fault. - const { service } = await boot({ - ...ARTICLE, - publicSharing: { ...ARTICLE.publicSharing, eligibility: 'record.owner_id == null' }, - }); + }); + + /** + * [#9085 / #8489] The declared-field binding, pinned by cases that CANNOT + * pass without it. + * + * ## What was wrong with the pin these replace + * + * The previous case — *"a DECLARED field the row left empty is judged, not + * faulted"* — evaluated `record.owner_id == null` against row `a_ok`, and + * every seeded row carries `owner_id: 'u1'`. So the declared field was never + * actually absent, the binder never had anything to bind, and the case + * reached `RECORD_NOT_ELIGIBLE` **identically with the binder fully ablated** + * (measured). It pinned the predicate's plumbing, not the binding — which is + * the one thing it was written to guard. + * + * ## How these two discriminate + * + * The declared field has to be genuinely missing from the stored row, so the + * schema the engine reports declares `archived_at` while the table the driver + * was initialised from does not carry it. That skew is the real production + * shape, not a contrivance: it is a driver that omits NULL columns, a + * migration that has not run yet, a projection that dropped the column — all + * of them "a field the driver simply did not return", which is the sentence + * the binder exists to answer. Measured directly on a first-party driver: + * `InMemoryDriver` returns no `status` key at all for a row created without + * one. + * + * The two cases fail in OPPOSITE directions under ablation, which is what + * makes them a tripwire rather than a pair that happens to be red together: + * + * | predicate | bound (correct) | binder ABLATED | + * |:-----------------------------|:----------------------|:-----------------------------| + * | `record.archived_at == null` | `true` → **mints** | FAULT → `ELIGIBILITY_UNEVALUABLE` | + * | `!has(record.archived_at)` | `false` → **refuses** | `true` → **mints a link** | + * + * The second row is the #6454 rule and the reason this card was filed: once + * bindings are materialised `has(record.)` is uniformly TRUE, + * so `has()` guards an UNDECLARED key and never an empty value. A binder that + * misses the row therefore MINTS a link on a predicate every other + * server-side surface refuses — over-acceptance, in the dangerous direction. + */ + describe('[#9085] the declared-field binding is pinned discriminatingly', () => { + /** + * Declares one more field than the table carries. Handed to `getSchema`; + * the driver is initialised from `ARTICLE`, so the stored row genuinely has + * no `archived_at` key. + */ + const withUnstoredField = (eligibility: string) => ({ + ...ARTICLE, + fields: { + ...ARTICLE.fields, + archived_at: { type: 'datetime', name: 'archived_at', label: 'Archived at' }, + }, + publicSharing: { ...ARTICLE.publicSharing, eligibility }, + }); + + it('a declared field the row genuinely does not carry is JUDGED, not faulted', async () => { + const { driver, service } = await boot( + withUnstoredField('record.archived_at == null'), + { ddl: ARTICLE }, + ); + + // Without the binding this read faults `No such key: archived_at`, and a + // fault on this fail-closed gate is `ELIGIBILITY_UNEVALUABLE` — the + // eligible row would be refused for a reason that is not about the row. + const link = await service.createLink( + { object: 'article', recordId: 'a_ok', audience: 'public', permission: 'view' }, + CALLER, + ); + expect(link.token).toBeTruthy(); + expect((await mintedLinks(driver)).map((r) => r.record_id)).toEqual(['a_ok']); + }); + + it('`!has()` on a declared field is FALSE even when the row omits it — and mints nothing', async () => { + const { driver, service } = await boot( + withUnstoredField('!has(record.archived_at)'), + { ddl: ARTICLE }, + ); + + // The #6454 semantics. An unbound record answers this `true` and MINTS — + // the over-acceptance that this seam carried while it kept its own copy + // of the binder. + await expectRefusal( + () => service.createLink( + { object: 'article', recordId: 'a_ok', audience: 'public', permission: 'view' }, + CALLER, + ), + { status: 422, code: 'RECORD_NOT_ELIGIBLE' }, + ); + expect(await mintedLinks(driver)).toHaveLength(0); + }); + }); + + /** + * [#8489] The verdict change this card's ruling accepted knowingly. + * + * The retired local mirror bound a declared field by KEY PRESENCE + * (`!(name in record)`); the canonical `materializeDeclaredFields` binds by + * VALUE (`record[name] === undefined`). They agree on every input class but + * one: a declared field held as an own key whose value is `undefined`. + * + * The canonical rule is the correct one, and the canonical helper's own doc + * comment says so in as many words — *"`undefined` counts as absent (not just + * a missing key): CEL treats an own key holding `undefined` exactly as it + * treats no key at all"*. Measured against the real `@objectstack/formula` + * engine, that is exactly what CEL does: `has(record.status)` is `false` and + * `record.status == null` FAULTS `No such key: status`. So the mirror's `in` + * check left a key bound that the evaluator still read as absent. + * + * ## Why the row is reshaped instead of stored + * + * `SqlDriver` cannot express this shape — a SQL NULL arrives as `null`, which + * is a value, not `undefined`. `InMemoryDriver` can and does: measured, + * `create('article', { …, status: undefined })` preserves the own key and + * `find` returns it holding `undefined`. So the shape is real and first-party + * reachable; it is applied on top of the real driver's row here rather than + * pulling a second backend into this suite for one row. + * + * The two cases below are the two directions of the accepted change: the + * widening the maintainer accepted, and the over-acceptance it closes. + */ + describe('[#8489] a declared field held as an own key with `undefined`', () => { + /** The exact shape `InMemoryDriver` produces for an explicitly-undefined write. */ + const asOwnKeyUndefined = (row: any) => { + const out = { ...row }; + out.status = undefined; + return out; + }; + + const withEligibility = (eligibility: string) => ({ + ...ARTICLE, + publicSharing: { ...ARTICLE.publicSharing, eligibility }, + }); + + it('is bound to `null` and JUDGED — the accepted widening', async () => { + const { driver, service } = await boot( + withEligibility('record.status == null'), + { shapeRow: asOwnKeyUndefined }, + ); + + // The retired mirror left the key unbound, CEL faulted `No such key`, and + // this fail-closed gate refused with `ELIGIBILITY_UNEVALUABLE`. The + // predicate is now ANSWERED rather than unevaluable, and the answer is + // `true`. + const link = await service.createLink( + { object: 'article', recordId: 'a_ok', audience: 'public', permission: 'view' }, + CALLER, + ); + expect(link.token).toBeTruthy(); + expect((await mintedLinks(driver)).map((r) => r.record_id)).toEqual(['a_ok']); + }); + + it('`!has()` over it refuses — closing the over-acceptance the mirror carried', async () => { + const { driver, service } = await boot( + withEligibility('!has(record.status)'), + { shapeRow: asOwnKeyUndefined }, + ); + + // This is the direction that matters for a security-relevant gate: the + // retired mirror MINTED here, on a predicate every other server-side + // surface refuses. await expectRefusal( () => service.createLink( { object: 'article', recordId: 'a_ok', audience: 'public', permission: 'view' }, @@ -293,6 +468,7 @@ describe('[#7861] publicSharing.eligibility is enforced at createLink', () => { ), { status: 422, code: 'RECORD_NOT_ELIGIBLE' }, ); + expect(await mintedLinks(driver)).toHaveLength(0); }); }); diff --git a/packages/plugins/plugin-sharing/src/share-link-service.ts b/packages/plugins/plugin-sharing/src/share-link-service.ts index 6c29f633b9..c6d25ec137 100644 --- a/packages/plugins/plugin-sharing/src/share-link-service.ts +++ b/packages/plugins/plugin-sharing/src/share-link-service.ts @@ -23,6 +23,15 @@ import type { Expression } from '@objectstack/spec'; // server-side validation and hook-condition gates evaluate predicates through. // See `assertEligible` for why this and not `compileCelToFilter`. import { ExpressionEngine } from '@objectstack/formula'; +// [#8489] The declared-field binding CONTRACT, imported rather than re-derived. +// This file used to carry its own `bindDeclaredFields` — a hand-written mirror +// whose own doc comment named what it was a copy of, which is exactly how a +// second copy of a contract goes stale behind a convention. `@objectstack/objectql` +// is a runtime `dependencies` entry here (not dev), and the canonical helper is +// published from the lean `./core` entry, so there is no structural reason to +// keep a copy. `declared-fields.ts`'s doc comment is the canonical statement of +// the rule; this seam defers to it instead of restating it. +import { materializeDeclaredFields } from '@objectstack/objectql/core'; import type { SharingEngine } from './sharing-service.js'; import { deleteRowsForDeletedRecords, @@ -105,29 +114,6 @@ function getPolicy(schema: any): { }; } -/** - * [#7861] Bind the candidate record's DECLARED fields before evaluating, so a - * predicate over a field the driver simply did not return is not a fault. - * - * The same materialisation the two server-side CEL gates in `@objectstack/objectql` - * do (`materializeDeclaredFields`, behind the validation and hook-condition - * evaluators). It matters more here than there: several drivers omit NULL - * columns entirely, so `record.status == 'published'` on a row whose `status` - * is null would fault — and this gate FAILS CLOSED, so a fault is a refusal. - * Without this, an eligibility policy would refuse links for exactly the rows - * whose field is empty rather than judging them. Declared-and-absent therefore - * binds to `null`, which is what the row means; an UNdeclared key still faults, - * because that one really is an author typo. - */ -function bindDeclaredFields(record: Record, schema: any): Record { - const declared = schema?.fields; - if (!declared || typeof declared !== 'object') return record; - const bound: Record = { ...record }; - for (const name of Object.keys(declared)) { - if (!(name in bound)) bound[name] = null; - } - return bound; -} /** Parse `expiresAt` as either an ISO string or a relative duration like "7d", "24h", "30m". */ function normaliseExpiresAt(input: string | null | undefined, maxDays: number): string | null { @@ -270,8 +256,24 @@ function assertEligible( ); } + // [#7861 / #8489] Bind the candidate record's DECLARED fields before + // evaluating, so a predicate over a field the driver simply did not return is + // not a fault. It matters more on this seam than on the others: several + // drivers omit NULL columns entirely, so `record.status == 'published'` on a + // row whose `status` is empty would fault — and this gate FAILS CLOSED, so a + // fault is a refusal. Without the binding, an eligibility policy would refuse + // links for exactly the rows whose field is empty rather than judging them. + // + // The RULE itself (declared-and-absent binds to `null`; an UNdeclared key + // still faults, because that one really is an author typo; `undefined` in an + // own key counts as absent, because CEL reads it exactly as it reads no key + // at all) is stated once, in `declared-fields.ts`. Do not restate it here. + // + // The spread is load-bearing: the canonical helper materialises IN PLACE and + // returns the same reference, while `record` is the row the caller just read + // out of `engine.find`. Copying keeps this gate a pure read of it. const verdict = ExpressionEngine.evaluate(expr, { - record: bindDeclaredFields(record, schema), + record: materializeDeclaredFields({ ...record }, schema?.fields), }); if (!verdict.ok) { throw makeError(