Invalid complete* request - error or exception?

[judgej]

When the user us returned back to the merchant site with the results of a transaction through POST or GET, that message will generally be signed to detect tampering. It will also contain the original transactionId so that out-of-order attacks can be detected.

The question is, when either of these two conditions are detected, should the driver throw an exception, or just return non-successful codes for the transaction success. There are arguments that swing both ways.

The usual flow when a user returns is something like this:

$gateway = Omnipay::create('...');
$gateway->setSigningSecret('...');

$completeRequest = $gateway->completeAuthorize([
    'transactionId' => $expectedTransactionId,
]);

$completeResponse = $completeRequest->send();

At this point, $completeResponse can provide the result of the transaction and various details. The $completeRequest can not normally do this - it can provide the raw data and not much else. Its purpose is to parse the message data into a form that it can pass to the completeResponse value object. It may also do some additional calls back to the gateway to get some additional details (e.g. some gateways do NOT sign the result, so it cannot be trusted at all, and so the gateway driver must go interrogate the gateway to get the trusted result - but that's a diversion from my main point).

So, if the signature is invalid, should send() through an exception? If it does, then we are left only with the request object and its raw data.

Similarly, if the transactions are out-of-order (wrong transactionId), should we through an exception here too? The signing may be valid, and the message may represent a valid transaction result, so I feel we do still need the $completeResponse object to at least log the result against the correct pending transaction, but the isSuccessful() check should never be true.

Any thoughts?

[judgej]

When we have a decision, it would be nice to get it into the documentation, since I see many different approaches being taken (with me being guilty of some inconsisency).

[devnix]

It would be bad practice to make the OmnipayException implement a custom constructor and a custom getter with optional parameters containing the response if it exists? I would find it a useful way to differentiate between an error reported by the gateway platform and an error detected by the driver (wrong transactionId, wrong request, etc).