Skip to content

Record the account a client error was reported for - #4594

Open
TaprootFreak wants to merge 9 commits into
developfrom
feat/log-client-error-account
Open

Record the account a client error was reported for#4594
TaprootFreak wants to merge 9 commits into
developfrom
feat/log-client-error-account

Conversation

@TaprootFreak

@TaprootFreak TaprootFreak commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

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 own
key=value field:

Client error: client="dfx-services" accountId="123456" route="/buy" version="…" userAgent="…" type="ChunkLoadError" message="…"

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 field
the 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 safe
integers, 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, stack
and 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 empty
    value alongside the other context fields.
  • create-client-error.dto.spec.ts (new): keeps a valid id and Number.MAX_SAFE_INTEGER, accepts
    a 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 the
    value 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.

@TaprootFreak

Copy link
Copy Markdown
Collaborator Author

Eight review passes were needed to reach zero findings. The substantive ones, in the order they came up:

  • The log field was named account=, while every other id-carrying context field in the logs ends in Id=. Renamed.
  • @IsInt() alone let a number past Number.MAX_SAFE_INTEGER through, where parsing rounds it — two different ids sent would have been recorded as the same one. That is this service altering a value, not merely recording an unverified one.
  • Bounding it with validators turned out to be the wrong instrument: the pipe answers 400 for the whole body, so a bad hint would have cost the report its message, stack and route — the blind spot this endpoint exists to close. The range is enforced by a transformation instead, which drops the value and keeps the report.
  • With that in place the validators could never fire again. They were removed; the accepted range is stated in the API docs instead, where the annotations would not have appeared anyway (the schema plugin is not enabled in this repository).
  • Closes on the issue would have closed it on merge, although the frontend half ships separately. Changed to a reference; the issue is closed by Report which account a client error happened to services#1238.
  • Several comments and description lines asserted more than the code holds — a guaranteed customer association, a value that is always what was sent, a session this endpoint never sees. Shortened rather than reworded again.

Two tests run through the pipe the application is bootstrapped with, using the same options as main.ts: one pins that a valid account is recorded rather than stripped by whitelist, the other that a bad one costs the value and not the report.

@TaprootFreak
TaprootFreak marked this pull request as ready for review August 2, 2026 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant