fix(sharing): eligibility binds declared fields through the canonical materializeDeclaredFields (#8489)

.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.<declared field>)` 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.

packages/plugins/plugin-sharing/src/share-link-eligibility.test.ts

@@ -94,27 +94,48 @@ 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.
  *
  * Only the verbs `ShareLinkService` actually calls are exposed; `getSchema` is
  * 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<string, any> = { 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,21 +299,176 @@ 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.<declared field>)` 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' },
           CALLER,
         ),
         { status: 422, code: 'RECORD_NOT_ELIGIBLE' },
       );
+      expect(await mintedLinks(driver)).toHaveLength(0);
     });
   });
 

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<string, unknown>, schema: any): Record<string, unknown> {
-  const declared = schema?.fields;
-  if (!declared || typeof declared !== 'object') return record;
-  const bound: Record<string, unknown> = { ...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<unknown>(expr, {
-    record: bindDeclaredFields(record, schema),
+    record: materializeDeclaredFields({ ...record }, schema?.fields),
   });
   if (!verdict.ok) {
     throw makeError(