Merge pull request #18 from SatoDri/18.0-fix-security-and-ipn

Fix amount tampering, broken IPN URL and invoice status handling

README.md

@@ -1,12 +1,12 @@
-# BTCPay Server payment gateway for Odoo 16, 17, 18 and 19
+# BTCPay Server payment gateway for Odoo 16, 17 and 18
 
-## Accept bitcoin payments on Odoo 16, 17, 18 and 19
+## Accept bitcoin payments on Odoo 16, 17 and 18
 This odoo addon allows you to accept bitcoin and Lightning Network (and other cryptocurrency) payments in your Odoo e-commerce store.  
 ![BTCPay Server Banner](https://raw.githubusercontent.com/btcpayserver/odoo/17.0/payment_btcpayserver/static/description/BTCPay-Odoo-17-featured.png)
 
 ## How to install
 
 Please find the installation instructions here:
-- [Odoo 18 + 19 documentation](https://github.com/btcpayserver/odoo/blob/19.0/payment_btcpayserver/README.md)
+- [Odoo 18 documentation](https://github.com/btcpayserver/odoo/blob/18.0/payment_btcpayserver/README.md)
 - [Odoo 17 documentation](https://github.com/btcpayserver/odoo/blob/17.0/payment_btcpayserver/README.md)
 - [Odoo 16 documentation](https://github.com/btcpayserver/odoo/blob/16.0/payment_btcpayserver/README.md)

payment_btcpayserver/README.md

@@ -1,6 +1,6 @@
-# BTCPay Server payment gateway for Odoo 18 / 19
+# BTCPay Server payment gateway for Odoo 18
 
-## This is the module to connect Odoo 18 / 19 and BTCPay Server
+## This is the module to connect Odoo 18 and BTCPay Server
 This module allows you to accept bitcoin (and other cryptocurrency) payments in your Odoo e-commerce store.  
 ![BTCPay Server Banner](https://raw.githubusercontent.com/btcpayserver/odoo/18.0/payment_btcpayserver/static/description/BTCPay-Odoo-17-featured.png)
 
@@ -8,13 +8,9 @@ This module allows you to accept bitcoin (and other cryptocurrency) payments in
 If you use Odoo 16 you can find the documentation [here](https://github.com/btcpayserver/odoo/blob/16.0/payment_btcpayserver/README.md) and for Odoo 17 [here](https://github.com/btcpayserver/odoo/blob/17.0/payment_btcpayserver/README.md).
 :::
 
-:::tip
-The following instructions work for Odoo 18 and 19. You just need to make sure to download the matching branch release or use the matching branch (18.0 or 19.0) mentioned in the steps below.
-:::
-
 ## Install the module
 * Clone our [repository](https://github.com/btcpayserver/odoo) or download the .zip from the [releases page](https://github.com/btcpayserver/odoo/releases)
-* Make sure you are on branch `19.0` (or `18.0`) or downloaded a [release](https://github.com/btcpayserver/odoo/releases) tagged with version v19.x (or v18.x)
+* Make sure you are on branch `18.0` or downloaded a release tagged with version v18.x
 * Place the `payment_btcpayserver` directory in your Odoo addons directory
 * Install dependencies by running `pip install -r requirements.txt` (from inside the `payment_btcpayserver` directory)
 * Restart Odoo
@@ -29,16 +25,15 @@ The following instructions work for Odoo 18 and 19. You just need to make sure t
 In the BTCPay settings form, tab "Credentials":
 * Set field "State" to enabled
 * Set field "BTCPay Server URL" as test or live URL including https://. Example URL: https://testnet.demo.btcpayserver.org
-* Get a pairing code from your BTCPay Server store: 
+* Get a pairing code from your BTCPay Server store: Settings -> Access Tokens
-  * Go to: Settings -> Access Tokens
   * Click on "Create Token" button
   * Label: enter e.g. "My odoo store"
   * Public Key: leave empty
   * Click on "Request Pairing" button, on next page click "Approve" button
   * At the top copy the code next to "Server initiated pairing code", e.g. "hg7z8wN"
-* Back in Odoo: paste the code into "Pairing Code" field
+* Back in Odoo, paste the code into "Pairing Code" field
 * Hit Tab key on your keyboard (or click on another field) and the pairing process will start automatically
-* When the pairing is successful the "Token" and "Private Key" field will be filled automatically (see screenshot below)
+* When the pairing is successful the "Token" and "Private Key" field will be filled automatically
 * Field Facade, keep default 'merchant'.
 
 On the tab "Configuration":

payment_btcpayserver/__manifest__.py

@@ -21,11 +21,11 @@
 
 {
     'name': 'Payment Provider: BTCPay Server',
-    'summary': 'This module integrates BTCPAY - pay with Bitcoin - with Odoo v19.0',
+    'summary': 'This module integrates BTCPAY - pay with Bitcoin - with Odoo v18.0',
     'author': 'BTCPay Server team and contributors',
     'website': 'https://github.com/btcpayserver/odoo',
     'category': 'Accounting/Payment Providers',
-    'version': '19.0.1.1',
+    'version': '18.0.1.1',
     'license': 'GPL-3',
     'currency': 'USD',
     'application': False,

payment_btcpayserver/controllers/main.py

@@ -20,6 +20,7 @@
 # ******************************************************************************
 
 import json
+import logging
 
 from werkzeug import urls
 
@@ -27,11 +28,9 @@ from odoo import _, http
 from odoo.exceptions import ValidationError
 from odoo.http import request
 
-from odoo.addons.payment.logging import get_payment_logger
 from ..models.libs.client import BTCPayClient
 
-
+_logger = logging.getLogger(__name__)
-_logger = get_payment_logger(__name__)
 
 
 class BTCPayController(http.Controller):
@@ -47,13 +46,6 @@ class BTCPayController(http.Controller):
             tokens={provider.btcpay_facade: provider.btcpay_token},
         )
 
-    @staticmethod
-    def _find_transaction(reference):
-        return request.env['payment.transaction'].sudo().search([
-            ('reference', '=', reference),
-            ('provider_code', '=', 'btcpayserver'),
-        ], limit=1)
-
     @staticmethod
     def _lock_transaction(tx_sudo):
         """Lock a transaction until the current database transaction ends.
@@ -184,11 +176,8 @@ class BTCPayController(http.Controller):
         data, so a buyer cannot have an invoice created for a tampered amount.
         """
         _logger.info("BTCPay: checkout request for transaction %s", data.get('reference'))
-        reference = data.get('reference')
+        tx_sudo = request.env['payment.transaction'].sudo()._get_tx_from_notification_data(
-        tx_sudo = self._find_transaction(reference)
+            'btcpayserver', data)
-        if not tx_sudo:
-            raise ValidationError(
-                _("BTCPay: No transaction found matching reference %s.", reference))
         self._lock_transaction(tx_sudo)
         if tx_sudo.state != 'draft':
             # Already processed (e.g. double submission): fall back to the
@@ -250,7 +239,7 @@ class BTCPayController(http.Controller):
                      invoice['id'], tx_sudo.reference)
         return request.redirect(invoice['url'], local=False)
 
-    @http.route(_notify_url, type='jsonrpc', auth='public', methods=['POST'], csrf=False)
+    @http.route(_notify_url, type='json', auth='public', methods=['POST'], csrf=False)
     def btcpay_ipn(self, **post):
         """ BTCPay IPN. """
         _logger.info('BTCPay: IPN received')
@@ -285,25 +274,30 @@ class BTCPayController(http.Controller):
                 fetched_invoice.get('id'), fetched_invoice.get('status'), tx_sudo.reference,
             )
 
-            payment_data = {
+            notification_data = {
                 "reference": tx_sudo.reference,
                 "status": fetched_invoice.get('status'),
                 "invoiceID": incoming_invoice_id,
                 "txid": fetched_invoice.get('url'),
-                # Settled amount and currency, validated against the transaction
+                # Settled amount and currency, verified against the transaction
-                # by the payment framework before it is set done.
+                # before it is set done.
                 "amount": fetched_invoice.get('price'),
                 "currency": fetched_invoice.get('currency'),
             }
 
-            tx_sudo._process('btcpayserver', payment_data)
+            # Handle the notification data
-        except ValidationError as error:
+            tx_sudo._handle_notification_data('btcpayserver', notification_data)
+        except ValidationError as error:  # Acknowledge rejected notifications without retries.
             _logger.warning("BTCPay: rejected notification: %s", error)
         return ''
 
     @http.route(_return_url, type='http', auth="public", methods=['GET'],
                 csrf=False, save_session=False)
     def btcpay_return_from_redirect(self, **data):
-        """ BTCPay return """
+        """ BTCPay return
+
+            We could process and check the invoice status here but there is no need to as the status get's updated via
+            IPN anyway, so just show the user the order confirmation / payment status page.
+        """
         _logger.info("BTCPay: user returned to shop after payment")
         return request.redirect('/payment/status')

payment_btcpayserver/models/payment_provider.py

@@ -1,11 +1,10 @@
+import logging
+
 from odoo import api, fields, models
 from .libs.client import BTCPayClient
 from .libs import crypto
 
-from odoo.addons.payment.logging import get_payment_logger
+_logger = logging.getLogger(__name__)
-
-
-_logger = get_payment_logger(__name__)
 
 
 class PaymentProvider(models.Model):
@@ -24,6 +23,7 @@ class PaymentProvider(models.Model):
     @api.onchange('btcpay_pairingCode')
     def _onchange_pairingCode(self):
         if not self.btcpay_token and self.code == 'btcpayserver' and self.btcpay_pairingCode:
+            #_logger.info("ONCHANGE PAIRING CODE***SELF:  %s %s %s", self.btcpay_location, self.btcpay_privateKey, self.btcpay_pairingCode)
             self.btcpay_privateKey = crypto.generate_privkey()
             client = BTCPayClient(host=self.btcpay_location, pem=self.btcpay_privateKey)
             token = client.pair_client(self.btcpay_pairingCode)
@@ -33,10 +33,13 @@ class PaymentProvider(models.Model):
     def _onchange_token(self):
         if self.code == 'btcpayserver':
             self.btcpay_pairingCode = ''
+            #_logger.info("ONCHANGE TOKEN")
+
 
     @api.onchange('btcpay_location')
     def _onchange_location(self):
         if self.code == 'btcpayserver':
             self.btcpay_token = ''
+            #_logger.info("ONCHANGE LOCATION ***SELF:  %s %s %s", self.btcpay_location, self.btcpay_privateKey, self.btcpay_pairingCode)
             self.btcpay_privateKey = ''
             self.btcpay_pairingCode = ''

payment_btcpayserver/models/payment_transaction.py

@@ -1,12 +1,10 @@
+import logging
 import pprint
 
-from odoo import _, api, fields, models
+from odoo import _, fields, models
 from odoo.exceptions import ValidationError
 
-from odoo.addons.payment.logging import get_payment_logger
+_logger = logging.getLogger(__name__)
-
-
-_logger = get_payment_logger(__name__)
 
 
 class PaymentTransaction(models.Model):
@@ -37,82 +35,66 @@ class PaymentTransaction(models.Model):
         return {
             'api_url': '/btcpay/checkout',
             'reference': self.reference,
-            # Keep the pre-19.0.1.1 key during rolling deployments. QWeb
-            # templates live in the database, so an instance restarted before
-            # the module is upgraded can still render the previous template.
-            'item_number': self.reference,
         }
 
-    @api.model
+    def _get_tx_from_notification_data(self, provider_code, notification_data):
-    def _extract_reference(self, provider_code, payment_data):
+        """ Override of payment to find the transaction based on BTCPay data.
-        """ Override of payment to extract the transaction reference from BTCPay data.
 
-        :param str provider_code: The code of the provider handling the transaction.
+        :param str provider_code: The code of the provider that handled the transaction
-        :param dict payment_data: The payment data sent by the provider.
+        :param dict notification_data: The notification data sent by the provider
-        :return: The transaction reference.
+        :return: The transaction if found
-        :rtype: str
+        :rtype: recordset of `payment.transaction`
+        :raise: ValidationError if the data match no transaction
         """
-        if provider_code != 'btcpayserver':
+        tx = super()._get_tx_from_notification_data(provider_code, notification_data)
-            return super()._extract_reference(provider_code, payment_data)
+        if provider_code != 'btcpayserver' or len(tx) == 1:
+            return tx
 
-        return payment_data.get('reference')
+        reference = notification_data.get('reference')
+        tx = self.search([('reference', '=', reference), ('provider_code', '=', 'btcpayserver')])
+        if not tx:
+            raise ValidationError(
+                "BTCPay: " + _("No transaction found matching reference %s.", reference)
+            )
+        return tx
 
-    def _extract_amount_data(self, payment_data):
+    def _process_notification_data(self, notification_data):
-        """ Override of payment to return the settled amount and currency.
-
-        The amount and currency are read back from BTCPay on notification and
-        returned here so that the generic amount validation compares them
-        against the transaction. This prevents a tampered or mismatched invoice
-        from confirming the transaction (and thus the order).
-
-        :param dict payment_data: The payment data sent by the provider.
-        :return: The settled amount data, or ``None`` for other providers.
-        :rtype: dict|None
-        """
-        if self.provider_code != 'btcpayserver':
-            return super()._extract_amount_data(payment_data)
-
-        try:
-            amount = float(payment_data.get('amount'))
-        except (TypeError, ValueError):
-            amount = None
-        return {
-            'amount': amount,
-            'currency_code': payment_data.get('currency'),
-        }
-
-    def _apply_updates(self, payment_data):
         """ Override of payment to process the transaction based on BTCPay data.
 
-        Note: self.ensure_one() from `_process`
+        Only the transaction state is updated here; confirming the related
+        sale order, creating the payment and sending the confirmation email are
+        handled by the generic post-processing of the `payment` framework.
 
-        :param dict payment_data: The payment data sent by the provider.
+        Note: self.ensure_one()
+
+        :param dict notification_data: The notification data sent by the provider
         :return: None
+        :raise: ValidationError if inconsistent data were received
         """
+        super()._process_notification_data(notification_data)
         if self.provider_code != 'btcpayserver':
-            return super()._apply_updates(payment_data)
+            return
 
-        _logger.info("BTCPay _apply_updates: %s", pprint.pformat(payment_data))
+        _logger.info(
-
+            "processing notification data for BTCPay transaction with reference %s:\n%s",
-        if not payment_data.get('reference'):
+            self.reference, pprint.pformat(notification_data))
+        if not notification_data.get('reference'):
             raise ValidationError(
-                "BTCPay: " + _("Received payment data with missing reference."))
+                "BTCPay: " + _("Received notification data with missing reference."))
 
-        # Keep both external-reference fields bound to the authenticated
+        if notification_data.get('invoiceID'):
-        # BTCPay invoice. The controller validates this identity before
+            self.btcpay_invoiceId = notification_data['invoiceID']
-        # entering `_process`.
+            self.provider_reference = notification_data['invoiceID']
-        if invoice_id := payment_data.get('invoiceID'):
+        if notification_data.get('txid'):
-            self.btcpay_invoiceId = invoice_id
+            self.btcpay_txid = notification_data['txid']
-            self.provider_reference = invoice_id
+        status = notification_data.get('status')
-        if payment_data.get('txid'):
-            self.btcpay_txid = payment_data['txid']
-        status = payment_data.get('status')
         self.btcpay_status = status
 
         if status in ('paid', 'processing'):
-            self._set_pending(state_message=payment_data.get('pending_reason'))
+            self._set_pending(state_message=notification_data.get('pending_reason'))
         elif status in ('confirmed', 'complete'):
-            self._set_done()
+            if self._btcpay_verify_amount(notification_data):
+                self._set_done()
         elif status == 'new':
             pass  # Invoice created on BTCPay, waiting for the buyer to pay.
         elif status in ('expired', 'cancel', 'cancelled'):
@@ -120,17 +102,42 @@ class PaymentTransaction(models.Model):
                 state_message="BTCPay: " + _("Invoice status: %s.", status))
         elif status == 'invalid':
             _logger.warning(
-                "Received data with invalid payment status (%s) for transaction with reference %s",
+                "received invalid payment status for transaction with reference %s", self.reference)
-                status, self.reference
-            )
             self._set_error(
-                "BTCPay: " + _("Received data with invalid payment status: %s.", status)
+                "BTCPay: " + _("Received data with invalid payment status: %s.", status))
-            )
         else:
             _logger.warning(
-                "Received data with unknown payment status (%s) for transaction with reference %s",
+                "received unrecognized payment status (%s) for transaction with reference %s",
-                status, self.reference
+                status, self.reference)
-            )
             self._set_error(
-                "BTCPay: " + _("Received data with unknown payment status: %s.", status)
+                "BTCPay: " + _("Received data with unknown payment status: %s.", status))
-            )
+
+    def _btcpay_verify_amount(self, notification_data):
+        """ Check that the settled BTCPay invoice matches the transaction.
+
+        The BTCPay invoice is created from the transaction, but its settled
+        amount and currency are read back from BTCPay on notification and
+        compared here so that a tampered or mismatched invoice can never
+        confirm the transaction (and thus the order). On mismatch the
+        transaction is set in error and ``False`` is returned.
+
+        :param dict notification_data: The notification data sent by the provider
+        :return: Whether the settled amount and currency match the transaction
+        :rtype: bool
+        """
+        self.ensure_one()
+        currency = notification_data.get('currency')
+        try:
+            amount = float(notification_data.get('amount'))
+        except (TypeError, ValueError):
+            amount = None
+        if (amount is None
+                or currency != self.currency_id.name
+                or self.currency_id.compare_amounts(amount, self.amount) != 0):
+            self._set_error("BTCPay: " + _(
+                "The settled amount (%(amount)s %(currency)s) does not match the "
+                "expected amount (%(expected)s %(expected_currency)s).",
+                amount=notification_data.get('amount'), currency=currency,
+                expected=self.amount, expected_currency=self.currency_id.name))
+            return False
+        return True