diff --git a/docs/bank-frick-operations.md b/docs/bank-frick-operations.md index 9e9a22a2e4..bec8366685 100644 --- a/docs/bank-frick-operations.md +++ b/docs/bank-frick-operations.md @@ -161,6 +161,15 @@ All values remain blank in `.env.example`. Deployment must provide: EUR personal IBANs; opt-in — when unset, the Frick virtual-IBAN provider is unavailable and there is no behaviour change +**vIBAN transport contract (bodyless GET Content-Type):** Bodyless vIBAN GET calls (list and detail) +must **omit** the `Content-Type` request header entirely. Production evidence: Bank Frick's vIBAN +gateway returns a signed HTTP 200 when no `Content-Type` is sent, but the production Azure +Application Gateway in front of it returns an **unsigned HTTP 403** when `Content-Type: */*` is +present. Mutating vIBAN requests (create POST, activation-approval PUT) remain signed +`Content-Type: application/json`. Request signing (`Signature` / `algorithm`) and fail-closed +response signature verification are unchanged for all vIBAN methods. The standard WebAPI path is +deliberately different and still sends `Content-Type: */*` on bodyless GETs. + `BankFrickService.isAvailable()` requires the base URL, API key, customer identifier, private signing key and server verification key. Every request signs the exact serialized body. Every response remains raw text until its detached `Signature` and `algorithm` headers have been verified diff --git a/docs/coverage-gate.md b/docs/coverage-gate.md index a7ab0982cd..1926931c8c 100644 --- a/docs/coverage-gate.md +++ b/docs/coverage-gate.md @@ -6,7 +6,7 @@ the other. | Gate | Config | Scope | Question it answers | | ---------------- | ------------------------------ | ------------------------------------------ | -------------------------------------------------------- | | Frick gate | `jest.frick.config.js` | 10 Frick files, run by 10 Frick specs only | Do _these specs alone_ fully cover _these files_? | -| Coverage ratchet | `jest.coverage-gate.config.js` | 422 files, whole suite | Has coverage regressed anywhere it was already complete? | +| Coverage ratchet | `jest.coverage-gate.config.js` | 439 files, whole suite | Has coverage regressed anywhere it was already complete? | ## What the ratchet is, and what it is not @@ -25,8 +25,8 @@ It is a **regression gate**, not a statement about test quality: ratchet only protects files already on the list, and that list grows by hand (see "How the list grows"). That is the price of the threshold approach. -Of the 422 pinned files, **233 carry real logic** (they have functions and/or branches) and -**189 are purely declarative today** (NestJS modules, constant files with neither). The two groups +Of the 439 pinned files, **246 carry real logic** (they have functions and/or branches) and +**193 are purely declarative today** (NestJS modules, constant files with neither). The two groups are kept visibly separate in the config so the count is not mistaken for test depth. Pinning the declarative ones is deliberate and not vacuous. Istanbul reports a metric with a total @@ -161,7 +161,7 @@ deleting them would be a separate cleanup. | Class | Files | Meaning | | -------- | ----- | ----------------------------------------------- | -| Complete | 422 | Pinned by the ratchet | +| Complete | 422 | Pinned by the ratchet at that commit | | Partial | 1,057 | Some coverage, below 100 on at least one metric | | None | 127 | No coverage at all | @@ -175,7 +175,7 @@ six under `subdomains/generic/admin` have no coverage at all. ## How the list grows Any PR may add files to `coverageThreshold` once they reach 100%. -`jest.coverage-gate.config.js` holds the 422 paths in two arrays, `PINNED_LOGIC` (logic-carrying +`jest.coverage-gate.config.js` holds the 439 paths in two arrays, `PINNED_LOGIC` (logic-carrying files) and `PINNED_DECLARATIVE` (purely declarative files), from which `coverageThreshold` is generated. Adding a file means appending its path to the matching array, not writing out a `coverageThreshold` object entry by hand. @@ -211,8 +211,8 @@ To regenerate the full picture, run the gate and read `coverage-gate/coverage-su below 100, the expected response is to extend the tests. Unpinning is an explicit decision that belongs in the PR description, not a silent edit. -That rule stays hard for the 233 logic-carrying files. A foreseeable friction case is different: -when one of the 189 purely declarative files (a NestJS module, a constants file) first gains +That rule stays hard for the 246 logic-carrying files. A foreseeable friction case is different: +when one of the 193 purely declarative files (a NestJS module, a constants file) first gains executable logic — for example a `useFactory` on a module — the function metric jumps from 0/0 to 0/N and the gate turns red. Tests remain the preferred fix, but unpinning that one file is an allowed outcome if the PR description names and justifies it (not as a silent edit). For diff --git a/jest.coverage-gate.config.js b/jest.coverage-gate.config.js index 093831ff80..51f3b5eae8 100644 --- a/jest.coverage-gate.config.js +++ b/jest.coverage-gate.config.js @@ -41,6 +41,7 @@ const PINNED_LOGIC = [ 'src/integration/blockchain/shared/enums/blockchain.enum.ts', 'src/integration/blockchain/shared/errors/tx-broadcast.error.ts', 'src/integration/blockchain/shared/evm/paymaster/pimlico-paymaster.service.ts', + 'src/integration/blockchain/shared/services/blockchain-config-check.service.ts', 'src/integration/blockchain/zano/zano-helper.ts', 'src/integration/checkout/dto/checkout.dto.ts', 'src/integration/exchange/dto/mexc.dto.ts', @@ -60,13 +61,19 @@ const PINNED_LOGIC = [ 'src/integration/sift/dto/sift.dto.ts', 'src/polyfills.ts', 'src/shared/auth/allow-tfa-pending.decorator.ts', + 'src/shared/auth/exceptions/staff-kyc-required.exception.ts', 'src/shared/auth/get-jwt.decorator.ts', + 'src/shared/auth/role.guard.ts', + 'src/shared/auth/staff-kyc-clearance.ts', 'src/shared/auth/user-role.enum.ts', 'src/shared/decorators/log-rejected-value.decorator.ts', + 'src/shared/models/fiat/fiat.controller.ts', + 'src/shared/pipes/detailed-validation.pipe.ts', 'src/shared/services/typeorm-logger.ts', 'src/shared/utils/bitbox-ascii.util.ts', 'src/shared/utils/cron.ts', 'src/shared/utils/custom-cron-expression.ts', + 'src/shared/utils/request-caller.ts', 'src/shared/utils/request-client.ts', 'src/shared/validators/is-ssrf-safe-url.validator.ts', 'src/shared/validators/xor.validator.ts', @@ -141,6 +148,7 @@ const PINNED_LOGIC = [ 'src/subdomains/generic/user/models/user-data/kyc-identification-type.enum.ts', 'src/subdomains/generic/user/models/user-data/user-data.enum.ts', 'src/subdomains/generic/user/models/user/dto/verify-mail.dto.ts', + 'src/subdomains/generic/user/models/user/staff-kyc-clearance.service.ts', 'src/subdomains/generic/user/models/user/user.enum.ts', 'src/subdomains/generic/user/services/webhook/dto/webhook.dto.ts', 'src/subdomains/supporting/bank-tx/bank-tx/dto/sepa.dto.ts', @@ -148,6 +156,7 @@ const PINNED_LOGIC = [ 'src/subdomains/supporting/bank-tx/bank-tx/services/bank-tx-outgoing-match.service.ts', 'src/subdomains/supporting/bank/bank/dto/bank.dto.ts', 'src/subdomains/supporting/bank/bank/dto/receive-iban.enum.ts', + 'src/subdomains/supporting/bank/virtual-iban/dto/virtual-iban.mapper.ts', 'src/subdomains/supporting/bank/virtual-iban/providers/frick-viban.provider.ts', 'src/subdomains/supporting/bank/virtual-iban/providers/viban-account-holder.enum.ts', 'src/subdomains/supporting/bank/virtual-iban/providers/yapeal-viban.provider.ts', @@ -161,6 +170,7 @@ const PINNED_LOGIC = [ 'src/subdomains/supporting/fiat-output/dto/create-fiat-output.dto.ts', 'src/subdomains/supporting/fiat-output/dto/update-fiat-output.dto.ts', 'src/subdomains/supporting/fiat-output/fiat-output-frick.service.ts', + 'src/subdomains/supporting/log/client-error.controller.ts', 'src/subdomains/supporting/log/log.entity.ts', 'src/subdomains/supporting/notification/enums/index.ts', 'src/subdomains/supporting/payin/strategies/register/impl/base/polling.strategy.ts', @@ -377,8 +387,11 @@ const PINNED_DECLARATIVE = [ 'src/subdomains/core/liquidity-management/dto/output/liquidity-management-rule-output.dto.ts', 'src/subdomains/core/liquidity-management/dto/resolve-uncertain-order.dto.ts', 'src/subdomains/core/monitoring/system-state-snapshot.entity.ts', + 'src/subdomains/core/payment-link/dto/assign-payment-link.dto.ts', + 'src/subdomains/core/payment-link/dto/create-payment-merchant.dto.ts', 'src/subdomains/core/payment-link/dto/payment-link-recipient-address.dto.ts', 'src/subdomains/core/payment-link/dto/payment-link.dto.ts', + 'src/subdomains/core/payment-link/dto/update-payment-link-payment.dto.ts', 'src/subdomains/core/payment-link/payment-link-payment.module.ts', 'src/subdomains/core/referral/process/ref.entity.ts', 'src/subdomains/core/referral/reward/dto/update-ref-reward.dto.ts', @@ -437,6 +450,7 @@ const PINNED_DECLARATIVE = [ 'src/subdomains/supporting/dex/dex.module.ts', 'src/subdomains/supporting/dex/strategies/sell-liquidity/impl/base/evm-coin.strategy.ts', 'src/subdomains/supporting/dex/strategies/sell-liquidity/impl/base/evm-token.strategy.ts', + 'src/subdomains/supporting/log/dto/create-client-error.dto.ts', 'src/subdomains/supporting/log/dto/create-log.dto.ts', 'src/subdomains/supporting/log/log.module.ts', 'src/subdomains/supporting/notification/notification.module.ts', diff --git a/src/integration/bank/services/__tests__/frick.service.spec.ts b/src/integration/bank/services/__tests__/frick.service.spec.ts index 24864eec9a..0cebf783ca 100644 --- a/src/integration/bank/services/__tests__/frick.service.spec.ts +++ b/src/integration/bank/services/__tests__/frick.service.spec.ts @@ -90,6 +90,8 @@ describe('BankFrickService', () => { .filter((request) => request.url.endsWith('/accounts/0000000')); expect(accountCalls).toHaveLength(2); expect(accountCalls[0].data).toBe(''); + // Standard WebAPI bodyless GETs must keep Content-Type: */* (vIBAN GETs deliberately omit it). + expect(accountCalls[0].headers['Content-Type']).toBe('*/*'); expect(accountCalls[0].headers.Authorization).toMatch(/^Bearer /); expectSignature('', accountCalls[0].headers.Signature); expect(http.request.mock.calls.filter(([request]) => request.url.endsWith('/authorize'))).toHaveLength(1); @@ -1022,6 +1024,7 @@ describe('BankFrickService', () => { expect(createRequest.url).toBe('https://vban.bank.invalid/vban/virtual-ibans'); expect(createRequest.method).toBe('POST'); expect(createRequest.data).toBe(JSON.stringify({ referenceAccountIban: debtorIban })); + expect(createRequest.headers['Content-Type']).toBe('application/json'); expectSignature(createRequest.data, createRequest.headers.Signature); expect(createRequest.headers.Authorization).toMatch(/^Bearer /); expect(createRequest.headers.algorithm).toBe('rsa-sha512'); @@ -1243,6 +1246,7 @@ describe('BankFrickService', () => { expect(approveRequest.url).toBe('https://vban.bank.invalid/vban/virtual-ibans/activations/approvals'); expect(approveRequest.method).toBe('PUT'); expect(approveRequest.data).toBe(JSON.stringify({ vban: response.vban })); + expect(approveRequest.headers['Content-Type']).toBe('application/json'); expectSignature(approveRequest.data, approveRequest.headers.Signature); expect(approveRequest.headers.Authorization).toMatch(/^Bearer /); expect(approveRequest.headers.algorithm).toBe('rsa-sha512'); @@ -1265,6 +1269,8 @@ describe('BankFrickService', () => { expect(getRequest.url).toBe(`https://vban.bank.invalid/vban/virtual-ibans/${encodeURIComponent(vbanWithSlash)}`); expect(getRequest.method).toBe('GET'); expect(getRequest.data).toBe(''); + // Bodyless vIBAN GETs must omit Content-Type: Azure gateway rejects Content-Type: */* with unsigned 403. + expect(getRequest.headers['Content-Type']).toBeUndefined(); expectSignature('', getRequest.headers.Signature); }); @@ -1277,9 +1283,12 @@ describe('BankFrickService', () => { http.request.mockResolvedValueOnce({ token: jwt() }).mockResolvedValueOnce(listResponse); await expect(service.listVibans(undefined, undefined, 0, 50)).resolves.toEqual(listResponse); - expect(http.request.mock.calls[1][0].url).toBe( - 'https://vban.bank.invalid/vban/virtual-ibans?pageIndex=0&pageSize=50', - ); + const listRequest = http.request.mock.calls[1][0]; + expect(listRequest.url).toBe('https://vban.bank.invalid/vban/virtual-ibans?pageIndex=0&pageSize=50'); + // Bodyless vIBAN list GETs must omit Content-Type (unlike standard WebAPI GETs which send */*). + expect(listRequest.headers['Content-Type']).toBeUndefined(); + expect(listRequest.data).toBe(''); + expectSignature('', listRequest.headers.Signature); http.request.mockResolvedValueOnce(listResponse); await service.listVibans(debtorIban, [FrickVirtualIbanState.ACTIVE, FrickVirtualIbanState.PREPARED], 0, 50); @@ -1289,6 +1298,7 @@ describe('BankFrickService', () => { FrickVirtualIbanState.ACTIVE, FrickVirtualIbanState.PREPARED, ]); + expect(http.request.mock.calls[2][0].headers['Content-Type']).toBeUndefined(); http.request.mockResolvedValueOnce({ pagination: { hasMore: false, pageIndex: 0, pageSize: 50, totalCount: 0 }, diff --git a/src/integration/bank/services/frick.service.ts b/src/integration/bank/services/frick.service.ts index 0001ffb4c0..9838527b55 100644 --- a/src/integration/bank/services/frick.service.ts +++ b/src/integration/bank/services/frick.service.ts @@ -35,6 +35,15 @@ import { CamtTransaction, Iso20022Service } from './iso20022.service'; type FrickResponseType = 'json' | 'text'; +/** + * Bodyless request Content-Type policy for signed Bank Frick calls. + * Required explicitly at every call site — no default — so WebAPI and vIBAN cannot silently share + * the wrong policy. WebAPI bodyless GETs send a wildcard Content-Type; vIBAN bodyless GETs must + * omit it entirely (production Azure gateway otherwise returns unsigned HTTP 403). + * Requests with a body always use `application/json` on both APIs and ignore this value. + */ +type FrickBodylessContentType = '*/*' | undefined; + /** A vIBAN create failure for which no Bank Frick object can have been created. */ export class FrickVibanNotCreatedError extends Error {} @@ -776,6 +785,7 @@ export class BankFrickService { responseType: FrickResponseType, allowUnauthorizedRetry: boolean, classifyVibanCreateFailure: boolean, + bodylessContentType: FrickBodylessContentType, ): Promise { this.assertAvailable(); let token: string; @@ -793,6 +803,20 @@ export class BankFrickService { throw error; } + const requestHeaders: Record = { + Accept: accept, + Authorization: `Bearer ${token}`, + Signature: signature, + algorithm: 'rsa-sha512', + }; + // Bodyful requests are always application/json on WebAPI and vIBAN. Bodyless requests use the + // explicit API-specific policy: WebAPI sends Content-Type: */*; vIBAN omits the header entirely. + if (body === undefined) { + if (bodylessContentType !== undefined) requestHeaders['Content-Type'] = bodylessContentType; + } else { + requestHeaders['Content-Type'] = 'application/json'; + } + try { return await this.http.request({ url, @@ -801,13 +825,7 @@ export class BankFrickService { responseType, tryCount: 1, timeout: BankFrickService.HTTP_TIMEOUT_MS, - headers: { - Accept: accept, - 'Content-Type': body === undefined ? '*/*' : 'application/json', - Authorization: `Bearer ${token}`, - Signature: signature, - algorithm: 'rsa-sha512', - }, + headers: requestHeaders, responseVerifier: (rawBody, headers, status) => this.verifyResponse(rawBody, headers, status), }); } catch (error) { @@ -826,7 +844,17 @@ export class BankFrickService { ); throw refreshError; } - return this.requestSigned(url, path, method, body, accept, responseType, false, classifyVibanCreateFailure); + return this.requestSigned( + url, + path, + method, + body, + accept, + responseType, + false, + classifyVibanCreateFailure, + bodylessContentType, + ); } const message = `Bank Frick API request failed (${method} ${this.sanitizeApiPathForError(path)}): ${this.getHttpFailureReason(error)}`; @@ -854,6 +882,7 @@ export class BankFrickService { responseType, allowUnauthorizedRetry, false, + '*/*', ); } @@ -876,6 +905,7 @@ export class BankFrickService { responseType, allowUnauthorizedRetry, classifyVibanCreateFailure, + undefined, ); } diff --git a/src/integration/blockchain/shared/evm/__tests__/evm-decimals.service.spec.ts b/src/integration/blockchain/shared/evm/__tests__/evm-decimals.service.spec.ts new file mode 100644 index 0000000000..ffa6d28010 --- /dev/null +++ b/src/integration/blockchain/shared/evm/__tests__/evm-decimals.service.spec.ts @@ -0,0 +1,76 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { Blockchain } from 'src/integration/blockchain/shared/enums/blockchain.enum'; +import { createCustomAsset } from 'src/shared/models/asset/__mocks__/asset.entity.mock'; +import { AssetService } from 'src/shared/models/asset/asset.service'; +import { BlockchainRegistryService } from '../../services/blockchain-registry.service'; +import { EvmDecimalsService } from '../evm-decimals.service'; + +// The decimals it writes are read back through AssetService's cache. Writing past the service would +// leave that cache serving the previous rows, so these tests pin that the write goes through it. +describe('EvmDecimalsService.setDecimals', () => { + const usdt = createCustomAsset({ id: 7, dexName: 'USDT', blockchain: Blockchain.ETHEREUM }); + const dai = createCustomAsset({ id: 9, dexName: 'DAI', blockchain: Blockchain.ETHEREUM }); + + let service: EvmDecimalsService; + let assetService: DeepMocked; + let blockchainRegistry: DeepMocked; + let getToken: jest.Mock; + + beforeEach(() => { + assetService = createMock(); + blockchainRegistry = createMock(); + + getToken = jest.fn().mockResolvedValue({ decimals: 6 }); + blockchainRegistry.getEvmClient.mockReturnValue({ getToken } as never); + + service = new EvmDecimalsService(assetService, blockchainRegistry); + }); + + it('writes the decimals through the service that owns the cache', async () => { + assetService.getEvmAssetsWithoutDecimals.mockResolvedValue([usdt]); + + await service.setDecimals(); + + expect(assetService.updateAssets).toHaveBeenCalledWith([[7, { decimals: 6 }]]); + }); + + it('collects every asset into a single write', async () => { + assetService.getEvmAssetsWithoutDecimals.mockResolvedValue([usdt, dai]); + + await service.setDecimals(); + + expect(assetService.updateAssets).toHaveBeenCalledTimes(1); + expect(assetService.updateAssets).toHaveBeenCalledWith([ + [7, { decimals: 6 }], + [9, { decimals: 6 }], + ]); + }); + + // The empty case is handed on rather than short-circuited here; AssetService.updateAssets is what + // decides that an empty list must not touch the cache. + it('collects nothing when no asset is missing its decimals', async () => { + assetService.getEvmAssetsWithoutDecimals.mockResolvedValue([]); + + await service.setDecimals(); + + expect(assetService.updateAssets).toHaveBeenCalledWith([]); + }); + + it('keeps the assets it could read when one lookup fails', async () => { + assetService.getEvmAssetsWithoutDecimals.mockResolvedValue([usdt, dai]); + getToken.mockRejectedValueOnce(new Error('node unreachable')).mockResolvedValueOnce({ decimals: 18 }); + + await service.setDecimals(); + + expect(assetService.updateAssets).toHaveBeenCalledWith([[9, { decimals: 18 }]]); + }); + + it('collects nothing when every lookup fails', async () => { + assetService.getEvmAssetsWithoutDecimals.mockResolvedValue([usdt, dai]); + getToken.mockRejectedValue(new Error('node unreachable')); + + await service.setDecimals(); + + expect(assetService.updateAssets).toHaveBeenCalledWith([]); + }); +}); diff --git a/src/integration/blockchain/shared/evm/evm-decimals.service.ts b/src/integration/blockchain/shared/evm/evm-decimals.service.ts index ead63937cd..19ac56f539 100644 --- a/src/integration/blockchain/shared/evm/evm-decimals.service.ts +++ b/src/integration/blockchain/shared/evm/evm-decimals.service.ts @@ -1,11 +1,11 @@ import { Injectable } from '@nestjs/common'; import { CronExpression } from '@nestjs/schedule'; -import { AssetType } from 'src/shared/models/asset/asset.entity'; -import { RepositoryFactory } from 'src/shared/repositories/repository.factory'; +import { Asset } from 'src/shared/models/asset/asset.entity'; +import { AssetService } from 'src/shared/models/asset/asset.service'; +import { UpdateResult } from 'src/shared/models/entity'; import { DfxLogger } from 'src/shared/services/dfx-logger'; import { Process } from 'src/shared/services/process.service'; import { DfxCron } from 'src/shared/utils/cron'; -import { In, IsNull, Not } from 'typeorm'; import { BlockchainRegistryService } from '../services/blockchain-registry.service'; import { EvmBlockchains } from '../util/blockchain.util'; @@ -14,28 +14,27 @@ export class EvmDecimalsService { private readonly logger = new DfxLogger(EvmDecimalsService); constructor( - private readonly repoFactory: RepositoryFactory, + private readonly assetService: AssetService, private readonly blockchainRegistry: BlockchainRegistryService, ) {} // --- JOBS --- // @DfxCron(CronExpression.EVERY_HOUR, { process: Process.ASSET_DECIMALS, timeout: 1800 }) async setDecimals() { - const assets = await this.repoFactory.asset.findBy({ - chainId: Not(IsNull()), - blockchain: In(EvmBlockchains), - decimals: IsNull(), - type: In([AssetType.COIN, AssetType.TOKEN]), - }); + const assets = await this.assetService.getEvmAssetsWithoutDecimals(EvmBlockchains); + + const updates: UpdateResult[] = []; for (const asset of assets) { try { const client = this.blockchainRegistry.getEvmClient(asset.blockchain); const currency = await client.getToken(asset); - await this.repoFactory.asset.update(asset.id, { decimals: currency.decimals }); + updates.push([asset.id, { decimals: currency.decimals }]); } catch (e) { this.logger.error(`Failed to update decimals of asset ${asset.id}:`, e); } } + + await this.assetService.updateAssets(updates); } } diff --git a/src/shared/filters/__tests__/exception.filter.spec.ts b/src/shared/filters/__tests__/exception.filter.spec.ts index 1fd196eadc..8c878a9361 100644 --- a/src/shared/filters/__tests__/exception.filter.spec.ts +++ b/src/shared/filters/__tests__/exception.filter.spec.ts @@ -11,6 +11,7 @@ import { import { ValidationError } from 'class-validator'; import { LogRejectedValue } from 'src/shared/decorators/log-rejected-value.decorator'; import { ApiExceptionFilter } from 'src/shared/filters/exception.filter'; +import { MAX_MASKED_PATTERN } from 'src/shared/middlewares/api-trace.middleware'; import { ValidationFailedException } from 'src/shared/pipes/detailed-validation.pipe'; describe('ApiExceptionFilter', () => { @@ -113,6 +114,37 @@ describe('ApiExceptionFilter', () => { expect(msg).toContain('abcWARN'); }); + it('masks only the front of a request-sized reason, and does so quickly', () => { + // Masking is regex work over what it is given; the cap throws the rest away anyway. + const message = `${'a'.repeat(480)}someone@example.com${'b'.repeat(5_000_000)}`; + + const started = process.hrtime.bigint(); + filter.catch(new BadRequestException(message), host(req(), { status })); + const elapsedMs = Number(process.hrtime.bigint() - started) / 1e6; + + const msg = warn.mock.calls[0][0] as string; + expect(msg).not.toContain('someone@example.com'); + expect(msg).toContain('***'); + // Masking this whole message takes seconds; the bound is what is being shown, so the limit is + // loose enough to survive a loaded machine and still far below what it would cost unbounded. + expect(elapsedMs).toBeLessThan(1000); + }); + + it('does not let the point where masking stopped expose a pattern it split', () => { + // Masking only shortens, so text from far past the cap moves into view: three long addresses + // collapse to nine characters. A pattern the masking never saw whole would be what moves with + // them - unless a pattern length is dropped from the end of what was masked. + const scanLength = 500 + 2 * MAX_MASKED_PATTERN; + const shrinking = Array.from({ length: 3 }, () => `a@${'d'.repeat(240)}.com`).join(' '); + const secret = 'ZZTOPSECRET@example.com'; + const filler = 'f'.repeat(scanLength - 8 - shrinking.length - 1); + filter.catch(new BadRequestException(`${shrinking} ${filler}${secret}`), host(req(), { status })); + + const msg = warn.mock.calls[0][0] as string; + expect(msg).not.toContain('ZZTOPSEC'); + expect(msg).toContain('***'); + }); + it('caps a reason as large as the body it came from, and still masks up to the cap', () => { // Exception messages interpolate request values, and a request body is large; the reason is // cut to the cap, and a pattern that starts inside it is masked even though it runs past it. @@ -146,62 +178,191 @@ describe('ApiExceptionFilter', () => { expect(() => filter.catch(unreadable, host(req(), { status }))).not.toThrow(); expect(status).toHaveBeenCalledWith(400); - expect(json).toHaveBeenCalledWith({ statusCode: 400, message: 'x' }); + // the exception's own message stays where it was: it is what it says to us, not to the caller + expect(json).toHaveBeenCalledWith({ statusCode: 400, message: 'BAD_REQUEST' }); }); - it('keeps sending the response when the message cannot be read', () => { - const unreadable = new BadRequestException({ - statusCode: 400, - message: [ - { - toString: () => { - throw new Error('nope'); - }, - }, - ], - }); + it('takes nothing out of a body it could not pass on, the message included', () => { + // What such a body holds is not what it would have sent, so reading any of it reads something + // else - and the exception's own message is what it says to us, not to the caller. + const divergent = new HttpException({ statusCode: 418, message: 'public' }, 400); + divergent.message = 'INTERNAL_SECRET'; + + filter.catch(divergent, host(req(), { status })); - expect(() => filter.catch(unreadable, host(req(), { status }))).not.toThrow(); expect(status).toHaveBeenCalledWith(400); - expect(json).toHaveBeenCalled(); + expect(json).toHaveBeenCalledWith({ statusCode: 400, message: 'BAD_REQUEST' }); }); - it('keeps a failure to write the line away from a caller that already has its answer', () => { - warn.mockImplementation(() => { - throw new Error('logger down'); + it('names the status when a plain error says nothing about itself', () => { + filter.catch(new Error(), host(req(), { status })); + + expect(json).toHaveBeenCalledWith({ statusCode: 500, message: 'INTERNAL_SERVER_ERROR' }); + }); + + it('names the status when a plain error answers with something that is not text', () => { + const odd = new Error('x'); + Object.defineProperty(odd, 'message', { value: 42 }); + + filter.catch(odd, host(req(), { status })); + + expect(json).toHaveBeenCalledWith({ statusCode: 500, message: 'INTERNAL_SERVER_ERROR' }); + }); + + it('sends the response even when a plain error cannot be asked what it says', () => { + const mute = new Error('x'); + Object.defineProperty(mute, 'message', { + get: () => { + throw new Error('nope'); + }, }); - expect(() => filter.catch(new BadRequestException('bad'), host(req(), { status }))).not.toThrow(); - expect(status).toHaveBeenCalledWith(400); - expect(json).toHaveBeenCalled(); + expect(() => filter.catch(mute, host(req(), { status }))).not.toThrow(); + expect(json).toHaveBeenCalledWith({ statusCode: 500, message: 'INTERNAL_SERVER_ERROR' }); }); - it('describes the response it is sending, not the one the exception named', () => { - // The status was replaced, so the body the exception carries no longer says what is being sent. - const mismatched = new BadRequestException('x'); - jest.spyOn(mismatched, 'getStatus').mockReturnValue(600); + it('passes on a body that serializes itself, as it would have been without any of this', () => { + // What such a body holds says nothing about what it sends, so there is nothing to judge and + // nothing to take out of it. + const rewriting = new HttpException({ statusCode: 418, message: 'INTERNAL_SECRET' }, 400); + const body = rewriting.getResponse(); + Object.defineProperty(body, 'toJSON', { value: () => ({ statusCode: 400, message: 'public' }) }); - filter.catch(mismatched, host(req(), { status })); + filter.catch(rewriting, host(req(), { status })); - expect(status).toHaveBeenCalledWith(500); - expect(json).toHaveBeenCalledWith({ statusCode: 500, message: 'x' }); + expect(json).toHaveBeenCalledWith(body); }); - it('sends the response even when the message cannot be read either', () => { - const mute = new BadRequestException('x'); - jest.spyOn(mute, 'getResponse').mockImplementation(() => { + it('names a status that has no name, rather than leaving the message out', () => { + const unnamed = new HttpException('x', 599); + jest.spyOn(unnamed, 'getResponse').mockImplementation(() => { throw new Error('nope'); }); - Object.defineProperty(mute, 'message', { + Object.defineProperty(unnamed, 'message', { get: () => { throw new Error('nope'); }, }); - expect(() => filter.catch(mute, host(req(), { status }))).not.toThrow(); + filter.catch(unnamed, host(req(), { status })); + + expect(json).toHaveBeenCalledWith({ statusCode: 599, message: 'Error' }); + }); + + it('reads the status once, so a body cannot arrive under a status it does not name', () => { + const shifting = new BadRequestException('x'); + let call = 0; + const getStatus = jest.spyOn(shifting, 'getStatus').mockImplementation(() => (call++ === 0 ? 600 : 400)); + + filter.catch(shifting, host(req(), { status })); + + expect(getStatus).toHaveBeenCalledTimes(1); + expect(status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ statusCode: 500, message: 'INTERNAL_SERVER_ERROR' }); + }); + + it('does not take a status it can only be told when it asks as agreement', () => { + // An accessor is serialized, so the body does carry a status - and it answers again when it is + // serialized, so what it would carry cannot be read here. The body cannot be passed on. + const accessor = new BadRequestException('x'); + Object.defineProperty(accessor.getResponse(), 'statusCode', { get: () => 418, enumerable: true }); + + filter.catch(accessor, host(req(), { status })); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ statusCode: 400, message: 'BAD_REQUEST' }); + }); + + it('never reads a body whose status had to be replaced', () => { + const replaced = new HttpException({ statusCode: 418, message: 'INTERNAL_SECRET' }, 600); + const getResponse = jest.spyOn(replaced, 'getResponse'); + + filter.catch(replaced, host(req(), { status })); + + expect(getResponse).not.toHaveBeenCalled(); + expect(json).toHaveBeenCalledWith({ statusCode: 500, message: 'INTERNAL_SERVER_ERROR' }); + }); + + it('keeps a failure to report a failed send away from the caller as well', () => { + const response = { + status: () => { + throw new Error('transport down'); + }, + }; + error.mockImplementation(() => { + throw new Error('logger down'); + }); + + expect(() => filter.catch(new BadRequestException('bad'), host(req(), response))).not.toThrow(); + }); + + it('drops a body that names a status other than the one being sent', () => { + filter.catch(new HttpException({ statusCode: 418, message: 'teapot' }, 400), host(req(), { status })); + + expect(status).toHaveBeenCalledWith(400); + expect(json).toHaveBeenCalledWith({ statusCode: 400, message: 'BAD_REQUEST' }); + }); + + it('keeps a body that names no status of its own', () => { + filter.catch(new HttpException({ message: 'plain' }, 400), host(req(), { status })); + + expect(json).toHaveBeenCalledWith({ message: 'plain' }); + }); + + it('keeps the diagnostic text when nothing had to be cut away', () => { + // The tail only has to go where the masking could have split a pattern and the shortening could + // bring its head into view; a short reason loses nothing. + filter.catch(new BadRequestException('amount must be a positive number'), host(req(), { status })); + + expect(warn.mock.calls[0][0]).toContain('amount must be a positive number'); + }); + + it('keeps a name put on an array body out of the status it is judged by', () => { + // An array is sent as its elements; a name alongside them is not sent at all, so it names no + // status to contradict the one being sent. + const listed = new HttpException(['a', 'b'], 400); + (listed.getResponse() as unknown as { statusCode: number }).statusCode = 418; + + filter.catch(listed, host(req(), { status })); + + expect(json).toHaveBeenCalledWith(listed.getResponse()); + }); + + it('judges a body whose toJSON is only there when asked for like any other', () => { + // An accessor might answer with a function and might not; the serialization would then send + // what the body holds, under a status the body does not name. + const shifting = new HttpException({ statusCode: 418, message: 'INTERNAL_SECRET' }, 400); + Object.defineProperty(shifting.getResponse(), 'toJSON', { get: () => 42 }); + + filter.catch(shifting, host(req(), { status })); + + expect(status).toHaveBeenCalledWith(400); expect(json).toHaveBeenCalledWith({ statusCode: 400, message: 'BAD_REQUEST' }); }); + it('passes on a body whose status is a value the serialization leaves out', () => { + for (const carried of [undefined, () => 418, Symbol('418')]) { + json.mockClear(); + const omitted = new BadRequestException('x'); + Object.defineProperty(omitted.getResponse(), 'statusCode', { value: carried, enumerable: true }); + + filter.catch(omitted, host(req(), { status })); + + expect(json).toHaveBeenCalledWith(omitted.getResponse()); + } + }); + + it('sends the response even when asking what the exception is throws', () => { + const hostile = new Proxy(new BadRequestException('x'), { + getPrototypeOf: () => { + throw new Error('nope'); + }, + }); + + expect(() => filter.catch(hostile, host(req(), { status }))).not.toThrow(); + expect(json).toHaveBeenCalledWith({ statusCode: 500, message: 'INTERNAL_SERVER_ERROR' }); + }); + it('sends a server error when the status cannot be read or is not one Express sends', () => { const broken = new BadRequestException('x'); jest.spyOn(broken, 'getStatus').mockImplementation(() => { diff --git a/src/shared/filters/exception.filter.ts b/src/shared/filters/exception.filter.ts index 5a256de564..e203e912d1 100644 --- a/src/shared/filters/exception.filter.ts +++ b/src/shared/filters/exception.filter.ts @@ -1,6 +1,6 @@ import { ArgumentsHost, Catch, ExceptionFilter, HttpException, HttpStatus } from '@nestjs/common'; import { Request } from 'express'; -import { capCharacters, maskLogText, maskUrl } from 'src/shared/middlewares/api-trace.middleware'; +import { MAX_MASKED_PATTERN, capCharacters, maskLogText, maskUrl } from 'src/shared/middlewares/api-trace.middleware'; import { ValidationFailedException, describeRejectedValues } from 'src/shared/pipes/detailed-validation.pipe'; import { DfxLogger } from 'src/shared/services/dfx-logger'; import { describeCaller } from 'src/shared/utils/request-caller'; @@ -17,27 +17,44 @@ export class ApiExceptionFilter implements ExceptionFilter { // Cap the (masked) reason: exception messages can embed user-supplied free text. private static readonly REASON_MAX_LENGTH = 500; + // How much of the message is masked at all. An exception message can be as large as the body it + // interpolated a value from, and masking is regex work over what it is given - a request-sized one + // held the thread for seconds. Two pattern lengths of margin over the cap: one for a pattern that + // starts inside what stays visible, one for the tail that is dropped again below. + private static readonly REASON_SCAN_LENGTH = ApiExceptionFilter.REASON_MAX_LENGTH + 2 * MAX_MASKED_PATTERN; + private readonly logger = new DfxLogger(ApiExceptionFilter); - catch(exception: Error, host: ArgumentsHost) { + catch(exception: Error, host: ArgumentsHost): void { const ctx = host.switchToHttp(); const response = ctx.getResponse(); - const status = ApiExceptionFilter.statusOf(exception); + const status = ApiExceptionFilter.resolveStatus(exception); // The response goes out first, and nothing it does not need is read before it. Everything the // line renders comes from the request or from the thrower, and reading either can throw - which // used to leave the caller with no response at all rather than with a line missing a detail. + let responseError: unknown; + let responseSent = true; try { - response.status(status).json(this.responseBody(exception, status)); + response.status(status.effective).json(this.responseBody(exception, status)); } catch (e) { - this.logger.error(`Failed to set error response content:`, e); + responseError = e; + responseSent = false; } - // The response is out; what follows only describes it. A failure to do that must not travel back - // to a caller who already has an answer, so it ends here - including a failure of the logger, - // which is the one thing that could not be used to report it anyway. + // What follows only describes what happened, this failure included. It must not travel back to + // a caller who by now either has an answer or is past being given one, so it ends here - + // including a failure of the logger, which is the one thing that could not report it anyway. try { - this.describe(exception, ctx.getRequest(), status); + if (!responseSent) { + // Only a thrown `Error` is reported as what it is. Anything else - a string included - could + // carry whatever the body carried, and this line is not the place to find that out. + this.logger.error( + `Failed to set error response content:`, + responseError instanceof Error ? responseError : new Error('Non-error thrown'), + ); + } + this.describe(exception, ctx.getRequest(), status.effective); } catch { return; } @@ -61,7 +78,7 @@ export class ApiExceptionFilter implements ExceptionFilter { // // All three are untrusted input and rendered as such - single-line, masked and capped. The // reason included: an exception message can interpolate a value the request supplied. - const reason = capCharacters(maskLogText(this.getReason(exception)), ApiExceptionFilter.REASON_MAX_LENGTH); + const reason = ApiExceptionFilter.maskReason(this.getReason(exception)); const rejected = exception instanceof ValidationFailedException ? ` (received: ${describeRejectedValues(exception.validationErrors)})` @@ -70,39 +87,132 @@ export class ApiExceptionFilter implements ExceptionFilter { } } + /** + * The masked, single-line, capped part of the message that reaches the line. + * + * Only the front of the message is masked, because masking is the expensive part and the cap + * would throw the rest away anyway. A pattern crossing the point where the masking stopped + * arrives halved and is not recognized, and its head is then the last thing the masking produced + * - so a pattern length is dropped from the end of what came back. What is kept can only hold + * patterns that ended before that point, and those were seen whole. The tail goes after the + * masking rather than before it: masking only shortens, so a position past the cap can move into + * view, and dropping the tail first would leave that halved head to move with it. + */ + private static maskReason(reason: string): string { + const scanned = capCharacters(reason, ApiExceptionFilter.REASON_SCAN_LENGTH); + const masked = maskLogText(scanned); + + return capCharacters( + ApiExceptionFilter.removeSplitTail(masked, scanned === reason), + ApiExceptionFilter.REASON_MAX_LENGTH, + ); + } + + // Nothing was cut, or what the masking left is long enough that its end stays past the cap either + // way: the tail costs diagnostic text and buys nothing there. + private static removeSplitTail(masked: string, complete: boolean): string { + const length = [...masked].length; + if (complete || length > ApiExceptionFilter.REASON_MAX_LENGTH + MAX_MASKED_PATTERN) return masked; + + return capCharacters(masked, Math.max(0, length - MAX_MASKED_PATTERN)); + } + // The status an HttpException carries is whatever the thrower put there: reading it can throw, and // what comes back is not necessarily a final response at all - a 1xx is an interim one, and // nothing outside the range it can send leaves a reading other than server error. - private static statusOf(exception: Error): number { + private static resolveStatus(exception: Error): { effective: number; declared: number | undefined } { try { - if (!(exception instanceof HttpException)) return HttpStatus.INTERNAL_SERVER_ERROR; + if (!(exception instanceof HttpException)) + return { effective: HttpStatus.INTERNAL_SERVER_ERROR, declared: undefined }; + + const declared = exception.getStatus(); + const usable = Number.isInteger(declared) && declared >= 200 && declared <= 599; - const status = exception.getStatus(); - return Number.isInteger(status) && status >= 200 && status <= 599 ? status : HttpStatus.INTERNAL_SERVER_ERROR; + return { effective: usable ? declared : HttpStatus.INTERNAL_SERVER_ERROR, declared }; } catch { - return HttpStatus.INTERNAL_SERVER_ERROR; + return { effective: HttpStatus.INTERNAL_SERVER_ERROR, declared: undefined }; } } // The body an HttpException carries is whatever the thrower put there, and reading it can throw. - // A caller gets the generic body then rather than none - and also when the status it was going to - // be sent with is not the one it names, which is what a replaced status leaves behind. - private responseBody(exception: Error, status: number): unknown { + // A caller gets the generic body then rather than none - and also when the body is not the one + // being sent, which is what a replaced status or a body naming another one leaves behind. + private responseBody(exception: Error, status: { effective: number; declared: number | undefined }): unknown { try { - if (exception instanceof HttpException && exception.getStatus() === status) return exception.getResponse(); + if (!(exception instanceof HttpException)) { + return { + statusCode: status.effective, + message: ApiExceptionFilter.readOwnMessage(exception, status.effective), + }; + } + + // A status that had to be replaced takes the body with it: what the thrower wrote belongs to + // the status it wrote it for, and reading it for another one is reading it for something it + // was not. + if (status.declared !== status.effective) { + return { statusCode: status.effective, message: HttpStatus[status.effective] ?? 'Error' }; + } + + const body = exception.getResponse(); + + // A body that serializes itself is sent as it is, the way it would have been without any of + // this, and so is one that agrees with the status being sent. Anything else is replaced whole: + // a body that cannot be passed on cannot be read either, because what it holds is not what it + // would have sent, and the name of the status is the one thing that is certain here. + if (ApiExceptionFilter.isSelfSerializing(body) || ApiExceptionFilter.isConsistentWith(body, status.effective)) + return body; + + return { statusCode: status.effective, message: HttpStatus[status.effective] ?? 'Error' }; } catch { - // an exception that cannot say what it is gets described by what is being sent + return { statusCode: status.effective, message: HttpStatus[status.effective] ?? 'Error' }; } - - return { statusCode: status, message: ApiExceptionFilter.messageOf(exception, status) }; } - private static messageOf(exception: Error, status: number): string { + // What the exception says about itself, for the case where there is no body to say it better. + // Reading it can throw, and an exception that says nothing is not worth sending in place of the + // name of what is being sent. + private static readOwnMessage(exception: Error, status: number): string { try { - return exception.message || (HttpStatus[status] as string); + const message: unknown = exception.message; + + return typeof message === 'string' && message ? message : (HttpStatus[status] ?? 'Error'); } catch { - return HttpStatus[status] as string; + return HttpStatus[status] ?? 'Error'; + } + } + + // A body that answers `toJSON` is serialized from what that returns, not from what it holds, so + // what it holds says nothing about what it sends - which is why it is passed on rather than read. + private static isSelfSerializing(body: unknown): boolean { + if (typeof body !== 'object' || body === null) return false; + + // Asked for as a descriptor rather than read: reading it would be one read, and the + // serialization another. Only a function that is simply there serializes the body; an accessor + // might answer with one and might not, and a body that only might is judged like any other. + for (let level: object | null = body; level; level = Object.getPrototypeOf(level)) { + const declared = Object.getOwnPropertyDescriptor(level, 'toJSON'); + if (declared) return 'value' in declared && typeof declared.value === 'function'; } + + return false; + } + + // A body agrees with the status being sent unless it carries one of its own that differs. What + // `JSON.stringify` leaves out carries nothing: a name that is not enumerable, one whose value is + // `undefined`, a function, a symbol, a name put on an array alongside its elements. An accessor is the other way round - it + // is serialized, so it does carry one, and it answers again when it is, so what it would carry + // cannot be read here. That is not agreement. + private static isConsistentWith(body: unknown, status: number): boolean { + if (typeof body !== 'object' || body === null || Array.isArray(body)) return true; + + const declared = Object.getOwnPropertyDescriptor(body, 'statusCode'); + if (!declared?.enumerable) return true; + if (!('value' in declared)) return false; + + const carried: unknown = declared.value; + if (carried === undefined || typeof carried === 'function' || typeof carried === 'symbol') return true; + + return carried === status; } // Human-readable rejection reason. For HttpExceptions the useful text is in the diff --git a/src/shared/middlewares/__tests__/api-trace.middleware.spec.ts b/src/shared/middlewares/__tests__/api-trace.middleware.spec.ts index 69aedcf068..d3d65c4036 100644 --- a/src/shared/middlewares/__tests__/api-trace.middleware.spec.ts +++ b/src/shared/middlewares/__tests__/api-trace.middleware.spec.ts @@ -163,6 +163,20 @@ describe('apiTraceMiddleware', () => { expect(line).not.toContain('jane@example.com'); }); + it('leaves percent-encoded personal data in a path visible', () => { + // A path carries its values encoded, and no pattern matches `%40`. Reading them decoded is a + // question about what this trace is for rather than a wider pattern list, and is left open. + const req = { + method: 'GET', + originalUrl: '/v1/realunit/user/victim%40example.com', + headers: {}, + body: undefined, + }; + const { lines } = runTrace(req, 200, (res) => res.send('ok')); + + expect(lines[0]).toContain('victim%40example.com'); + }); + it('stops redaction at the compute budget instead of walking a huge body', () => { const huge = { items: Array.from({ length: 50_000 }, (_, i) => `leaf-value-${i}-${'x'.repeat(400)}`) }; const { lines } = runTrace(realunitReq(huge), 200, (res) => res.json({})); @@ -208,6 +222,22 @@ describe('apiTraceMiddleware', () => { expect([...line].some(isLoneSurrogate)).toBe(false); }); + it('caps the request target after reading it, so a pattern across the cap is still masked', () => { + // The address starts before the cap and runs past it: capping first would have cut it in half + // and left the front of it in the line. + const prefix = '/v1/realunit/user/'; + const req = { + method: 'GET', + originalUrl: `${prefix}${'a'.repeat(505 - prefix.length)}jane@example.com`, + headers: {}, + body: undefined, + }; + const { lines } = runTrace(req, 200, (res) => res.send('ok')); + + expect(lines[0]).not.toContain('jane'); + expect(lines[0]).toContain('***'); + }); + it('keeps the request target on one line as well', () => { const req = { method: 'GET', @@ -303,13 +333,30 @@ describe('maskLogValue', () => { expect(maskLogValue('\u{1F600}'.repeat(257), 96)).toBe('<514 code units>'); }); - it('masks a pattern a control character was placed next to, which removing it would join', () => { - // Removing the character puts what followed it against the end of the pattern, and the address - // no longer ends on a word boundary - so the masking also runs before the removal. + it('masks an address that removing a character puts a letter against', () => { expect(maskLogValue('192.0.2.123\u0000a', 96)).toBe('***a'); + }); + + it('masks a wallet address that removing a character would turn into a longer hex run', () => { + // The pass before the removal is what sees it: afterwards it is a longer run, and a longer run + // is left alone on purpose because that is what a transaction hash looks like. expect(maskLogValue(`0x${'a'.repeat(40)}\u0000b`, 96)).toBe('0x…b'); }); + it('errs towards masking where the second pass folds what the first one wrote', () => { + // `***` reads as the local part of an address, so the domain after it goes too. Masking more + // than was there is the safe direction to be wrong in. + expect(maskLogValue('192.0.2.123\u2028@error.code', 96)).toBe('***'); + }); + + it('masks an address that a letter or an underscore stands next to', () => { + // Only a digit on either side means it is a longer number rather than an address. + expect(maskLogValue('ip192.0.2.123', 96)).toBe('ip***'); + expect(maskLogValue('_192.0.2.123', 96)).toBe('_***'); + expect(maskLogValue('x192.0.2.123y', 96)).toBe('x***y'); + expect(maskLogValue('1192.0.2.123', 96)).toBe('1192.0.2.123'); + }); + it('masks a pattern that a control character was placed inside', () => { // Removing the character rather than replacing it puts the pattern back together, so the // masking sees it as the value it is. diff --git a/src/shared/middlewares/api-trace.middleware.ts b/src/shared/middlewares/api-trace.middleware.ts index 9abee57c6e..954de55879 100644 --- a/src/shared/middlewares/api-trace.middleware.ts +++ b/src/shared/middlewares/api-trace.middleware.ts @@ -21,23 +21,34 @@ export const REDACT_KEY = // backtracking on the request path. const WALLET_ADDRESS = /0x[0-9a-f]{40}(?![0-9a-f])/gi; const EMAIL = /[^\s"@/]{1,64}@[^\s"@/]{1,255}\.[^\s"@/.]{1,24}/g; -const IPV4 = /\b\d{1,3}(?:\.\d{1,3}){3}\b/g; +// The boundaries are against digits rather than word characters: an address is as much an address +// for having a letter or an underscore next to it, and `ip192.0.2.123` is how one usually arrives. +// A digit on either side still leaves it alone - that is a longer number, not an address. +const IPV4 = /(^|[^\d])(\d{1,3}(?:\.\d{1,3}){3})(?!\d)/g; export const MAX_STRING = 512; // per logged string: beyond this only its length is reported const MAX_CLIENT = 32; // per trace line: the client header is a name, not a payload +const MAX_URL = 512; // per log line: a route, and one nobody follows past either way const MAX_PART = 4000; // per serialized section (headers / req body / res body) const REDACT_BUDGET = 2 * MAX_PART; // per section: bounds the compute, not just the output const REDACTED = '***'; +const WALLET_SHORT = '0x…'; const TRUNCATED = '<…truncated…>'; +// The longest match the patterns above can produce - the email's 64 + `@` + 255 + `.` + 24. A caller +// that masks only the front of a value needs it: it is how far a pattern can reach past where that +// caller stopped looking. +export const MAX_MASKED_PATTERN = 64 + 1 + 255 + 1 + 24; + export function maskValue(s: string): string { - return s.replace(WALLET_ADDRESS, '0x…').replace(EMAIL, REDACTED).replace(IPV4, REDACTED); + return s.replace(WALLET_ADDRESS, WALLET_SHORT).replace(EMAIL, REDACTED).replace(IPV4, `$1${REDACTED}`); } export function maskUrl(url: string): string { - // The request target is client-supplied and reaches a log line: what is left of it after the - // query is dropped is rendered like any other value from the request. - return maskLogText(url.split('?')[0]); + // The request target is client-supplied and reaches a log line: what is left of it after the query + // is dropped is rendered like any other value from the request, and capped like one. The cap comes + // after the masking, so a pattern that straddles it is still recognized as one. + return capCharacters(maskLogText(url.split('?')[0]), MAX_URL); } // Everything that can break a line or move a cursor in a log viewer: the control characters @@ -54,14 +65,15 @@ export function singleLine(value: string): string { } /** - * Renders free-form text for a log line: masked, on one line, masked again. + * Renders free-form text for a log line: masked, stripped of everything that could break a line, + * masked again. * * Both passes are needed, because a character that breaks a line also breaks a pattern in either - * direction. Put inside one, it hides the pattern from a pass that runs before the removal - * (`victim\u0001@example.com`). Removing it joins what stood on either side, which can hide a - * pattern that was whole from a pass that runs after (`192.0.2.123\u0000a` becomes `192.0.2.123a`, - * where the address no longer ends on a word boundary). Neither order sees both, so both run - and - * the second pass cannot invent a match, since what the first one leaves behind is `***` and `0x…`. + * direction. Put inside one, it hides the pattern from the pass before the removal, and the pass + * after finds it; removing it joins what stood on either side, which can hide a pattern the pass + * after would have to find whole, and the pass before already saw. The second pass can fold what the + * first one wrote into a match of its own - `***` reads as the local part of an address - which + * costs the text around it and is the direction to be wrong in. */ export function maskLogText(value: string): string { return maskValue(singleLine(maskValue(value))); @@ -163,13 +175,13 @@ function format(value: unknown): string { try { // redact() handles Buffer + the array case (Array.isArray first), so the // raw value is never length/type-inspected here. - // `JSON.stringify` escapes the control characters but leaves U+2028 / U+2029 as they are, so - // the serialized section is put through the same collapse as the free-form values above - it is - // what keeps the trace the single line the caller below documents. + // `JSON.stringify` escapes the control characters but leaves U+2028 / U+2029 as they are, and + // the trace is the single line the caller below documents. s = singleLine(JSON.stringify(redact(value, undefined, { left: REDACT_BUDGET }))); } catch { return '(unserializable)'; } + return s.length > MAX_PART ? `${cutAtCodeUnits(s, MAX_PART)}(${s.length} code units)` : s; } diff --git a/src/shared/models/asset/__tests__/asset.service.update.spec.ts b/src/shared/models/asset/__tests__/asset.service.update.spec.ts new file mode 100644 index 0000000000..4bb68c1579 --- /dev/null +++ b/src/shared/models/asset/__tests__/asset.service.update.spec.ts @@ -0,0 +1,80 @@ +import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { UpdateResult } from '../../entity'; +import { Asset } from '../asset.entity'; +import { AssetRepository } from '../asset.repository'; +import { AssetService } from '../asset.service'; + +// updateAssets is the only write path that reaches the repository instance the cached reads are +// served from, so what it does around invalidateCache decides whether readers see the new rows. +describe('AssetService.updateAssets', () => { + const updates: UpdateResult[] = [ + [7, { decimals: 6 }], + [9, { decimals: 18 }], + ]; + + let service: AssetService; + let assetRepo: DeepMocked; + + beforeEach(() => { + assetRepo = createMock(); + service = new AssetService(assetRepo); + }); + + it('writes every update and invalidates the cache once', async () => { + await service.updateAssets(updates); + + expect(assetRepo.update).toHaveBeenCalledTimes(2); + expect(assetRepo.update).toHaveBeenCalledWith(7, { decimals: 6 }); + expect(assetRepo.update).toHaveBeenCalledWith(9, { decimals: 18 }); + expect(assetRepo.invalidateCache).toHaveBeenCalledTimes(1); + }); + + it('invalidates the cache even when an update fails, so the earlier writes are not left stale', async () => { + assetRepo.update.mockResolvedValueOnce(undefined as never).mockRejectedValueOnce(new Error('deadlock')); + + await expect(service.updateAssets(updates)).rejects.toThrow('9'); + + expect(assetRepo.invalidateCache).toHaveBeenCalledTimes(1); + }); + + // A row that keeps failing used to be harmless because each asset was written on its own. It must + // not become a blocker for every asset queued behind it. + it('attempts every update even when one in the middle fails', async () => { + const three: UpdateResult[] = [ + [7, { decimals: 6 }], + [8, { decimals: 8 }], + [9, { decimals: 18 }], + ]; + assetRepo.update + .mockResolvedValueOnce(undefined as never) + .mockRejectedValueOnce(new Error('constraint violation')) + .mockResolvedValueOnce(undefined as never); + + await expect(service.updateAssets(three)).rejects.toThrow('8'); + + expect(assetRepo.update).toHaveBeenCalledTimes(3); + expect(assetRepo.update).toHaveBeenLastCalledWith(9, { decimals: 18 }); + expect(assetRepo.invalidateCache).toHaveBeenCalledTimes(1); + }); + + // Whoever reads the log needs the database error itself, not a rendering of it. + it('carries every original error, not just their text', async () => { + const first = new Error('deadlock'); + const second = new Error('constraint violation'); + assetRepo.update.mockRejectedValueOnce(first).mockRejectedValueOnce(second); + + const error = await service.updateAssets(updates).catch((e) => e); + + expect(error).toBeInstanceOf(AggregateError); + expect(error.message).toContain('7'); + expect(error.message).toContain('9'); + expect(error.errors).toEqual([first, second]); + }); + + it('does not touch the cache when there is nothing to write', async () => { + await service.updateAssets([]); + + expect(assetRepo.update).not.toHaveBeenCalled(); + expect(assetRepo.invalidateCache).not.toHaveBeenCalled(); + }); +}); diff --git a/src/shared/models/asset/asset.service.ts b/src/shared/models/asset/asset.service.ts index 3f9e8c7981..9c76d8c50b 100644 --- a/src/shared/models/asset/asset.service.ts +++ b/src/shared/models/asset/asset.service.ts @@ -127,12 +127,39 @@ export class AssetService { .then((assets) => Array.from(new Set(assets.map((a) => a.blockchain)))); } - async updatePrices(updates: UpdateResult[]): Promise { - for (const update of updates) { - await this.assetRepo.update(...update); + // Writes have to go through here: the cached reads above are served from this instance, and + // invalidateCache() only clears the instance it is called on. + async updateAssets(updates: UpdateResult[]): Promise { + if (!updates.length) return; + + // Every update is attempted: one row that keeps failing must not stop the rest from ever being + // written. The cache is invalidated in any case, because whatever succeeded is already stored. + const failed: number[] = []; + const errors: unknown[] = []; + + try { + for (const [id, update] of updates) { + try { + await this.assetRepo.update(id, update); + } catch (e) { + failed.push(id); + errors.push(e); + } + } + } finally { + this.assetRepo.invalidateCache(); } - this.assetRepo.invalidateCache(); + if (failed.length) throw new AggregateError(errors, `Failed to update asset(s) ${failed.join(', ')}`); + } + + async getEvmAssetsWithoutDecimals(blockchains: Blockchain[]): Promise { + return this.assetRepo.findBy({ + chainId: Not(IsNull()), + blockchain: In(blockchains), + decimals: IsNull(), + type: In([AssetType.COIN, AssetType.TOKEN]), + }); } async getAssetsUsedOn(exchange: string): Promise { diff --git a/src/subdomains/core/buy-crypto/routes/buy/__tests__/buy.service.spec.ts b/src/subdomains/core/buy-crypto/routes/buy/__tests__/buy.service.spec.ts index cd44fc4136..9ed1b7c4b9 100644 --- a/src/subdomains/core/buy-crypto/routes/buy/__tests__/buy.service.spec.ts +++ b/src/subdomains/core/buy-crypto/routes/buy/__tests__/buy.service.spec.ts @@ -395,6 +395,34 @@ describe('BuyService', () => { expect(errorLog).toHaveBeenCalledTimes(1); }); + it('propagates a business rejection (KYC required) from an explicit Frick request instead of the collection account', async () => { + // Same guarantee as the implicit path: the authoritative, merge-resolved KYC_REQUIRED that issuance + // raises must reach the caller, not be swallowed into the collection-account fallback, even for a + // request whose own KYC snapshot is level 50. + jest + .spyOn(virtualIbanService, 'getOrCreateFrickForUser') + .mockRejectedValue(new BadRequestException(QuoteError.KYC_REQUIRED)); + jest.spyOn(virtualIbanService, 'hasProviderSupportingCurrency').mockReturnValue(true); + const getBank = jest.spyOn(bankService, 'getBank').mockResolvedValue({ + id: 16, + name: IbanBankName.OLKY, + iban: 'FR7616798060015010806550926', + receive: true, + } as any); + + await expect( + service['resolveBankInfo']( + { currency: 'EUR', paymentMethod: FiatPaymentMethod.BANK, userData }, + buy, + asset, + undefined, + PersonalIbanProvider.FRICK, + ), + ).rejects.toThrow(QuoteError.KYC_REQUIRED); + // The fallback must not even be reached for a business rejection, even when a collection bank exists. + expect(getBank).not.toHaveBeenCalled(); + }); + it('uses the standard bank for CARD when implicit providers are ineligible for EUR', async () => { const standardBank = { id: 16, @@ -833,6 +861,26 @@ describe('BuyService', () => { expect(errorLog).toHaveBeenCalledTimes(1); }); + it('propagates a business rejection (KYC required) from issuance instead of showing the collection account', async () => { + // Issuance re-reads and merge-resolves the owner under its own lock, so its KYC_REQUIRED is + // authoritative over the possibly-stale KYC-50 snapshot on the request. It must reach the caller, + // not be swallowed into the fallback - which would hand a rejected customer a usable account. + jest.spyOn(virtualIbanService, 'getActiveReceivingForUserAndCurrency').mockResolvedValue(null); + jest.spyOn(virtualIbanService, 'isUserEligible').mockReturnValue(true); + jest.spyOn(virtualIbanService, 'hasProviderSupportingCurrency').mockReturnValue(true); + jest + .spyOn(virtualIbanService, 'getOrCreateFrickForUser') + .mockRejectedValue(new BadRequestException(QuoteError.KYC_REQUIRED)); + const getBank = jest.spyOn(bankService, 'getBank').mockResolvedValue(collectionBank); + + await expect( + service.getBankInfo({ currency: 'EUR', paymentMethod: FiatPaymentMethod.BANK, userData }, buy), + ).rejects.toThrow(QuoteError.KYC_REQUIRED); + + // The fallback must not even be attempted for a business rejection. + expect(getBank).not.toHaveBeenCalled(); + }); + it('does not show a collection account without a reference, even for an eligible EUR transfer', async () => { jest.spyOn(virtualIbanService, 'getActiveReceivingForUserAndCurrency').mockResolvedValue(null); jest.spyOn(virtualIbanService, 'isUserEligible').mockReturnValue(true); diff --git a/src/subdomains/core/buy-crypto/routes/buy/buy.service.ts b/src/subdomains/core/buy-crypto/routes/buy/buy.service.ts index b38355a860..3c05fd26db 100644 --- a/src/subdomains/core/buy-crypto/routes/buy/buy.service.ts +++ b/src/subdomains/core/buy-crypto/routes/buy/buy.service.ts @@ -492,7 +492,7 @@ export class BuyService { const virtualIban = await this.virtualIbanService .getOrCreateFrickForUser(selector.userData, selector.currency) - .catch(() => null); + .catch((e) => this.infrastructureFailureOrRethrow(e)); if (virtualIban?.bank.receive && virtualIban.bank.name === IbanBankName.FRICK) { return { bankInfo: this.buildVirtualIbanResponse(virtualIban, selector.userData, buy?.bankUsage), @@ -564,7 +564,7 @@ export class BuyService { selector.currency === 'EUR' ? this.virtualIbanService.getOrCreateFrickForUser(selector.userData, selector.currency) : this.virtualIbanService.createForUser(selector.userData, selector.currency) - ).catch(() => null); + ).catch((e) => this.infrastructureFailureOrRethrow(e)); } if (virtualIban?.bank.receive) { @@ -592,6 +592,17 @@ export class BuyService { }; } + // Personal-IBAN issuance can fail two ways that must be told apart before the collection-account + // fallback applies. An infrastructure failure (the provider is down) is exactly what the fallback + // exists for, so it degrades to null. A business rejection is a BadRequestException - above all the + // authoritative KYC_REQUIRED that issuance raises under its own lock against the freshly loaded, + // merge-resolved owner - and must propagate: swallowing it would let a stale KYC-50 snapshot on + // `selector.userData` pass the fallback's own KYC check and hand a rejected customer a usable account. + private infrastructureFailureOrRethrow(error: unknown): null { + if (error instanceof BadRequestException) throw error; + return null; + } + // Fallback when a personal IBAN could not be issued for a bank transfer (e.g. the provider is down). // Three reasons are told apart, because sending a customer after the wrong one wastes their time: no // provider covers the currency at all; the customer has not reached KYC 50; or issuance failed for diff --git a/src/subdomains/supporting/pricing/services/__tests__/asset-prices-job.service.spec.ts b/src/subdomains/supporting/pricing/services/__tests__/asset-prices-job.service.spec.ts index b71b9a6e0e..29757157b0 100644 --- a/src/subdomains/supporting/pricing/services/__tests__/asset-prices-job.service.spec.ts +++ b/src/subdomains/supporting/pricing/services/__tests__/asset-prices-job.service.spec.ts @@ -54,7 +54,7 @@ describe('AssetPricesJobService', () => { await service.updatePrices(); - expect(assetService.updatePrices).toHaveBeenCalledWith([ + expect(assetService.updateAssets).toHaveBeenCalledWith([ [264, { approxPriceUsd: 0.0075, approxPriceChf: 0.0061, approxPriceEur: 0.0064 }], ]); }); @@ -72,7 +72,7 @@ describe('AssetPricesJobService', () => { await service.updatePrices(); - expect(assetService.updatePrices).toHaveBeenCalledWith([]); + expect(assetService.updateAssets).toHaveBeenCalledWith([]); expect(asset.approxPriceChf).toBe(0.0061); expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Arbitrum/TGT')); }); @@ -93,7 +93,7 @@ describe('AssetPricesJobService', () => { await service.updatePrices(); - expect(assetService.updatePrices).toHaveBeenCalledWith([ + expect(assetService.updateAssets).toHaveBeenCalledWith([ [265, { approxPriceUsd: 1.07, approxPriceChf: 0.86, approxPriceEur: 0.92 }], ]); }); diff --git a/src/subdomains/supporting/pricing/services/asset-prices-job.service.ts b/src/subdomains/supporting/pricing/services/asset-prices-job.service.ts index addfbfe69a..c6ca2ed6b6 100644 --- a/src/subdomains/supporting/pricing/services/asset-prices-job.service.ts +++ b/src/subdomains/supporting/pricing/services/asset-prices-job.service.ts @@ -56,7 +56,7 @@ export class AssetPricesJobService { } // update DB - await this.assetService.updatePrices(updates); + await this.assetService.updateAssets(updates); } @DfxCron(CronExpression.EVERY_5_MINUTES, { process: Process.PRICING, timeout: 3600 })