Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PayPal Gateway not sending Credit Card and CVV to ActiveMerchant #132

Closed
davekiss opened this issue Jan 20, 2014 · 3 comments
Closed

PayPal Gateway not sending Credit Card and CVV to ActiveMerchant #132

davekiss opened this issue Jan 20, 2014 · 3 comments

Comments

@davekiss
Copy link

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
Copy link
Contributor

radar commented Jan 22, 2014

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.

@radar radar closed this as completed Jan 22, 2014
@schof
Copy link
Contributor

schof commented Jan 22, 2014

Yes thank you @davekiss and @kfn8dkodemonkey

radar added a commit to spree/spree that referenced this issue Jan 22, 2014
This was causing spree/spree_gateway#132, and would've caused similar issues for all non-token gateways (i.e. gateways that send CC info rather than getting a token and using that instead)

Super incredibly glad that we found this out before master was released as a real gem.
@pusewicz
Copy link

:(

radar added a commit to spree/spree that referenced this issue Jan 28, 2014
This was causing spree/spree_gateway#132, and would've caused similar issues for all non-token gateways (i.e. gateways that send CC info rather than getting a token and using that instead)

Super incredibly glad that we found this out before master was released as a real gem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants