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

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

Open
pmzandbergen opened this issue Sep 26, 2024 · 6 comments
Assignees
Labels
Bug report Indicates that issue has been marked as a possible bug

Comments

@pmzandbergen
Copy link
Contributor

Original bug report: #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?

@pmzandbergen pmzandbergen added the Bug report Indicates that issue has been marked as a possible bug label Sep 26, 2024
@candemiralp candemiralp self-assigned this Sep 27, 2024
@candemiralp
Copy link
Member

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

@candemiralp candemiralp changed the title PlaceOrder mutation (Magento GraphQL) is broken with due to missing State Data [ECP-9488] PlaceOrder mutation (Magento GraphQL) is broken with due to missing State Data Sep 27, 2024
@pmzandbergen
Copy link
Contributor Author

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

dimitriBouteille commented Oct 2, 2024

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() : #2573 (comment)

I think, you can also use adyenSaveStateData mutation :)

@candemiralp
Copy link
Member

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

@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
Copy link
Member

candemiralp commented Nov 13, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug report Indicates that issue has been marked as a possible bug
Projects
None yet
Development

No branches or pull requests

3 participants