From dd59d8a78b0ed5f29c33c0161d1afb0876d41010 Mon Sep 17 00:00:00 2001 From: David May <85513542+davidleomay@users.noreply.github.com> Date: Fri, 31 Jul 2026 21:47:47 +0200 Subject: [PATCH] fix: two production 500s on the invoice and payment-link paths (#4544) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(transaction): load statement relations as separate queries PUT /v1/transaction/:id/invoice and /receipt returned 500 for every request: QueryFailedError "target lists can have at most 1664 entries". getTxStatementDetails' relation tree resolves to 61 joined nodes. TypeORM 0.3 expands eager relations recursively from the root entity and one level deep for each explicitly requested relation, so Asset ends up joined 10 times and Country 9 times — 537 columns between them. Measured against real entity metadata the tree selects exactly 1664 columns, precisely Postgres' MaxTupleAttributeNumber, which means a single new @Column anywhere in it overflows. settlementEventId on TransactionRequest (#4469) was that column: TransactionRequest appears once in the tree, taking it to 1665 and failing deterministically. Pass relationLoadStrategy 'query' so each relation is fetched by its own query instead of one 60-way join: 1664 columns/60 joins becomes 98 columns/2 joins. Eager relations are preserved — under this strategy each relation becomes the main alias of its own query, where eager expansion is recursive, so sub-queries load strictly more than the join did (verified per branch against the metadata). Also prune three branches the statement path never dereferences (cryptoRoute, buyFiat.sell, refReward.user) and drop the explicit userData.organization, which is already eager and so cost a redundant round-trip under the new strategy. Under 'query' an unread branch costs a dependent query rather than just join columns; pruning also leaves the tree at 1219 columns as a plain join, so the fix no longer depends on the strategy alone. * fix(payment-link): reject non-integer ids before they reach Postgres GET /v1/paymentLink/payment, /v1/plp and /v1/paymentLink/recipient returned 500 with QueryFailedError "invalid input syntax for type integer" for NaN, Infinity, 1.9 and 1e+21. All three are reachable without authentication, and the filter echoes the raw Postgres message — including the parameter — back to the caller. The cause is coercing a request string with !isNaN(+x) or Number.isInteger(+x) and handing the result to a query. Neither is sufficient: +'Infinity' is Infinity, Number.isInteger(1e21) is true, and +'abc' is NaN, all of which pg serialises straight into an int4 comparison. Add Util.isDbId — digits only, within 1..2147483647 — as the single predicate, and apply it where each value is actually consumed: - deposit-route.service.getPaymentRoute: a non-id is a route label, so it now goes to the label lookup instead of the id branch (/paymentLink/recipient). - payment-link.service.createInvoice: validated at the point of use rather than on the DTO, because `route` wins over `routeId` and the `r` alias is resolved after validation — so a junk routeId is only an error when it is the value actually used (/paymentLink/payment, /plp). - payment-link.controller.parseLinkId: linkId is one of several optional lookup keys, so a malformed value is dropped and the request still resolves via externalLinkId/externalPaymentId/key. That is what the falsy NaN did implicitly; the point is only to keep the value out of SQL. - generateOcpStickers: the ids list is the primary selector, so it still 400s, but on the correct predicate — Number.isInteger accepted 1e+21 and 2147483648 and 500'd on an endpoint anonymous callers reach with no route knowledge. - transaction.controller: the same coercion on :id/invoice and :id/receipt. Transaction UIDs are ^T[A-Za-z0-9]{16}$, so a digit-only split is unambiguous. Also closes the 1e+21 hole in the existing Number.isInteger guards, which turned the error into a 22003 overflow rather than fixing it. Relocate PG_INTEGER_MAX to shared and point the two custody copies of this predicate at Util.isDbId, so the bound is expressed once rather than in three places kept in step by a comment. * fix(payment-link): require a usable identifier before assigning a link Round-2 review of this branch found that the previous commit introduced a hole in PUT /v1/paymentLink/assign, which has no auth guard. assignPaymentLink builds `where: { id, externalId, status: UNASSIGNED }` with no dispatch and no user scoping. Dropping a malformed linkId to undefined — correct for the other call sites, where linkId is one of several lookup keys — left both identifiers absent, and TypeORM omits undefined keys, so the query degenerated to `WHERE status = 'Unassigned'` and matched an arbitrary merchant's unassigned link, which was then re-pointed at the caller's route and returned to them. `?linkId=abc` reached that state because the presence check tested the raw query string rather than the parsed id. The check the previous commit removed had blocked it by accident. Guard in the service, where the dangerous query lives, so no caller can reach it, and check the parsed id in the controller so the endpoint answers 400. Also replace Util.isDbId with Util.toDbId, which returns the parsed id instead of a boolean. Callers previously re-derived the value with `+x` after the check, and whitespace was handled inconsistently — parseLinkId and the sticker id list trimmed, getPaymentRoute and the transaction routes did not, so `?id=+42` (a query string decodes `+` to a space) resolved on one endpoint and 404'd on another. Trimming now happens once, inside the parse, and there is no boolean form to pair with a laxer coercion. Adds coverage for the three behaviours the review found unpinned: the assign guard, the createInvoice routeId guard, and the pruned relation tree, which was asserted only as expect.any(Object) so any branch could have reappeared. * test: pin the four behaviours mutation testing found unguarded Round-3 review mutation-tested every production change on this branch: 11 of 15 failed loudly when reverted, 4 did not. - TransactionService.getTransactionById/ByUid forwarding relationLoadStrategy to the repository. The helper spec asserted the argument reaching the service, so the service dropping it on the floor would not have failed anything — and that forwarding is the whole fix for the 1664-column overflow. - createInvoicePayment classifying the `r` alias. Reverting it to !isNaN(+r) sent 'Infinity' and '1e+21' back down the routeId branch with nothing failing. - The id and order-id guards in getTransaction, which the earlier commit rewrote from Number.isInteger to Util.toDbId with no test on either. No production code changes. --- src/shared/utils/__tests__/util.spec.ts | 44 +++++ src/shared/utils/util.ts | 29 +++ .../controllers/custody-account.controller.ts | 18 +- .../guards/custody-account-access.guard.ts | 16 +- .../services/custody-account.service.ts | 4 +- .../__tests__/transaction-helper.spec.ts | 45 +++++ .../__tests__/transaction.controller.spec.ts | 79 ++++++++ .../controllers/transaction.controller.ts | 12 +- .../__tests__/payment-link.controller.spec.ts | 170 ++++++++++++++++++ .../controllers/payment-link.controller.ts | 35 ++-- .../__tests__/payment-link-id-guards.spec.ts | 114 ++++++++++++ .../services/payment-link.service.ts | 13 +- .../__tests__/deposit-route.service.spec.ts | 56 ++++++ .../route/deposit-route.service.ts | 8 +- .../__tests__/transaction.service.spec.ts | 47 +++++ .../payment/services/transaction-helper.ts | 20 ++- .../payment/services/transaction.service.ts | 30 +++- 17 files changed, 674 insertions(+), 66 deletions(-) create mode 100644 src/subdomains/core/payment-link/controllers/__tests__/payment-link.controller.spec.ts create mode 100644 src/subdomains/core/payment-link/services/__tests__/payment-link-id-guards.spec.ts create mode 100644 src/subdomains/supporting/address-pool/route/__tests__/deposit-route.service.spec.ts diff --git a/src/shared/utils/__tests__/util.spec.ts b/src/shared/utils/__tests__/util.spec.ts index 8c3e9a21c1..9b5a647296 100644 --- a/src/shared/utils/__tests__/util.spec.ts +++ b/src/shared/utils/__tests__/util.spec.ts @@ -95,3 +95,47 @@ describe('sanitizeLogValue', () => { expect(Util.sanitizeLogValue('a=b', 64)).toBe('a?b'); }); }); + +describe('toDbId', () => { + // Regression: request params were coerced with `!isNaN(+x)` / `Number.isInteger(+x)`, both of which + // accept values Postgres rejects as an integer. Reaching SQL, they surfaced as 500s on endpoints + // anonymous callers can reach (GET /v1/paymentLink/payment, /v1/plp, /v1/paymentLink/recipient). + it.each(['Infinity', '-Infinity', '1.9', '1e+21', '1E5', '0x10', '0b11', '-1', 'abc', '', ' '])( + 'rejects %j', + (value) => { + expect(Util.toDbId(value)).toBeUndefined(); + }, + ); + + it('rejects NaN, which the old isNaN(+x) coercion also caught', () => { + expect(Util.toDbId('NaN')).toBeUndefined(); + }); + + it('rejects 0, since SERIAL ids start at 1', () => { + expect(Util.toDbId('0')).toBeUndefined(); + }); + + it('rejects ids beyond the Postgres INTEGER range', () => { + expect(Util.toDbId('2147483647')).toBe(2147483647); + expect(Util.toDbId('2147483648')).toBeUndefined(); + expect(Util.toDbId('9'.repeat(309))).toBeUndefined(); + }); + + // Query params are not guaranteed to be strings: `?id=1&id=2` arrives as an array, which + // `RegExp.test` would coerce to a passing value. + it.each([[['12']], [12], [null], [undefined], [{}]])('rejects the non-string %p', (value) => { + expect(Util.toDbId(value)).toBeUndefined(); + }); + + it('returns the parsed id for a plain positive integer', () => { + expect(Util.toDbId('1')).toBe(1); + expect(Util.toDbId('42')).toBe(42); + expect(Util.toDbId('007')).toBe(7); + }); + + // A query string decodes `+` to a space, so `?id=+42` arrives padded and resolved before this + // existed. Trimming lives here so every call site treats padding the same way. + it.each([' 42', '42 ', ' 42 ', '\t42\n'])('tolerates the surrounding whitespace in %j', (value) => { + expect(Util.toDbId(value)).toBe(42); + }); +}); diff --git a/src/shared/utils/util.ts b/src/shared/utils/util.ts index 72f98c8c92..c74fac82e8 100644 --- a/src/shared/utils/util.ts +++ b/src/shared/utils/util.ts @@ -15,6 +15,9 @@ export type KeyType = { type CryptoAlgorithm = 'md5' | 'sha256' | 'sha384' | 'sha512'; +/** Postgres INTEGER / SERIAL upper bound (positive ids only). */ +export const PG_INTEGER_MAX = 2_147_483_647; + export enum AmountType { ASSET = 'Asset', FIAT = 'Fiat', @@ -513,6 +516,32 @@ export class Util { return ILike(`%${search}%`); } + /** + * Parses a request-supplied value into a usable Postgres INTEGER/SERIAL id, or `undefined` if it + * is not one. + * + * Deliberately stricter than `+value` / `Number.isInteger(+value)`: both of those accept values JS + * coerces to a finite-looking number but Postgres rejects, which surfaces as a 500 rather than a + * 400. Notably `+'Infinity'` is `Infinity` and `Number.isInteger(+'1e+21')` is `true`. Surrounding + * whitespace is tolerated because a query string decodes `+` to a space, so `?id=+42` resolved + * before this existed. + * + * Returns the id rather than a boolean so callers cannot re-derive it with a laxer coercion, and + * takes `unknown` because query params are not guaranteed to be strings at runtime (`?id=1&id=2` + * arrives as an array, which `RegExp.test` would coerce to something that passes). + */ + static toDbId(value: unknown): number | undefined { + if (typeof value !== 'string') return undefined; + + const trimmed = value.trim(); + if (!/^\d+$/.test(trimmed)) return undefined; + + // The digit-only test above already rules out anything non-integral or negative, so the range is + // the only remaining constraint — a value above it is also beyond Number.MAX_SAFE_INTEGER. + const id = Number(trimmed); + return id >= 1 && id <= PG_INTEGER_MAX ? id : undefined; + } + // --- MISC --- // static async readFileFromDisk(fileName: string): Promise { diff --git a/src/subdomains/core/custody/controllers/custody-account.controller.ts b/src/subdomains/core/custody/controllers/custody-account.controller.ts index 73723cddef..310ba1edd5 100644 --- a/src/subdomains/core/custody/controllers/custody-account.controller.ts +++ b/src/subdomains/core/custody/controllers/custody-account.controller.ts @@ -18,6 +18,7 @@ import { JwtPayload } from 'src/shared/auth/jwt-payload.interface'; import { RoleGuard } from 'src/shared/auth/role.guard'; import { UserActiveGuard } from 'src/shared/auth/user-active.guard'; import { UserRole } from 'src/shared/auth/user-role.enum'; +import { Util } from 'src/shared/utils/util'; import { PdfDto } from 'src/subdomains/core/buy-crypto/routes/buy/dto/pdf.dto'; import { CreateCustodyAccountAccessDto } from '../dto/input/create-custody-account-access.dto'; import { CreateCustodyAccountDto } from '../dto/input/create-custody-account.dto'; @@ -30,12 +31,7 @@ import { CustodyOrderHistoryDto } from '../dto/output/custody-order-history.dto' import { CustodyAccessLevel } from '../enums/custody'; import { CustodyAccountReadGuard, CustodyAccountWriteGuard } from '../guards/custody-account-access.guard'; import { CustodyAccountDtoMapper } from '../mappers/custody-account-dto.mapper'; -import { - CustodyAccountId, - CustodyAccountService, - LegacyAccountId, - PG_INTEGER_MAX, -} from '../services/custody-account.service'; +import { CustodyAccountId, CustodyAccountService, LegacyAccountId } from '../services/custody-account.service'; import { CustodyOrderService } from '../services/custody-order.service'; import { CustodyPdfService } from '../services/custody-pdf.service'; import { CustodyService } from '../services/custody.service'; @@ -253,15 +249,11 @@ export class CustodyAccountController { * Rejects non-digits, Infinity (e.g. 309 nines), zero, and values outside column range → 400. */ private parsePositiveIntParam(value: string, name: string): number { - if (!/^\d+$/.test(value)) { - throw new BadRequestException(`Invalid ${name}`); - } - - const n = Number(value); - if (!Number.isSafeInteger(n) || n < 1 || n > PG_INTEGER_MAX) { + const id = Util.toDbId(value); + if (!id) { throw new BadRequestException(`Invalid ${name}`); } - return n; + return id; } } diff --git a/src/subdomains/core/custody/guards/custody-account-access.guard.ts b/src/subdomains/core/custody/guards/custody-account-access.guard.ts index aa76ea5848..e07003dd1d 100644 --- a/src/subdomains/core/custody/guards/custody-account-access.guard.ts +++ b/src/subdomains/core/custody/guards/custody-account-access.guard.ts @@ -1,11 +1,7 @@ import { CanActivate, ExecutionContext, ForbiddenException, HttpException, Injectable } from '@nestjs/common'; +import { Util } from 'src/shared/utils/util'; import { CustodyAccessLevel } from '../enums/custody'; -import { - CustodyAccountId, - CustodyAccountService, - LegacyAccountId, - PG_INTEGER_MAX, -} from '../services/custody-account.service'; +import { CustodyAccountId, CustodyAccountService, LegacyAccountId } from '../services/custody-account.service'; abstract class CustodyAccountAccessGuard implements CanActivate { protected abstract readonly requiredLevel: CustodyAccessLevel; @@ -42,12 +38,8 @@ abstract class CustodyAccountAccessGuard implements CanActivate { // Same constraints as the controller: digits only, finite safe positive int in SERIAL range. // (Guards map validation failures to 403; the controller answers 400 on grant routes.) - if (!/^\d+$/.test(id)) { - throw new ForbiddenException('Invalid custody account ID'); - } - - const parsed = Number(id); - if (!Number.isSafeInteger(parsed) || parsed < 1 || parsed > PG_INTEGER_MAX) { + const parsed = Util.toDbId(id); + if (!parsed) { throw new ForbiddenException('Invalid custody account ID'); } diff --git a/src/subdomains/core/custody/services/custody-account.service.ts b/src/subdomains/core/custody/services/custody-account.service.ts index e4a3da3f48..8126f4a6ed 100644 --- a/src/subdomains/core/custody/services/custody-account.service.ts +++ b/src/subdomains/core/custody/services/custody-account.service.ts @@ -6,6 +6,7 @@ import { NotFoundException, } from '@nestjs/common'; import { UserRole } from 'src/shared/auth/user-role.enum'; +import { PG_INTEGER_MAX } from 'src/shared/utils/util'; import { UserData } from 'src/subdomains/generic/user/models/user-data/user-data.entity'; import { UserDataService } from 'src/subdomains/generic/user/models/user-data/user-data.service'; import { EntityManager } from 'typeorm'; @@ -21,9 +22,6 @@ import { CustodyService } from './custody.service'; export const LegacyAccountId = 'legacy'; export type CustodyAccountId = number | typeof LegacyAccountId; -/** Postgres INTEGER / SERIAL upper bound (positive ids only). */ -export const PG_INTEGER_MAX = 2_147_483_647; - /** * Owner-scoped advisory lock key for ordinary creation vs legacy materialisation. * Must stay identical everywhere — a second key scheme would re-open races. diff --git a/src/subdomains/core/history/__tests__/transaction-helper.spec.ts b/src/subdomains/core/history/__tests__/transaction-helper.spec.ts index 0ed3d5dc16..72adb8b252 100644 --- a/src/subdomains/core/history/__tests__/transaction-helper.spec.ts +++ b/src/subdomains/core/history/__tests__/transaction-helper.spec.ts @@ -428,4 +428,49 @@ describe('TransactionHelper', () => { expect(buyService.getBankInfoForRequest).not.toHaveBeenCalled(); expect(buyService.getBankInfo).not.toHaveBeenCalled(); }); + + // Regression: resolved as a single join this relation tree selects 1664 columns (61 joined nodes — + // Asset appears 10x and Country 9x, dragged in by eager relations), so one more @Column anywhere in + // it trips Postgres' "target lists can have at most 1664 entries" and 500s every invoice/receipt. + // Loading relations as separate queries keeps the statement path off that cliff for good. + it('loads statement relations as separate queries instead of one join', async () => { + const userData = createCustomUserData({ + accountType: AccountType.PERSONAL, + firstname: 'Test', + surname: 'User', + }); + const buyCrypto = createCustomBuyCrypto({ + checkoutTx: createDefaultCheckoutTx(), + inputAmount: 125, + isComplete: true, + outputAmount: 0.005, + }); + const transaction = createCustomTransaction({ + buyCrypto, + sourceType: TransactionSourceType.CHECKOUT_TX, + userData, + }); + const uid = 'T0123456789ABCDEF'; + jest.spyOn(transactionService, 'getTransactionById').mockResolvedValue(transaction); + jest.spyOn(transactionService, 'getTransactionByUid').mockResolvedValue(transaction); + jest.spyOn(fiatService, 'getFiatByName').mockResolvedValue(createCustomFiat({ name: 'EUR' })); + + await txHelper.getTxStatementDetails(userData.id, transaction.id, TxStatementType.INVOICE); + await txHelper.getTxStatementDetails(userData.id, uid, TxStatementType.INVOICE); + + // The tree is pinned, not just the strategy: pruning is only safe because organization, + // buyFiat.outputAsset and cryptoInput.asset are eager, so a branch silently reappearing here + // (or an eager flag being dropped elsewhere) should fail loudly rather than cost round-trips. + const expectedRelations = { + userData: true, + buyCrypto: { buy: { user: { wallet: true } }, cryptoInput: true }, + buyFiat: { cryptoInput: true }, + refReward: true, + bankTxReturn: true, + request: true, + }; + + expect(transactionService.getTransactionById).toHaveBeenCalledWith(transaction.id, expectedRelations, 'query'); + expect(transactionService.getTransactionByUid).toHaveBeenCalledWith(uid, expectedRelations, 'query'); + }); }); diff --git a/src/subdomains/core/history/__tests__/transaction.controller.spec.ts b/src/subdomains/core/history/__tests__/transaction.controller.spec.ts index 569178053a..77c6bc30fd 100644 --- a/src/subdomains/core/history/__tests__/transaction.controller.spec.ts +++ b/src/subdomains/core/history/__tests__/transaction.controller.spec.ts @@ -1,4 +1,5 @@ import { createMock } from '@golevelup/ts-jest'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; import { Config } from 'src/config/config'; import { JwtPayload } from 'src/shared/auth/jwt-payload.interface'; @@ -21,6 +22,10 @@ import { createCustomTransactionRequest } from 'src/subdomains/supporting/paymen import { createCustomTransaction } from 'src/subdomains/supporting/payment/__mocks__/transaction.entity.mock'; import { VirtualIbanService } from 'src/subdomains/supporting/bank/virtual-iban/virtual-iban.service'; import { FiatPaymentMethod } from 'src/subdomains/supporting/payment/dto/payment-method.enum'; +import { + TxStatementDetails, + TxStatementType, +} from 'src/subdomains/supporting/payment/dto/transaction-helper/tx-statement-details.dto'; import { SwissQRService } from 'src/subdomains/supporting/payment/services/swiss-qr.service'; import { TransactionHelper } from 'src/subdomains/supporting/payment/services/transaction-helper'; import { TransactionRequestService } from 'src/subdomains/supporting/payment/services/transaction-request.service'; @@ -204,4 +209,78 @@ describe('TransactionController', () => { expect(transactionHelper.getTxStatementDetails).not.toHaveBeenCalled(); }); }); + + describe('getSingleTransactionDetails (malformed id / order-id)', () => { + // Both guards reject rather than drop, because id and order-id are the primary selectors here + // rather than one of several optional lookup keys. + it.each(['NaN', 'Infinity', '1.9', '1e+21', '-1', '0', '2147483648', 'abc'])( + 'rejects id=%j before it reaches the lookup', + async (id) => { + await expect(controller.getSingleTransactionDetails(jwt, id)).rejects.toBeInstanceOf(BadRequestException); + + expect(transactionService.getTransactionById).not.toHaveBeenCalled(); + }, + ); + + it.each(['NaN', 'Infinity', '1e+21', '2147483648', 'abc'])( + 'rejects order-id=%j before it reaches the lookup', + async (orderId) => { + await expect(controller.getSingleTransactionDetails(jwt, undefined, undefined, orderId)).rejects.toBeInstanceOf( + BadRequestException, + ); + + expect(transactionService.getTransactionByRequestId).not.toHaveBeenCalled(); + }, + ); + + it('passes a well-formed id through as a number', async () => { + jest.spyOn(transactionService, 'getTransactionById').mockResolvedValue(undefined); + + await expect(controller.getSingleTransactionDetails(jwt, ' 42 ')).rejects.toBeInstanceOf(NotFoundException); + + expect(transactionService.getTransactionById).toHaveBeenCalledWith(42, expect.any(Object)); + }); + }); + + describe('generateInvoiceFromTransaction (malformed id)', () => { + // Regression: `isNaN(+id)` classified all of these as numeric ids, so they reached Postgres as + // integers and came back as `invalid input syntax for type integer` -> 500. None of them can be a + // real transaction id, so they must go down the UID path instead and never be coerced to a number. + const malformedIds = ['NaN', 'Infinity', '1.9', '1e+21', '-1', '99999999999', '0x10', 'abc']; + + beforeEach(() => { + Config.invoice.currencies = ['EUR', 'CHF']; + jest.spyOn(transactionRequestService, 'getTransactionRequestByUid').mockResolvedValue(undefined); + // Resolves rather than rejects on purpose: a rejecting mock would make the assertions below + // pass regardless of how the id was classified, which is the bug this pins. + jest + .spyOn(transactionHelper, 'getTxStatementDetails') + .mockResolvedValue({ currency: 'EUR' } as unknown as TxStatementDetails); + jest.spyOn(swissQrService, 'createTxStatement').mockResolvedValue('pdf-data'); + }); + + it.each(malformedIds)('passes %j through as a UID, never as a numeric id', async (id) => { + await controller.generateInvoiceFromTransaction(jwt, id); + + expect(transactionHelper.getTxStatementDetails).toHaveBeenCalledWith(jwt.account, id, TxStatementType.INVOICE); + }); + + // A query string decodes `+` to a space, so `/v1/transaction/+42/invoice` arrives padded; it + // resolved as id 42 before the guard existed and still must. + it.each(['42', ' 42 '])('still resolves the well-formed numeric id %j to a number', async (id) => { + await controller.generateInvoiceFromTransaction(jwt, id); + + expect(transactionHelper.getTxStatementDetails).toHaveBeenCalledWith(jwt.account, 42, TxStatementType.INVOICE); + }); + + it('guards the receipt endpoint the same way', async () => { + await controller.generateReceiptFromTransaction(jwt, 'Infinity'); + + expect(transactionHelper.getTxStatementDetails).toHaveBeenCalledWith( + jwt.account, + 'Infinity', + TxStatementType.RECEIPT, + ); + }); + }); }); diff --git a/src/subdomains/core/history/controllers/transaction.controller.ts b/src/subdomains/core/history/controllers/transaction.controller.ts index 9cece8b793..e1135c54fd 100644 --- a/src/subdomains/core/history/controllers/transaction.controller.ts +++ b/src/subdomains/core/history/controllers/transaction.controller.ts @@ -505,7 +505,7 @@ export class TransactionController { @UseGuards(AuthGuard(), RoleGuard(UserRole.ACCOUNT), IpGuard, UserActiveGuard()) @ApiOkResponse({ type: PdfDto }) async generateInvoiceFromTransaction(@GetJwt() jwt: JwtPayload, @Param('id') id: string): Promise { - const txIdOrUid = isNaN(+id) ? id : +id; + const txIdOrUid = Util.toDbId(id) ?? id; // For string UIDs, first try to find a TransactionRequest (for pending transactions) if (typeof txIdOrUid === 'string') { @@ -572,7 +572,7 @@ export class TransactionController { @UseGuards(AuthGuard(), RoleGuard(UserRole.ACCOUNT), IpGuard, UserActiveGuard()) @ApiOkResponse({ type: PdfDto }) async generateReceiptFromTransaction(@GetJwt() jwt: JwtPayload, @Param('id') id: string): Promise { - const txIdOrUid = isNaN(+id) ? id : +id; + const txIdOrUid = Util.toDbId(id) ?? id; const txStatementDetails = await this.transactionHelper.getTxStatementDetails( jwt.account, txIdOrUid, @@ -801,8 +801,8 @@ export class TransactionController { let tx: Transaction | TransactionRequest; if (id) { - const transactionId = +id; - if (!Number.isInteger(transactionId)) throw new BadRequestException('id must be an integer'); + const transactionId = Util.toDbId(id); + if (!transactionId) throw new BadRequestException('id must be an integer'); tx = await this.transactionService.getTransactionById(transactionId, baseRelations); } @@ -815,8 +815,8 @@ export class TransactionController { } if (orderId) { - const requestId = +orderId; - if (!Number.isInteger(requestId)) throw new BadRequestException('order-id must be an integer'); + const requestId = Util.toDbId(orderId); + if (!requestId) throw new BadRequestException('order-id must be an integer'); tx = (await this.transactionService.getTransactionByRequestId(requestId, baseRelations)) ?? (await this.transactionRequestService.getTransactionRequest(requestId, { user: { userData: true } })); diff --git a/src/subdomains/core/payment-link/controllers/__tests__/payment-link.controller.spec.ts b/src/subdomains/core/payment-link/controllers/__tests__/payment-link.controller.spec.ts new file mode 100644 index 0000000000..ffd1bdc525 --- /dev/null +++ b/src/subdomains/core/payment-link/controllers/__tests__/payment-link.controller.spec.ts @@ -0,0 +1,170 @@ +import { createMock } from '@golevelup/ts-jest'; +import { BadRequestException } from '@nestjs/common'; +import { Test, TestingModule } from '@nestjs/testing'; +import { Response } from 'express'; +import { JwtPayload } from 'src/shared/auth/jwt-payload.interface'; +import { UserRole } from 'src/shared/auth/user-role.enum'; +import { TestSharedModule } from 'src/shared/utils/test.shared.module'; +import { TestUtil } from 'src/shared/utils/test.util'; +import { UserDataService } from 'src/subdomains/generic/user/models/user-data/user-data.service'; +import { DepositRouteService } from 'src/subdomains/supporting/address-pool/route/deposit-route.service'; +import { PaymentLinkDtoMapper } from '../../dto/payment-link-dto.mapper'; +import { PaymentLinkDto } from '../../dto/payment-link.dto'; +import { PaymentLink } from '../../entities/payment-link.entity'; +import { StickerQrMode, StickerType } from '../../enums'; +import { OCPStickerService } from '../../services/ocp-sticker.service'; +import { PaymentLinkPaymentService } from '../../services/payment-link-payment.service'; +import { PaymentLinkService } from '../../services/payment-link.service'; +import { PaymentMerchantService } from '../../services/payment-merchant.service'; +import { PaymentLinkController } from '../payment-link.controller'; + +describe('PaymentLinkController', () => { + let controller: PaymentLinkController; + + let userDataService: UserDataService; + let paymentLinkService: PaymentLinkService; + let paymentLinkPaymentService: PaymentLinkPaymentService; + let depositRouteService: DepositRouteService; + let paymentLinkStickerService: OCPStickerService; + let paymentMerchantService: PaymentMerchantService; + + const jwt: JwtPayload = { role: UserRole.USER, ip: '1.1.1.1', account: 1, user: 7 } as JwtPayload; + + beforeEach(async () => { + userDataService = createMock(); + paymentLinkService = createMock(); + paymentLinkPaymentService = createMock(); + depositRouteService = createMock(); + paymentLinkStickerService = createMock(); + paymentMerchantService = createMock(); + + const module: TestingModule = await Test.createTestingModule({ + imports: [TestSharedModule], + providers: [ + PaymentLinkController, + { provide: UserDataService, useValue: userDataService }, + { provide: PaymentLinkService, useValue: paymentLinkService }, + { provide: PaymentLinkPaymentService, useValue: paymentLinkPaymentService }, + { provide: DepositRouteService, useValue: depositRouteService }, + { provide: OCPStickerService, useValue: paymentLinkStickerService }, + { provide: PaymentMerchantService, useValue: paymentMerchantService }, + TestUtil.provideConfig(), + ], + }).compile(); + + controller = module.get(PaymentLinkController); + }); + + describe('linkId parsing', () => { + beforeEach(() => { + jest.spyOn(paymentLinkService, 'getOrThrow').mockResolvedValue({ payments: [] } as unknown as PaymentLink); + // The mapper needs a fully-populated entity; this suite only cares how linkId was parsed. + jest.spyOn(PaymentLinkDtoMapper, 'toLinkDto').mockReturnValue({} as PaymentLinkDto); + }); + + // Regression: `+linkId` handed Postgres 'Infinity'/'1.9'/'1e+21' as integers -> 500. linkId is only + // one of several optional lookup keys, so a malformed value is dropped and the request still + // resolves via externalLinkId/externalPaymentId — which is what the falsy NaN did implicitly. + it.each(['NaN', 'Infinity', '1.9', '1e+21', '-1', '2147483648', 'abc', 'undefined', '0'])( + 'drops the malformed linkId %j instead of passing it to the id lookup', + async (linkId) => { + await controller.getAllPaymentLinks(jwt, linkId, 'shop-1', undefined); + + expect(paymentLinkService.getOrThrow).toHaveBeenCalledWith(7, undefined, 'shop-1', undefined); + }, + ); + + it.each(['42', ' 42 '])('still resolves the well-formed linkId %j', async (linkId) => { + await controller.getAllPaymentLinks(jwt, linkId, undefined, undefined); + + expect(paymentLinkService.getOrThrow).toHaveBeenCalledWith(7, 42, undefined, undefined); + }); + }); + + describe('createInvoicePayment r alias', () => { + const baseDto = { amount: '10', message: 'x' }; + + beforeEach(() => { + jest.spyOn(paymentLinkService, 'createInvoice').mockResolvedValue({ uniqueId: 'pl_1' } as PaymentLink); + jest.spyOn(paymentLinkService, 'createPayRequestWithCompletionCheck').mockResolvedValue({} as never); + }); + + // `r` is either a route id or a route label. Classifying it with !isNaN(+r) sent 'Infinity' and + // '1e+21' down the routeId branch and on to Postgres as an integer. + it.each(['Infinity', '1e+21', '1.9', 'NaN', 'my-label', '0'])('treats r=%j as a route label', async (r) => { + await controller.createInvoicePayment({ ...baseDto, r } as never); + + expect(paymentLinkService.createInvoice).toHaveBeenCalledWith(expect.objectContaining({ route: r })); + }); + + it.each(['42', ' 42 '])('treats the numeric r=%j as a route id', async (r) => { + await controller.createInvoicePayment({ ...baseDto, r } as never); + + expect(paymentLinkService.createInvoice).toHaveBeenCalledWith(expect.objectContaining({ routeId: r })); + }); + }); + + describe('assignPaymentLink', () => { + // The presence check must run on the PARSED id: a malformed linkId leaves no usable identifier, + // and this endpoint has no auth guard, so reaching the service with both absent would let the + // query match an arbitrary unassigned link. + it.each(['abc', 'Infinity', '1e+21', '0', '2147483648'])( + 'rejects a malformed linkId %j when no externalLinkId is supplied', + async (linkId) => { + await expect( + controller.assignPaymentLink(linkId, undefined, { publicName: 'attacker' } as never), + ).rejects.toBeInstanceOf(BadRequestException); + + expect(paymentLinkService.assignPaymentLink).not.toHaveBeenCalled(); + }, + ); + + it('ignores a malformed linkId when an externalLinkId is supplied', async () => { + jest.spyOn(PaymentLinkDtoMapper, 'toLinkDto').mockReturnValue({} as PaymentLinkDto); + jest.spyOn(paymentLinkService, 'assignPaymentLink').mockResolvedValue({} as PaymentLink); + + await controller.assignPaymentLink('abc', 'shop-1', { publicName: 'shop' } as never); + + expect(paymentLinkService.assignPaymentLink).toHaveBeenCalledWith(undefined, 'shop-1', { publicName: 'shop' }); + }); + }); + + describe('generateOcpStickers ids parsing', () => { + const run = (ids: string) => + controller.generateOcpStickers( + undefined, + 'my-route', + undefined, + ids, + StickerType.BITCOIN_FOCUS, + 'en', + StickerQrMode.CUSTOMER, + { set: jest.fn() } as unknown as Response, + ); + + // Regression: `Number.isInteger(+id)` accepts both of these, and they reached Postgres as + // out-of-range integers on an endpoint reachable without any authentication. + it.each(['1e+21', '2147483648', 'Infinity', '1.9', '0x10', '-1', '0', ''])('rejects ids=%j', async (ids) => { + await expect(run(ids)).rejects.toBeInstanceOf(BadRequestException); + expect(paymentLinkStickerService.generateOcpStickersPdf).not.toHaveBeenCalled(); + }); + + it('accepts a comma-separated list of well-formed ids', async () => { + jest + .spyOn(paymentLinkStickerService, 'generateOcpStickersPdf') + .mockResolvedValue(Buffer.from('pdf') as unknown as never); + + await run('1, 2,3'); + + expect(paymentLinkStickerService.generateOcpStickersPdf).toHaveBeenCalledWith( + 'my-route', + undefined, + [1, 2, 3], + StickerType.BITCOIN_FOCUS, + 'en', + StickerQrMode.CUSTOMER, + undefined, + ); + }); + }); +}); diff --git a/src/subdomains/core/payment-link/controllers/payment-link.controller.ts b/src/subdomains/core/payment-link/controllers/payment-link.controller.ts index 77034ec064..ce86d27294 100644 --- a/src/subdomains/core/payment-link/controllers/payment-link.controller.ts +++ b/src/subdomains/core/payment-link/controllers/payment-link.controller.ts @@ -89,7 +89,7 @@ export class PaymentLinkController { ): Promise { if (linkId || externalLinkId || externalPaymentId) return this.paymentLinkService - .getOrThrow(+jwt.user, +linkId, externalLinkId, externalPaymentId) + .getOrThrow(+jwt.user, Util.toDbId(linkId), externalLinkId, externalPaymentId) .then(PaymentLinkDtoMapper.toLinkDto); return this.paymentLinkService.getAll(+jwt.user).then(PaymentLinkDtoMapper.toLinkDtoList); @@ -142,7 +142,7 @@ export class PaymentLinkController { @Body() dto: UpdatePaymentLinkDto, ): Promise { return this.paymentLinkService - .update(+jwt.user, dto, +linkId, externalLinkId, externalPaymentId) + .update(+jwt.user, dto, Util.toDbId(linkId), externalLinkId, externalPaymentId) .then(PaymentLinkDtoMapper.toLinkDto); } @@ -160,7 +160,7 @@ export class PaymentLinkController { @Query('externalPaymentId') externalPaymentId: string, ): Promise { return this.paymentLinkService - .createPosLinkUser(+jwt.user, +linkId, externalLinkId, externalPaymentId) + .createPosLinkUser(+jwt.user, Util.toDbId(linkId), externalLinkId, externalPaymentId) .then((url) => ({ url })); } @@ -173,12 +173,12 @@ export class PaymentLinkController { @Query('externalLinkId') externalLinkId: string, @Body() dto: AssignPaymentLinkDto, ): Promise { - if (!linkId && !externalLinkId) throw new BadRequestException('id or externalId is required'); - if (linkId && !Number.isInteger(+linkId)) throw new BadRequestException('linkId must be an integer'); + // Checked on the parsed id, not the raw string: a malformed linkId leaves no usable identifier, + // and the service's `where` would then match an arbitrary unassigned link on this open endpoint. + const id = Util.toDbId(linkId); + if (!id && !externalLinkId) throw new BadRequestException('id or externalId is required'); - return this.paymentLinkService - .assignPaymentLink(linkId && +linkId, externalLinkId, dto) - .then(PaymentLinkDtoMapper.toLinkDto); + return this.paymentLinkService.assignPaymentLink(id, externalLinkId, dto).then(PaymentLinkDtoMapper.toLinkDto); } // -- CONFIG --- // @@ -216,7 +216,9 @@ export class PaymentLinkController { @ApiExcludeEndpoint() async createInvoicePayment(@Query() dto: CreateInvoicePaymentDto): Promise { if (dto.r) { - const isRouteId = !isNaN(+dto.r); + // A non-id `r` is a route label, not a malformed id — Util.toDbId keeps 'Infinity'/'1e+21' out + // of the id branch (and out of SQL) by routing them to the label lookup instead. + const isRouteId = Boolean(Util.toDbId(dto.r)); if (isRouteId) { dto.routeId ??= dto.r; } else { @@ -255,7 +257,7 @@ export class PaymentLinkController { ): Promise { const link = key ? await this.paymentLinkService.createPaymentForRouteWithAccessKey(dto, key, externalLinkId, route) - : await this.paymentLinkService.createPayment(dto, jwt && +jwt.user, +linkId, externalLinkId, route); + : await this.paymentLinkService.createPayment(dto, jwt && +jwt.user, Util.toDbId(linkId), externalLinkId, route); return PaymentLinkDtoMapper.toLinkDto(link); } @@ -276,7 +278,7 @@ export class PaymentLinkController { @Query('key') key: string, ): Promise { return this.paymentLinkService - .waitForPayment(+jwt?.user, +linkId, externalLinkId, externalPaymentId, key) + .waitForPayment(+jwt?.user, Util.toDbId(linkId), externalLinkId, externalPaymentId, key) .then(PaymentLinkDtoMapper.toLinkDto); } @@ -296,7 +298,7 @@ export class PaymentLinkController { @Query('key') key: string, ): Promise { return this.paymentLinkService - .confirmPayment(+jwt?.user, +linkId, externalLinkId, externalPaymentId, key) + .confirmPayment(+jwt?.user, Util.toDbId(linkId), externalLinkId, externalPaymentId, key) .then(PaymentLinkDtoMapper.toLinkDto); } @@ -318,7 +320,7 @@ export class PaymentLinkController { @Query('route') route: string, ): Promise { return this.paymentLinkService - .cancelPayment(jwt && +jwt.user, +linkId, externalLinkId, externalPaymentId, key, route) + .cancelPayment(jwt && +jwt.user, Util.toDbId(linkId), externalLinkId, externalPaymentId, key, route) .then(PaymentLinkDtoMapper.toLinkDto); } @@ -409,10 +411,11 @@ export class PaymentLinkController { throw new UnauthorizedException('Authentication required for POS mode'); } + // Util.toDbId, not Number.isInteger(+id): the latter accepts '1e+21' and '2147483648', which + // reach Postgres as out-of-range integers and 500 on an endpoint anonymous callers can reach. const idArray = ids?.split(',').map((id) => { - const linkId = +id; - if (!id.trim() || !Number.isInteger(linkId)) - throw new BadRequestException('ids must be a comma-separated list of integers'); + const linkId = Util.toDbId(id); + if (!linkId) throw new BadRequestException('ids must be a comma-separated list of integers'); return linkId; }); const externalIdArray = externalIds?.split(',').map((id) => id.trim()); diff --git a/src/subdomains/core/payment-link/services/__tests__/payment-link-id-guards.spec.ts b/src/subdomains/core/payment-link/services/__tests__/payment-link-id-guards.spec.ts new file mode 100644 index 0000000000..9e3fe1d855 --- /dev/null +++ b/src/subdomains/core/payment-link/services/__tests__/payment-link-id-guards.spec.ts @@ -0,0 +1,114 @@ +import { createMock } from '@golevelup/ts-jest'; +import { BadRequestException } from '@nestjs/common'; +import { Test, TestingModule } from '@nestjs/testing'; +import { TestSharedModule } from 'src/shared/utils/test.shared.module'; +import { TestUtil } from 'src/shared/utils/test.util'; +import { UserDataService } from 'src/subdomains/generic/user/models/user-data/user-data.service'; +import { DepositRouteService } from 'src/subdomains/supporting/address-pool/route/deposit-route.service'; +import { CreateInvoicePaymentDto } from '../../dto/create-invoice-payment.dto'; +import { PaymentLinkRepository } from '../../repositories/payment-link.repository'; +import { C2BPaymentLinkService } from '../c2b-payment-link.service'; +import { PaymentLinkPaymentService } from '../payment-link-payment.service'; +import { PaymentLinkService } from '../payment-link.service'; +import { PaymentQuoteService } from '../payment-quote.service'; + +describe('PaymentLinkService id guards', () => { + let service: PaymentLinkService; + + let paymentLinkRepo: PaymentLinkRepository; + let depositRouteService: DepositRouteService; + + beforeEach(async () => { + paymentLinkRepo = createMock(); + depositRouteService = createMock(); + + const module: TestingModule = await Test.createTestingModule({ + imports: [TestSharedModule], + providers: [ + PaymentLinkService, + { provide: PaymentLinkRepository, useValue: paymentLinkRepo }, + { provide: PaymentLinkPaymentService, useValue: createMock() }, + { provide: PaymentQuoteService, useValue: createMock() }, + { provide: UserDataService, useValue: createMock() }, + { provide: DepositRouteService, useValue: depositRouteService }, + { provide: C2BPaymentLinkService, useValue: createMock() }, + TestUtil.provideConfig(), + ], + }).compile(); + + service = module.get(PaymentLinkService); + }); + + describe('assignPaymentLink', () => { + // PUT /v1/paymentLink/assign is unauthenticated. With both identifiers absent the `where` reduces + // to { status: UNASSIGNED } — TypeORM drops undefined keys — so the query would match an + // arbitrary merchant's unassigned link and re-point it at the caller's route. + it('refuses to query when neither identifier is usable', async () => { + await expect( + service.assignPaymentLink(undefined, undefined, { publicName: 'attacker' } as never), + ).rejects.toThrow(BadRequestException); + + expect(paymentLinkRepo.findOne).not.toHaveBeenCalled(); + }); + + it('still queries when an id is supplied', async () => { + jest.spyOn(paymentLinkRepo, 'findOne').mockResolvedValue(null); + + await expect(service.assignPaymentLink(42, undefined, { publicName: 'shop' } as never)).rejects.toThrow( + 'Payment link not found', + ); + + expect(paymentLinkRepo.findOne).toHaveBeenCalledWith( + expect.objectContaining({ where: expect.objectContaining({ id: 42 }) }), + ); + }); + + it('still queries when only an externalId is supplied', async () => { + jest.spyOn(paymentLinkRepo, 'findOne').mockResolvedValue(null); + + await expect(service.assignPaymentLink(undefined, 'shop-1', { publicName: 'shop' } as never)).rejects.toThrow( + 'Payment link not found', + ); + + expect(paymentLinkRepo.findOne).toHaveBeenCalledWith( + expect.objectContaining({ where: expect.objectContaining({ externalId: 'shop-1' }) }), + ); + }); + }); + + describe('createInvoice', () => { + // GET /v1/paymentLink/payment and /v1/plp are unauthenticated, and `+dto.routeId` handed Postgres + // NaN/Infinity/1e+21 as integers. Validated at point of use because `route` wins over `routeId`. + it.each(['NaN', 'Infinity', '1.9', '1e+21', 'abc', '0', '2147483648'])( + 'rejects routeId %j before it reaches the route lookup', + async (routeId) => { + await expect(service.createInvoice({ routeId } as CreateInvoicePaymentDto)).rejects.toThrow( + BadRequestException, + ); + + expect(depositRouteService.getById).not.toHaveBeenCalled(); + }, + ); + + it('ignores a malformed routeId when a route label is supplied instead', async () => { + jest.spyOn(depositRouteService, 'getByLabel').mockResolvedValue(undefined); + + await expect( + service.createInvoice({ route: 'my-label', routeId: 'abc' } as CreateInvoicePaymentDto), + ).rejects.toThrow('Only Lightning routes are allowed'); + + expect(depositRouteService.getByLabel).toHaveBeenCalledWith(undefined, 'my-label'); + expect(depositRouteService.getById).not.toHaveBeenCalled(); + }); + + it('passes a well-formed routeId through as a number', async () => { + jest.spyOn(depositRouteService, 'getById').mockResolvedValue(undefined); + + await expect(service.createInvoice({ routeId: ' 42 ' } as CreateInvoicePaymentDto)).rejects.toThrow( + 'Only Lightning routes are allowed', + ); + + expect(depositRouteService.getById).toHaveBeenCalledWith(42); + }); + }); +}); diff --git a/src/subdomains/core/payment-link/services/payment-link.service.ts b/src/subdomains/core/payment-link/services/payment-link.service.ts index e728cae428..57a5452678 100644 --- a/src/subdomains/core/payment-link/services/payment-link.service.ts +++ b/src/subdomains/core/payment-link/services/payment-link.service.ts @@ -156,9 +156,15 @@ export class PaymentLinkService { } async createInvoice(dto: CreateInvoicePaymentDto): Promise { + // Validated here rather than on the DTO because `route` wins over `routeId` and the `r` alias is + // resolved in the controller after validation — so a junk routeId is only an error when it is the + // value actually used. Unguarded, `+dto.routeId` hands Postgres NaN/Infinity/1e+21 and 500s. + const routeId = Util.toDbId(dto.routeId); + if (!dto.route && !routeId) throw new BadRequestException('routeId must be a positive integer'); + const route = dto.route ? await this.depositRouteService.getByLabel(undefined, dto.route) - : await this.depositRouteService.getById(+dto.routeId); + : await this.depositRouteService.getById(routeId); if (route?.deposit.blockchains !== Blockchain.LIGHTNING) throw new BadRequestException('Only Lightning routes are allowed'); @@ -490,6 +496,11 @@ export class PaymentLinkService { externalId: string | undefined, dto: AssignPaymentLinkDto, ): Promise { + // Both identifiers absent would leave `where: { status: UNASSIGNED }` — TypeORM drops undefined + // keys, so the query would match an arbitrary unassigned link and assign it to the caller. This + // endpoint is unauthenticated, so that must be impossible however the caller reached it. + if (!id && !externalId) throw new BadRequestException('id or externalId is required'); + const paymentLink = await this.paymentLinkRepo.findOne({ where: { id, externalId, status: PaymentLinkStatus.UNASSIGNED }, relations: { route: { user: { userData: { organization: true } } } }, diff --git a/src/subdomains/supporting/address-pool/route/__tests__/deposit-route.service.spec.ts b/src/subdomains/supporting/address-pool/route/__tests__/deposit-route.service.spec.ts new file mode 100644 index 0000000000..944032e844 --- /dev/null +++ b/src/subdomains/supporting/address-pool/route/__tests__/deposit-route.service.spec.ts @@ -0,0 +1,56 @@ +import { createMock } from '@golevelup/ts-jest'; +import { Test, TestingModule } from '@nestjs/testing'; +import { TestSharedModule } from 'src/shared/utils/test.shared.module'; +import { TestUtil } from 'src/shared/utils/test.util'; +import { DepositRouteRepository } from '../deposit-route.repository'; +import { DepositRouteService } from '../deposit-route.service'; + +describe('DepositRouteService', () => { + let service: DepositRouteService; + let depositRouteRepo: DepositRouteRepository; + + beforeEach(async () => { + depositRouteRepo = createMock(); + + const module: TestingModule = await Test.createTestingModule({ + imports: [TestSharedModule], + providers: [ + DepositRouteService, + { provide: DepositRouteRepository, useValue: depositRouteRepo }, + TestUtil.provideConfig(), + ], + }).compile(); + + service = module.get(DepositRouteService); + }); + + describe('getPaymentRoute', () => { + // Regression: `!isNaN(+idOrLabel)` sent these down the id branch, so they reached Postgres as + // integers and came back as `invalid input syntax for type integer` -> 500 on + // GET /v1/paymentLink/recipient, which is reachable without authentication. They are not valid + // ids, so they must be looked up as route labels instead. + it.each(['NaN', 'Infinity', '1.9', '1e+21', '-1', '0', '0x10', '2147483648'])( + 'looks up %j as a label, never as an id', + async (idOrLabel) => { + jest.spyOn(depositRouteRepo, 'findOne').mockResolvedValue(undefined); + + await expect(service.getPaymentRoute(idOrLabel)).rejects.toThrow('Payment route not found'); + + expect(depositRouteRepo.findOne).toHaveBeenCalledWith( + expect.objectContaining({ where: expect.objectContaining({ route: { label: idOrLabel } }) }), + ); + }, + ); + + // `?id=+42` decodes to ' 42', which resolved as id 42 before the guard existed. + it.each(['42', ' 42 '])('still resolves the well-formed id %j through the id branch', async (idOrLabel) => { + jest.spyOn(depositRouteRepo, 'findOne').mockResolvedValue(undefined); + + await expect(service.getPaymentRoute(idOrLabel)).rejects.toThrow('Payment route not found'); + + expect(depositRouteRepo.findOne).toHaveBeenCalledWith( + expect.objectContaining({ where: expect.objectContaining({ id: 42 }) }), + ); + }); + }); +}); diff --git a/src/subdomains/supporting/address-pool/route/deposit-route.service.ts b/src/subdomains/supporting/address-pool/route/deposit-route.service.ts index df1c4a4c96..77fe672f09 100644 --- a/src/subdomains/supporting/address-pool/route/deposit-route.service.ts +++ b/src/subdomains/supporting/address-pool/route/deposit-route.service.ts @@ -42,10 +42,10 @@ export class DepositRouteService { } async getPaymentRoute(idOrLabel: string, options?: FindOneOptions): Promise { - const isRouteId = !isNaN(+idOrLabel); - const route = isRouteId - ? await this.getById(+idOrLabel, options) - : await this.getByLabel(undefined, idOrLabel, options); + // Util.toDbId, not !isNaN(+x): the latter routes 'Infinity'/'1.9'/'1e+21' down the id branch and + // hands them to Postgres as an integer, which 500s on an endpoint anonymous callers can reach. + const routeId = Util.toDbId(idOrLabel); + const route = routeId ? await this.getById(routeId, options) : await this.getByLabel(undefined, idOrLabel, options); if (route?.deposit.blockchains !== Blockchain.LIGHTNING) throw new NotFoundException(`Payment route not found`); diff --git a/src/subdomains/supporting/payment/services/__tests__/transaction.service.spec.ts b/src/subdomains/supporting/payment/services/__tests__/transaction.service.spec.ts index bf2a08bad2..59ac1358d0 100644 --- a/src/subdomains/supporting/payment/services/__tests__/transaction.service.spec.ts +++ b/src/subdomains/supporting/payment/services/__tests__/transaction.service.spec.ts @@ -4,6 +4,8 @@ import { AmlSourceType } from 'src/subdomains/core/aml/entities/transaction-aml- import { CheckStatus } from 'src/subdomains/core/aml/enums/check-status.enum'; import { TransactionAmlCheckService } from 'src/subdomains/core/aml/services/transaction-aml-check.service'; import { BuyCryptoRepository } from 'src/subdomains/core/buy-crypto/process/repositories/buy-crypto.repository'; +import { TestSharedModule } from 'src/shared/utils/test.shared.module'; +import { TestUtil } from 'src/shared/utils/test.util'; import { BankDataService } from 'src/subdomains/generic/user/models/bank-data/bank-data.service'; import { UserDataService } from 'src/subdomains/generic/user/models/user-data/user-data.service'; import { UpdateTransactionDto } from '../../dto/update-transaction.dto'; @@ -99,3 +101,48 @@ describe('TransactionService (admin door — amlCheck audit trail)', () => { expect(transactionAmlCheckService.create).not.toHaveBeenCalled(); }); }); + +describe('TransactionService (relation load strategy)', () => { + let service: TransactionService; + let repo: TransactionRepository; + + beforeEach(async () => { + repo = createMock(); + + const module: TestingModule = await Test.createTestingModule({ + imports: [TestSharedModule], + providers: [ + TransactionService, + { provide: TransactionRepository, useValue: repo }, + { provide: UserDataService, useValue: createMock() }, + { provide: BankDataService, useValue: createMock() }, + { provide: SpecialExternalAccountService, useValue: createMock() }, + { provide: BuyCryptoRepository, useValue: createMock() }, + { provide: TransactionAmlCheckService, useValue: createMock() }, + TestUtil.provideConfig(), + ], + }).compile(); + + service = module.get(TransactionService); + }); + + // The statement path relies on this being forwarded: resolved as a join, its relation tree selects + // 1664 columns and Postgres rejects the query outright. + it('forwards the relation load strategy to the repository', async () => { + jest.spyOn(repo, 'findOne').mockResolvedValue(null); + + await service.getTransactionById(1, { userData: true }, 'query'); + expect(repo.findOne).toHaveBeenCalledWith(expect.objectContaining({ relationLoadStrategy: 'query' })); + + await service.getTransactionByUid('T0123456789ABCDEF', { userData: true }, 'query'); + expect(repo.findOne).toHaveBeenLastCalledWith(expect.objectContaining({ relationLoadStrategy: 'query' })); + }); + + it('leaves the strategy undefined when the caller does not ask for one', async () => { + jest.spyOn(repo, 'findOne').mockResolvedValue(null); + + await service.getTransactionById(1, { userData: true }); + + expect(repo.findOne).toHaveBeenCalledWith(expect.objectContaining({ relationLoadStrategy: undefined })); + }); +}); diff --git a/src/subdomains/supporting/payment/services/transaction-helper.ts b/src/subdomains/supporting/payment/services/transaction-helper.ts index 711cb2a708..b848730a9d 100644 --- a/src/subdomains/supporting/payment/services/transaction-helper.ts +++ b/src/subdomains/supporting/payment/services/transaction-helper.ts @@ -505,19 +505,27 @@ export class TransactionHelper implements OnModuleInit { txIdOrUid: number | string, statementType: TxStatementType, ): Promise { + // Only what the statement path actually reads. `userData.organization`, `buyFiat.outputAsset` and + // `cryptoInput.asset` arrive via eager relations, and cryptoRoute / buyFiat.sell / refReward.user + // are never dereferenced here — under the query strategy below each unread branch is a wasted + // round-trip rather than just a wasted join column. const relations = { - userData: { organization: true }, - buyCrypto: { buy: { user: { wallet: true } }, cryptoRoute: true, cryptoInput: true }, - buyFiat: { sell: true, cryptoInput: true }, - refReward: { user: { userData: true } }, + userData: true, + buyCrypto: { buy: { user: { wallet: true } }, cryptoInput: true }, + buyFiat: { cryptoInput: true }, + refReward: true, bankTxReturn: true, request: true, }; + // Load relations as separate queries, not one join. Resolved as a join this tree reaches ~1664 + // selected columns (61 joined nodes — Asset alone appears 10x, Country 9x, all pulled in by eager + // relations) and trips Postgres' "target lists can have at most 1664 entries" limit, which made + // every invoice/receipt request a 500. A join here is one @Column away from overflowing again. const transaction = typeof txIdOrUid === 'number' - ? await this.transactionService.getTransactionById(txIdOrUid, relations) - : await this.transactionService.getTransactionByUid(txIdOrUid, relations); + ? await this.transactionService.getTransactionById(txIdOrUid, relations, 'query') + : await this.transactionService.getTransactionByUid(txIdOrUid, relations, 'query'); if (!transaction) throw new BadRequestException('Transaction not found'); if (!transaction.userData.isInvoiceDataComplete) throw new BadRequestException('User data is not complete'); diff --git a/src/subdomains/supporting/payment/services/transaction.service.ts b/src/subdomains/supporting/payment/services/transaction.service.ts index 3f8f9a233d..60d02beaf1 100644 --- a/src/subdomains/supporting/payment/services/transaction.service.ts +++ b/src/subdomains/supporting/payment/services/transaction.service.ts @@ -8,7 +8,17 @@ import { BuyCryptoRepository } from 'src/subdomains/core/buy-crypto/process/repo import { BankDataType } from 'src/subdomains/generic/user/models/bank-data/bank-data.entity'; import { BankDataService } from 'src/subdomains/generic/user/models/bank-data/bank-data.service'; import { UserDataService } from 'src/subdomains/generic/user/models/user-data/user-data.service'; -import { Between, Brackets, EntityManager, FindOptionsRelations, In, IsNull, LessThanOrEqual, Not } from 'typeorm'; +import { + Between, + Brackets, + EntityManager, + FindOneOptions, + FindOptionsRelations, + In, + IsNull, + LessThanOrEqual, + Not, +} from 'typeorm'; import { CreateTransactionDto } from '../dto/input/create-transaction.dto'; import { UpdateTransactionInternalDto } from '../dto/input/update-transaction-internal.dto'; import { UpdateTransactionDto } from '../dto/update-transaction.dto'; @@ -17,6 +27,8 @@ import { Transaction, TransactionSourceType } from '../entities/transaction.enti import { TransactionRepository } from '../repositories/transaction.repository'; import { SpecialExternalAccountService } from './special-external-account.service'; +type RelationLoadStrategy = FindOneOptions['relationLoadStrategy']; + @Injectable() export class TransactionService { constructor( @@ -126,8 +138,12 @@ export class TransactionService { await this.buyCryptoRepo.save(entity.buyCrypto); } - async getTransactionById(id: number, relations: FindOptionsRelations = {}): Promise { - return this.repo.findOne({ where: { id }, relations }); + async getTransactionById( + id: number, + relations: FindOptionsRelations = {}, + relationLoadStrategy?: RelationLoadStrategy, + ): Promise { + return this.repo.findOne({ where: { id }, relations, relationLoadStrategy }); } async getTransactionsByIds(ids: number[]): Promise { @@ -135,8 +151,12 @@ export class TransactionService { return this.repo.find({ where: { id: In(ids) } }); } - async getTransactionByUid(uid: string, relations: FindOptionsRelations = {}): Promise { - return this.repo.findOne({ where: { uid }, relations }); + async getTransactionByUid( + uid: string, + relations: FindOptionsRelations = {}, + relationLoadStrategy?: RelationLoadStrategy, + ): Promise { + return this.repo.findOne({ where: { uid }, relations, relationLoadStrategy }); } async getTransactionByRequestId(