Adyen / adyen-magento2

Adyen Payment plugin for Magento2
MIT License
155 stars 211 forks source link

[ECP-9488] PlaceOrder mutation (Magento GraphQL) is broken with due to missing State Data #2754

Open pmzandbergen opened 1 month ago

pmzandbergen commented 1 month ago

Original bug report: https://github.com/Adyen/adyen-magento2/issues/1267

The original issue has been closed without providing a (solid) solution. The provided workaround doesn't work in all situations, for example when some kind of Apollo middleware is being used (e.g. Apollo Server). The client / frontend can still bundle the mutations in a single request, but the middleware will use multiple requests to Magento. Since the state data isn't being stored the placeOrder mutation will fail.

The documentation states "use the placeAdyenOrder mutation". It does not mention the placeOrder mutation is broken.

Deviating from standards is sometimes necessary, but is that really the case here? Why is saving the state data, only temporary and encrypted if necessary, an issue?

candemiralp commented 1 month ago

Hello @pmzandbergen,

Thank you for creating this Github issue and highlighting this issue about the implementation again.

As you mentioned, it's a long going discussion and there is yet no solution. That was a decision that we needed to make due to security concerns as state data carries payment data. On the other hand, we are are of the incomplete flow causes frustration on both our merchants and system integrators.

Let me discuss this issue with our internal security team and get back to you with a solution.

Best Regards, Can

pmzandbergen commented 1 month ago

In the meanwhile we're trying to fix this by using the (deprecated) setPaymentMethodAndPlaceOrder mutation. Unfortunately we encounter a bug in the Adyen module using this deprecated mutation, I'll create a separate issue for this.

dimitriBouteille commented 1 month ago

Hi @pmzandbergen I have same issue with codegen and yoga server , you can probably fix the bug with plugin on Adyen\Payment\Helper\StateData::SetStateData() : https://github.com/Adyen/adyen-magento2/issues/2573#issuecomment-2034023214

I think, you can also use adyenSaveStateData mutation :)

candemiralp commented 3 weeks ago

Hello @dimitriBouteille,

As you've mentioned, adyenSaveStateData mutation might solve this issue as a temporary workaround. But, please keep in mind that this mutation has been introduced for gift card payments only. Otherwise, credit card data might be stored in the database. We are currently trying to discourage saving card data (state data) in the DB for security concerns and to introduce a concrete way to solve this issue.

Best Regards, Can

pmzandbergen commented 3 weeks ago

@candemiralp if I could make a suggestion: You could choose to use client side encryption and store the encrypted data server side. The key used for encryption can, for example, be saved in local storage (the browser).

The data then should be save, even if the server (database + code) is compromised. An additional cron cleaning up expired state data could be added as an extra safety measure.

candemiralp commented 6 days ago

Hello @pmzandbergen,

Thank you very much for this suggestion. Currently, the card component already uses client side encryption and state data (card number, expiry and cvc) contains only encrypted card data. However, PCI-DSS requirement 3.2 states that this data can not be stored even if it is in encrypted form. It also mentions that a retention period can be set and the data can be deleted after this time but this clause is not very clear.

This solution might lead our merchants to lose their PCI compliance. We are still discussing the alternatives.

Best Regards, Can