From e013ee963f13fd2677cb348789dfee1303c9f8df Mon Sep 17 00:00:00 2001 From: huangyiirene Date: Wed, 12 Aug 2026 09:26:18 +0000 Subject: [PATCH] fix(objectql): fold `internal: true` into the aggregate guard (#7922) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .changeset/internal-flag-aggregation-guard.md | 42 +++++ packages/objectql/src/engine.ts | 50 ++++-- packages/objectql/src/internal-fields.test.ts | 146 +++++++++++++++++- packages/objectql/src/secret-fields.ts | 17 +- 4 files changed, 238 insertions(+), 17 deletions(-) create mode 100644 .changeset/internal-flag-aggregation-guard.md diff --git a/.changeset/internal-flag-aggregation-guard.md b/.changeset/internal-flag-aggregation-guard.md new file mode 100644 index 0000000000..076bb18949 --- /dev/null +++ b/.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: }` +and still mints show-once. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 14990be244..c08be93f7d 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -9559,14 +9559,31 @@ 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:` 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:` 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 @@ -9574,8 +9591,12 @@ export class ObjectQL implements IObjectQLEngine { */ 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(); 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.', ); } } diff --git a/packages/objectql/src/internal-fields.test.ts b/packages/objectql/src/internal-fields.test.ts index 3606811cbc..dc27108963 100644 --- a/packages/objectql/src/internal-fields.test.ts +++ b/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>; 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); + }); + }); }); diff --git a/packages/objectql/src/secret-fields.ts b/packages/objectql/src/secret-fields.ts index 2ee39c1851..ff6baf7ef4 100644 --- a/packages/objectql/src/secret-fields.ts +++ b/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`. */