PayPal Gateway not sending Credit Card and CVV to ActiveMerchant

[davekiss]

I am unable to process a test payment (and I assume a live payment) with PayPal Gateway. It seems the credit card number and CVV aren't being passed to the request in activemerchant and gives me the same error as seen in #128

I've used pry on ActiveMerchant's PayPal model and the request shows an empty credit card number and CVV value:

https://gist.github.com/davekiss/3b39a595509b2dc6d39f

To reproduce, simply use spree_gateway:master to create a new payment method based off of Spree::Gateway::PayPalGateway, enter your Sandbox API credentials and try to process a test payment. You can use any credit card number - the point isn't that the card is not validating, but that it isn't being sent at all.

[radar]

Bit of background: this was first asked about by @davekiss here on IRC. I suggested a little later on that it could be because the CC object is being reloaded during the request.

I was almost right. Today, I went back in the Git history to a point that I thought may have caused this problem, specifically: spree/spree@d06a3bd. I wound back to the commit before this using this command:

git checkout d06a3bd0a431556a4197d2b0009048ffb6cf5893~1

I ran @davekiss's application and the payment went through OK. So therefore I placed the blame on that commit, and then reverted it within master. I tried @davekiss's application again and the payment failed. That meant that I was wrong about the spree/spree@d06a3bd commit being the cause of this problem.

It therefore had to be something else, between spree/spree@d245dd0 (which is the commit before spree/spree@d06a3bd) and master. As of this point in time, there are 139 commits between that commit and master:

spree (master)⸘ git log --oneline d06a3bd0a431556a4197d2b0009048ffb6cf5893~1..master | wc -l
139

(I have no idea how we would do this next part if we weren't using Git.)

I ran git bisect, telling it that the "good" commit was spree/spree@d245dd0 and that the "bad" commit was master. It took about 8 runs to get the actual commit that was at fault: spree/spree@spree/spree@f30ea1b. This was the least painful part of this whole process. By this point I had spent about 2 hours investigating this.

The commit itself is pretty innocuous and seems harmless. I remember code reviewing it when it came in, and since all the tests passed I was happy merging this to the master branch. Turns out that it's this inverse_of declaration which is causing this problem.

Now, what particular problem it causes is a little difficult to explain, but here goes. When an order's payment details are submitted through the CheckoutController, they're persisted in memory. This includes all the payment details, so that then the order model can run process_payments which effectively is the crux of Spree: the transfer of money for goods. Obviously: If this method fails in any way at all, that is very very very very very very very bad. I may have left out a "very" or three hundred for brevity's sake.

By having the inverse_of association option on the payments association, the entire association was being re-fetched from the database which caused the in-memory credit card details to be "forgotten". That's working as it should: we are never supposed to store credit card details anywhere else besides memory. That's so very much against the law, and the consequences for doing that are huge. So we don't do that.

---

Removing the inverse_of association option stops the payments from being automatically reloaded from the database. I have a commit at radar/spree@5be2ad3 for the master branch, which does:

1. Removes the inverse option
2. Has a test that enforces the inverse option should never exist on that association and;
3. Has another test that checks that when an order receives an update_attributes call with payment data, that the payment data is still accessible through a call to order.pending_payments. This is a very distilled example of what goes on in the checkout. This test fails when the inverse_of option is added to the association, so I think it's a good guard against the problem.

There is also a similar commit within 2-1-stable at radar/spree@1d5eb09, which just adds the mentioned tests. There's no inverse_of option to remove on that branch.

---

I am really sorry that this happened. This kind of bug is exceptionally rare within Spree, considering how battle-tested our payments code is. I am stunned that something like this slipped through, but it's good that it did before we released 2.2 to the general public.

Thank you for living on the edge, @davekiss and @kfn8dkodemonkey. Without your help, I don't think we would've discovered this bug until a little later on.

[schof]

Yes thank you @davekiss and @kfn8dkodemonkey

[pusewicz]

:(