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

.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`.
  */