From b20a5f21af89a6abc388a8b56def3bea2098a85f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Jul 2026 06:49:06 +0000 Subject: [PATCH] feat(objectql): reject aggregations over secret/password fields (#3171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `find`/`findOne` mask credential fields (secret always, password on non-better-auth objects) so plaintext never leaves the engine, but `aggregate()` had no equivalent guard — a GROUP BY / MIN / MAX / array_agg over a `secret` or `password` column would surface the stored `secret:` ref or the password value. Post-hoc masking of aggregate output would corrupt group keys, so the correct fix is to refuse the aggregation. Add a fail-closed gate at the top of `ObjectQL.aggregate()` that scans the two output-bearing positions on the aggregate query — `aggregations[].field` (skipping COUNT(*)) and `groupBy` (string or `{ field }` bucket) — and throws when any references a credential field. The gate keys off a new unconditional `collectCredentialFields` collector (secret OR password, ignoring `managedBy`): unlike read-masking, aggregating a credential is never allowed, even on a better-auth object, where it would be an inference oracle over hashes. Keeping the two collectors separate stops the concerns from drifting. No legitimate caller aggregates a credential field (verified: the only schema-field rollup path, recomputeSummaries, targets numeric fields like `amount`/`estimate_hours`). Follows the http-dispatcher FLS aggregate gate. Tests cover: secret/password rejected as measure, as string groupBy, and as structured `{ field }` groupBy; no false positive when the field is unreferenced; and rejection on a better-auth object. Closes #3171; the gap was recorded as a follow-up in ADR-0100. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QJrKxe1jXGudFT3nUks1yN --- packages/objectql/src/core.ts | 1 + packages/objectql/src/engine.ts | 42 ++++++++++++++++++ packages/objectql/src/index.ts | 1 + packages/objectql/src/secret-fields.test.ts | 48 +++++++++++++++++++++ packages/objectql/src/secret-fields.ts | 25 +++++++++++ 5 files changed, 117 insertions(+) diff --git a/packages/objectql/src/core.ts b/packages/objectql/src/core.ts index 2a0381bd37..85bc050f60 100644 --- a/packages/objectql/src/core.ts +++ b/packages/objectql/src/core.ts @@ -76,6 +76,7 @@ export { parseSecretRef, collectSecretFields, collectMaskedReadFields, + collectCredentialFields, } from './secret-fields.js'; // Utilities diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 6a281fe44a..9f3df91615 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -20,6 +20,7 @@ import type { ICryptoProvider, CryptoHandle } from '@objectstack/spec/contracts' import { collectSecretFields, collectMaskedReadFields, + collectCredentialFields, makeSecretRef, parseSecretRef, isSecretRef, @@ -2919,8 +2920,49 @@ export class ObjectQL implements IDataEngine { return opCtx.result as number; } + /** + * 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. + * + * 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; + + const referenced = new Set(); + for (const agg of query?.aggregations ?? []) { + const field = (agg as { field?: string })?.field; + if (field && field !== '*') referenced.add(field); + } + for (const g of (query?.groupBy as unknown[]) ?? []) { + const field = typeof g === 'string' ? g : (g as { field?: string })?.field; + if (field) referenced.add(field); + } + + const hit = credentialFields.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.', + ); + } + } + async aggregate(object: string, query: EngineAggregateOptions, options?: EngineReadOptions): Promise { object = this.resolveObjectName(object); + this.rejectCredentialAggregation(object, query); const driver = this.getDriver(object); this.logger.debug(`Aggregate on ${object} using ${driver.name}`, query); diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 5df6fbd5d3..abb9520d8a 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -103,6 +103,7 @@ export { parseSecretRef, collectSecretFields, collectMaskedReadFields, + collectCredentialFields, } from './secret-fields.js'; // Export Utilities diff --git a/packages/objectql/src/secret-fields.test.ts b/packages/objectql/src/secret-fields.test.ts index 0609f13030..b41782b620 100644 --- a/packages/objectql/src/secret-fields.test.ts +++ b/packages/objectql/src/secret-fields.test.ts @@ -320,3 +320,51 @@ describe('objectql password-field masking (ADR-0100)', () => { expect(viaOne.password).toBe('hashed-by-auth'); }); }); + +describe('objectql aggregate() rejects credential fields (ADR-0100 / #3171)', () => { + it('rejects a `secret` field used as an aggregation measure', async () => { + const { engine } = await buildEngine(true); + await expect( + engine.aggregate('ext_datasource', { aggregations: [{ function: 'max', field: 'db_password', alias: 'x' }] as any }), + ).rejects.toThrow(/credential field.*db_password/i); + }); + + it('rejects a `secret` field used as a string groupBy dimension', async () => { + const { engine } = await buildEngine(true); + await expect( + engine.aggregate('ext_datasource', { aggregations: [{ function: 'count', alias: 'n' }], groupBy: ['db_password'] } as any), + ).rejects.toThrow(/db_password/); + }); + + it('rejects a `secret` field used as a structured {field} groupBy bucket', async () => { + const { engine } = await buildEngine(true); + await expect( + engine.aggregate('ext_datasource', { aggregations: [{ function: 'count', alias: 'n' }], groupBy: [{ field: 'db_password' }] } as any), + ).rejects.toThrow(/db_password/); + }); + + it('does NOT reject when the credential field is not referenced (no false positive)', async () => { + const { engine } = await buildEngine(true); + await engine.insert('ext_datasource', { name: 'pg', db_password: 's3cr3t' }); + // COUNT(*) touches no credential column — the object merely *has* one. + await expect( + engine.aggregate('ext_datasource', { aggregations: [{ function: 'count', alias: 'n' }] } as any), + ).resolves.toBeDefined(); + }); + + it('rejects a generic `password` field used as a groupBy dimension', async () => { + const { engine } = await buildPasswordEngine(); + await expect( + engine.aggregate('device', { aggregations: [{ function: 'count', alias: 'n' }], groupBy: ['admin_password'] } as any), + ).rejects.toThrow(/admin_password/); + }); + + it('rejects even on a better-auth object — read-masking is exempt there, aggregation is NOT', async () => { + const { engine } = await buildPasswordEngine(); + // authy_user is managedBy:'better-auth' (password NOT masked on read), but + // aggregating the credential is still refused (unconditional collector). + await expect( + engine.aggregate('authy_user', { aggregations: [{ function: 'max', field: 'password', alias: 'x' }] as any }), + ).rejects.toThrow(/password/); + }); +}); diff --git a/packages/objectql/src/secret-fields.ts b/packages/objectql/src/secret-fields.ts index 4a97461dee..15f743e254 100644 --- a/packages/objectql/src/secret-fields.ts +++ b/packages/objectql/src/secret-fields.ts @@ -97,3 +97,28 @@ export function collectMaskedReadFields(schema: ServiceObject | undefined | null } return out; } + +/** + * Collect the names of every credential-bearing field on an object — `secret` + * OR `password` — **unconditionally**, ignoring `managedBy`. + * + * This differs from {@link collectMaskedReadFields} on purpose. Read-masking + * exempts `password` on `managedBy: 'better-auth'` objects so login reads still + * see the stored value; but *aggregating* a credential field must never be + * allowed, even on a better-auth object — a GROUP BY / MIN / MAX over a password + * column is an inference oracle regardless of who owns the table. So the + * aggregate-rejection gate keys off this stricter, exemption-free collector, + * keeping the two concerns independent (they must not drift). See ADR-0100 / #3171. + * + * Returns an empty array when the schema has no fields or no credential fields, + * so callers can fast-path on `length === 0`. + */ +export function collectCredentialFields(schema: ServiceObject | undefined | null): string[] { + const fields = (schema as any)?.fields as Record | undefined; + if (!fields) return []; + const out: string[] = []; + for (const [name, def] of Object.entries(fields)) { + if (def && (def.type === 'secret' || def.type === 'password')) out.push(name); + } + return out; +}