feat(objectql): reject aggregations over secret/password fields (#3171)#3187
Merged
Merged
Conversation
`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:<id>`
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QJrKxe1jXGudFT3nUks1yN
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 13 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
marked this pull request as ready for review
July 18, 2026 07:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #3171 — the follow-up recorded in ADR-0100 when #3170 (#2036) landed password/secret read-masking.
find/findOnemask credential fields (secretalways;passwordon non-better-authobjects) so plaintext never leaves the engine. Butaggregate()had no equivalent guard: aGROUP BY/MIN/MAX/array_aggover asecretorpasswordcolumn would surface the storedsecret:<id>ref or the password value. Masking the aggregate output after the fact would corrupt group keys, so the correct fix is to refuse the aggregation.What changed
New gate
rejectCredentialAggregationat the top ofObjectQL.aggregate()(packages/objectql/src/engine.ts), called right after name resolution and before any driver work. It scans the only two output-bearing positions onEngineAggregateOptions:aggregations[].field— skippingCOUNT(*)(undefined field or'*')groupBy[]— each item a string or a{ field }bucket objectand throws a fail-closed error naming
object.fieldwhen any references a credential field.New collector
collectCredentialFields(packages/objectql/src/secret-fields.ts) — everysecretorpasswordfield, unconditionally (ignoresmanagedBy). This is deliberately stricter thancollectMaskedReadFields: read-masking exemptspasswordonbetter-authobjects so login reads work, but aggregating a credential is never legitimate — even on a better-auth object it would be an inference oracle over password hashes. Keeping the two collectors separate stops the read-mask and aggregate-reject concerns from drifting. Exported fromcore.ts/index.ts.Why no legitimate caller breaks
The only path that aggregates a schema-declared field is
recomputeSummaries()(roll-up summaries). Verified the shipped rollups target numeric fields (showcase_invoice_line.amount,showcase_task.estimate_hours, etc.) — none reference a credential field. A rollup over a credential column would be a misconfiguration, and rejecting it is correct. HTTP aggregate calls already funnel through the engine (the http-dispatcher FLS aggregate gate), so this gate is enforced there automatically; rawdriver.aggregate()is intentionally below the engine gate, consistent with that existing design.Tests
Added to
packages/objectql/src/secret-fields.test.ts(real engine + in-memory driver harness):secretrejected as an aggregation measure, as a stringgroupBy, and as a structured{ field }groupBypasswordrejected as agroupBydimensionmanagedBy: 'better-auth'object (unconditional collector)COUNT(*)on an object that merely has a credential field resolves fineVerification
@objectstack/objectql—secret-fields.test.ts18 tests (6 new) passengine-aggregate-timezone,in-memory-aggregation,engine-count-read-filter(22 tests) all pass🤖 Generated with Claude Code
Generated by Claude Code