fix: prevent payment race condition with atomic balance check#1424
Open
theeggorchicken wants to merge 1 commit intoidurar:devfrom
Open
fix: prevent payment race condition with atomic balance check#1424theeggorchicken wants to merge 1 commit intoidurar:devfrom
theeggorchicken wants to merge 1 commit intoidurar:devfrom
Conversation
The payment controller reads the invoice balance, checks if the requested amount fits, then creates the payment and increments credit in separate operations. Concurrent requests all read the same balance, all pass the check, and all create payments - resulting in credit exceeding the invoice total. Tested: 10 concurrent $500 payments against a $500 invoice all succeeded, producing $5,000 in credit on a $500 invoice. Replaces the read-then-check-then-write pattern with a single findOneAndUpdate using $expr to atomically verify remaining balance and reserve credit. Only one concurrent request can succeed when the remaining balance is insufficient for multiple payments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The payment controller in
create.jsreads the invoice, checks remaining balance, then creates the payment and updates the invoice in separate operations. There's no locking between the read and the write. If you fire multiple payment requests at the same time, they all read the same balance, all pass the check, and all go through.I tested this with 10 concurrent $500 payment requests against a $500 invoice. All 10 succeeded, leaving the invoice with $5,000 in credit on a $500 total.
Vulnerable Lines
File:
backend/src/controllers/appControllers/paymentController/create.js#L19-L41The window between
findOneandcreateis wide enough that even a single-process Node.js server allows the race. No special multi-worker configuration needed.How to verify
Then check the invoice:
What this PR does
Replaces the read-then-check-then-write pattern with a single
findOneAndUpdatethat uses$exprto atomically verify the remaining balance and reserve the credit in one operation. MongoDB guaranteesfindOneAndUpdateis atomic at the document level, so only one concurrent request can claim the remaining balance.If
invoiceUpdateis null, the balance was insufficient (another request got there first), and we return the same "max amount" error as before.Evidence
Full
curl --trace-asciicaptures and parallel race results: evidence gistRelated Issue
Security fix -- TOCTOU race condition in payment processing.
Steps to Test
Checklist