Merge pull request #2 from silverstripe-security/payment-completion-2

[CVE-2022-29254] Add extra validation on payment completion
silverstripe · May 25, 2022 · 2ac483e · 2ac483e
2 parents d1b0873 + f4c043a
commit 2ac483e
Show file tree

Hide file tree

Showing 8 changed files with 58 additions and 54 deletions.
diff --git a/code/Service/AuthorizeService.php b/code/Service/AuthorizeService.php
@@ -60,7 +60,7 @@ public function initiate($data = array())
             );
         } elseif ($serviceResponse->isError()) {
             $this->createMessage('AuthorizeError', $response);
-        } else {
+        } elseif ($serviceResponse->isSuccessful()) {
             $this->markCompleted('Authorized', $serviceResponse, $response);
         }
 
@@ -110,15 +110,12 @@ public function complete($data = array(), $isNotification = false)
 
         $serviceResponse = $this->wrapOmnipayResponse($response, $isNotification);
 
-        if ($serviceResponse->isError()) {
+        if ($serviceResponse->isAwaitingNotification()) {
+            Helper::safeExtend($this->payment, 'onAwaitingAuthorized', $serviceResponse);
+        } elseif ($serviceResponse->isError()) {
             $this->createMessage('CompleteAuthorizeError', $response);
-            return $serviceResponse;
-        }
-
-        if (!$serviceResponse->isAwaitingNotification()) {
+        } elseif ($serviceResponse->isSuccessful()) {
             $this->markCompleted('Authorized', $serviceResponse, $response);
-        } else {
-            Helper::safeExtend($this->payment, 'onAwaitingAuthorized', $serviceResponse);
         }
 
         return $serviceResponse;

diff --git a/code/Service/CaptureService.php b/code/Service/CaptureService.php
@@ -128,25 +128,23 @@ public function initiate($data = array())
 
         $serviceResponse = $this->wrapOmnipayResponse($response);
 
-        if ($serviceResponse->isAwaitingNotification()) {
+        if ($serviceResponse->isError()) {
+            $this->createMessage($this->errorMessageType, $response);
+        } elseif ($serviceResponse->isRedirect() || $serviceResponse->isAwaitingNotification()) {
             if ($diff < 0) {
                 $this->createPartialPayment(PaymentMath::multiply($amount, '-1'), $this->pendingState);
             } elseif ($diff > 0) {
                 $this->createPartialPayment($diff, $this->pendingState);
             }
             $this->payment->Status = $this->pendingState;
             $this->payment->write();
-        } else {
-            if ($serviceResponse->isError()) {
-                $this->createMessage($this->errorMessageType, $response);
-            } else {
-                if ($diff < 0) {
-                    $this->createPartialPayment(PaymentMath::multiply($amount, '-1'), $this->pendingState);
-                } elseif ($diff > 0) {
-                    $this->createPartialPayment($diff, $this->pendingState);
-                }
-                $this->markCompleted($this->endState, $serviceResponse, $response);
+        } elseif ($serviceResponse->isSuccessful()) {
+            if ($diff < 0) {
+                $this->createPartialPayment(PaymentMath::multiply($amount, '-1'), $this->pendingState);
+            } elseif ($diff > 0) {
+                $this->createPartialPayment($diff, $this->pendingState);
             }
+            $this->markCompleted($this->endState, $serviceResponse, $response);
         }
 
         return $serviceResponse;

diff --git a/code/Service/CreateCardService.php b/code/Service/CreateCardService.php
@@ -60,7 +60,7 @@ public function initiate($data = array())
             );
         } elseif ($serviceResponse->isError()) {
             $this->createMessage('CreateCardError', $response);
-        } else {
+        } elseif ($serviceResponse->isSuccessful()) {
             $this->markCompleted('CardCreated', $serviceResponse, $response);
         }
 
@@ -110,15 +110,12 @@ public function complete($data = array(), $isNotification = false)
 
         $serviceResponse = $this->wrapOmnipayResponse($response, $isNotification);
 
-        if ($serviceResponse->isError()) {
+        if ($serviceResponse->isAwaitingNotification()) {
+            Helper::safeExtend($this->payment, 'onAwaitingCreateCard', $serviceResponse);
+        } elseif ($serviceResponse->isError()) {
             $this->createMessage('CompleteCreateCardError', $response);
-            return $serviceResponse;
-        }
-
-        if (!$serviceResponse->isAwaitingNotification()) {
+        } elseif ($serviceResponse->isSuccessful()) {
             $this->markCompleted('CardCreated', $serviceResponse, $response);
-        } else {
-            Helper::safeExtend($this->payment, 'onAwaitingCreateCard', $serviceResponse);
         }
 
         return $serviceResponse;

diff --git a/code/Service/NotificationCompleteService.php b/code/Service/NotificationCompleteService.php
@@ -78,7 +78,10 @@ public function complete($data = array(), $isNotification = true)
         }
 
         // check if we're done
-        if (!$serviceResponse->isError() && !$serviceResponse->isAwaitingNotification()) {
+        if (!$serviceResponse->isError()
+            && !$serviceResponse->isAwaitingNotification()
+            && $serviceResponse->isSuccessful()
+        ) {
             $this->markCompleted($this->endState, $serviceResponse, $serviceResponse->getOmnipayResponse());
         }
 

diff --git a/code/Service/PurchaseService.php b/code/Service/PurchaseService.php
@@ -66,7 +66,7 @@ public function initiate($data = array())
             );
         } elseif ($serviceResponse->isError()) {
             $this->createMessage('PurchaseError', $response);
-        } else {
+        } elseif ($serviceResponse->isSuccessful()) {
             $this->markCompleted('Captured', $serviceResponse, $response);
         }
 
@@ -114,19 +114,15 @@ public function complete($data = array(), $isNotification = false)
         }
 
         $serviceResponse = $this->wrapOmnipayResponse($response, $isNotification);
-        if ($serviceResponse->isError()) {
-            $this->createMessage('CompletePurchaseError', $response);
-            return $serviceResponse;
-        }
 
-        // only update payment status if we're not waiting for a notification
-        if (!$serviceResponse->isAwaitingNotification()) {
-            $this->markCompleted('Captured', $serviceResponse, $response);
-        } else {
+        if ($serviceResponse->isAwaitingNotification()) {
             Helper::safeExtend($this->payment, 'onAwaitingCaptured', $serviceResponse);
+        } elseif ($serviceResponse->isError()) {
+            $this->createMessage('CompletePurchaseError', $response);
+        } elseif ($serviceResponse->isSuccessful()) {
+            $this->markCompleted('Captured', $serviceResponse, $response);
         }
 
-
         return $serviceResponse;
     }
 

diff --git a/code/Service/RefundService.php b/code/Service/RefundService.php
@@ -121,21 +121,20 @@ public function initiate($data = array())
 
         $serviceResponse = $this->wrapOmnipayResponse($response);
 
-        if ($serviceResponse->isAwaitingNotification()) {
+        if ($serviceResponse->isError()) {
+            $this->createMessage($this->errorMessageType, $response);
+        } elseif ($serviceResponse->isRedirect() || $serviceResponse->isAwaitingNotification()) {
             if ($isPartial) {
                 $this->createPartialPayment(PaymentMath::multiply($amount, '-1'), $this->pendingState);
             }
             $this->payment->Status = $this->pendingState;
             $this->payment->write();
-        } else {
-            if ($serviceResponse->isError()) {
-                $this->createMessage($this->errorMessageType, $response);
-            } else {
-                if ($isPartial) {
-                    $this->createPartialPayment(PaymentMath::multiply($amount, '-1'), $this->pendingState);
-                }
-                $this->markCompleted($this->endState, $serviceResponse, $response);
+        } elseif ($serviceResponse->isSuccessful()) {
+            if ($isPartial) {
+                $this->createPartialPayment(PaymentMath::multiply($amount, '-1'), $this->pendingState);
             }
+
+            $this->markCompleted($this->endState, $serviceResponse, $response);
         }
 
         return $serviceResponse;

diff --git a/code/Service/ServiceResponse.php b/code/Service/ServiceResponse.php
@@ -94,6 +94,22 @@ public function getPayment()
         return $this->payment;
     }
 
+    /**
+     * Whether the response is marked as successful by Omnipay.
+     *
+     * @return bool
+     */
+    public function isSuccessful()
+    {
+        if ($this->omnipayResponse instanceof NotificationInterface) {
+            return $this->omnipayResponse->getTransactionStatus() === NotificationInterface::STATUS_COMPLETED;
+        } elseif ($this->omnipayResponse instanceof AbstractResponse) {
+            return $this->omnipayResponse->isSuccessful();
+        }
+
+        return false;
+    }
+
     /**
      * Whether or not this is an *offsite* redirect.
      * This is only the case when there's an Omnipay response present that *is* a redirect.

diff --git a/code/Service/VoidService.php b/code/Service/VoidService.php
@@ -88,15 +88,13 @@ public function initiate($data = array())
 
         $serviceResponse = $this->wrapOmnipayResponse($response);
 
-        if ($serviceResponse->isAwaitingNotification()) {
+        if ($serviceResponse->isError()) {
+            $this->createMessage($this->errorMessageType, $response);
+        } elseif ($serviceResponse->isRedirect() || $serviceResponse->isAwaitingNotification()) {
             $this->payment->Status = $this->pendingState;
             $this->payment->write();
-        } else {
-            if ($serviceResponse->isError()) {
-                $this->createMessage($this->errorMessageType, $response);
-            } else {
-                $this->markCompleted($this->endState, $serviceResponse, $response);
-            }
+        } elseif ($serviceResponse->isSuccessful()) {
+            $this->markCompleted($this->endState, $serviceResponse, $response);
         }
 
         return $serviceResponse;