diff --git a/src/subdomains/supporting/log/__tests__/client-error.service.spec.ts b/src/subdomains/supporting/log/__tests__/client-error.service.spec.ts index 435306d9de..73fcbe8384 100644 --- a/src/subdomains/supporting/log/__tests__/client-error.service.spec.ts +++ b/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=""'); diff --git a/src/subdomains/supporting/log/__tests__/create-client-error.dto.spec.ts b/src/subdomains/supporting/log/__tests__/create-client-error.dto.spec.ts new file mode 100644 index 0000000000..2ee070459d --- /dev/null +++ b/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 { + 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' }); + }); + }); +}); diff --git a/src/subdomains/supporting/log/client-error.service.ts b/src/subdomains/supporting/log/client-error.service.ts index 7750470f80..3ec3b13ade 100644 --- a/src/subdomains/supporting/log/client-error.service.ts +++ b/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)}`, diff --git a/src/subdomains/supporting/log/dto/create-client-error.dto.ts b/src/subdomains/supporting/log/dto/create-client-error.dto.ts index 189f8fc61f..098eeabd66 100644 --- a/src/subdomains/supporting/log/dto/create-client-error.dto.ts +++ b/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; }