fix(objectql): fold internal: true into the aggregate guard (#7922) (#7984)

`rejectCredentialAggregation` decided what to refuse by asking
`collectCredentialFields`, a collector keyed on the field TYPE
(`secret` / `password`). That left it blind to ADR-0100's third
credential channel — an auth-subsystem one-way hash living in an
ordinary `text` column — which is exactly the channel #7728 minted the
type-independent `internal: true` flag for.

So the read path understood "protected by flag" while the guard still
only understood "protected by type": a flagged column that `find`
omitted could be named as a `groupBy` dimension or a MIN/MAX measure
and come back as the group key itself.

The guard now takes the deduped union of the two collectors.
Composition happens at the call site — the collectors stay separate
because their other consumers answer differently (the read path MASKS a
credential type and OMITS a flagged field, and a flagged column must
never acquire a mask).

Nothing is disclosed by this today: there is no `/data/:object/aggregate`
route and analytics requires a declared dataset, so no reachable caller
could reach the gap. It is closed because the inconsistency is what
bites the next adopter — `sys_api_key.key` is a SHA-256 hash, but
`sys_session.token` (#7823) is a live bearer credential.

Tests extend the existing #7728 floor (`internal-fields.test.ts`) rather
than forking a second copy. The three CONTROL cases come first and are
the load-bearing ones: an unflagged column on an object that HAS a
flagged one still aggregates, an object with no flagged field is
untouched, and COUNT(*) is not a false positive. An over-broad guard
breaks analytics silently, so all three were verified falsifiable
against two deliberate mutations of the guard.

Author: huangyiirene

.changeset/internal-flag-aggregation-guard.md

@@ -0,0 +1,42 @@
+---
+"@objectstack/objectql": patch
+---
+
+fix(objectql): the aggregate guard now refuses `internal: true` columns, not just the `secret`/`password` TYPES (#7922)
+
+`aggregate()`'s fail-closed guard (`rejectCredentialAggregation`, ADR-0100 /
+#3171) decided what to refuse by asking `collectCredentialFields` — a collector
+keyed on the field **TYPE**. That left it blind to exactly the channel #7728
+had just given the read path a way to protect.
+
+ADR-0100's third credential channel is an auth-subsystem one-way hash living in
+an ordinary `text` column (`sys_api_key.key`). No type-keyed collector can ever
+reach it, which is why #7728 minted the type-independent `internal: true` flag —
+*"the declared value is never returned on the generic data path"* — and taught
+`find` / `findOne` / the 201 create body / the by-id update body to omit it.
+
+The aggregation guard was never taught the same thing. So the read path
+understood "protected by flag" while the guard still only understood "protected
+by type", and a flagged column that `find` omitted could be named as a `groupBy`
+dimension or a MIN/MAX measure and come back as the group key itself — the
+promise in the flag's own declaration stopping at the edge of `aggregate()`.
+
+The guard now takes the **union** of the two collectors, deduped, so both the
+type-keyed and the flag-keyed sets are refused. Composition happens at the call
+site: the collectors stay separate because their other consumers answer
+differently — the read path MASKS a credential type and OMITS a flagged field,
+and a flagged column must never acquire a mask.
+
+**Nothing is disclosed by this today, and this is not a security fix.** There is
+no `/data/:object/aggregate` route and analytics requires a declared dataset, so
+no reachable caller could reach the gap. It is closed because the inconsistency
+is what bites the next adopter: `sys_api_key.key` is a SHA-256 hash, but
+`sys_session.token` (#7823) is a live bearer credential, and the flag reads as
+though it already covered both.
+
+**Unchanged.** An unflagged column still aggregates normally — including an
+ordinary column sitting on the same object as a flagged one, and `COUNT(*)` over
+an object that merely *has* one. Neither collector has a `managedBy` exemption,
+so the union does not acquire one, and the read-path behaviour from #7920 is
+untouched: `sys_api_key.key` still authenticates through `where: { key: <hash> }`
+and still mints show-once.

packages/objectql/src/engine.ts

@@ -9559,23 +9559,44 @@ export class ObjectQL implements IObjectQLEngine {
   }
 
   /**
-   * Fail-closed guard (ADR-0100 / #3171): refuse to aggregate over a credential
-   * field. `secret`/`password` values are masked on the generic read path so
-   * plaintext never leaves the engine, but `aggregate()` has no equivalent mask
-   * — a GROUP BY / MIN / MAX / array_agg over such a column would surface the
-   * stored `secret:<id>` ref or the password value, and post-hoc masking would
-   * corrupt group keys. So we reject instead. The check is unconditional
-   * (ignores `managedBy`): aggregating a credential is never legitimate, even on
-   * a better-auth object, where it would be an inference oracle over hashes.
+   * Fail-closed guard (ADR-0100 / #3171 / #7922): refuse to aggregate over a
+   * field whose value is withheld on the generic read path. Such fields reach
+   * this guard through **two independent collectors**, and it needs both:
+   *
+   *  - {@link collectCredentialFields} — keyed by field TYPE (`secret` /
+   *    `password`). A GROUP BY / MIN / MAX / array_agg over such a column would
+   *    surface the stored `secret:<id>` ref or the password value.
+   *  - {@link collectInternalReadFields} — keyed by the `internal: true` FLAG
+   *    (#7728). ADR-0100's third credential channel is a one-way hash living in
+   *    an ordinary `text` column, which no type-keyed collector can ever reach;
+   *    the flag is that channel's opt-in declaration. Without this half the
+   *    guard had the same type-vs-flag blind spot #7728 fixed on the read path:
+   *    a flagged column was omitted from `find`/`findOne` yet freely groupable
+   *    here, so the flag's promise ("never returned on the generic data path")
+   *    stopped at the edge of `aggregate()`.
+   *
+   * Post-hoc masking is not available on this path — the value is already a
+   * group key by the time there is a row, and masking group keys corrupts the
+   * result. So we reject instead.
+   *
+   * Neither collector carries a `managedBy` exemption, so the union does not
+   * acquire one, deliberately. Read-masking exempts `password` on better-auth
+   * objects so login reads still see the stored value; *aggregating* a
+   * credential is never legitimate, least of all on an identity table, where it
+   * is an inference oracle over hashes.
    *
    * Only the two output-bearing positions on `EngineAggregateOptions` carry
    * field names: `aggregations[].field` (skip COUNT(*) — undefined or '*') and
    * `groupBy[]` (a string, or a `{ field }` bucket object).
    */
   private rejectCredentialAggregation(object: string, query: EngineAggregateOptions): void {
     const schema = this._registry.getObject(object);
-    const credentialFields = collectCredentialFields(schema);
-    if (credentialFields.length === 0) return;
+    // Deduped: one field can be reachable through both collectors (a `secret`
+    // column that is also flagged `internal`), and it must be named once.
+    const protectedFields = [
+      ...new Set([...collectCredentialFields(schema), ...collectInternalReadFields(schema)]),
+    ];
+    if (protectedFields.length === 0) return;
 
     const referenced = new Set<string>();
     for (const agg of query?.aggregations ?? []) {
@@ -9587,13 +9608,14 @@ export class ObjectQL implements IObjectQLEngine {
       if (field) referenced.add(field);
     }
 
-    const hit = credentialFields.filter((f) => referenced.has(f));
+    const hit = protectedFields.filter((f) => referenced.has(f));
     if (hit.length > 0) {
       throw new Error(
         `Cannot aggregate credential field(s) ${hit.map((f) => `"${object}.${f}"`).join(', ')}: `
-          + 'secret/password fields are masked on read so plaintext never leaves the engine, and '
-          + 'aggregating them (group-by, min/max, array_agg, …) would surface the stored value. '
-          + 'Refusing (fail-closed) — see ADR-0100 / #3171.',
+          + 'secret/password fields are masked on read and `internal: true` fields are omitted '
+          + 'outright, so the value never leaves the engine on the generic data path; aggregating '
+          + 'them (group-by, min/max, array_agg, …) would surface it. '
+          + 'Refusing (fail-closed) — see ADR-0100 / #3171 / #7922.',
       );
     }
   }

packages/objectql/src/internal-fields.test.ts

@@ -23,9 +23,9 @@
  */
 
 import { describe, it, expect, beforeEach } from 'vitest';
-import { ObjectQL } from './engine.js';
+import { ObjectQL, type EngineReadOptions } from './engine.js';
 import { collectInternalReadFields, SECRET_MASK } from './secret-fields.js';
-import type { ServiceObject } from '@objectstack/spec/data';
+import type { EngineAggregateOptions, ServiceObject } from '@objectstack/spec/data';
 
 // ---- minimal stub driver (equality-only WHERE) ----------------------------
 // Rows leave the driver as COPIES, as a real driver's do — see the note in
@@ -145,6 +145,14 @@ async function buildEngine() {
 
 const HASH = 'sha256:deadbeefcafe';
 
+/**
+ * Trailing read options for the aggregate cases below. Declared with its
+ * contract type rather than inlined `as any`: erasing a read method's options
+ * argument is what `query-options/no-any-erasure` bans and the #4918 ratchet
+ * counts (`scripts/check-query-options-erasure-ratchet.mjs`).
+ */
+const SYSTEM: EngineReadOptions = { context: { isSystem: true } };
+
 describe('#7728: the `internal` field flag omits a value from the generic data path', () => {
   let ctx: Awaited<ReturnType<typeof buildEngine>>;
   beforeEach(async () => { ctx = await buildEngine(); });
@@ -280,4 +288,138 @@ describe('#7728: the `internal` field flag omits a value from the generic data p
       expect(found).toHaveLength(1);
     });
   });
+
+  /**
+   * [#7922] `aggregate()` has no strip: it groups and reduces the driver's raw
+   * rows, so a flagged column reached through `groupBy` or an aggregation
+   * measure would surface the very value the flag promises is "never returned
+   * on the generic data path". The type-keyed half of this guard has been in
+   * place since #3171 (see the `ADR-0100 / #3171` block in
+   * `secret-fields.test.ts`, which stays the floor for `secret` / `password`);
+   * what is pinned here is the flag-keyed half, which did not exist.
+   *
+   * The FIRST case is deliberately the negative one. A guard that refuses too
+   * much breaks analytics silently — nothing throws at the surface a reviewer
+   * looks at, the numbers just stop arriving — so the control that an
+   * unflagged column still aggregates has to be able to fail on its own.
+   */
+  describe('the aggregation guard', () => {
+    /** Two rows sharing a prefix and one on its own — enough for real buckets. */
+    const seedThree = async () => {
+      await ctx.engine.insert('itest_api_key', { name: 'k1', prefix: 'osk_', revoked: false, key: HASH }, { context: { isSystem: true } } as any);
+      await ctx.engine.insert('itest_api_key', { name: 'k2', prefix: 'osk_', revoked: false, key: `${HASH}-2` }, { context: { isSystem: true } } as any);
+      await ctx.engine.insert('itest_api_key', { name: 'k3', prefix: 'svc_', revoked: true, key: `${HASH}-3` }, { context: { isSystem: true } } as any);
+    };
+
+    it('CONTROL: an unflagged column on an object that HAS a flagged one still aggregates', async () => {
+      await seedThree();
+
+      // `prefix` is an ordinary text column on the same object as the flagged
+      // `key`. Grouping by it must keep working, and must return the real
+      // buckets — asserting only "does not throw" would still pass if the
+      // guard were replaced by a no-op that returned nothing.
+      const rows = await ctx.engine.aggregate('itest_api_key', {
+        aggregations: [{ function: 'count', alias: 'n' }],
+        groupBy: ['prefix'],
+      }, SYSTEM);
+
+      const byPrefix = Object.fromEntries(rows.map((r: any) => [r.prefix, Number(r.n)]));
+      expect(byPrefix).toEqual({ osk_: 2, svc_: 1 });
+    });
+
+    it('CONTROL: an object with NO flagged field aggregates untouched (the fast path)', async () => {
+      await ctx.engine.insert('itest_plain', { key: 'visible' });
+      await ctx.engine.insert('itest_plain', { key: 'visible' });
+      await ctx.engine.insert('itest_plain', { key: 'other' });
+
+      // `itest_plain.key` shares its NAME with the flagged column on the other
+      // object — a guard that collected field names globally rather than
+      // per-schema would refuse here.
+      const rows = await ctx.engine.aggregate('itest_plain', {
+        aggregations: [{ function: 'count', alias: 'n' }],
+        groupBy: ['key'],
+      });
+
+      const byKey = Object.fromEntries(rows.map((r: any) => [r.key, Number(r.n)]));
+      expect(byKey).toEqual({ visible: 2, other: 1 });
+    });
+
+    it('CONTROL: COUNT(*) on the flagged object is not a false positive', async () => {
+      await seedThree();
+      // The object merely HAS a flagged column; nothing references it.
+      const rows = await ctx.engine.aggregate('itest_api_key', {
+        aggregations: [{ function: 'count', alias: 'n' }],
+      }, SYSTEM);
+      expect(Number((rows[0] as any).n)).toBe(3);
+    });
+
+    it('rejects the flagged field as a string groupBy dimension', async () => {
+      await seedThree();
+      // The disclosure shape: one bucket per distinct hash, keyed BY the hash.
+      await expect(
+        ctx.engine.aggregate('itest_api_key', {
+          aggregations: [{ function: 'count', alias: 'n' }],
+          groupBy: ['key'],
+        }, SYSTEM),
+      ).rejects.toThrow(/key/);
+    });
+
+    it('rejects the flagged field as a structured {field} groupBy bucket', async () => {
+      await seedThree();
+      // `as unknown as` names the contract being bypassed rather than erasing
+      // it: `EngineAggregateOptions.groupBy` is declared `string[]`, while the
+      // engine reads structured `{ field, dateGranularity }` buckets too — so
+      // this is deliberately off-contract input, and the guard must walk that
+      // second spelling as well. (`as any` here would grow the #4918 ratchet.)
+      await expect(
+        ctx.engine.aggregate('itest_api_key', {
+          aggregations: [{ function: 'count', alias: 'n' }],
+          groupBy: [{ field: 'key' }],
+        } as unknown as EngineAggregateOptions, SYSTEM),
+      ).rejects.toThrow(/key/);
+    });
+
+    it('rejects the flagged field as an aggregation measure', async () => {
+      await seedThree();
+      // MIN/MAX over a credential is the inference oracle #3171 named.
+      await expect(
+        ctx.engine.aggregate('itest_api_key', {
+          aggregations: [{ function: 'max', field: 'key', alias: 'x' }],
+        }, SYSTEM),
+      ).rejects.toThrow(/key/);
+    });
+
+    it('rejects even though the object is `managedBy: better-auth`', async () => {
+      // The read path exempts better-auth from PASSWORD masking; neither
+      // collector feeding this guard has an exemption, so the union does not
+      // acquire one. `itest_api_key` is better-auth-managed and still refused.
+      expect((tokenObject as any).managedBy).toBe('better-auth');
+      await seedThree();
+      await expect(
+        ctx.engine.aggregate('itest_api_key', {
+          aggregations: [{ function: 'count', alias: 'n' }],
+          groupBy: ['key'],
+        }, SYSTEM),
+      ).rejects.toThrow(/itest_api_key\.key/);
+    });
+
+    it('names every refused field once, and only the refused ones', async () => {
+      await seedThree();
+      // Mixing a legitimate dimension with the flagged one refuses the whole
+      // query (fail-closed) but must not slander `prefix`.
+      const err = await ctx.engine.aggregate('itest_api_key', {
+        aggregations: [{ function: 'count', alias: 'n' }],
+        groupBy: ['prefix', 'key'],
+      }, SYSTEM).then(
+        () => null,
+        (e: unknown) => e as Error,
+      );
+      expect(err).toBeInstanceOf(Error);
+      expect(err!.message).toContain('itest_api_key.key');
+      expect(err!.message).not.toContain('prefix');
+      // Deduped: a field must not be listed twice if it is reachable through
+      // both collectors.
+      expect(err!.message.match(/itest_api_key\.key/g)).toHaveLength(1);
+    });
+  });
 });

packages/objectql/src/secret-fields.ts

@@ -133,11 +133,17 @@ export function collectMaskedReadFields(schema: ServiceObject | undefined | null
  *    that needs a column readable simply does not flag it. An exemption here
  *    would silently disable the flag on exactly the identity objects it was
  *    minted for.
- *  - **The caller OMITS the key rather than masking it** (see
+ *  - **The read-path caller OMITS the key rather than masking it** (see
  *    {@link SECRET_MASK}). The mask signals "a value is set"; on a `required`
  *    column that is zero bits of information, and shipping it would still put a
  *    value under a field whose declaration promises none.
  *
+ * [#7922] The read path is not the only consumer. `aggregate()` cannot omit —
+ * a flagged column reached through `groupBy` is already the group KEY, and
+ * masking keys corrupts the result — so the aggregate gate unions this collector
+ * with {@link collectCredentialFields} and REFUSES the query instead. Same
+ * question, two answers, because the two surfaces have different options.
+ *
  * Returns an empty array when the schema has no fields or none are flagged, so
  * callers can fast-path on `length === 0`.
  */
@@ -163,6 +169,15 @@ export function collectInternalReadFields(schema: ServiceObject | undefined | nu
  * aggregate-rejection gate keys off this stricter, exemption-free collector,
  * keeping the two concerns independent (they must not drift). See ADR-0100 / #3171.
  *
+ * [#7922] This is the **type-keyed half** of what that gate refuses. Being
+ * type-keyed it cannot see ADR-0100's third channel — a one-way hash in a `text`
+ * column — so the gate unions it with {@link collectInternalReadFields}, the
+ * flag-keyed half. ⛔ Do not collapse the two by widening either one — they
+ * answer different questions ("is this a credential type?" vs "is this field
+ * declared unreturnable?") and their other consumers respond differently: the
+ * read path MASKS a credential type and OMITS a flagged field. Compose at the
+ * call site, which is what the gate does.
+ *
  * Returns an empty array when the schema has no fields or no credential fields,
  * so callers can fast-path on `length === 0`.
  */