From d75760d79832151000daca06c63ca4f620feff4b Mon Sep 17 00:00:00 2001 From: David May <85513542+davidleomay@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:58:20 +0200 Subject: [PATCH 1/2] fix(payment-link): scope externalPaymentId lookups to the authorized link (BUG-1289) (#4436) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(payment-link): scope externalPaymentId lookups to the authorized link (BUG-1289) Cross-merchant payment-link disclosure: `getPaymentByExternalId` matched only on `externalId` — a merchant-supplied reconciliation identifier that is not unique across tenants. The link lookup in `payment-link.service.ts` was correctly owner-scoped, but the payment enrichment was global, so any authenticated caller who owned one link could iterate the sequential externalPaymentId keyspace and read (and cancel/confirm) foreign merchants' payment records by pairing the foreign id with their own linkId. Same helper backed the wait / confirm / cancel paths, giving the leak a write primitive (own-asset PoC in the report; third-party path not exercised). Fix: require the resolved `linkId` at the type level for the lookup, so the query becomes `findOne({ where: { externalId, link: { id: linkId } } })`. The access-key POS path (`getPaymentLinkByAccessKey`) is reversed to resolve links via the repository first, then match access-keys in memory, so foreign records never enter the process. `getAllPaymentsByExternalLinkId` was only used there and is removed. Verified against the four attack scenarios from the report (read, iteration, confirm, cancel) — all now return 404 with an empty payments array. Legitimate paths (own link + own payment, POS access-key) unchanged. Lint, type-check, and the payment-link jest suite (7 files / 62 tests) pass. * refactor(payment-link): move externalPaymentId link lookup into a named repository method Follow-up to the previous commit. The access-key POS path inlined a `paymentLinkRepo.find(...)` in the service, bypassing the "repositories own data access" layering the rest of `payment-link.service.ts` follows. Every other access-key branch delegates to a named repo method (`getAllPaymentLinksByExternalLinkId` right above it); this brings the externalPaymentId branch in line. - Adds `PaymentLinkRepository.getAllPaymentLinksByExternalPaymentId`, mirroring the shape of the sibling `getAllPaymentLinksByExternalLinkId`. - Drops the inline query and the now-unused `Equal` import from the service. No behavior change. Same 62/62 payment-link jest tests, lint and type-check clean. --- .../repositories/payment-link.repository.ts | 7 +++++++ .../services/payment-link-payment.service.ts | 14 +++++--------- .../payment-link/services/payment-link.service.ts | 12 +++++++----- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/subdomains/core/payment-link/repositories/payment-link.repository.ts b/src/subdomains/core/payment-link/repositories/payment-link.repository.ts index cffda11b93..cb28937c10 100644 --- a/src/subdomains/core/payment-link/repositories/payment-link.repository.ts +++ b/src/subdomains/core/payment-link/repositories/payment-link.repository.ts @@ -24,6 +24,13 @@ export class PaymentLinkRepository extends BaseRepository { }); } + async getAllPaymentLinksByExternalPaymentId(externalPaymentId: string): Promise { + return this.find({ + where: { payments: { externalId: Equal(externalPaymentId) } }, + relations: { route: { user: { userData: { organization: true } } } }, + }); + } + async getHistoryByStatus( userId: number, paymentStatus: PaymentLinkPaymentStatus[], diff --git a/src/subdomains/core/payment-link/services/payment-link-payment.service.ts b/src/subdomains/core/payment-link/services/payment-link-payment.service.ts index 7161152200..e7f223f2d9 100644 --- a/src/subdomains/core/payment-link/services/payment-link-payment.service.ts +++ b/src/subdomains/core/payment-link/services/payment-link-payment.service.ts @@ -118,16 +118,12 @@ export class PaymentLinkPaymentService { }); } - async getPaymentByExternalId(externalPaymentId: string): Promise { + // externalPaymentId is a merchant-supplied reconciliation identifier and is NOT unique across + // merchants; scope the lookup to a link the caller has already been authorized against, or the + // response leaks foreign merchants' payment records (BUG-1289). + async getPaymentByExternalId(linkId: number, externalPaymentId: string): Promise { return this.paymentLinkPaymentRepo.findOne({ - where: { externalId: externalPaymentId }, - }); - } - - async getAllPaymentsByExternalLinkId(externalPaymentId: string): Promise { - return this.paymentLinkPaymentRepo.find({ - where: { externalId: externalPaymentId }, - relations: { link: { route: { user: { userData: true } } } }, + where: { externalId: externalPaymentId, link: { id: linkId } }, }); } 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 d7f9426f21..e728cae428 100644 --- a/src/subdomains/core/payment-link/services/payment-link.service.ts +++ b/src/subdomains/core/payment-link/services/payment-link.service.ts @@ -71,7 +71,7 @@ export class PaymentLinkService { if (loadPayments) { const payment = externalPaymentId - ? await this.paymentLinkPaymentService.getPaymentByExternalId(externalPaymentId) + ? await this.paymentLinkPaymentService.getPaymentByExternalId(link.id, externalPaymentId) : await this.paymentLinkPaymentService.getMostRecentPayment(link.uniqueId); if (payment) link.payments.push(payment); } @@ -626,11 +626,13 @@ export class PaymentLinkService { } if (externalPaymentId) { - const payments = await this.paymentLinkPaymentService.getAllPaymentsByExternalLinkId(externalPaymentId); - const payment = payments.find((p) => p.link.configObj.accessKeys?.includes(key)); - if (!payment) throw new NotFoundException('No payment found'); + // Resolve the access-key against payment links first, then look up the payment scoped to + // those links — externalPaymentId is not unique across merchants (BUG-1289). + const candidateLinks = await this.paymentLinkRepo.getAllPaymentLinksByExternalPaymentId(externalPaymentId); + const paymentLink = candidateLinks.find((pl) => pl.configObj.accessKeys?.includes(key)); + if (!paymentLink) throw new NotFoundException('No payment found'); - return this.getOrThrow(payment.link.route.user.id, payment.link.id, payment.link.externalId, payment.externalId); + return this.getOrThrow(paymentLink.route.user.id, paymentLink.id, paymentLink.externalId, externalPaymentId); } throw new BadRequestException('Either externalLinkId or externalPaymentId must be provided'); From 8d2d35c469632cebaee007834155c250397d892a Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:23:16 +0200 Subject: [PATCH 2/2] 441fed04 - fix(custody): hide an empty legacy Safe next to read-only accounts too (#4444) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(custody): hide an empty legacy Safe next to read-only accounts too The hide rule only fired when the customer held write access on another entry. A viewer with read grants only — the operator-side case — kept an empty "Custody" entry in the selector next to the accounts they may see. Drop the access-level condition: any other visible entry is enough. The balance check is unchanged, so a legacy Safe that actually holds something still stays. The trade-off is stated on the branch: a read-only viewer is left without a writable entry, and the selector offers them no deposit path. * test(custody): assert the balance check actually runs for read-only viewers Two findings from the review. The new test passed without proving anything: with a read grant next to an empty Safe, the old write-access rule never consulted the balance either, so the case was green before the fix as well. Asserting the call is what ties it to the changed condition. The comment also claimed the balance check keeps the entry for anyone holding a balance. With no other entry visible that check never runs — the entry stays because the condition short-circuits, which is a different reason. --- .../__tests__/custody-account.service.spec.ts | 31 ++++++++++++++++--- .../services/custody-account.service.ts | 20 +++++++----- 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts b/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts index 7db1104b78..0a10f59724 100644 --- a/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts +++ b/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts @@ -530,7 +530,7 @@ describe('CustodyAccountService', () => { ); }); - it('keeps the legacy entry when the only other visible account is read-only', async () => { + it('drops the empty legacy entry when the only other visible account is read-only', async () => { const foreignAccount = foreignCustodyAccount(); const readOnlyGrant = accessGrant({ account: foreignAccount, @@ -541,14 +541,37 @@ describe('CustodyAccountService', () => { const custodyUserId = 55; userDataService.getUserData.mockResolvedValue(ownerUserData({ users: [custodyRoleUser(custodyUserId)] })); mockFindActiveGrants([readOnlyGrant]); + custodyService.hasNonZeroCustodyBalance.mockResolvedValue(false); + + const result = await service.getCustodyAccountsForUser(ownerId); + + // The operator-side viewer: read access on someone else's account, own Safe empty. The + // empty entry goes, and with it the only writable one — see the note on the branch. + expect(result).toHaveLength(1); + expect(result.some((dto) => dto.isLegacy)).toBe(false); + expect(custodyService.hasNonZeroCustodyBalance).toHaveBeenCalledWith([custodyUserId]); + }); + + it('keeps the legacy entry when the only other visible account is read-only but the Safe holds a balance', async () => { + const foreignAccount = foreignCustodyAccount(); + const readOnlyGrant = accessGrant({ + account: foreignAccount, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.READ, + active: true, + }); + const custodyUserId = 55; + userDataService.getUserData.mockResolvedValue(ownerUserData({ users: [custodyRoleUser(custodyUserId)] })); + mockFindActiveGrants([readOnlyGrant]); + custodyService.hasNonZeroCustodyBalance.mockResolvedValue(true); const result = await service.getCustodyAccountsForUser(ownerId); expect(result).toHaveLength(2); expect(result.some((dto) => dto.isLegacy)).toBe(true); - // A read-only grant elsewhere cannot substitute for the legacy Safe — checkAccess() - // rejects any write on a Read account, so the balance is never even queried here. - expect(custodyService.hasNonZeroCustodyBalance).not.toHaveBeenCalled(); + // Asserting the call is what makes this test about the fix: without it the case would + // pass under the old write-access rule too, where the balance was never consulted here. + expect(custodyService.hasNonZeroCustodyBalance).toHaveBeenCalledWith([custodyUserId]); }); it('drops the empty legacy entry when the user has write access elsewhere', async () => { diff --git a/src/subdomains/core/custody/services/custody-account.service.ts b/src/subdomains/core/custody/services/custody-account.service.ts index bf4c540b90..e4a3da3f48 100644 --- a/src/subdomains/core/custody/services/custody-account.service.ts +++ b/src/subdomains/core/custody/services/custody-account.service.ts @@ -87,17 +87,21 @@ export class CustodyAccountService { ]; // Legacy Safe = absence of any owned account row; independent of shared grants. Hidden - // only when it is both empty AND the customer already has write access on another entry - // — a Read-only grant elsewhere cannot substitute for it: checkAccess() rejects any write - // on a Read account, so someone with no writable account left would lose their only path - // to a deposit. A non-empty legacy Safe always stays, regardless of what else is writable. + // when it is both empty AND at least one other entry is visible, so an empty "Custody" + // never sits next to real accounts in the selector. The access level of those other + // entries deliberately does NOT matter: a viewer holding only Read grants is exactly the + // case this hides — an operator-side account with an empty Safe of its own. The cost is + // that such a viewer keeps no writable entry in the list (checkAccess() rejects writes on + // a Read account), so the selector offers them no deposit path. Whenever another entry is + // visible, the balance check below has the last word: a legacy Safe that holds something + // keeps its entry. With nothing else visible the check never runs and the entry stays. if (allOwnedAccounts.length === 0) { const custodyUserIds = account.users.filter((u) => u.role === UserRole.CUSTODY).map((u) => u.id); if (custodyUserIds.length > 0) { - // The balance check only runs when a legacy entry is actually in play AND the - // customer already has write access elsewhere — every other case stays cheap, no query. - const hasWriteElsewhere = custodyAccounts.some((ca) => ca.accessLevel === CustodyAccessLevel.WRITE); - const hideLegacy = hasWriteElsewhere && !(await this.custodyService.hasNonZeroCustodyBalance(custodyUserIds)); + // The balance check only runs when a legacy entry is actually in play AND something + // else is visible next to it — every other case stays cheap, no query. + const hasOtherAccounts = custodyAccounts.length > 0; + const hideLegacy = hasOtherAccounts && !(await this.custodyService.hasNonZeroCustodyBalance(custodyUserIds)); if (!hideLegacy) custodyAccounts.push(CustodyAccountDtoMapper.toLegacyDto(account)); } }