Record the account a client error was reported for - #4594
Open
TaprootFreak wants to merge 9 commits into
Open
Conversation
Collaborator
Author
|
Eight review passes were needed to reach zero findings. The substantive ones, in the order they came up:
Two tests run through the pipe the application is bootstrapped with, using the same options as |
TaprootFreak
marked this pull request as ready for review
August 2, 2026 14:15
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.
Part of DFXswiss/services#1237 — the API half. The issue is closed by the frontend PR, which goes
out after this one.
What
A client error report now carries an optional
accountId, and the log line records it as its ownkey=valuefield:Left out by the client when it has no account to report — an error before sign-in is the common
case. The field is then logged as
accountId="", like the other context fields.Why
Today a report answers what broke and where, never who it happened to. When a customer
reports "I can't buy anything", there is no way to look up whether their failure is among the
recorded ones, how often it hit them, or whether it stopped. Reconstructing that from request
traces works only while an incident is recent and narrow, and stays circumstantial.
This is the prerequisite half: the global validation pipe runs with
whitelist: true, so a fieldthe DTO does not declare is stripped silently. Sending it from the frontend before this is merged
would look like it works and record nothing.
What this value is not
The endpoint is unauthenticated on purpose — the errors worth catching happen before or without a
session — so the id is whatever the request carried. It is a correlation hint and must never be
used for authorization or ownership checks. Both the DTO description and the service comment say
so, and nothing else in the code reads the field.
Accepted range, and what happens outside it
An integer between 1 and
Number.MAX_SAFE_INTEGER. The upper bound is not cosmetic: past the safeintegers, ids that differ reach the log as the same number — this service altering the value rather
than merely recording an unverified one.
The bound is on what arrives. The body is parsed before any of this runs, so a value written with
a fractional part near the bound arrives as an integer and is recorded as one; nothing on this side
can tell the two apart. A test pins that, so the guarantee is not read as wider than it is.
Anything that does not arrive as such — text, a numeric string, a fraction, a boolean, zero, a
negative,
null, a number past the safe integer range — is dropped, not rejected. The pipe answers 400 for the whole body, and losing message, stackand route over the one field that only helps to find them is the blind spot this endpoint exists to
close. So the range is enforced by the transformation rather than by validators — a validator would
reject the report along with the value. The range is stated in the API docs as
minimum/maximum.Tests
client-error.service.spec.ts: the account is logged, and an absent one is logged as an emptyvalue alongside the other context fields.
create-client-error.dto.spec.ts(new): keeps a valid id andNumber.MAX_SAFE_INTEGER, acceptsa report carrying no account, and drops each of the eight shapes above while keeping the report. Two cases run through the pipe the app is bootstrapped with: one pins that a
valid account is recorded rather than stripped by
whitelist, the other that a bad one costs thevalue and not the report.
Frontend synchronisation
DFXswiss/services#1238 sends the field and closes the issue. It must go out after this one: the
validation pipe strips a property the DTO does not declare, so sending it earlier would look like
it works and record nothing.