Reconcile settlement-gate: full accept-side verification, D7/D8/D12 fixes

CHANGELOG.md

@@ -5,6 +5,130 @@ This project adheres to [Semantic Versioning](https://semver.org/).
 
 The entries below cover every release since the last tag, **v3.0.10**.
 
+## [Unreleased]
+
+Every payment-verification path now confirms, from Paystack's verify response,
+that the transaction actually settles the order before the order is advanced
+to Processing: transaction status must be `success`, the paid amount must
+cover the order total (in subunits), the currency must match the order's
+currency, the order must have been placed with the Paystack payment method,
+and the response's live/test `domain` must match the store's configured mode.
+Overpaying is accepted — normal when the customer bears Paystack's transaction
+fee — and recorded; where a verify response also reports the amount actually
+*requested* at initialize, that figure is checked against a tight window
+around the order's expected total (catching a wrong-exponent client bug
+regardless of any fee added on top), and the paid amount only has to cover
+what was requested.
+
+### Security
+- **A transaction that did not pay for an order can no longer advance it.**
+  Previously, the inline (popup) REST verification endpoint never checked the
+  verify response's status, and no path compared the paid amount or currency to
+  the order — a smaller or differently-denominated payment could mark an order
+  as Processing. All three paths (redirect callback, inline REST endpoint,
+  webhook) now share one settlement check (`Gateway/Validator/TransactionValidator`).
+- **`/paystack/payment/recreate` no longer cancels a paid order.** The route
+  only acts on orders still in the `new` or `pending_payment` state, and only
+  when the order was placed with the Paystack payment method; a
+  processing/complete order, or one paid via another method, can no longer
+  have its quote restored by an anonymous GET. (Side effect: an
+  already-cancelled order no longer re-triggers a quote restore — the first
+  call has already restored the quote.)
+- **The inline verification endpoint no longer leaks internal detail.** The
+  success response now returns only the transaction status and reference
+  (previously the full transaction object, including card BIN/last4, customer
+  email/phone, and IP, went to the browser), and error responses return a fixed
+  message instead of raw gateway/cURL text.
+- **The webhook's HMAC signature check no longer fails open on an
+  unconfigured secret key.** An empty Paystack secret key (a store never
+  configured, or misconfigured for the active mode) made the signature check
+  trivially satisfiable by anyone, since `hash_hmac()` against an empty key is
+  computable without knowing any secret. The check now rejects outright when
+  no secret key is configured.
+- **Both payment-verification observers are now also registered in the
+  `webapi_rest` area, not only `frontend`.** Previously
+  `etc/webapi_rest/events.xml` did not exist at all: the
+  `paystack_payment_verify_after` event (advances the order past pending and
+  sends the post-payment confirmation email) and `sales_order_place_before`
+  (suppresses the initial placement email until payment verifies) were only
+  wired in `etc/frontend/events.xml`. The inline flow's own REST endpoint
+  (`Model/PaymentManagement.php`) runs in the `webapi_rest` area, so neither
+  observer fired there — the module's default integration type sent the
+  placement confirmation email (that was never suppressed) but never
+  advanced the order or sent the post-payment confirmation. Registering only
+  `paystack_payment_verify_after` without also registering
+  `ObserverBeforeSalesOrderPlace` would have caused a duplicate email (the
+  unsuppressed placement email, plus a new post-payment one); both are
+  registered together.
+
+### Security
+- **A single Paystack reference can no longer settle two different orders
+  (D7, narrowed to a race window — not fully closed; see the reconciliation
+  plan's Risks section).** All three verification paths now bind the
+  reference to the order via a cross-order lookup before registering the
+  payment; a reference already bound to a different order is rejected
+  (`reference_bound_elsewhere`) rather than silently advancing whichever
+  order asks second.
+- **Verified payments are now actually registered against the order**
+  (`total_paid`, an invoice, and a `sales_payment_transaction` row) — D8's
+  accounting half. Previously, every consumer dispatched
+  `paystack_payment_verify_after` without ever calling
+  `registerCaptureNotification()`, so a "Processing" order had no invoice and
+  no recorded payment. Registration is idempotent per (reference, order): a
+  repeat verify of an already-registered payment is a no-op, not a duplicate
+  capture. An order no longer in a payable state (canceled/closed) is
+  rejected (`order_not_payable`).
+
+### Changed
+- **`Observer/ObserverAfterPaymentVerify.php` no longer advances the order
+  state itself.** `Model/PaymentSettlement::register()` (above) now owns that
+  side effect via `registerCaptureNotification()`; the observer is email-only,
+  gated on `!$order->getEmailSent()` instead of the order's status. The
+  human-readable "Paystack Payment Verified and Order is being processed"
+  history comment this observer used to write is gone — replaced by core's
+  own transaction-ID comment (`registerCaptureNotification()` →
+  `addTransactionCommentsToOrder()`), not an equivalent line.
+
+### Fixed
+- **Webhook responses now distinguish transient from permanent failures.**
+  Transient conditions (transaction still settling via bank transfer/USSD,
+  Paystack API errors, order not yet found) return HTTP 503 so Paystack retries
+  within its ~72h window — previously every outcome returned HTTP 200, which
+  silently cancelled retries and could permanently strand a legitimate payment's
+  confirmation. Genuine rejections (failed status, amount/currency mismatch)
+  return HTTP 200 so Paystack does not pointlessly retry.
+- **Rejected and surplus payments are now visible to the merchant on all three
+  paths, not only the webhook.** `Model/PaymentSettlement` writes an order
+  status-history comment (paid vs expected amount, reference) for every
+  settlement rejection and every overpayment, whichever of the redirect
+  callback, inline REST endpoint, or webhook produced it — previously only
+  the webhook recorded this, so a rejected callback/inline verification
+  (including a cross-order `reference_bound_elsewhere` hit) left zero
+  merchant-visible trace.
+- **A malformed inline verification reference no longer causes a 500** on the
+  anonymous REST route.
+- **After a payment fails post-charge on the inline flow, the Place Order
+  button is no longer re-enabled** — re-enabling it invited a double charge
+  while money was already moving.
+- **`Controller/Payment/Recreate.php` no longer calls the deprecated
+  `Order::save()`.** Order cancellation is now persisted through
+  `OrderRepositoryInterface::save()`.
+
+### Known limitations, not addressed by this change
+- The webhook's fallback lookup of an order by `quote_id`
+  (`Controller/Payment/Webhook.php`, used when the popup flow's
+  Paystack-generated reference has no matching order) still resolves an
+  ambiguous match via `getTotalCount() == 1`/`getFirstItem()` rather than
+  disambiguating by amount. This is a separate, still-open issue (tracked as
+  D9/R2.8) — not fixed by this settlement-gate work, which only changed the
+  webhook's *retry* semantics (transient vs. permanent), not its order-lookup
+  logic.
+
+### Upgrade note
+If a store's checkout was relying (unknowingly) on under- or mis-paid
+transactions being accepted, those orders now stay pending and the webhook
+records the mismatch in the order history. No configuration change is needed.
+
 ## [3.0.11] - 2026-08-17
 
 Corrects the transaction payload sent to Paystack: the amount is now always an

CLAUDE.md

@@ -108,27 +108,28 @@ There are two integration types, selectable in admin config:
 **Inline (default)**: Paystack popup opens in the browser after order is placed.
 1. JS calls `afterPlaceOrder()` → Paystack popup opens
 2. On success, JS calls `GET /V1/paystack/verify/{reference}_{quoteId}` (REST API, anonymous)
-3. `PaymentManagement::verifyPayment()` verifies with Paystack API, dispatches `paystack_payment_verify_after`
-4. `ObserverAfterPaymentVerify` sets order to Processing and sends confirmation email
+3. `PaymentManagement::verifyPayment()` verifies with Paystack API, calls `Model/PaymentSettlement::register()` (binds the reference, registers the payment, advances order state), then dispatches `paystack_payment_verify_after`
+4. `ObserverAfterPaymentVerify` sends the confirmation email (email-only — see below)
 
 **Standard (redirect)**: Customer is redirected to Paystack's hosted page.
 1. `/paystack/payment/setup` — initializes transaction, redirects to Paystack
-2. `/paystack/payment/callback` — Paystack returns here; verifies transaction, dispatches `paystack_payment_verify_after`
+2. `/paystack/payment/callback` — Paystack returns here; verifies transaction, calls `Model/PaymentSettlement::register()`, dispatches `paystack_payment_verify_after`
 3. `/paystack/payment/recreate` — retry path: cancels the failed/abandoned order, restores the quote, and redirects back to the checkout payment step
 
 **Webhook** (independent, server-to-server):
 - `/paystack/payment/webhook` — receives `charge.success` events from Paystack
-- Validates HMAC-SHA512 signature, verifies transaction, dispatches `paystack_payment_verify_after`
+- Validates HMAC-SHA512 signature, verifies transaction, calls `Model/PaymentSettlement::register()`, dispatches `paystack_payment_verify_after`
 - CSRF validation skipped via `Plugin/CsrfValidatorSkip.php`
 
-The custom event `paystack_payment_verify_after` is the single point where order status is updated to Processing and confirmation email is sent (`Observer/ObserverAfterPaymentVerify.php`). Initial order confirmation email is suppressed by `ObserverBeforeSalesOrderPlace` until payment is verified.
+`Model/PaymentSettlement::register()` is the single class that binds the Paystack reference to an order, registers the captured payment (`total_paid`, invoice, transaction row via `registerCaptureNotification()`), and records rejection history — for all three verification paths above, before any of them dispatches `paystack_payment_verify_after`. Each consumer dispatches the settled order instance `register()` returns, not its own stale one. `ObserverAfterPaymentVerify.php` no longer advances order state at all: it is email-only now, gated on `!$order->getEmailSent()` (register() already saved the order by the time the observer runs). Initial order confirmation email is suppressed by `ObserverBeforeSalesOrderPlace` until payment is verified. Both observers must be registered in **every** DI area a checkout path can place/verify an order from — `Model/PaymentManagement.php` (the inline flow's default integration type) runs in `webapi_rest`, not `frontend`, so both `etc/frontend/events.xml` and `etc/webapi_rest/events.xml` register both events; registering only one observer in `webapi_rest` without the other caused either a missing post-payment confirmation (inline orders stuck unadvanced) or a duplicate confirmation email (placement email unsuppressed, post-payment email also sent).
 
 ### Key Classes
 
 | Class | Responsibility |
 |---|---|
 | `Gateway/PaystackApiClient.php` | All Paystack API calls: initialize transaction, verify, validate webhook signature |
 | `Model/PaymentManagement.php` | REST API endpoint for inline payment verification |
+| `Model/PaymentSettlement.php` | Binds the Paystack reference to an order, registers the captured payment, and records rejection/overpayment history — shared by all three verification paths |
 | `Model/Ui/ConfigProvider.php` | Injects public key, integration type, and URLs into checkout JS config |
 | `Controller/Payment/AbstractPaystackStandard.php` | Base controller with shared utilities (quote loading, message handling) |
 | `etc/csp_whitelist.xml` | Whitelists Paystack domains in Magento's Content Security Policy (additive; the Magento-standard mechanism) |

Controller/Payment/AbstractPaystackStandard.php

@@ -82,6 +82,18 @@ abstract class AbstractPaystackStandard extends \Magento\Framework\App\Action\Ac
      */
     protected $request;
 
+    /**
+     *
+     * @var \Pstk\Paystack\Gateway\Validator\TransactionValidator
+     */
+    protected $transactionValidator;
+
+    /**
+     *
+     * @var \Pstk\Paystack\Model\PaymentSettlement
+     */
+    protected $paymentSettlement;
+
     /**
      * Constructor
      *
@@ -101,7 +113,9 @@ public function __construct(
             \Magento\Framework\Event\Manager $eventManager,
             \Magento\Framework\App\Request\Http $request,
             \Psr\Log\LoggerInterface $logger,
-            PaystackApiClient $paystackClient
+            PaystackApiClient $paystackClient,
+            ?\Pstk\Paystack\Gateway\Validator\TransactionValidator $transactionValidator = null,
+            ?\Pstk\Paystack\Model\PaymentSettlement $paymentSettlement = null
     ) {
         $this->resultPageFactory = $resultPageFactory;
         $this->orderRepository = $orderRepository;
@@ -115,6 +129,25 @@ public function __construct(
         $this->request = $request;
         $this->logger = $logger;
         $this->paystackClient = $paystackClient;
+        // Defaulted, not because the settlement gate is optional — it is not, and
+        // a null here would silently disable it — but because this is a public
+        // abstract base in a Marketplace-distributed module. A required parameter
+        // added to it fatals any third-party subclass on upgrade. Magento's own
+        // convention for widening a released constructor applies: fall back to the
+        // ObjectManager so the guard is always present either way.
+        $this->transactionValidator = $transactionValidator
+            ?: \Magento\Framework\App\ObjectManager::getInstance()
+                ->get(\Pstk\Paystack\Gateway\Validator\TransactionValidator::class);
+        // Same nullable+ObjectManager-fallback pattern, same BC reasoning —
+        // wired in via a \Pstk\Paystack\Model\PaymentSettlement\Proxy in
+        // etc/frontend/di.xml so Setup.php/Recreate.php, which never call it,
+        // don't eagerly build its dependency graph. Only TransactionRepositoryInterface
+        // and SearchCriteriaBuilder are actually deferred by the Proxy —
+        // OrderRepositoryInterface is already an eager, direct constructor
+        // dependency of this class itself, so the Proxy buys it nothing.
+        $this->paymentSettlement = $paymentSettlement
+            ?: \Magento\Framework\App\ObjectManager::getInstance()
+                ->get(\Pstk\Paystack\Model\PaymentSettlement::class);
 
         parent::__construct($context);
     }