Fix amount tampering, broken IPN URL and invoice status handling (19.0) - #19
Open
SatoDri wants to merge 3 commits into
Open
Fix amount tampering, broken IPN URL and invoice status handling (19.0)#19SatoDri wants to merge 3 commits into
SatoDri wants to merge 3 commits into
Conversation
The bundled BTCPay client (models/libs) imports ecdsa, while the manifest and requirements only listed btcpay-python, which is not used (the client is vendored). Installations that did not already have ecdsa available failed the external-dependency check. Declare ecdsa instead and bump the module version.
The override guarded on self.code, which is empty on the model-level recordset passed to create(), so the branch never ran. The private key is generated during pairing (in the onchange) instead, so the override is dead code; removing it also keeps copying a provider from regenerating (and thus invalidating) an already paired key.
Security: the checkout controller built the BTCPay invoice from the POSTed redirect-form data (price, currency, orderId), and _extract_amount_data returned None to opt out of amount validation, so the settled amount was never checked against the transaction. A buyer could therefore submit an arbitrary price and confirm an order after paying far less. The invoice is now built server-side from the transaction, the redirect form only carries the reference, and _extract_amount_data returns the amount and currency settled on BTCPay (read back from the invoice in the IPN) so the framework validates them against the transaction before it is set done. Other fixes in the notification path: - Fix the IPN notification URL: it was concatenated without a leading slash (base_url + "payment/btcpay/ipn"), producing an unreachable host so notifications never arrived. It is now built with urls.url_join. - Handle the "expired" invoice status (cancel the transaction) instead of leaving it pending forever, and set an error on unknown statuses.
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
1 participant
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.
What this fixes
This PR fixes a security issue and a couple of bugs in the checkout and notification flow of
payment_btcpayserveron the19.0branch. (A companion PR does the same on18.0: #18.)Security: the invoice amount could be tampered with
The checkout controller built the BTCPay invoice from the redirect form's POST data (
price,currency,orderId), and_extract_amount_datareturnedNoneto opt out of the framework's amount validation. Nothing ever compared the settled amount to the transaction, so a buyer could submit an arbitraryamount, pay that (e.g. a few cents) and still have the order confirmed for its full value.Fix:
_extract_amount_datanow returns the amount and currency actually settled on BTCPay (read back from the invoice in the IPN), so the generic_validate_amountcompares them against the transaction before it is set todone.Other fixes
base_url + "payment/btcpay/ipn"), producing an unreachable host, so payments could never be confirmed. It is now built withurls.url_join.expiredinvoices stayed pending forever: theexpiredstatus is now handled (transaction cancelled), and unknown statuses set an error instead of being ignored.payment.provider.create()override (it guarded on an emptyself.code, so it never ran; the private key is generated during pairing).models/libs) importsecdsa, while the manifest listed the unusedbtcpay-python.Notes
versionbumped to19.0.1.1._process/_extract_amount_data/_apply_updates); the double-confirmation-email andcrsftypo issues present on 18.0 are already gone here thanks to that refactor.pyflakesclean); I have not run it against a live BTCPay store on this branch, so a functional test on your side is welcome.