Notify hash check for SIM is not correct about omnipay-authorizenet HOT 28 CLOSED

Also the amount used in the hash check must be supplied by the merchant account. Arguably that is a safer thing to do, because if the remote gateway has authorized a different amount from what you are expecting, then an invalid has exception will be raised.

However, that is not a useful exception to raise in this instance. The hash used to confirm where the callback came from, and that the amount has not been tampered with. By using the amount from the POST data (x_amount) this check works. Now, the amount may still not be what the merchant account was expecting, but IMO that is a separate check that must take place after the validity of the callback sender is confirmed. This is how I have coded the getDpmHash() ad getData() methods in the DPM gateway driver:

https://github.com/academe/omnipay-authorizenet/blob/dpm_support/src/Message/DPMCompleteRequest.php

I propose that this approach should be moved to the SIM gateway too. That does change the functionality in the callback a little too. Changes needed would be:

1. Do not pass the POST x_trans_id into setTransactionId().
2. Do continue to pass the expected amount into setAmount()
3. The InvalidRequestException raised in send() can now have one of two different messages 'Incorrect hash' or 'Incorrect amount'.

TBH that does not look too bad now I've written it down. It will probably just involve removing the single setTransactionId() and with no other consequences, unless I'm missing something.

from omnipay-authorizenet.

AuthorizeNet_SIM is not working for us either because we get this execption: "Incorrect hash"

from omnipay-authorizenet.

Do you use $request->setTransactionId($_POST['x_trans_id']) in your notification handler for the Complete request?

In fact, could you show us what you are doing in there? I'm out for a few hours, but will put an hour aside later on this evening. I am using AuthorizeNet_DPM on a live site, and that works great. The DPM is based on the SIM, but I did rewrite the hash checker for that (see here). Just needs some back-porting to here I guess, and maybe some better documentation.

It would be a BC break change for SIM, but would be more secure, hence raising it here for discussion before trying to fix it. The DPM version gives us the model to work from, so if people are happy with that, I can move the DPM hash check to the SIM gateway and just inherit that back to DPM.

from omnipay-authorizenet.

We use a more generic approach and in fact pass the complete $_REQUEST array into completePurchase():

$response = $provider->completePurchase( $_REQUEST )->send();

from omnipay-authorizenet.

Is that just in your drivers? No driver I know of will make use of $_REQUEST being passed in like that. All the parameters the drivers take are "normalised" to OmniPay names. A driver would be expecting an "amount" or "transactionId" element passed in, rather than, say, "x_amnt" or "trans_ref_code" that the remote gateway would send.

If there are POST or GET parameters that the completePurchase() message needs, then it will get those itself using the client that is automatically provided to it by OmniPay Common.

from omnipay-authorizenet.

That was a bit oversimplified. The standard values are added to the received parameters like this:

$data = $_REQUEST;
$data['transactionId'] = $order->getId();
$data['amount'] = $order->getValue();
$data['currency'] = $order->getCurrencyId();

$response = $gateway->completePurchase( $data )->send();

We use

$gateway->initialize([
    'hashSecret' => '...',
    'apiLoginId' => '...',
    'transactionKey' => '...',
    'developerMode' => '1'
]);

to initialize the gateway. According to the source, for hash calculation "hashSecret", "apiLoginId", "transactionId" and "amount" are used for the hash calculation with a big fat "CHECKME" if the "transactionReference" parameter should be used instead of the "transactionId" parameter.

from omnipay-authorizenet.

Authorize.Net names the transaction ID and reference the opposite way around to OmniPay, so that can lead to some confusion. I think it should be getTransactionReference() here. And the transaction reference comes from $_POST['x_trans_id'] in that notification.

If you pass in 'transactionId' => $_POST['x_trans_id'] (the transaction reference, generated by Authorize.Net) rather than your order ID (aka OmniPay transactionId) then does the "invalid hash" error go away?

Passing the $_REQUEST data in - don't bother. It's not doing anything useful. Do feel free to save $_REQUEST in the database alongside the transaction though, if you want to use it for debugging.

from omnipay-authorizenet.

In fact, our transactionId is not actually handled by Authorize.Net at all by default. I believe SIM sticks it in the "incoiceId" field, which is wrong IMO. Both SIM and DPM will accept any custom fields we define, so we can put it into a "transactionId" field and know that Authorize.Net will safely pass it on to the notify handler.

There is a "seq number" field that is used in the hashes, which looks like it ought to be the transactionId. However, it is numeric only, which is no good for many sites, and it is not actually passed to the notify handler, which is no good if you need it to look up a transaction in the database. It is only used for hashing the data initially sent from the shop to Authorize.Net, and not beyond that point. Beyond that, I have no idea what the point of it is, because it seems like a half-implemented idea.

from omnipay-authorizenet.

Yes, that fixes the problem so getTransactionReference() seems to be the right choice there. I've already created a pull request: #21

from omnipay-authorizenet.

If you don't pass in $_POST['x_trans_id'], does it still work? If not, then it may be better to transfer the code from the DPM gateway back up to the SIM gateway.

What does $complete_response->getTransactionReference() (note response) give you, without setting it yourself? Does it give you the same value as $_POST['x_trans_id']? $complete_request->getTransactionReference() (note resquest) should be null though.

Thanks for helping with this BTW.

from omnipay-authorizenet.

You may be right. Let's use your DPM code.

from omnipay-authorizenet.

I'll give a go later today. Edit: getting too late, will have to wait for the morning.

from omnipay-authorizenet.

I've made the bulk of the changes in the above "issue_19" branch here https://github.com/judgej/omnipay-authorizenet/tree/issue_19/ and updated the tests. I'm out for most of tomorrow, but feel free to give this branch a try.

In the notification handler, you no longer need to pass in anything to transactionId, so that line can be removed. Any existing systems that do pass in data to that field, can continue to do so with no ill effect - the field is no longer used in the has check. You do still need to pass in the expected authorized amount ($completeRequest->setAmount()) iff you want the amount to be validated against what was actually authorized. I would recommend doing that for simple authorisations.

I have cheekily put in a note deprecating the automatic passing of the transactionId through the invoice number (x_invoice_num) field. That's a silly idea - the transaction ID is not an invoice number, so it should not be misused like that, as it can make automatic reconciling between payment systems and accounting packages a little tricky.

In the notify handler, you will find the transactionId, before you load up the gateway request, here:

$_POST[\Omnipay\AuthorizeNet\Message\AbstractRequest::TRANSACTION_ID_PARAM]

You can use that instead of having to know that the field name is x_trans_id. Or don't - your choice. I do think we need a standard place across the gateways to statically find the name of the POST field that holds the transaction ID for use in the notify handlers.

This passes all the phpunit tests, but I have not actually run it against a live Authorize.Net account yet (just run out of time for today). Best of luck.

from omnipay-authorizenet.

It would be nice to replace that manual check of $_POST listed above with a method in completeRequest(). At the moment, only the complete*Response() message gives you decoded values from POST and GET values, so it is kind of a special case. Being able to fetch the transactionId before any other actions are taken (such as completeRequest->send() is important, as the notify handler needs to be able to fetch the correct transaction from the database before asking OmniPay to "complete" process the transaction. Having a standard way to get this across the gateways would be very helpful.

from omnipay-authorizenet.

Your branch fixed the problem. Thank you very much!
When will you be able to integrate it into master and is a new minor release of the omnipay-authorizenet package planned in the near future?

from omnipay-authorizenet.

Cool, and sorry it took so long. It's been on my TODO list for ages, so your help in testing it has sped things up a tad.

I'm busy on a number of things at the moment, but I'll try and squeeze some more tests in over the next few days and get a PR together. I need to make sure it really is a minor release and does not introduce and BC breaks. I don't think it will, because the transactionId that people may be passing in now should be totally ignored. Also the new amount validation check will only kick in if the expected amount is passed in to the completeAuthorize() message. But I have something niggling at the back of my mind that I need to check out.

In the meantime - and this is what I do - feel free to use my branch directly from composer. Once any PR is accepted, I'll leave that branch up for a while so you can switch to a more stable release. Or you can clone it into somewhere outside of the vendor directory in your project and point a manual psr-4 autoload entry at it.

This is how composer.json looks in the project I used to make the changes:

{
    "require": {
        "omnipay/authorizenet": "dev-master",
        "omnipay/tests": "^2.0"
    },
    "autoload": {
        "psr-4": {
            "Omnipay\\AuthorizeNet\\": "omnipay-authorizenet/src/"
        }
    }
}

I cloned omnipay-authorizenet into the root of the project (dev-master just to get the absolute latest dependencies), then did a composer dump-autoload. Even though there is a stable version of omnipay-authorizenet in the vendor directory, this autoload entry overrides that and uses my cloned version instead.

Anyway - lots of ways to use the dev fork version if you need it urgently. I won't drop it until I know you've finished with it.

from omnipay-authorizenet.

It's not us who needs this immediately but we want to offer that as part of our supported payments for the Aimeos web shop components. Thus, we will wait to mark this as supported until a new minor release has been made.

Thanks a lot for your help!

from omnipay-authorizenet.

Issue #16 and #22 have also been incorporated into this "issue_19" branch, as they are all so closely tied together, it was difficult to split them. And I got carried away.

I've extended the tests, and run it against a developer account on Authorize.Net. It should be completely BC with how it worked previously, but is now more consistent across SIM and DPM, provides some short-cuts in the notify handler that you can use to make it much, much simpler and more generic (following the standard OmniPay approach).

If this works for you, I'll fire it off as a PR. Complete branch here: https://github.com/judgej/omnipay-authorizenet/tree/issue_19

from omnipay-authorizenet.

It works but one change breaks compatibility to previous versions:
In SIMAuthorizeRequest you've changed

$data['x_relay_url'] = $this->getReturnUrl();
to
$data['x_relay_url'] = $this->getNotifyUrl() ?: $this->getReturnUrl();

Authorize.net is special because it posts the notification and expects the confirm page of the shop at the same time. Nevertheless, I think using the notification URL is wrong here because the notification URL is for machine to machine communication only and shouldn't display the confirmation page while the confirmation pages usually handle the first payment status update too.

We can work around this by adding no notification URL for omnipay/authorizenet but it will break other implementations as well for sure.

from omnipay-authorizenet.

For SIM and DPM, the URL given during the authorisation request is the URL for a machine-to-machine communication, and always is. That is the notify URL, where Authorize.Net informs the site of the result of the authorisation.

What the merchant site returns for that notify request, is a HTML page that takes the user off to the merchant site's final confirmation page. That HTML page is displayed by the Authorize.Net site, in the authorize.net domain. Normally, that page will contain a meta refresh or JavaScript redirect to take the browser back to the merchant site. I put a manual link on that page too, just in case the redirect fails to operate.

Now, the user's browser will never be given the x_relay_url directly. The user's browser is never taken to that page. The contents of that page is read by Authorize.Net and then displayed by that site (minus any HTTP headers it has, which are discarded, hence a http "Location:" redirect will not work here).

That's how I understand it, hence notifyUrl because it is machine-to-machine only.

from omnipay-authorizenet.

I've only left getReturnUrl() in there for BC. In version 3.0 I would argue it be removed.

The actual return URL is generated by the merchant site notify page for use in its JS redirect. Authorize.Net does not ever see that URL as a parameter, so it has no idea (and does not care) where the final completion page is.

from omnipay-authorizenet.

Thinking about it, I suspect your use-case is when the notify page does not have an automatic redirect on. Instead it could display, "thank you for the payment, click [here] to return to the merchant site", a bit like PayPal. It could operate like that, and I guess it would be a bit of a grey area. I've not personally done that, and I would still argue that because the contents of that page are not displayed in the merchant site, that page is effectively just content or data that the Authorize.Net site reads and displays in its part of the process - the user has not got back to the merchant site yet, so not returned.

from omnipay-authorizenet.

Take a look at the network flow here if it helps:

http://academe.co.uk/wp-content/uploads/2015/06/OmniPay-AuthorizeNet-DPM.png

Everything inside the "handle result" dotted box, which defined by the notifyUrl, is machine-to-machine. I'll add a note to that diagram to make it clear the redirect could be manual, clicked by the user, on a page with additional transaction details, and perhaps a big "thank you".

from omnipay-authorizenet.

I didn't thought about redirecting the customer at the notification page to split notification and confirm by hand until you said this is an option. Changed that and it works. Thanks :-)

from omnipay-authorizenet.

It's a weird one, this gateway (well, they all are, in their own sweet ways). When there was just SIM, the documentation described how you make that confirmation page. Then came DPM, which was based on SIM, but the idea was not to let the user see that they had left the merchant site, hence the documentation described in detail how the redirect approach works and how you use it.

Then they realised there were so many other mistakes and duplicated information in that documentation, that they withdrew it completely. Now the DPM documentation says (still erroneously in parts) that DPM is identical to SIM, but with a few extra fields and values, then points you to the SIM documentation. Unfortunately some of the subtle details that were in the old DPM documents, especially regarding the redirect, have just been lost in this whole move :-(

Anyway - it is ready to go as a PR so far as I am concerned. Old tests work, new tests work, end-to-end testing against the live developer account works. You happy with it? You say "go", as I'm not sure anyone else is really using this gateway enough at this point to get involved.

from omnipay-authorizenet.

Go! :-)

from omnipay-authorizenet.

Gone! :-)

Thanks @kayladnls :-) What needs to happen to get a release done for this gateway? 2.2.0 is from January this year, and the DPM extensions haven't made it into a stable release tag yet. Does anything need to be done before that can happen?

Tickets #14 and #19 should be closed once a stable tag is available (2.3.0?).

from omnipay-authorizenet.

This all seems to be working nicely in release 2.3.0. Thanks all :-)

from omnipay-authorizenet.