Skip to content

Fix handling of empty payment IDs #621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Klakurka merged 2 commits into from
Feb 20, 2026
+117 −72
Merged

Fix handling of empty payment IDs #621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f9d52b
Fix handling of empty payment IDs
Fabcien Feb 19, 2026
00f524e
test: add (undefined, '') paymentId matching matrix test
chedieck Feb 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions react/lib/components/Widget/WidgetContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,12 +342,6 @@ export const WidgetContainer: React.FunctionComponent<WidgetContainerProps> =
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();

Expand Down
174 changes: 113 additions & 61 deletions react/lib/tests/util/validate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ describe('Validate Util Tests', () => {
transaction,
currency,
price,
false,
false,
/*randomSatoshis=*/ false,
/*disablePaymentId=*/ false,
expectedPaymentId,
expectedAmount,
expectedOpReturn
Expand All @@ -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: '',
Expand All @@ -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: '',
Expand All @@ -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 () => {
Expand All @@ -119,8 +137,8 @@ describe('Validate Util Tests', () => {
transaction,
currency,
price,
false,
false,
/*randomSatoshis=*/ false,
/*disablePaymentId=*/ false,
expectedPaymentId,
expectedAmount,
expectedOpReturn
Expand Down Expand Up @@ -149,8 +167,8 @@ describe('Validate Util Tests', () => {
transaction,
currency,
price,
false,
false,
/*randomSatoshis=*/ false,
/*disablePaymentId=*/ false,
expectedPaymentId,
expectedAmount,
expectedOpReturn
Expand Down Expand Up @@ -179,8 +197,8 @@ describe('Validate Util Tests', () => {
transaction,
currency,
price,
false,
false,
/*randomSatoshis=*/ false,
/*disablePaymentId=*/ false,
expectedPaymentId,
expectedAmount,
expectedOpReturn
Expand Down Expand Up @@ -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
Expand All @@ -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',
Expand All @@ -269,8 +257,8 @@ describe('Validate Util Tests', () => {
transaction,
currency,
price,
false,
false,
/*randomSatoshis=*/ false,
/*disablePaymentId=*/ false,
expectedPaymentId,
expectedAmount,
expectedOpReturn
Expand All @@ -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',
Expand All @@ -302,8 +292,8 @@ describe('Validate Util Tests', () => {
transaction,
currency,
price,
false,
false,
/*randomSatoshis=*/ false,
/*disablePaymentId=*/ false,
expectedPaymentId,
expectedAmount,
expectedOpReturn
Expand Down Expand Up @@ -332,8 +322,8 @@ describe('Validate Util Tests', () => {
transaction,
currency,
price,
false,
false,
/*randomSatoshis=*/ false,
/*disablePaymentId=*/ false,
expectedPaymentId,
expectedAmount,
expectedOpReturn
Expand All @@ -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;
Expand All @@ -367,8 +361,8 @@ describe('Validate Util Tests', () => {
transaction,
currency,
price,
false,
false,
/*randomSatoshis=*/ false,
/*disablePaymentId=*/ false,
expectedPaymentId,
expectedAmount,
expectedOpReturn,
Expand All @@ -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',
Expand All @@ -403,8 +405,8 @@ describe('Validate Util Tests', () => {
transaction,
currency,
price,
true,
false,
/*randomSatoshis=*/ true,
/*disablePaymentId=*/ false,
expectedPaymentId,
expectedAmount,
expectedOpReturn,
Expand All @@ -416,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<Transaction, 'paymentId'> = {
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',
Comment on lines +451 to +452
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

%s placeholders produce identical, unreadable test names for object-based test.each rows.

Jest's $variable syntax is required for object rows, and "you cannot use $variable with the printf formatting except for %%" — i.e. %s is only for Array<Array> rows where each element maps to a positional argument. With Array<Object> rows, all three %s resolve to [object Object], so all four matrix cases will surface with the same name when one fails, making it impossible to identify which combination broke.

🐛 Proposed fix — use `$propertyName` syntax 
-    'expectedPaymentId=%s tx.paymentId=%s -> %s',
+    'expectedPaymentId=$expLabel tx.paymentId=$txLabel -> $expected',
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test.each(cases)(
'expectedPaymentId=%s tx.paymentId=%s -> %s',
test.each(cases)(
'expectedPaymentId=$expLabel tx.paymentId=$txLabel -> $expected',
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@react/lib/tests/util/validate.test.ts` around lines 451 - 452, The test name
uses printf `%s` placeholders which render object rows as “[object Object]”;
update the `test.each(cases)` title to use Jest’s object interpolation with the
actual row property names — replace 'expectedPaymentId=%s tx.paymentId=%s -> %s'
with something like 'expectedPaymentId=$expectedPaymentId
tx.paymentId=$tx.paymentId -> $expected' (using the real property name used in
`cases` for the final field) so each object-based row produces unique, readable
test names.

(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);
},
);
});
9 changes: 4 additions & 5 deletions react/lib/util/validate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +50 to 51
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

rg -n --type=ts -A 10 -B 2 'shouldTriggerOnSuccess\s*\('

Repository: PayButton/paybutton

Length of output: 15179

🏁 Script executed:

rg -n --type=ts -C 5 'disablePaymentId'

Repository: PayButton/paybutton

Length of output: 50375

🏁 Script executed:

sed -n '87,113p' react/lib/tests/util/validate.test.ts

Repository: PayButton/paybutton

Length of output: 975

🏁 Script executed:

sed -n '42,85p' react/lib/tests/util/validate.test.ts

Repository: PayButton/paybutton

Length of output: 1407

🏁 Script executed:

sed -n '87,115p' react/lib/tests/util/validate.test.ts

Repository: PayButton/paybutton

Length of output: 1131

🏁 Script executed:

# Also check the full validate.ts function to understand all validation steps
cat -n react/lib/util/validate.ts | head -65

Repository: PayButton/paybutton

Length of output: 2735

🏁 Script executed:

# Check how PayButton sets up the checkSuccessInfo and what paymentId/disablePaymentId values flow through
sed -n '130,320p' react/lib/components/PayButton/PayButton.tsx | grep -A 50 -B 5 'checkSuccessInfo'

Repository: PayButton/paybutton

Length of output: 645

🏁 Script executed:

# Check how thisPaymentId is determined in PayButton
sed -n '130,250p' react/lib/components/PayButton/PayButton.tsx | grep -B 10 -A 3 'thisPaymentId\|paymentId ='

Repository: PayButton/paybutton

Length of output: 45

🏁 Script executed:

# Look at PayButton's useState and variable initialization for paymentId and disablePaymentId
sed -n '64,130p' react/lib/components/PayButton/PayButton.tsx

Repository: PayButton/paybutton

Length of output: 2377

🏁 Script executed:

# Check WidgetContainer logic around line 111 and how it determines disablePaymentId
sed -n '106,220p' react/lib/components/Widget/WidgetContainer.tsx

Repository: PayButton/paybutton

Length of output: 3742

🏁 Script executed:

# Check the test expectations more carefully - does it expect true or false?
sed -n '87,130p' react/lib/tests/util/validate.test.ts | tail -40

Repository: PayButton/paybutton

Length of output: 1488

🏁 Script executed:

# Check PayButton getPaymentId logic to confirm when paymentId is actually set
sed -n '136,162p' react/lib/components/PayButton/PayButton.tsx

Repository: PayButton/paybutton

Length of output: 872

🏁 Script executed:

# Check WidgetContainer initialization of paymentId and disablePaymentId flow
sed -n '154,250p' react/lib/components/Widget/WidgetContainer.tsx | grep -A 3 -B 3 'paymentId\|disablePaymentId'

Repository: PayButton/paybutton

Length of output: 505

🏁 Script executed:

# Check what isPropsTrue does
rg -n 'function isPropsTrue|const isPropsTrue' react/lib/

Repository: PayButton/paybutton

Length of output: 149

🏁 Script executed:

# Check isPropsTrue implementation
sed -n '66,75p' react/lib/util/format.ts

Repository: PayButton/paybutton

Length of output: 282

🏁 Script executed:

# Check if Widget/WidgetContainer has logic to fetch paymentId like PayButton does
rg -n 'getPaymentId\|createPayment' react/lib/components/Widget/

Repository: PayButton/paybutton

Length of output: 45

🏁 Script executed:

# Check if there's a way paymentId gets generated in WidgetContainer - check for useEffect with paymentId
sed -n '300,450p' react/lib/components/Widget/WidgetContainer.tsx | grep -A 20 'useEffect'

Repository: PayButton/paybutton

Length of output: 1509

🏁 Script executed:

# Check Widget.tsx to see if it has paymentId generation logic
rg -n 'getPaymentId\|createPayment' react/lib/components/Widget/Widget.tsx

Repository: PayButton/paybutton

Length of output: 45

🏁 Script executed:

# Search for where internalPaymentId gets set in WidgetContainer or if there's initialization logic
rg -n 'setInternalPaymentId\|setPaymentId' react/lib/components/Widget/WidgetContainer.tsx

Repository: PayButton/paybutton

Length of output: 45

🏁 Script executed:

# Check if WidgetContainer has any effect that initializes paymentId
sed -n '250,400p' react/lib/components/Widget/WidgetContainer.tsx | head -80

Repository: PayButton/paybutton

Length of output: 2127

🏁 Script executed:

# Get the full context of WidgetContainer to understand the complete flow from start to when shouldTriggerOnSuccess is called
wc -l react/lib/components/Widget/WidgetContainer.tsx

Repository: PayButton/paybutton

Length of output: 114

🏁 Script executed:

# Check the signature and props of WidgetContainer to see if paymentId is passed in
sed -n '20,50p' react/lib/components/Widget/WidgetContainer.tsx

Repository: PayButton/paybutton

Length of output: 981

🏁 Script executed:

# Check how PayButton initializes and manages the socket setup with checkSuccessInfo
sed -n '290,320p' react/lib/components/PayButton/PayButton.tsx

Repository: PayButton/paybutton

Length of output: 868

🏁 Script executed:

# Final check: look at the test setup for the critical case - what does the test comment say?
sed -n '87,120p' react/lib/tests/util/validate.test.ts

Repository: PayButton/paybutton

Length of output: 1340

🏁 Script executed:

# Verify the complete picture - check if there are any other callers besides PayButton and WidgetContainer
rg -n 'new WidgetContainer\|<WidgetContainer' react/lib/

Repository: PayButton/paybutton

Length of output: 45

Verify that WidgetContainer honors the implicit disablePaymentId = true contract when expectedPaymentId is absent.

The fix correctly prevents false positives by requiring expectedPaymentId to be truthy before matching. However, it introduces a critical implicit contract: when expectedPaymentId is absent or empty and disablePaymentId is false (its default), isPaymentIdValid becomes false, permanently blocking all transactions from triggering success.

This is safe in PayButton (which calls getPaymentId() to either fetch a valid paymentId or return undefined when disablePaymentId is true), but WidgetContainer violates it. WidgetContainer defaults disablePaymentId to false via isPropsTrue(props.disablePaymentId) (line 111) and accepts paymentId as an optional prop without fetching it. When WidgetContainer is instantiated without providing a paymentId and without explicitly setting disablePaymentId = true, every transaction will be rejected.

The test at lines 87-120 confirms this contract is intentional—the comment states: "Payment IDs are not set when the button is built so any tx with the same address and amount could match." Ensure WidgetContainer always sets disablePaymentId = true when no paymentId is provided or will be fetched.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@react/lib/util/validate.ts` around lines 50 - 51, WidgetContainer must honor
the implicit contract that payment checks are disabled when no paymentId will
ever be provided; update the WidgetContainer props normalization so that if
props.paymentId is missing/undefined (and WidgetContainer will not call
getPaymentId itself) you force disablePaymentId to true (i.e., set the value
used by validate.ts’s isPaymentIdValid calculation). Concretely, in the
WidgetContainer initialization (where isPropsTrue(props.disablePaymentId) is
used) detect absence of props.paymentId (and/or lack of a call to getPaymentId)
and override/assign disablePaymentId = true so isPaymentIdValid (and the logic
around expectedPaymentId/paymentId matching) does not block transactions when no
paymentId exists.

}
}
if(!isBCH){

const rawOpReturnIsEmptyOrUndefined = rawOpReturn === '' || rawOpReturn === undefined;
const opReturn = rawOpReturnIsEmptyOrUndefined ? message : rawOpReturn
const opReturnIsEmptyOrUndefined = opReturn === '' || opReturn === undefined;
Expand Down