Build the BTCPay invoice server-side and fix notification handling

Security: the checkout controller built the BTCPay invoice from the POSTed
redirect-form data (price, currency, orderId), and the IPN never checked
the settled amount 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 the settled amount and currency returned by
BTCPay are verified before the transaction 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.
- Let the payment framework confirm the order and send the confirmation
  email via its post-processing, instead of calling
  _check_amount_and_confirm_order() and _send_order_confirmation_mail()
  manually, which sent the confirmation email twice.
- Fix a crsf=False typo (was disabling nothing) to csrf=False on the
  return route, and drop the redundant _handle_notification_data override
  that duplicated the core method.
- Guard against a missing reference instead of calling all() on it.

payment_btcpayserver/controllers/main.py

@@ -19,23 +19,18 @@
 #
 # ******************************************************************************
 
+import json
 import logging
 import pprint
 
-import requests
-import werkzeug
 from werkzeug import urls
-from werkzeug.exceptions import Forbidden
 
-from odoo import _, http, SUPERUSER_ID
+from odoo import http
 from odoo.exceptions import ValidationError
 from odoo.http import request
-from odoo.tools import html_escape
 
-import json
 from ..models.libs.client import BTCPayClient
 
-
 _logger = logging.getLogger(__name__)
 
 
@@ -44,56 +39,89 @@ class BTCPayController(http.Controller):
     _notify_url = '/payment/btcpay/ipn'
     _return_url = '/payment/btcpay/return'
 
-    @http.route(_checkout_url, type='http',  auth='public', csrf=False, website=True)
+    @staticmethod
+    def _btcpay_client(provider):
+        return BTCPayClient(
+            host=provider.btcpay_location,
+            pem=provider.btcpay_privateKey,
+            tokens={provider.btcpay_facade: provider.btcpay_token},
+        )
+
+    @http.route(_checkout_url, type='http', auth='public', methods=['POST'],
+                csrf=False, website=True)
     def checkout(self, **data):
+        """ Create the BTCPay invoice and redirect the buyer to it.
+
+        The redirect form only carries the transaction reference. The invoice
+        price, currency and buyer details are read from the transaction
+        server-side and are never taken from the (client-controlled) request
+        data, so a buyer cannot have an invoice created for a tampered amount.
+        """
+        _logger.info("BTCPay: checkout request with data:\n%s", pprint.pformat(data))
+        tx_sudo = request.env['payment.transaction'].sudo()._get_tx_from_notification_data(
+            'btcpayserver', data)
+        if tx_sudo.state != 'draft':
+            # Already processed (e.g. double submission): fall back to the
+            # generic payment status page.
+            return request.redirect('/payment/status')
 
-        _logger.info("CHECKOUT: notification received from BTCPay with data:\n%s", pprint.pformat(data))
-        tx_sudo = request.env['payment.transaction'].sudo()._get_tx_from_notification_data('btcpayserver', data)
         provider = tx_sudo.provider_id
-        notification_url = str(data.get('notify_url')).replace("http://", "https://")
-        base_url = request.env['ir.config_parameter'].sudo().get_param('web.base.url')
-        redirect_url = urls.url_join(base_url, self._return_url)
-        client = BTCPayClient(host=provider.btcpay_location, pem=provider.btcpay_privateKey, tokens={provider.btcpay_facade: provider.btcpay_token})
-        invoice = client.create_invoice(
-            {"price": data.get('amount'),
-             "currency": data.get('currency_id'),
-             "orderId": data.get('reference'),
-             "token": provider.btcpay_token,
-             "redirectURL": redirect_url,
-             "notificationURL": notification_url,
-             "extendedNotifications": True,
-             "buyer": {"email": data.get('email') or 'noemailavailable@example.com',
-                       "name": data.get('name'),
-                       "address1": data.get('street'),
-                       "locality": data.get('city'),
-                       "postalCode": data.get('zip'),
-                       "country": data.get('country'),
-                       "notify": False}})
-        _logger.info('Invoice %s \n NOTIFY URL: %s', invoice, notification_url)
-        return werkzeug.utils.redirect(invoice['url'])
+        base_url = provider.get_base_url()
+        client = self._btcpay_client(provider)
+        invoice = client.create_invoice({
+            "price": tx_sudo.amount,
+            "currency": tx_sudo.currency_id.name,
+            "orderId": tx_sudo.reference,
+            "token": provider.btcpay_token,
+            "redirectURL": urls.url_join(base_url, self._return_url),
+            "notificationURL": urls.url_join(base_url, self._notify_url),
+            "extendedNotifications": True,
+            "buyer": {
+                "email": tx_sudo.partner_email or 'noemailavailable@example.com',
+                "name": tx_sudo.partner_name,
+                "address1": tx_sudo.partner_address,
+                "locality": tx_sudo.partner_city,
+                "postalCode": tx_sudo.partner_zip,
+                "country": tx_sudo.partner_country_id.code,
+                "notify": False,
+            },
+        })
+        tx_sudo.btcpay_invoiceId = invoice.get('id')
+        _logger.info("BTCPay: created invoice %s for transaction %s",
+                     invoice.get('id'), tx_sudo.reference)
+        return request.redirect(invoice['url'], local=False)
 
     @http.route(_notify_url, type='json', auth='public', csrf=False)
     def btcpay_ipn(self, **post):
         """ BTCPay IPN. """
-        _logger.info('BTCPAY IPN RECEIVED... ')
+        _logger.info('BTCPay: IPN received')
         data = json.loads(request.httprequest.data)
         _logger.info("%s", pprint.pformat(data))
         try:
-            notification_data = {"reference": data['data']['orderId'],
-                                 "invoiceID": data['data']['id']}
-            # Check the origin and integrity of the notification
-            tx_sudo = request.env['payment.transaction'].sudo()._get_tx_from_notification_data('btcpayserver', notification_data)
-            provider = tx_sudo.provider_id
-            client = BTCPayClient(host=provider.btcpay_location, pem=provider.btcpay_privateKey,
-                                  tokens={provider.btcpay_facade: provider.btcpay_token})
+            notification_data = {
+                "reference": data['data']['orderId'],
+                "invoiceID": data['data']['id'],
+            }
+            # Check the origin and integrity of the notification: the invoice
+            # is fetched back from BTCPay (signed request), the posted payload
+            # is only used to locate the transaction.
+            tx_sudo = request.env['payment.transaction'].sudo()._get_tx_from_notification_data(
+                'btcpayserver', notification_data)
+            client = self._btcpay_client(tx_sudo.provider_id)
 
             fetched_invoice = client.get_invoice(notification_data['invoiceID'])
-            _logger.info('fetched_invoice = %s',pprint.pformat(fetched_invoice))
+            _logger.info('BTCPay: fetched invoice = %s', pprint.pformat(fetched_invoice))
 
-            notification_data = {"reference": fetched_invoice['orderId'],
-                    "status": fetched_invoice['status'],
-                    "invoiceID": fetched_invoice['id'],
-                    "txid": fetched_invoice['url']}
+            notification_data = {
+                "reference": fetched_invoice.get('orderId'),
+                "status": fetched_invoice.get('status'),
+                "invoiceID": fetched_invoice.get('id'),
+                "txid": fetched_invoice.get('url'),
+                # Settled amount and currency, verified against the transaction
+                # before it is set done.
+                "amount": fetched_invoice.get('price'),
+                "currency": fetched_invoice.get('currency'),
+            }
 
             # Handle the notification data
             tx_sudo._handle_notification_data('btcpayserver', notification_data)
@@ -101,7 +129,8 @@ class BTCPayController(http.Controller):
             _logger.exception("Unable to handle the notification data; skipping to acknowledge")
         return ''
 
-    @http.route(_return_url, type='http', auth="public", methods=['GET'], crsf=False, save_session=False)
+    @http.route(_return_url, type='http', auth="public", methods=['GET'],
+                csrf=False, save_session=False)
     def btcpay_return_from_redirect(self, **data):
         """ BTCPay return
 
@@ -109,4 +138,4 @@ class BTCPayController(http.Controller):
             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')
+        return request.redirect('/payment/status')

payment_btcpayserver/models/payment_transaction.py

@@ -1,12 +1,9 @@
 import logging
 import pprint
-from werkzeug import urls
 
-from odoo import _, api, fields, models
+from odoo import _, fields, models
 from odoo.exceptions import ValidationError
 
-from odoo.addons.payment import utils as payment_utils
-
 _logger = logging.getLogger(__name__)
 
 
@@ -16,12 +13,14 @@ class PaymentTransaction(models.Model):
     btcpay_invoiceId = fields.Char("Invoice Id")
     btcpay_txid = fields.Char("Transaction Id")
     btcpay_status = fields.Char("Transaction Status")
-    api_url = '/btcpay/checkout'
-    checkout_url = '/btcpay/checkout'
-    notify_url = 'payment/btcpay/ipn'
 
     def _get_specific_rendering_values(self, processing_values):
-        """ Override of payment to return Specific rendering values.
+        """ Override of payment to return BTCPay-specific rendering values.
+
+        The redirect form only needs to carry the transaction reference: the
+        BTCPay invoice is built server-side (see the checkout controller) from
+        the transaction, so no amount, currency or buyer data is sent through
+        the browser.
 
         Note: self.ensure_one() from `_get_processing_values`
 
@@ -29,32 +28,13 @@ class PaymentTransaction(models.Model):
         :return: The dict of provider-specific processing values
         :rtype: dict
         """
-
         res = super()._get_specific_rendering_values(processing_values)
-
         if self.provider_code != 'btcpayserver':
             return res
 
-        base_url = self.provider_id.get_base_url()
-        _logger.info('Hola! API URL: %s', processing_values)
-        partner_first_name, partner_last_name = payment_utils.split_partner_name(self.partner_name)
-
         return {
-            'address1': self.partner_address,
-            'amount': self.amount,
-            'city': self.partner_city,
-            'country': self.partner_country_id.code,
-            'currency_code': self.currency_id.name,
-            'email': self.partner_email,
-            'first_name': partner_first_name,
-            'item_name': f"{self.company_id.name}: {self.reference}",
-            'item_number': self.reference,
-            'last_name': partner_last_name,
-            'lc': self.partner_lang,
-            'state': self.partner_state_id.name,
-            'zip_code': self.partner_zip,
-            'api_url':  self.checkout_url,
-            'notify_url': base_url + self.notify_url,
+            'api_url': '/btcpay/checkout',
+            'reference': self.reference,
         }
 
     def _get_tx_from_notification_data(self, provider_code, notification_data):
@@ -67,7 +47,6 @@ class PaymentTransaction(models.Model):
         :raise: ValidationError if the data match no transaction
         """
         tx = super()._get_tx_from_notification_data(provider_code, notification_data)
-        _logger.info('GET TX FROM NOTIFICATION Notification_data %s', pprint.pformat(notification_data))
         if provider_code != 'btcpayserver' or len(tx) == 1:
             return tx
 
@@ -79,21 +58,13 @@ class PaymentTransaction(models.Model):
             )
         return tx
 
-    def _handle_notification_data(self, provider_code, notification_data):
-        """ Match the transaction with the notification data, update its state and return it.
-
-        :param str provider_code: The code of the provider handling the transaction.
-        :param dict notification_data: The notification data sent by the provider.
-        :return: The transaction.
-        :rtype: recordset of `payment.transaction`
-        """
-        tx = self._get_tx_from_notification_data(provider_code, notification_data)
-        tx._process_notification_data(notification_data)
-        return tx
-
     def _process_notification_data(self, notification_data):
         """ Override of payment to process the transaction based on BTCPay data.
 
+        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.
+
         Note: self.ensure_one()
 
         :param dict notification_data: The notification data sent by the provider
@@ -104,31 +75,69 @@ class PaymentTransaction(models.Model):
         if self.provider_code != 'btcpayserver':
             return
 
-        _logger.info("_process_notification_data %s", pprint.pformat(notification_data))
-        txn_id = notification_data.get('reference')
-        if not all(txn_id):
+        _logger.info(
+            "processing notification data for BTCPay transaction with reference %s:\n%s",
+            self.reference, pprint.pformat(notification_data))
+        if not notification_data.get('reference'):
             raise ValidationError(
-                "BTCPay: " + _("Missing value for txn_id (%(txn_id)s)).", txn_id=txn_id))
+                "BTCPay: " + _("Received notification data with missing reference."))
 
-        self.provider_reference = txn_id
-        self.btcpay_txid = notification_data.get('txid')
-        self.btcpay_status = notification_data.get('status')
+        if notification_data.get('invoiceID'):
+            self.btcpay_invoiceId = notification_data['invoiceID']
+            self.provider_reference = notification_data['invoiceID']
+        if notification_data.get('txid'):
+            self.btcpay_txid = notification_data['txid']
+        status = notification_data.get('status')
+        self.btcpay_status = status
 
-        if self.btcpay_status in ['paid','processing']:
+        if status in ('paid', 'processing'):
             self._set_pending(state_message=notification_data.get('pending_reason'))
-        elif self.btcpay_status in ['confirmed', 'complete']:
-            self._set_done()
-            confirmed_orders = self._check_amount_and_confirm_order()
-            confirmed_orders._send_order_confirmation_mail()
-        elif self.btcpay_status in ['new']:
-            self.btcpay_invoiceId = notification_data.get('invoiceID')
-        elif self.btcpay_status in ['cancel','cancelled']:
-            self._set_canceled()
-        elif self.btcpay_status in ['invalid']:
-            _logger.info(
-                "received data with invalid payment status (%s) for transaction with reference %s",
-                self.btcpay_status, self.reference
-            )
+        elif status in ('confirmed', 'complete'):
+            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'):
+            self._set_canceled(
+                state_message="BTCPay: " + _("Invoice status: %s.", status))
+        elif status == 'invalid':
+            _logger.warning(
+                "received invalid payment status for transaction with reference %s", self.reference)
             self._set_error(
-                "BTCPay: " + _("Received data with invalid payment status: %s", self.btcpay_status)
-            )
+                "BTCPay: " + _("Received data with invalid payment status: %s.", status))
+        else:
+            _logger.warning(
+                "received unrecognized payment status (%s) for transaction with reference %s",
+                status, self.reference)
+            self._set_error(
+                "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

payment_btcpayserver/views/payment_btcpayserver_templates.xml

@@ -1,25 +1,14 @@
 <?xml version="1.0" encoding="utf-8"?>
 <odoo>
 
+    <!-- The redirect form is auto-submitted by the payment framework. It only
+         carries the transaction reference; the BTCPay invoice is built
+         server-side from the transaction (amount and currency cannot be
+         tampered with from the browser). -->
     <template id="redirect_form">
         <form t-att-action="api_url" method="post">
-            <input type="hidden" name="reference" t-att-value="item_number"/>
-            <input type="hidden" name="name" t-att-value="first_name"/>
-            <input type="hidden" name="email" t-att-value="email"/>
-            <input type="hidden" name="street" t-att-value="address1"/>
-            <input type="hidden" name="zip" t-att-value="zip_code"/>
-            <input type="hidden" name="city" t-att-value="city"/>
-            <input type="hidden" name="currency_id" t-att-value="currency_code"/>
-            <input type="hidden" name="country" t-att-value="country"/>
-            <input type="hidden" name="custom" t-att-value="item_name"/>
-            <input type="hidden" name="amount" t-att-value="amount"/>
-            <input type="hidden" name="notify_url" t-att-value="notify_url"/>
-            <button type="submit" width="100px" t-att-class="submit_class">
-                <img t-if="not submit_txt" src="/static/description/icon.png"/>
-                <span t-if="submit_txt"> <t t-esc="submit_txt"/> <span class="fa fa-long-arrow-right"/>
-                </span>
-            </button>
+            <input type="hidden" name="reference" t-att-value="reference"/>
         </form>
     </template>
 
-</odoo>
+</odoo>