Add filter-only columns to /gs/debug and allow user_data.mail lookups - #4396
Closed
TaprootFreak wants to merge 3 commits into
Closed
Add filter-only columns to /gs/debug and allow user_data.mail lookups#4396TaprootFreak wants to merge 3 commits into
TaprootFreak wants to merge 3 commits into
Conversation
The structured debug endpoint governed select, where, order by and group by through a single per-table allowlist, so a column was either fully exposed or not exposed at all. Support needs to resolve a customer's userData.id from a mail address they already have, while the endpoint must never be able to return a mail address. Introduce DebugTableSpec.filterOnlyColumns: columns usable only as a WHERE leaf, restricted to = and IN via DebugFilterOnlyAllowedOps. Range and pattern operators stay rejected, because they would let a caller binary-search or pattern-match an unknown value and reconstruct it without ever selecting it. Split assertDebugColumnAllowed into a select-side and a filter-side check; order by and group by keep validating against columns only, which is what keeps a filter-only column unorderable and ungroupable. Extend the startup invariants so filter-only columns stay disjoint from columns and jsonbColumns and free of duplicates, and widen the staleness counter-check to accept either allowlist capacity - without that, a restricted column could never be configured as filter-only, since the two checks would contradict each other. Allowlist user_data.mail as the first filter-only column and add a --user-by-mail mode to scripts/db-debug.sh returning all matching ids, since one address can belong to several user_data rows.
An independent review of the initial implementation found that the operator gate could be bypassed and that the guarantee was weaker than documented. The WHERE tree supports a `not` node, so `NOT (mail = x)` emitted the semantic equivalent of the prohibited `mail != x`, sidestepping the gate entirely. A filter-only column is now refused anywhere below a `not`, at any nesting depth and regardless of double negation - no attempt is made to reason about negations cancelling out. Drop `IN` from the allowed operators. It was never unsafe in itself, but a list of up to 100 candidates per request multiplied the throughput of a guessing attack; equality alone forces one request per candidate, each separately audit-logged. Emit filter-only equality as `LOWER(col) = LOWER($n)`. The application itself resolves mail addresses that way, because historical user_data rows carry mixed-case values - a strict `=` would have silently failed to find exactly the records this feature exists for. Equality stays equality: the caller must still know the address, only letter case is forgiven. The client printed the request payload, putting the address into terminal scrollback and captured output. Payload echoes now redact WHERE values for every mode, mirroring the server's audit-log redaction; query structure stays visible and the transmitted body is unchanged.
Second review round. The negated flag is propagated into `and` / `or` children and the behaviour was correct, but nothing tested it - a regression that stopped propagating through a boolean node would have stayed green. The added cases cover NOT over AND, over OR and over a deeper nesting, plus the positive control `AND [ NOT (id = 1), mail = x ]`, where the filter-only leaf sits outside the negation and must be accepted. That last one is what pins the semantics rather than a blanket refusal. Case-insensitive emission presupposes a text column: a non-text filter-only column would fail at query time with `function lower(integer) does not exist`. A real type check would need schema introspection, so the precondition is documented at the declaration and at the emission site instead. Correct three inaccurate claims in the docs: the endpoint emits SQL manually with bound parameters rather than through a QueryBuilder; the caller must know the address exactly except for letter case; and queries are audit-logged with their WHERE values redacted, not verbatim.
Collaborator
Author
|
Superseded by #4400 — same commits, branch renamed to |
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
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.
Problem
The structured
/gs/debugendpoint governsselect,where,order byandgroup bythrough a single per-table allowlist (DebugTableSpec.columns). A column is therefore either fully exposed or not exposed at all.Support needs to resolve a customer's
userData.idfrom a mail address they already have. Addingmailtocolumnswould also make it selectable — turning the endpoint into a way to read customer mail addresses out, which is not acceptable. The capability we need is strictly narrower than anything the current model can express.Change
DebugTableSpecgainsfilterOnlyColumns: columns usable only as a WHERE leaf.SELECTkeeps validating againstcolumnsonly. All three select kinds (column,aggregate,jsonb) route through the same check, socount(mail)and ajsonbPathonmailare rejected by construction, as ismailunder anasalias.ORDER BY/GROUP BYkeep validating againstcolumnsonly — that is what keeps a filter-only column unorderable and ungroupable, including via the alias/ordinal path.WHEREvalidates againstcolumns∪filterOnlyColumns, restricted to=.notnode, at any depth, including double negation. Without this,NOT (mail = x)would emit the semantic equivalent of the prohibitedmail != x.LOWER(col) = LOWER($n).user_data.mailis allowlisted as the first filter-only column and stays out ofcolumns.Why only
=INwas deliberately dropped. It is not unsafe in itself, but a list of up to 100 candidates per request multiplies the throughput of a guessing attack. Equality alone forces one request per candidate, each separately audit-logged.Why case-insensitive
The application itself resolves addresses with
LOWER(mail) = :mail(user-data.service.ts:219) because historicaluser_datarows carry mixed-case values. A strict=would silently fail to find exactly the records this feature exists for. Equality stays equality — the caller must still know the address, only letter case is forgiven.Note this presupposes a text column; a non-text filter-only column would fail at query time. There is no runtime type check, so the precondition is documented at the declaration and at the emission site.
Deviation from the #4393 precedent — deliberate
#4393 established the pattern for exposing a sensitive column on
/gs/debug: add it tocolumns, and register an overlap exception if it is also inGsRestrictedColumns. That pattern cannot express this requirement, because it necessarily makes the column selectable. Hence the new, narrower concept rather than a second use of the existing escape hatch. What is reused is the enforcement style: consistency is guaranteed by the existingassertDebugAllowlistInvariantsguard, which aborts module load on a misconfiguration.Invariants added: a filter-only column must be disjoint from
columnsandjsonbColumns, must not repeat, and must not be aGsRestrictedColumnscolumn unless explicitly excepted. The pre-existing staleness counter-check was widened to accept either allowlist capacity — without that, a restricted column could never be configured as filter-only, since the forward check would demand an exception that the counter-check would then reject as stale.Client
scripts/db-debug.sh --user-by-mail <MAIL> [N]resolves the ids. One address can belong to severaluser_datarows, so it returns all matches (default limit 100, never 1) and selectsid, created, kycLevel, statusto tell them apart. The address is bound as a parameter, never interpolated. Payload echoes now redact WHERE values for every mode, mirroring the server's audit-log redaction — previously the address was printed into terminal scrollback and captured output.Verification
npm test -- src/subdomains/generic/gs: 397 passed. The tests prove the negative cases individually — SELECT, aggregate, jsonb, alias laundering, ORDER BY, GROUP BY, every non-equality operator, andNOToverAND/OR/ deeper nestings — plus the positive controlAND [ NOT (id = 1), mail = x ], which pins the propagation semantics rather than a blanket refusal. The real configuration is pinned (filterOnlyColumnsis exactly['mail']andcolumnsdoes not containmail), so a later "cleanup" intocolumnsbreaks the build.Accepted residual risks
Two properties are inherent to the requested capability and are accepted deliberately rather than fixed:
Debug-role holder can test whether a guessed address exists and obtain its ids. That is the feature. There is no endpoint rate limit, so guessing is bounded only by request volume; every attempt is audit-logged and attributable, and the attacker would already need theDebugrole. Adding rate limiting would be a change to the endpoint as a whole, beyond this PR.alice@…andALICE@…both stored, querying either returns both ids. This is intended: it adopts the application's own case-insensitive address identity, which is what support needs. No address is disclosed either way.Neither allows reading an address that is not already known.
Note for reviewers
This widens what the
Debugrole can do: a holder can confirm whether a given address exists and obtain its ids. It cannot enumerate or read addresses./gs/dbmasking behaviour is untouched.