From 9f9d52b055c3083fbde0512b6ebcded419749db6 Mon Sep 17 00:00:00 2001 From: Fabien Date: Thu, 19 Feb 2026 23:15:11 +0100 Subject: [PATCH 1/2] Fix handling of empty payment IDs When the expected payment ID is empty, it's possible to match against existing transaction even if they are not paybutton transactions at all. If the payment ID is not set it should not be relied upon. Update and clarify the tests accordingly. This update allows for code simplification by removing a workaround from the widget container. Test Plan: yarn test --- .../lib/components/Widget/WidgetContainer.tsx | 6 - react/lib/tests/util/validate.test.ts | 124 +++++++++--------- react/lib/util/validate.ts | 9 +- 3 files changed, 67 insertions(+), 72 deletions(-) diff --git a/react/lib/components/Widget/WidgetContainer.tsx b/react/lib/components/Widget/WidgetContainer.tsx index 18615cfb..481899ce 100644 --- a/react/lib/components/Widget/WidgetContainer.tsx +++ b/react/lib/components/Widget/WidgetContainer.tsx @@ -342,12 +342,6 @@ export const WidgetContainer: React.FunctionComponent = return; } - if (!disablePaymentId && !thisPaymentId) { - // Skip if paymentId is required but not yet set. This avoids matching - // transactions against undefined payment IDs. - return; - } - // Run immediately (attempt 1) const checkCompleted = await checkForTransactions(); diff --git a/react/lib/tests/util/validate.test.ts b/react/lib/tests/util/validate.test.ts index 79523efe..8231b8b2 100644 --- a/react/lib/tests/util/validate.test.ts +++ b/react/lib/tests/util/validate.test.ts @@ -29,8 +29,8 @@ describe('Validate Util Tests', () => { transaction, currency, price, - false, - false, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn @@ -39,7 +39,7 @@ describe('Validate Util Tests', () => { expect(result).toBe(true); }); - it('true when paymentId is undefined and received paymentId is empty', async () => { + it('true if disablePaymentId is set even if both paymentId and received paymentId are empty or undefined', async () => { const transaction: Transaction = { amount: '101.00', paymentId: '', @@ -49,27 +49,42 @@ describe('Validate Util Tests', () => { timestamp: 0, address: 'ecash:qrmm7edwuj4jf7tnvygjyztyy0a0qxvl7quss2vxek', }; - const expectedPaymentId = undefined; + let expectedPaymentId: string | undefined = undefined; const expectedAmount = resolveNumber('101.00'); const expectedOpReturn = 'test opReturn message'; const price = 1; const currency = 'XEC'; - const result = shouldTriggerOnSuccess( + const result1 = shouldTriggerOnSuccess( transaction, currency, price, - false, - true, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ true, expectedPaymentId, expectedAmount, expectedOpReturn ); - expect(result).toBe(true); + expect(result1).toBe(true); + + expectedPaymentId = ''; + + const result2 = shouldTriggerOnSuccess( + transaction, + currency, + price, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ true, + expectedPaymentId, + expectedAmount, + expectedOpReturn + ); + + expect(result2).toBe(true); }); - it('true when both expected and received paymentId are empty', async () => { + it('false if disablePaymentId is unset when both expected and received paymentId are empty', async () => { const transaction: Transaction = { amount: '101.00', paymentId: '', @@ -89,14 +104,17 @@ describe('Validate Util Tests', () => { transaction, currency, price, - false, - false, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn ); - expect(result).toBe(true); + // It shouldn't match an empty payment ID unless explicitly instructed + // to do so. Payment IDs are not set when the button is built so any + // tx with the same address and amount (even not PayButton) could match. + expect(result).toBe(false); }); it('false when paymentId does not match received paymentId', async () => { @@ -119,8 +137,8 @@ describe('Validate Util Tests', () => { transaction, currency, price, - false, - false, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn @@ -149,8 +167,8 @@ describe('Validate Util Tests', () => { transaction, currency, price, - false, - false, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn @@ -179,8 +197,8 @@ describe('Validate Util Tests', () => { transaction, currency, price, - false, - false, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn @@ -209,38 +227,8 @@ describe('Validate Util Tests', () => { transaction, currency, price, - false, - false, - expectedPaymentId, - expectedAmount, - expectedOpReturn - ); - - expect(result).toBe(true); - }); - - it('should ignore paymentId validation when disablePaymentId is true', async () => { - const transaction: Transaction = { - amount: '101.00', - paymentId: '123', - message: 'test opReturn', - rawMessage: 'test opReturn', - hash: '', - timestamp: 0, - address: 'ecash:qrmm7edwuj4jf7tnvygjyztyy0a0qxvl7quss2vxek', - }; - const expectedPaymentId = undefined; - const expectedAmount = resolveNumber('101.00'); - const expectedOpReturn = 'test opReturn'; - const price = 1; - const currency = 'XEC'; - - const result = shouldTriggerOnSuccess( - transaction, - currency, - price, - false, - true, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn @@ -249,7 +237,7 @@ describe('Validate Util Tests', () => { expect(result).toBe(true); }); - it('false when opReturn is undefined and received opReturn message is not empty or undefined', async () => { + it('false when opReturn is undefined and received opReturn message is not empty', async () => { const transaction: Transaction = { amount: '101.00', paymentId: '123', @@ -269,8 +257,8 @@ describe('Validate Util Tests', () => { transaction, currency, price, - false, - false, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn @@ -279,6 +267,8 @@ describe('Validate Util Tests', () => { expect(result).toBe(false); }); + // FIXME: I have no idea what is the use case for this, so assuming it's + // useful. Fabien 2026-02-19 it('true when opReturn is in a different format than received opReturn message but rawMessage matches expected opReturn', async () => { const transaction: Transaction = { amount: '101.00', @@ -302,8 +292,8 @@ describe('Validate Util Tests', () => { transaction, currency, price, - false, - false, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn @@ -332,8 +322,8 @@ describe('Validate Util Tests', () => { transaction, currency, price, - false, - false, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn @@ -353,6 +343,10 @@ describe('Validate Util Tests', () => { address: 'ecash:qrmm7edwuj4jf7tnvygjyztyy0a0qxvl7quss2vxek', }; const expectedPaymentId = '123'; + // FIXME: This value is unused in shouldTriggerOnSuccess (the amount is + // compared to currencyObject.float divided by price) but it has to be + // set nontheless otherwise the price is not even checked. It has to be + // set to any value that don't convert to false. Fabien 2026-02-19 const expectedAmount = resolveNumber('101.00'); const expectedOpReturn = 'test opReturn'; const price = 0.00003172; @@ -367,8 +361,8 @@ describe('Validate Util Tests', () => { transaction, currency, price, - false, - false, + /*randomSatoshis=*/ false, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn, @@ -378,6 +372,14 @@ describe('Validate Util Tests', () => { expect(result).toBe(true); }); + // FIXME: this is another test for an actual bug. Here the + // expectedPaymentId is defined and disablePaymentId is false so it's + // explicitely asking for a payment ID match. However because the random + // satoshis are another (vert bad) way to add entropy the + // shouldTriggerOnSuccess assumes you don't need the payment ID and skips + // the check. Random satoshis should be removed completely as it's a + // terrible idea for a payment system to randomly change the price. + // Fabien 2026-02-19 it('true when randomSatoshis is true and paymentId does not match', async () => { const transaction: Transaction = { amount: '3152585.12', @@ -403,8 +405,8 @@ describe('Validate Util Tests', () => { transaction, currency, price, - true, - false, + /*randomSatoshis=*/ true, + /*disablePaymentId=*/ false, expectedPaymentId, expectedAmount, expectedOpReturn, diff --git a/react/lib/util/validate.ts b/react/lib/util/validate.ts index 2632f9ef..aa1d3cb6 100644 --- a/react/lib/util/validate.ts +++ b/react/lib/util/validate.ts @@ -45,13 +45,12 @@ export const shouldTriggerOnSuccess = ( let isPaymentIdValid = true let isOpReturnValid = true - if(!randomSatoshis || randomSatoshis === 0){ - if(!isBCH){ - const paymentIdsMatch = expectedPaymentId === paymentId; + if(!isBCH){ + if(!randomSatoshis || randomSatoshis === 0){ + const paymentIdsMatch = !!expectedPaymentId && expectedPaymentId === paymentId; isPaymentIdValid = disablePaymentId ? true : paymentIdsMatch; } - } - if(!isBCH){ + const rawOpReturnIsEmptyOrUndefined = rawOpReturn === '' || rawOpReturn === undefined; const opReturn = rawOpReturnIsEmptyOrUndefined ? message : rawOpReturn const opReturnIsEmptyOrUndefined = opReturn === '' || opReturn === undefined; From 00f524ebbde2b378cb47dea34b395262c83685c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Estev=C3=A3o?= Date: Fri, 20 Feb 2026 10:25:49 -0300 Subject: [PATCH 2/2] test: add (undefined, '') paymentId matching matrix test --- react/lib/tests/util/validate.test.ts | 50 +++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/react/lib/tests/util/validate.test.ts b/react/lib/tests/util/validate.test.ts index 8231b8b2..04469044 100644 --- a/react/lib/tests/util/validate.test.ts +++ b/react/lib/tests/util/validate.test.ts @@ -418,3 +418,53 @@ describe('Validate Util Tests', () => { }); }); +describe('paymentId x expectedPaymentId empty matrix (disablePaymentId=false)', () => { + const currency = 'XEC'; + const price = 1; + const expectedAmount = resolveNumber('101.00'); + const expectedOpReturn = 'test opReturn'; + + const baseTx: Omit = { + amount: '101.00', + message: 'test opReturn', + rawMessage: 'test opReturn', + hash: '', + timestamp: 0, + address: 'ecash:qrmm7edwuj4jf7tnvygjyztyy0a0qxvl7quss2vxek', + }; + + const emptyVals = [ + { label: 'undefined', value: undefined }, + { label: 'empty', value: '' }, + ] as const; + + const cases = emptyVals.flatMap((exp) => + emptyVals.map((tx) => ({ + expLabel: exp.label, + txLabel: tx.label, + expectedPaymentId: exp.value as string | undefined, + txPaymentId: tx.value as unknown as string, + expected: false + })), + ); + + test.each(cases)( + 'expectedPaymentId=%s tx.paymentId=%s -> %s', + (row) => { + const tx = { ...baseTx, paymentId: row.txPaymentId as unknown as string } as Transaction; + + const result = shouldTriggerOnSuccess( + tx, + currency, + price, + false, // randomSatoshis + false, // disablePaymentId + row.expectedPaymentId, + expectedAmount, + expectedOpReturn, + ); + + expect(result).toBe(row.expected); + }, + ); +});