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

Payum and PCI DSS Compliance #78

Closed
mtudor opened this issue Nov 12, 2013 · 15 comments · Fixed by #79
Closed

Payum and PCI DSS Compliance #78

mtudor opened this issue Nov 12, 2013 · 15 comments · Fixed by #79
Assignees

Comments

@mtudor
Copy link
Contributor

mtudor commented Nov 12, 2013

Hi @makasim,

We're still working through a Payum implementation here and are now looking at the finer points of PCI DSS. Does PCI DSS apply in the Ukraine? It certainly does over here. One of the guidelines is that we're not allowed to store card details for any length of time, or we'll be elevated to the highest level of self assessment questionnaire to gain compliance.

The SecuredCaptureRequest relies on storing all of the card information across a redirect (from what I can see). Even though this is only done for a very small amount of time, the PCI DSS requirements are pretty black and white. Even encryption does not help here!

My question is twofold:

  1. What was the purpose of introducing tokens and the SecuredCaptureRequest?
  2. Can the SecuredCaptureRequest be used without storing the data? And if not, would it still be valid to use the standard CaptureRequest in its place?

Cheers,

Mark.

@makasim
Copy link
Member

makasim commented Nov 12, 2013

@mtudor I agree on that fact that card details should not be stored for any time even temporally somewhere.

That's why I am telling filesystem is not good. It saves all the info

In symfony2 world I solve this problem using forward instead of redirect with the model which does not store credit card details. So everything kept in memory.

@mtudor
Copy link
Contributor Author

mtudor commented Nov 12, 2013

I need to look into it a bit more I think, but we do have "forward" in Zend Framework too so it should be possible to do something similar. You'd still use the SecuredCaptureController then?

@makasim
Copy link
Member

makasim commented Nov 12, 2013

@mtudor I thing zend2 has something like forward in symfony. Here's the link where you can get more about symfony's forward: http://bluehorn.co.nz/2009/08/29/symfony-redirect-vs-forward/.

So there is two way to go:

  • one process and model that does not store credit card info
  • custom action which removes sensative info.

@makasim
Copy link
Member

makasim commented Nov 12, 2013

from sandbox:

<?php

return $this->forward('PayumBundle:Capture:do', array(
    'payum_token' => $captureToken,
));

// vs redirect

return $this->redirect($captureToken->getTargetUrl());

both variant work fine for symfony bundle though I think zend module should be tweaked to support forward

@makasim
Copy link
Member

makasim commented Nov 12, 2013

You'd still use the SecuredCaptureController then?

yes

@mtudor
Copy link
Contributor Author

mtudor commented Nov 12, 2013

What was the reason for originally introducing the SecuredCaptureController instead of the standard CaptureController?

@ghost ghost assigned makasim Nov 12, 2013
@makasim
Copy link
Member

makasim commented Nov 12, 2013

What was the purpose of introducing tokens and the SecuredCaptureRequest?

There are several reasons

  • First, Do not pass sensitive data like order id and payment name in the url directly.
  • Second, It is hard to abstract after capture logic. Right now the token stores after url and the capture controller just redirects to it. What should Capture controller do in case we do not have token?

@makasim
Copy link
Member

makasim commented Nov 12, 2013

What about introduce SensativeValue class.

<?php

$value = new SensativeValue('1111222233334444');

echo $value->get();
// 1111222233334444

echo serialize($value);
// empty string

echo $value;
// empty string

so whenever you try to serialize it or cast to string you get nothing. Of course you still have to use something like forward to porcess everything in one call.

@mtudor
Copy link
Contributor Author

mtudor commented Nov 12, 2013

A SensitiveValue class might be a good protection to prevent accidental exposure of the card details etc. but in a production system I think it would be a protection that we would hope would never be relied on. It may not be necessary - I'm currently attempting to refactor to use forward and we'll see how far that gets us.

The main issue I was having is that with the redirect method that data HAS to be persisted somehow, and that will break PCI DSS compliance requirements. Hopefully forward may solve that.

@makasim
Copy link
Member

makasim commented Nov 12, 2013

@mtudor I did some progress in the referenced PR. Could you review?

@mtudor
Copy link
Contributor Author

mtudor commented Nov 13, 2013

I managed to get forward working (by actually changing code in my controller before I pass over to PayumModule)...

// Add the token to the request query and forward to PayumModule to begin processing.
$this->getRequest()->getQuery()->set('payum_token', $captureToken);
return $this->forward()->dispatch('PayumCapture', array('action' => 'do'));

So far, I've actually found that I don't have to store the capture token, or the payment details, anywhere... The done token needs storing as once the payment is complete I do a full redirect (to lower the chances of the user hitting refresh and having issues - although double payments will be prevented by the "is new payment" check done before processing.

How does that all sound?

I'll update further as I get more of the implementation completed.

@mtudor
Copy link
Contributor Author

mtudor commented Nov 13, 2013

Quick question on the HttpRequestVerifier... You added in a check to make sure that the URL set in the token is the same as the URL at which the token is used. What protection is this intended to offer?

@makasim
Copy link
Member

makasim commented Nov 13, 2013

Quick question on the HttpRequestVerifier... You added in a check to make sure that the URL set in the token is the same as the URL at which the token is used. What protection is this intended to offer?

There are could exist not only capture tokens but sync\void\refund and so on. This url protection allows to expose them to user and dont worry that the user would try to miss use them. Other words you cannot use sync token to void or refund payments.

Make sense?

@makasim
Copy link
Member

makasim commented Nov 13, 2013

So far, I've actually found that I don't have to store the capture token ...

Sounds good to me.

@mtudor
Copy link
Contributor Author

mtudor commented Nov 13, 2013

Make sense?

Ah yes, I hadn't considered the other card actions. Thanks!

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

Successfully merging a pull request may close this issue.

2 participants