Record the account a client error was reported for

src/subdomains/supporting/log/__tests__/client-error.service.spec.ts

@@ -40,10 +40,19 @@ describe('ClientErrorService', () => {
     expect(loggedLine()).toContain('userAgent="Mozilla/5.0"');
   });
 
+  // What the report is for: matching a support case against the failures recorded under the same
+  // id. The value comes from the request, which is why it is logged as context and nothing else.
+  it('logs the reported account', () => {
+    service.logError(dto({ accountId: 123456 }));
+
+    expect(loggedLine()).toContain('accountId="123456"');
+  });
+
   it('logs absent context as an empty value', () => {
     service.logError(dto());
 
     expect(loggedLine()).toContain('client=""');
+    expect(loggedLine()).toContain('accountId=""');
     expect(loggedLine()).toContain('route=""');
     expect(loggedLine()).toContain('version=""');
     expect(loggedLine()).toContain('userAgent=""');

src/subdomains/supporting/log/__tests__/create-client-error.dto.spec.ts

@@ -0,0 +1,82 @@
+import { plainToInstance } from 'class-transformer';
+import { validate } from 'class-validator';
+import { DetailedValidationPipe } from 'src/shared/pipes/detailed-validation.pipe';
+import { CreateClientErrorDto } from '../dto/create-client-error.dto';
+
+// The account is the one field on this DTO that is read back as a number rather than as text, and
+// the only one whose value may be discarded without the report going with it. Both are pinned here
+// because the endpoint is unauthenticated: whatever shape arrives is a shape anyone can post, and
+// the field exists to be looked up in the logs, not to carry prose.
+describe('CreateClientErrorDto.accountId', () => {
+  async function submit(accountId: unknown): Promise<{ rejected: string[]; accountId?: number }> {
+    const dto = plainToInstance(CreateClientErrorDto, { message: 'boom', accountId });
+    const errors = await validate(dto);
+
+    return { rejected: errors.map((e) => e.property), accountId: dto.accountId };
+  }
+
+  it('keeps an account id', async () => {
+    await expect(submit(123456)).resolves.toEqual({ rejected: [], accountId: 123456 });
+  });
+
+  it('keeps the largest safe account id', async () => {
+    await expect(submit(Number.MAX_SAFE_INTEGER)).resolves.toEqual({
+      rejected: [],
+      accountId: Number.MAX_SAFE_INTEGER,
+    });
+  });
+
+  // The limit is on what arrives, not on what was written: the body is parsed before any of this
+  // runs, and a fractional value near the limit arrives as an integer. Recorded as parsed - pinned
+  // so the guarantee is not read as wider than it is.
+  it('keeps what the parser made of a value written with a fraction', async () => {
+    await expect(submit(JSON.parse('9007199254740991.1'))).resolves.toEqual({
+      rejected: [],
+      accountId: Number.MAX_SAFE_INTEGER,
+    });
+  });
+
+  it('accepts a report that carries no account at all', async () => {
+    const errors = await validate(plainToInstance(CreateClientErrorDto, { message: 'boom' }));
+
+    expect(errors).toEqual([]);
+  });
+
+  // Everything below is dropped rather than rejected. A 400 would take message, stack and route
+  // with it, and those are what the report exists for.
+  it.each([
+    ['free text', 'Robert'],
+    // The pipe runs without implicit conversion, so an id sent as text is not silently read as one.
+    ['the id as a string', '123456'],
+    ['a fraction', 1.5],
+    ['a boolean', true],
+    ['null', null],
+    ['zero, which is no account', 0],
+    ['a negative id', -1],
+    // What 9007199254740993 arrives as once the body is parsed. Dropping it is the point: without
+    // the bound, ids that differ would be recorded under the same number.
+    ['an id past the safe integer range', Number.MAX_SAFE_INTEGER + 1],
+  ])('drops %s and keeps the report', async (_case, value) => {
+    await expect(submit(value)).resolves.toEqual({ rejected: [], accountId: undefined });
+  });
+
+  // What the field does under the pipe the app is actually bootstrapped with - same options as
+  // main.ts passes. Two things only hold there: `whitelist: true` strips a property the DTO does
+  // not declare, so an account that is declared but loses its decorators would silently stop being
+  // recorded; and the transformation is what drops a bad value instead of rejecting the report.
+  describe('under the pipe the app runs', () => {
+    const pipe = new DetailedValidationPipe({ whitelist: true, transformOptions: { exposeUnsetFields: false } });
+
+    function submitTo(accountId: unknown): Promise<unknown> {
+      return pipe.transform({ message: 'boom', accountId }, { type: 'body', metatype: CreateClientErrorDto });
+    }
+
+    it('records the account rather than stripping it', async () => {
+      await expect(submitTo(123456)).resolves.toEqual({ message: 'boom', accountId: 123456 });
+    });
+
+    it('drops a bad account instead of answering 400', async () => {
+      await expect(submitTo('Robert')).resolves.toEqual({ message: 'boom' });
+    });
+  });
+});

src/subdomains/supporting/log/client-error.service.ts

@@ -84,12 +84,17 @@ export class ClientErrorService {
     // Checked before the fields are built, so a flood cannot buy sanitizing work it never uses.
     if (!this.isWithinBudget()) return;
 
-    const { message, type, stack, route, version } = dto;
+    const { message, type, stack, route, version, accountId } = dto;
 
     // Context first, free text last and quoted: message, type and stack are attacker-controlled
     // and would otherwise be indistinguishable from the key=value context a log query parses.
+    //
+    // The account is a correlation hint and nothing else. This endpoint takes no session (see the
+    // controller), so the id is whatever the request carried: it lets a support case be matched
+    // against the reports carrying the same id, and says nothing about who sent them.
     const fields = [
       `client=${ClientErrorService.quote(client)}`,
+      `accountId=${ClientErrorService.quote(accountId?.toString())}`,
       `route=${ClientErrorService.quote(ClientErrorService.toPath(route))}`,
       `version=${ClientErrorService.quote(version)}`,
       `userAgent=${ClientErrorService.quote(userAgent)}`,

src/subdomains/supporting/log/dto/create-client-error.dto.ts

@@ -40,4 +40,24 @@ export class CreateClientErrorDto {
   @Transform(Util.trim)
   @MaxLength(50)
   version?: string;
+
+  @ApiPropertyOptional({
+    type: 'integer',
+    minimum: 1,
+    maximum: Number.MAX_SAFE_INTEGER,
+    description:
+      'Account ID the client reports for the signed-in user, left out when it has none to report. Correlation ' +
+      'hint only: this endpoint is unauthenticated, so the value is whatever the request carried and says ' +
+      'nothing about who sent it. A value that does not arrive as a positive safe integer is dropped rather ' +
+      'than rejected, so a bad hint does not cost the report.',
+  })
+  @IsOptional()
+  // Dropped here rather than rejected by a validator, which would reject the report along with the
+  // value: 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.
+  //
+  // What it checks is the value that arrived. The body is parsed before this runs, and what parsing
+  // did to the number on the way is not visible from here.
+  @Transform(({ value }) => (Number.isSafeInteger(value) && value > 0 ? value : undefined))
+  accountId?: number;
 }