From 1c00f852fb0ef1d52fcd21e8c71b7837578234c9 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sat, 1 Aug 2026 18:00:45 +0200 Subject: [PATCH 1/5] chore(ci): pin the files that reached full coverage (#4579) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(ci): pin the files that reached full coverage The ratchet run reports files that reach 100 percent on all four metrics but are not guarded yet, so a later change can erode them without turning the gate red. Fourteen had accumulated: ten carrying logic, four declarative. Only the fiat controller comes from recent work on the specification cache; the other thirteen were completed by other pull requests and reported since. * docs(ci): re-derive the pinned counts in the coverage gate document The document quoted the pinned count in five places and split it into logic-carrying and declarative files. Those numbers had drifted before this change already: the document said 422 while the config held 425. They are now counted from the config rather than incremented. The measured class counts under "Current state" stay as they are — they name the commit they were measured at, and deriving a new Partial figure by subtraction would invent a number nobody measured. That table's Complete cell now says so explicitly. --- docs/coverage-gate.md | 14 +++++++------- jest.coverage-gate.config.js | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 7 deletions(-) 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', From fd48f8a7ed6b6e14d06f0afc1f6d156c1f863b1c Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sat, 1 Aug 2026 18:51:19 +0200 Subject: [PATCH 2/5] Fall back to the collection account only on infrastructure failures (#4582) * Fall back to the collection account only on infrastructure failures The collection-account fallback swallowed every issuance error with `.catch(() => null)`, including the authoritative KYC rejection that issuance raises under its own lock against the freshly loaded, merge-resolved owner. The fallback then re-checked only the request's own, possibly stale KYC-50 snapshot, so a customer the locked check had just rejected could still receive a usable collection account with a reference. Only degrade to the fallback on an infrastructure failure; re-throw business rejections (BadRequestException, above all KYC_REQUIRED) so they reach the caller. Applies to both the explicit and the implicit Frick issuance paths. A test covers the discrepancy case. * Test the explicit path and reorder the helper Cover the explicit personal-IBAN path with the same business-rejection test the implicit path already has: a KYC_REQUIRED raised by issuance for a level-50 request must reach the caller and never reach the collection-account fallback. Move the infrastructure-vs-business helper above the fallback method so each comment sits directly over the method it describes. --- .../routes/buy/__tests__/buy.service.spec.ts | 48 +++++++++++++++++++ .../core/buy-crypto/routes/buy/buy.service.ts | 15 +++++- 2 files changed, 61 insertions(+), 2 deletions(-) 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 From f4d0ebc50b43ac76921a6f7496946437f437affd Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sat, 1 Aug 2026 19:01:29 +0200 Subject: [PATCH 3/5] fix(asset): write decimals through the service that owns the cache (#4584) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(asset): write decimals through the service that owns the cache The hourly decimals job wrote through the repository instance built by RepositoryFactory, while every reader goes through AssetService and its own instance. invalidateCache() only clears the instance it is called on, so the freshly written decimals stayed invisible to readers until the entry expired. The write now goes through AssetService, which invalidates the cache the readers are served from. updatePrices did exactly that already and was the only caller of its kind, so it is renamed to updateAssets and shared instead of copied. The job also collects its updates and writes once: it runs hourly and usually finds nothing, so invalidating unconditionally would drop the whole asset cache every hour for no reason. * fix(asset): invalidate the cache even when an update fails Batching the writes had given up the failure isolation the per-asset loop used to have: if one update threw, invalidateCache() was never reached and the rows written before it stayed out of the cache — the exact staleness this change exists to remove. The invalidation now sits in a finally block, so it also runs when an update fails. The empty case is guarded in the service instead of at the call site, which makes it hold for every caller: the price job passed an empty list every five minutes and dropped the whole asset cache for nothing. * fix(asset): attempt every update instead of stopping at the first failure The finally block covered the cache but not the loop: the first failing update still aborted the batch, so every asset queued behind it was skipped. Before this change each asset was written on its own, and a row that keeps failing was harmless — as a batch it would block all following assets on every run. Every update is attempted now, failures are collected, the cache is invalidated either way, and the collected ids are reported afterwards. * fix(asset): keep the original errors when several updates fail Collecting the failures had reduced them to text: the caller lost the type, the stack and the code of the database error, and every cause after the first was dropped. Whoever reads the log needs the error itself. AggregateError carries all of them and names the affected ids in its message. No dedicated error class with a failedIds field: neither caller inspects the error, both hand it to the cron, so that would be structure without a consumer. --- .../__tests__/evm-decimals.service.spec.ts | 76 ++++++++++++++++++ .../shared/evm/evm-decimals.service.ts | 21 +++-- .../__tests__/asset.service.update.spec.ts | 80 +++++++++++++++++++ src/shared/models/asset/asset.service.ts | 35 +++++++- .../asset-prices-job.service.spec.ts | 6 +- .../services/asset-prices-job.service.ts | 2 +- 6 files changed, 201 insertions(+), 19 deletions(-) create mode 100644 src/integration/blockchain/shared/evm/__tests__/evm-decimals.service.spec.ts create mode 100644 src/shared/models/asset/__tests__/asset.service.update.spec.ts 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/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/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 }) From a00072df4631b8c9b209391b81c9251f52373515 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sat, 1 Aug 2026 20:02:13 +0200 Subject: [PATCH 4/5] Bound the masking work on a log line, and stop the two passes from eating each other (#4571) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Bound the masking work, and stop the passes from eating each other Four findings against what was just merged, all in the same log line. The reason was masked whole before it was capped, twice over since the masking became two passes. An exception message can be as large as the body it interpolated a value from: 1 MiB held the thread for 663 ms, 20 MiB for 14.5 s. Only the front is masked now, wide enough past the cap that a pattern starting inside what stays visible is still seen whole - and a pattern length is dropped from the end of what came back, because a pattern crossing the point where the masking stopped arrives halved and its head would otherwise be what the shortening pulls into view. The tail goes after the masking, not before it; before it, the head moves into view with everything else. The first of the two passes wrote the placeholders the second one reads. `***` is a valid local part as far as the address pattern is concerned, so `192.0.2.123@error.code` collapsed to `***` and took the diagnostic text with it. The first pass writes markers the patterns cannot match instead, and they are turned into the placeholders once both passes are done. `format` collapsed the serialized section but never masked it, and `redact` walks the values rather than the keys - so a body key that is an address reached the log as one. The section goes through the same rendering as any other value now. The status was read twice and could answer differently, which put a body under a status it does not name; it is read once. A status in the accepted range does not have to have a name, so the message falls back past it. * Cover the plain key too, not only the one with a character in it A body key reached the trace whether or not anything was placed inside it - the serialization was never masked at all. The test said so only for the harder case. * Close what a request could still forge, decode, or lose Six findings across both reviews of this branch, four of them in what it added. A request could send the markers the first masking pass writes. They survived both passes untouched and were turned into `***` and `0x…` on the far side, a redaction that never redacted anything. Each marker is now grown until it does not occur in the value it is used on. The body an exception carries names a status of its own, and that was never compared to the one being sent: `new HttpException({ statusCode: 418 }, 400)` shipped `{"statusCode":418}` under HTTP 400. The body is kept only when it either names the status being sent or names none. Masking the whole serialized section masked the keys but masked the values twice, and the second time took their surroundings with them - a wallet address in front of a domain became the whole string. The keys are rendered where the values already are, in `redact`, and the section itself only needs collapsing again. The tail dropped from a shortened reason was dropped even where it could not have reached the visible part. It only goes where the shortening could bring it into view. Two more are older than this branch and unchanged by it, but they are in the same lines and cheap to close. An address stayed readable whenever a letter or an underscore stood next to it, because the boundary was against word characters rather than digits - `ip192.0.2.123` is how one usually arrives. And a request target carries its values percent-encoded, so `victim%40example.com` was an address to everyone reading the line and to no pattern matching it; the segments are decoded before they are masked. * Take the second masking pass back out The pass was added because removing what breaks a line can join two halves and hide a pattern from a masking that runs after it. Closing that needed markers the patterns could not match, and the markers needed to be chosen per value so a request could not send them - which grew quadratically on a value full of them, and could still be forged with a character that only disappears later. The threat it defends against is a request hiding what it sent about itself. That is not what this masking is for: it is here so a value nobody meant to log does not end up in one, and it is best effort by construction. What a request must not be able to do is put a second line in the log, and the removal alone does that. So the machinery goes and the limit is written down instead: a wallet address with a character inside it comes out as the longer hex run it becomes, which is left alone on purpose because that is what a transaction hash looks like. Two keys can render the same once they are masked. The second one kept the entry and the first one disappeared; it gets its own now. * Give an object key the guard a value already had Masking the keys reopened the thing this branch is about. A value longer than is reported by length instead of masked, because the masking is regex work over whatever it is handed; a key had no such guard and a request chooses one as freely and as long as the body allows. A 20 MB key held the thread for 20 seconds - the same shape as the reason, in the one place that had just been given more to do. It goes through the guard now, and is capped like the name it is. The comment about the serialized section still said the keys were not reached. * Count collisions instead of searching for them, and keep the path as it came Renaming a key that rendered the same as an earlier one searched the object it was building, once per candidate suffix: a thousand keys that all mask to the same thing cost a hundred milliseconds and wrote past the section budget. They are counted now, and what the rename adds is charged to the budget like anything else. The object has no prototype either, so `constructor` and `toString` are keys like any other rather than collisions with something it never held. Decoding a path segment is how a pattern is recognized in it, not how the path is rendered. `%2F` became a separator and `%3F` the start of a query, and the line named a route that was never called. The decoded form is only what gets rendered when something in it was masked. A body carrying an accessor for `statusCode` is read again when it is serialized and can answer differently then; only a plain value counts as naming a status. The failure to send a response is reported inside the same guard as the rest of the line, since it is as much a description of what happened and as unable to travel back to the caller. The previous commit's message lost the name `MAX_STRING` to a shell expansion: the sentence should read that a value longer than `MAX_STRING` is reported by length instead of masked. * Render the section instead of its keys, and settle what is sent Masking the keys where they are walked needed a length guard, then a collision counter, then budget accounting for what the counter added - three rounds, and the last one still let a real key named like a generated one take another entry. The section is cut to its budget and rendered whole instead, which reaches the keys in two lines and in an affordable size. A value is masked a second time that way and the second time can take its surroundings with it; that is the direction to be wrong in. A path segment is read decoded and rendered as it arrived, unless something in it was masked - and if the decoding would have put a separator or a query into the path, nothing of it is rendered at all, because the line would otherwise name a route with a segment more than the one that was called. Each escape is decoded on its own, so one that is not a valid sequence no longer costs the whole segment. The target itself is capped before any of that: reading it is regex work like everything else here. The body an exception carries is settled once before it is checked, so what is sent is the thing that was checked - a body that answers differently the second time it is asked cannot put a status in the response that the status line does not have. The error from a failed send is kept as what it is rather than asserted to be an `Error`, and one that is falsy is still an error. * Put back what develop already had, and stop widening the masking Two of these are corrections of this branch rather than of what it set out to fix. The masking on develop runs before and after the removal. This branch had taken the second pass out on the grounds that it defended against a request hiding its own data - which is true of one direction and not of the other: removing a character also joins what stood on either side, and a wallet address followed by one more hex character stops being one. Against develop that was a weakening, not a simplification. It is back as develop has it, and what it costs - the second pass folding `***` into an address of its own - is written down and tested. Masking the object keys is out too, in every form it was tried: in `redact` with a length guard, then a collision counter, then budget accounting for the counter; and over the serialized section, which masks each value a second time. Each round of review found the next thing wrong with it. A body key that is an address still reaches the trace, exactly as it does today, and what to do about that is a question about what this trace is for - the same question the encoded values in a path raise, and the same one raised by tracing bodies that carry free customer text at all. It does not belong in a change about bounding work. The response body is checked as it is rather than settled first: making a copy of it before checking added a full serialization of a body that has no bound, which is the thing this branch is about. What is left is what it set out to be: the reason is masked only as far as it is shown, the request target is capped after being read rather than before, the status is read once and the body has to name it, a failure to log cannot reach the caller, and an address stays an address when a letter stands next to it. * Take the replacement message out of the body, not out of the exception Dropping a body that names another status put `exception.message` on the wire in its place. That message is what an exception says to us; the body is what its thrower wrote for the caller, and the two can differ - a thrower can set an internal one on the exception and a public one in the body. Against develop and against the framework's own filter, this branch was the only thing that would have sent the first. The replacement message comes out of the body now, as a plain string or a plain array of them, and out of the status when the body has none. Six comments and tests were behind the code. The two masking docstrings had the passes the wrong way round or missing; the guard around the logging said the response was already out, which is not true of the path where sending it failed; and four properties had no test that would fail without them - the status being read once, the accessor checks on the body, the cap on the request target coming after the masking, and a failure to report a failed send staying inside the guard. * Send only what the body would have sent itself A property that is not enumerable is left out when the response is serialized, so reading one to build the replacement message put something on the wire that would never otherwise have been on it. Both the message and the status are now read the way the serialization would read them: the body's own property, enumerable, and a value rather than something asked again. The timing bound in the reason test is looser. What it shows is the bound - the same message costs seconds without it - and a limit that tight only measured how busy the machine was. * Give a plain error the same footing as a body Taking the message helper out left the branch for everything that is not an HttpException reading the exception directly and unguarded. An error that says nothing about itself put an empty message on the wire in place of the name of what is being sent, and one whose message cannot be read at all took the response with it - the thing this branch exists to prevent. It goes through a reading that can fail and falls back to the status, like every other message here. * Read a body only where reading it says anything A body that answers toJSON is serialized from what that returns, not from what it holds. Judging it by its own properties therefore judged something that was never going to be sent - and worse, a body holding an internal message and serializing a public one was dropped for naming the wrong status and then rebuilt out of the internal one. That is more than develop sends, which is the one thing this branch must not be. Such a body is not read at all now: neither for the status it names nor for the message it carries, and what goes out in its place is the name of the status being sent. An exception message that is not text is not a message either. The other path already checked; this one now does the same. * Judge a body only where judging it means anything Five ways the replacement body read something the wire would never have carried, or replaced one the wire would have carried unchanged. A status that had to be replaced now takes the body with it. What a thrower wrote belongs to the status it wrote it for; reading its message for a different status reads it for something it was not, and the message it wrote for the caller and the one it kept for itself are not always the same. The generic body goes out instead. A body that serializes itself is passed on rather than replaced. What it holds says nothing about what it sends, so there is nothing here to judge - and building a replacement out of what it holds was how the one thing this branch must not do happened again. `JSON.stringify` leaves out more than an accessor: a function, a symbol, and any name put on an array alongside its elements. None of those name a status, so a body carrying one is no longer replaced for contradicting a status it never carried. A sparse array is sent with its holes as `null`, which is not an array of strings. `every` skips the holes; the check no longer does. And asking what an exception is can throw - a proxy is enough - which left the caller without a response at all. That question is inside the guard now. * Read a body once, and send what was read A status carried by an accessor is serialized, so the body does carry one - and it answers again when it is, so what it would carry cannot be read here. That is not agreement, and such a body is replaced rather than passed on. What the serialization leaves out entirely - a name that is not enumerable, a function, a symbol, a name put on an array alongside its elements - carries nothing and is left alone, which is what the round before this one asked for. `toJSON` is asked for as a descriptor rather than read. Reading it would be one read and the serialization another, and something that answers differently between them would be sent as whatever it holds. A message array is copied out rather than passed on. Checking it with `Array.from` ran whatever iterator it brought along, and what that runs can leave different elements behind than the ones that were checked; `JSON.stringify` reads an array by index, so that is how it is read here. Deliberately not done: confirming the status a second time before the body is passed on. It would close the case of a `getStatus` that answers differently on the second call - but reading the status twice was itself a finding two rounds ago, and what it defends against is an exception this code base builds against itself. * Take nothing out of a body that could not be passed on Four rounds of review have all found the same shape: a field read off a body was not the field the serialization would have sent. A message array with a `toJSON` of its own, a `statusCode` that answers when asked, a hole in an array, a name a prototype supplies. Each one was closed and the next one appeared, because the premise was wrong - a body that cannot be passed on is a body whose contents say nothing about what it would have sent, and reading it at all is reading something else. So it is not read. What goes out in its place is the status being sent and the name of that status, and the reading of messages, arrays and descriptors that served no other purpose goes with it - thirty-nine lines of it. A body agrees with the status it is sent under unless it carries one of its own that differs, and a value of `undefined` is not carried at all: the serialization leaves it out like a function, a symbol, or a name put on an array. What was thrown while sending a response is only turned into text if it already is text. Anything else could hold what the body held, and the line that reports a failed send is not the place to find that out. * Name the question the way it is answered The check returns true when the body agrees with the status being sent and also when it says nothing about one, which is not what `names` reads as at the call site. * Say what the code does with what was thrown Only a thrown `Error` is reported as what it is; a thrown string is replaced by a fixed one like anything else. The comment said the opposite, and the fixed one started lower case. * Trust a toJSON that is simply there, not one that might answer An accessor might answer with a function and might not. Taking it for one let a body past the check that the status it names is the status being sent, and the serialization then ignored what the accessor returned and sent what the body holds - a status in the body that the status line does not have. Only a function that is simply there serializes a body; a body that only might is judged like any other. Names: the effective status is called that rather than `sent`, which read like the response had already gone out and collided with the flag that says whether it had. The methods are verbs. * Drop two tests that describe a mechanism there no longer is Both said they were about what is read out of a message, and both used a body whose status already differed from the one being sent - so the status decided and the message never came into it. There is no path left that reads a message out of a body at all: what cannot be passed on is replaced whole. What they were meant to show is covered by the test that says exactly that. * Use the nullish default the repository asks for Five fallbacks fell back on falsiness where the name of a status is either there or not there at all. * Give the two questions the prefix the repository asks of them Both answer yes or no, and the rule for that is the same for both. --- .../__tests__/exception.filter.spec.ts | 225 +++++++++++++++--- src/shared/filters/exception.filter.ts | 160 +++++++++++-- .../__tests__/api-trace.middleware.spec.ts | 53 ++++- .../middlewares/api-trace.middleware.ts | 40 ++-- 4 files changed, 404 insertions(+), 74 deletions(-) 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; } From 414d5248725654e75c2be9ced89993bec824579b Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Sat, 1 Aug 2026 20:59:56 +0200 Subject: [PATCH 5/5] 019fbdf5 - Fix Bank Frick vIBAN GET requests (#4585) * fix: omit Content-Type from Frick vIBAN GET requests * fix: keep Frick transport comment parseable --- docs/bank-frick-operations.md | 9 ++++ .../services/__tests__/frick.service.spec.ts | 16 +++++-- .../bank/services/frick.service.ts | 46 +++++++++++++++---- 3 files changed, 60 insertions(+), 11 deletions(-) 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/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, ); }