Skip to content

Commit b2d2939

Browse files
FabcienFabcien
committed
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
1 parent 3f27322 commit b2d2939

3 files changed

Lines changed: 64 additions & 69 deletions

File tree

‎react/lib/components/Widget/WidgetContainer.tsx‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -342,12 +342,6 @@ export const WidgetContainer: React.FunctionComponent<WidgetContainerProps> =
342342
return;
343343
}
344344

345-
if (!disablePaymentId && !thisPaymentId) {
346-
// Skip if paymentId is required but not yet set. This avoids matching
347-
// transactions against undefined payment IDs.
348-
return;
349-
}
350-
351345
// Run immediately (attempt 1)
352346
const checkCompleted = await checkForTransactions();
353347

‎react/lib/tests/util/validate.test.ts‎

Lines changed: 60 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ describe('Validate Util Tests', () => {
2929
transaction,
3030
currency,
3131
price,
32-
false,
33-
false,
32+
/*randomSatoshis=*/ false,
33+
/*disablePaymentId=*/ false,
3434
expectedPaymentId,
3535
expectedAmount,
3636
expectedOpReturn
@@ -39,7 +39,7 @@ describe('Validate Util Tests', () => {
3939
expect(result).toBe(true);
4040
});
4141

42-
it('true when paymentId is undefined and received paymentId is empty', async () => {
42+
it('true if disablePaymentId is set even if both paymentId and received paymentId are empty or undefined', async () => {
4343
const transaction: Transaction = {
4444
amount: '101.00',
4545
paymentId: '',
@@ -59,8 +59,23 @@ describe('Validate Util Tests', () => {
5959
transaction,
6060
currency,
6161
price,
62-
false,
63-
true,
62+
/*randomSatoshis=*/ false,
63+
/*disablePaymentId=*/ true,
64+
expectedPaymentId,
65+
expectedAmount,
66+
expectedOpReturn
67+
);
68+
69+
expect(result).toBe(true);
70+
71+
const expectedPaymentId = '';
72+
73+
const result = shouldTriggerOnSuccess(
74+
transaction,
75+
currency,
76+
price,
77+
/*randomSatoshis=*/ false,
78+
/*disablePaymentId=*/ true,
6479
expectedPaymentId,
6580
expectedAmount,
6681
expectedOpReturn
@@ -69,7 +84,7 @@ describe('Validate Util Tests', () => {
6984
expect(result).toBe(true);
7085
});
7186

72-
it('true when both expected and received paymentId are empty', async () => {
87+
it('false if disablePaymentId is unset when both expected and received paymentId are empty', async () => {
7388
const transaction: Transaction = {
7489
amount: '101.00',
7590
paymentId: '',
@@ -89,14 +104,17 @@ describe('Validate Util Tests', () => {
89104
transaction,
90105
currency,
91106
price,
92-
false,
93-
false,
107+
/*randomSatoshis=*/ false,
108+
/*disablePaymentId=*/ false,
94109
expectedPaymentId,
95110
expectedAmount,
96111
expectedOpReturn
97112
);
98113

99-
expect(result).toBe(true);
114+
// It shouldn't match an empty payment ID unless explicitly instructed
115+
// to do so. Payment IDs are not set when the button is built so any
116+
// tx with the same address and amount (even not PayButton) could match.
117+
expect(result).toBe(false);
100118
});
101119

102120
it('false when paymentId does not match received paymentId', async () => {
@@ -119,8 +137,8 @@ describe('Validate Util Tests', () => {
119137
transaction,
120138
currency,
121139
price,
122-
false,
123-
false,
140+
/*randomSatoshis=*/ false,
141+
/*disablePaymentId=*/ false,
124142
expectedPaymentId,
125143
expectedAmount,
126144
expectedOpReturn
@@ -149,8 +167,8 @@ describe('Validate Util Tests', () => {
149167
transaction,
150168
currency,
151169
price,
152-
false,
153-
false,
170+
/*randomSatoshis=*/ false,
171+
/*disablePaymentId=*/ false,
154172
expectedPaymentId,
155173
expectedAmount,
156174
expectedOpReturn
@@ -179,8 +197,8 @@ describe('Validate Util Tests', () => {
179197
transaction,
180198
currency,
181199
price,
182-
false,
183-
false,
200+
/*randomSatoshis=*/ false,
201+
/*disablePaymentId=*/ false,
184202
expectedPaymentId,
185203
expectedAmount,
186204
expectedOpReturn
@@ -209,38 +227,8 @@ describe('Validate Util Tests', () => {
209227
transaction,
210228
currency,
211229
price,
212-
false,
213-
false,
214-
expectedPaymentId,
215-
expectedAmount,
216-
expectedOpReturn
217-
);
218-
219-
expect(result).toBe(true);
220-
});
221-
222-
it('should ignore paymentId validation when disablePaymentId is true', async () => {
223-
const transaction: Transaction = {
224-
amount: '101.00',
225-
paymentId: '123',
226-
message: 'test opReturn',
227-
rawMessage: 'test opReturn',
228-
hash: '',
229-
timestamp: 0,
230-
address: 'ecash:qrmm7edwuj4jf7tnvygjyztyy0a0qxvl7quss2vxek',
231-
};
232-
const expectedPaymentId = undefined;
233-
const expectedAmount = resolveNumber('101.00');
234-
const expectedOpReturn = 'test opReturn';
235-
const price = 1;
236-
const currency = 'XEC';
237-
238-
const result = shouldTriggerOnSuccess(
239-
transaction,
240-
currency,
241-
price,
242-
false,
243-
true,
230+
/*randomSatoshis=*/ false,
231+
/*disablePaymentId=*/ false,
244232
expectedPaymentId,
245233
expectedAmount,
246234
expectedOpReturn
@@ -249,7 +237,7 @@ describe('Validate Util Tests', () => {
249237
expect(result).toBe(true);
250238
});
251239

252-
it('false when opReturn is undefined and received opReturn message is not empty or undefined', async () => {
240+
it('false when opReturn is undefined and received opReturn message is not empty', async () => {
253241
const transaction: Transaction = {
254242
amount: '101.00',
255243
paymentId: '123',
@@ -269,8 +257,8 @@ describe('Validate Util Tests', () => {
269257
transaction,
270258
currency,
271259
price,
272-
false,
273-
false,
260+
/*randomSatoshis=*/ false,
261+
/*disablePaymentId=*/ false,
274262
expectedPaymentId,
275263
expectedAmount,
276264
expectedOpReturn
@@ -279,6 +267,8 @@ describe('Validate Util Tests', () => {
279267
expect(result).toBe(false);
280268
});
281269

270+
// FIXME: I have no idea what is the use case for this, so assuming it's
271+
// useful. Fabien 2026-02-19
282272
it('true when opReturn is in a different format than received opReturn message but rawMessage matches expected opReturn', async () => {
283273
const transaction: Transaction = {
284274
amount: '101.00',
@@ -302,8 +292,8 @@ describe('Validate Util Tests', () => {
302292
transaction,
303293
currency,
304294
price,
305-
false,
306-
false,
295+
/*randomSatoshis=*/ false,
296+
/*disablePaymentId=*/ false,
307297
expectedPaymentId,
308298
expectedAmount,
309299
expectedOpReturn
@@ -332,8 +322,8 @@ describe('Validate Util Tests', () => {
332322
transaction,
333323
currency,
334324
price,
335-
false,
336-
false,
325+
/*randomSatoshis=*/ false,
326+
/*disablePaymentId=*/ false,
337327
expectedPaymentId,
338328
expectedAmount,
339329
expectedOpReturn
@@ -353,6 +343,10 @@ describe('Validate Util Tests', () => {
353343
address: 'ecash:qrmm7edwuj4jf7tnvygjyztyy0a0qxvl7quss2vxek',
354344
};
355345
const expectedPaymentId = '123';
346+
// FIXME: This value is unused in shouldTriggerOnSuccess (the amount is
347+
// compared to currencyObject.float divided by price) but it has to be
348+
// set nontheless otherwise the price is not even checked. It has to be
349+
// set to any value that don't convert to false. Fabien 2026-02-19
356350
const expectedAmount = resolveNumber('101.00');
357351
const expectedOpReturn = 'test opReturn';
358352
const price = 0.00003172;
@@ -367,8 +361,8 @@ describe('Validate Util Tests', () => {
367361
transaction,
368362
currency,
369363
price,
370-
false,
371-
false,
364+
/*randomSatoshis=*/ false,
365+
/*disablePaymentId=*/ false,
372366
expectedPaymentId,
373367
expectedAmount,
374368
expectedOpReturn,
@@ -378,6 +372,14 @@ describe('Validate Util Tests', () => {
378372
expect(result).toBe(true);
379373
});
380374

375+
// FIXME: this is another test for an actual bug. Here the
376+
// expectedPaymentId is defined and disablePaymentId is false so it's
377+
// explicitely asking for a payment ID match. However because the random
378+
// satoshis are another (vert bad) way to add entropy the
379+
// shouldTriggerOnSuccess assumes you don't need the payment ID and skips
380+
// the check. Random satoshis should be removed completely as it's a
381+
// terrible idea for a payment system to randomly change the price.
382+
// Fabien 2026-02-19
381383
it('true when randomSatoshis is true and paymentId does not match', async () => {
382384
const transaction: Transaction = {
383385
amount: '3152585.12',
@@ -403,8 +405,8 @@ describe('Validate Util Tests', () => {
403405
transaction,
404406
currency,
405407
price,
406-
true,
407-
false,
408+
/*randomSatoshis=*/ true,
409+
/*disablePaymentId=*/ false,
408410
expectedPaymentId,
409411
expectedAmount,
410412
expectedOpReturn,

‎react/lib/util/validate.ts‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,12 @@ export const shouldTriggerOnSuccess = (
4545
let isPaymentIdValid = true
4646
let isOpReturnValid = true
4747

48-
if(!randomSatoshis || randomSatoshis === 0){
49-
if(!isBCH){
50-
const paymentIdsMatch = expectedPaymentId === paymentId;
48+
if(!isBCH){
49+
if(!randomSatoshis || randomSatoshis === 0){
50+
const paymentIdsMatch = !!expectedPaymentId && expectedPaymentId === paymentId;
5151
isPaymentIdValid = disablePaymentId ? true : paymentIdsMatch;
5252
}
53-
}
54-
if(!isBCH){
53+
5554
const rawOpReturnIsEmptyOrUndefined = rawOpReturn === '' || rawOpReturn === undefined;
5655
const opReturn = rawOpReturnIsEmptyOrUndefined ? message : rawOpReturn
5756
const opReturnIsEmptyOrUndefined = opReturn === '' || opReturn === undefined;

0 commit comments

Comments
 (0)