Release: develop -> main - #4410
Merged
Merged
Conversation
…#4400) * Add filter-only columns to /gs/debug and allow user_data.mail lookups 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. * Harden filter-only columns: equality only, no NOT, case-insensitive 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. * Pin NOT propagation through boolean nodes, document LOWER precondition 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. * Limit a query to one filter-only predicate; audit before the table check Dropping `IN` was meant to force one guessed address per request, but a single query may carry up to 50 WHERE leaves, so `OR(mail='a', mail='b', ...)` batched candidates just as effectively and binary-splitting the set found matches in few requests. The guarantee was therefore not enforced. A query now accepts at most one filter-only predicate anywhere in the WHERE tree, nested boolean nodes included; the comments state the actual reasoning instead of crediting the `IN` removal alone. Emit the audit line before the table allowlist check, so a request naming a non-allowlisted table is attributable too - previously it threw first and was never logged, while the docs claimed every request is audited. Values stay redacted. Correct two comments that still described a TypeORM QueryBuilder, and reword the allowlist policy comments: PII is excluded from selectable and result columns, with explicitly reviewed filter-only exceptions such as user_data.mail. Add a rejecting test for the offset upper bound, which the existing test could not catch because it bypasses the validation pipe. * Make the alias test actually probe alias resolution; fix audit claim The test covering ORDER BY / GROUP BY of an alias named like a filter-only column never reached those clauses: it failed during SELECT validation, so it would have stayed green with the alias checks deleted. It now selects `id AS mail`, so the alias really registers, and asserts that GROUP BY resolves to the alias ordinal and ORDER BY to the quoted alias, with neither binding to the physical user_data.mail column. Correct the documented audit guarantee: requests rejected by the global ValidationPipe never reach the service and are therefore not audit-logged. Coverage is every request that passes DTO validation, including those later rejected for an unknown table or a disallowed column - the document previously contradicted itself on this point. Sweep the remaining comments claiming the debug endpoint uses a TypeORM QueryBuilder; the /gs/db and support paths genuinely do use one and are left untouched. * Log database failures by error code, never by message The endpoint redacts WHERE values in its audit log, but the query-execution catch block logged the raw database error, and PostgreSQL echoes offending parameter values in some of them. Filtering `mail = 'x@example.com'` together with `id = 'x@example.com'` makes PostgreSQL reject the second parameter with `invalid input syntax for type integer: "x@example.com"`, so the address reached the log in clear text - defeating the redaction for exactly the value it protects. Failures are now logged as value-free diagnostics: SQLSTATE plus severity and routine where available, with an explicit placeholder when no code is present rather than falling back to the message. The response to the caller is unchanged. The existing test demanded the raw message and would have stayed green while values leaked, so it now asserts the value-free form. A regression test feeds an error whose message embeds an address and asserts no log line contains it while the code is still reported. * Bind an overlap exception to the access it approved Widening the staleness check to accept either allowlist capacity left a hole: an exception approving a restricted column for equality-only lookup stayed valid after someone moved that column from filterOnlyColumns into columns, silently upgrading it to full disclosure. The exception recorded the pair but not the access that had been reviewed. Approvals are now capacity-specific. DebugRestrictedOverlapExceptions keeps its original meaning - selectable access - and its staleness check is restored accordingly, so the existing transaction_aml_check.amlResponsible entry is unaffected. A separate DebugFilterOnlyRestrictedExceptions covers filter-only access and is empty today, since no filter-only column is restricted. Registering the same pair in both is rejected, because an approval has to be unambiguous. Moving a column between the two lists therefore breaks module load until the approval is moved as well, which is the visible step this is meant to force. The log-redaction test also inspected only the first logger argument, so passing the error object as a second one would have leaked the address through its stack while the test stayed green; it now pins the argument count too. * Correct the rationale for case-insensitive equality and the index claim The justification given for `LOWER(col) = LOWER($n)` was wrong. It cited historical mixed-case rows, but migration NormalizeUserDataMailLowercase - already in the base - lowercased every stored address, and the write paths lowercase on input. The behaviour is right for a different reason: it matches the identity the application itself uses, since getUsersByMail resolves via LOWER(mail), and it tolerates a caller typing a different case than stored. The performance note claimed the comparison cannot use an index. The opposite holds: the same migration creates a functional index on LOWER(mail), which is exactly what this emission uses. That index is deliberately non-unique while case-collision duplicates await a merge campaign - which is also why one address can legitimately resolve to several user_data rows. The /gs/db masking test asserted only the contents of GsRestrictedColumns, so it would have passed even if masking had stopped being applied. It now runs getDbData for both roles and checks that ADMIN sees [RESTRICTED] while SUPER_ADMIN sees the value. * Fix the type-check failure and use absolute imports The DbQueryDto fixture added for the /gs/db masking test omitted the required identifier property, so `tsc --noEmit` failed and the CI check went red. It stayed invisible locally because `nest build` excludes test files and Jest transpiles without type checking - both were green throughout. The fixture now carries every required property with a real value rather than being cast into shape, since a fixture that only type-checks through an assertion defeats its own purpose. The two imports this branch introduced also used relative paths, which CONTRIBUTING disallows; they are absolute now, matching the surrounding files. * Keep the mail address and the request body out of process argv The script promised that a customer address never appears outside the request payload, but argv is world-readable through ps and /proc, and the address reached it three ways: as the positional argument of --user-by-mail, through `jq --arg`, and inside `curl -d "$PAYLOAD"`. The address is now read as one line from stdin - prompted on stderr when stdin is a TTY so an operator can answer interactively, read silently otherwise so pipes keep working. Empty input or EOF fails loudly instead of querying with an empty filter. The value reaches jq through stdin. Request bodies go to curl via `-d @-` for every mode, not only the new one: the other modes bind user ids and asset names the same way, so fixing only this one would have left the same exposure in place and the guarantee only half true. What is sent is unchanged. Help text and documentation now describe the stdin form and state the guarantee precisely. * Accept stdin without a trailing newline; reject stray arguments; state the guarantee precisely `printf %s address | … --user-by-mail` failed even though a complete value had been read: Bash `read` returns non-zero at EOF without a trailing newline. The read is now treated as successful whenever it produced a value, with the empty/whitespace rejection unchanged. Trailing arguments were silently dropped, so `--user-by-mail 50 --typo` used 50 and ignored the typo. Unconsumed arguments are now rejected and the limit is validated as a positive integer, matching the script's fail-loud discipline. Three statements promised more than the code delivers and are now exact. Examples no longer pipe through `echo`, which could place the address in an external echo's argv - the very exposure the stdin input removes; they show interactive entry or reading from a protected file. Interactive entry is documented as visible on the terminal, which is accepted since the operator is typing an address they already know. And enabling TypeORM query logging on the server would print bound parameters, defeating the audit redaction; it is off in production, and that condition is now written down instead of implied. * Scope the argv guarantee to what holds; fix the help example and limit range Request bodies now reach curl through stdin, but `--query '<json>'` still takes the payload as an argument, so an inline DTO stays visible in the process list and in shell history. The form is kept - it is convenient for ordinary queries - and the documentation now says plainly that sensitive values, in particular a filter-only column, must go through `--query @file` or `--query -` instead. `--user-by-mail` is unaffected, it reads from stdin. The `--help` output rendered its heredoc terminator indented, so the printed direct-API example could not be pasted and run. It is emitted correctly now. `--user-by-mail [N]` accepted any positive integer while the server caps limit at 10000, so an oversized value authenticated and was then rejected remotely. The client validates 1..10000 and names the range in the help text and docs. * Match the sibling's secret handling; fix two argument-validation bugs The equivalent command in the team tooling reads the address hidden and keeps values out of its error text, while this script still echoed the address on a TTY and reflected submitted limits, trailing arguments and a positionally mistyped address back to stderr. Two scripts serving the same purpose gave different guarantees. Interactive entry is now hidden with a newline after the prompt, piped input is unchanged, and every error in this mode reports only the kind of problem plus how to supply the address. Two validation bugs came out of the same review. A long digit string passed the regex but overflowed the arithmetic comparison, which emitted `integer expected`, evaluated false and let the script continue to authentication despite the promised client-side rejection; the digit length is now bounded before any arithmetic. And trailing arguments were detected by testing whether the third argument was non-empty, so `--user-by-mail 50 "" ignored` slipped through while an explicitly empty limit was mistaken for an omitted one; both are now decided by argument count. * Pin the value-validation branches of a filter-only equality filter The test rejecting a non-scalar WHERE value only submitted an object, so the array branch of that check could have been removed unnoticed. Filter-only equality also had no coverage for an omitted value, null, an empty array, a single-element array or an empty string - security-relevant branches that were correct but unpinned. The added cases are table-driven and, for every input that must be rejected, also assert that no query was issued: a rejection that still reached the database would be a real defect that asserting only the thrown error would miss. The empty string is pinned as accepted on purpose - it is a legitimate exact filter that simply matches nothing. A synthetic fixture now also pins an empty filterOnlyColumns list as a valid configuration.
github-actions
Bot
requested review from
TaprootFreak and
davidleomay
as code owners
July 27, 2026 17:06
TaprootFreak
approved these changes
Jul 27, 2026
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
1 participant
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.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist