Formkey in window.checkoutConfig gets stale when logging in during checkout process

k4emic commented 6 years ago

Preconditions

Magento 2.2.3/2.2.4
PHP 7.0.28

Steps to reproduce

Create a customer account and take any steps to make the account valid
Log out, clear application cache or change browser
Add any simple product to the cart
Watch the following expressions 4.1. jQuery('[name=form_key]').val() 4.2. jQuery.cookie('form_key') 4.3. window.checkoutConfig.formKey
Proceed to checkout
In the billing step, login using the account you previously created
Observe the previously mentioned expressions, 4.1 and 4.2 will show the same while 4.3 will show a different (the previously used) value.

Expected result

window.checkoutConfig.formKey should match current formkey

Actual result

window.checkoutConfig contains the stale formkey, making any validation of the key fail (Magento_Checkout/js/payment.js uses this formkey for the payment form).

In effect, this means that any payment methods that does validation of the formkey in the payment form will fail, after a user has logged in during checkout.

Why this happens

The HTTP request to the login action will unset the form_key session and cookie
The following request to /checkout will include the new formkey in the markup (in several places)
The mage.formKey (page-cache.js) widget will see that no form_key cookie is set, and will generate a new form_key, setting the cookie while updating any "plain" input fields with the name "form_key". window.checkoutConfig is not affected.

k4emic commented 6 years ago

I am not entirely sure what is the best course of action for fixing this problem.

mage.formKey shouldn't be forcing a new formKey when one is already present on the page
- should the login or checkout response send the form_key cookie in the response? Would this present a security risk?
formKey doesn't belong in window.checkoutConfig: it seems like a codesmell to me. Ideally, some object/component would maintain information about the current form key.

k4emic commented 6 years ago

I have made the following patch for the PageCache module, it is currently undergoing internal testing here at Vaimo.

+++ etc/events.xml  2018-06-12 16:00:28.704963409 +0200
@@ -52,7 +52,7 @@
         <observer name="FlushFormKey" instance="Magento\PageCache\Observer\FlushFormKey"/>
     </event>
     <event name="customer_login">
-        <observer name="FlushFormKey" instance="Magento\PageCache\Observer\FlushFormKey"/>
+        <observer name="RenewFormKey" instance="Magento\PageCache\Observer\RenewFormKey"/>
     </event>
     <event name="customer_logout">
         <observer name="FlushFormKey" instance="Magento\PageCache\Observer\FlushFormKey"/>

--- Observer/RenewFormKey.php   1970-01-01 01:00:00.000000000 +0100
+++ Observer/RenewFormKey.php   2018-06-12 15:59:54.438311720 +0200
@@ -0,0 +1,95 @@
+<?php
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+namespace Magento\PageCache\Observer;
+
+use Magento\Framework\Event\ObserverInterface;
+use Magento\Framework\App\PageCache\FormKey as CookieFormKey;
+use Magento\Framework\Data\Form\FormKey as DataFormKey;
+use Magento\Framework\Session\Config\ConfigInterface;
+use Magento\Framework\Stdlib\Cookie\CookieMetadataFactory;
+
+/**
+ * Set new form key cookie to avoid generation in frontend, causing mismatch
+ * between backend and frontend key.
+ * @see https://github.com/magento/magento2/issues/16020
+ * @package Magento\PageCache\Observer
+ */
+class RenewFormKey implements ObserverInterface
+{
+    /**
+     * @var CookieFormKey
+     */
+    private $cookieFormKey;
+
+    /**
+     * @var DataFormKey
+     */
+    private $dataFormKey;
+
+    /**
+     * @var CookieMetadataFactory
+     */
+    private $cookieMetadataFactory;
+
+    /**
+     * @var ConfigInterface
+     */
+    private $sessionConfig;
+
+    /**
+     * @var FlushFormKey
+     */
+    private $flushFormKey;
+
+    /**
+     * @param CookieFormKey $cookieFormKey
+     * @param DataFormKey $dataFormKey
+     * @param CookieMetadataFactory $cookieMetadataFactory
+     * @param ConfigInterface $sessionConfig
+     * @param FlushFormKey $flushFormKey
+     */
+    public function __construct(
+        CookieFormKey $cookieFormKey,
+        DataFormKey $dataFormKey,
+        CookieMetadataFactory $cookieMetadataFactory,
+        ConfigInterface $sessionConfig,
+        FlushFormKey $flushFormKey
+    ) {
+        $this->cookieFormKey = $cookieFormKey;
+        $this->dataFormKey = $dataFormKey;
+        $this->cookieMetadataFactory = $cookieMetadataFactory;
+        $this->sessionConfig = $sessionConfig;
+        $this->flushFormKey = $flushFormKey;
+    }
+
+    /**
+     * @param \Magento\Framework\Event\Observer $observer
+     * @return void
+     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
+     */
+    public function execute(\Magento\Framework\Event\Observer $observer)
+    {
+        $this->flushFormKey->execute($observer);
+
+        $this->setNewFormKey();
+    }
+
+    private function setNewFormKey()
+    {
+        $formKey = $this->dataFormKey->getFormKey();
+
+        $cookieMetadata = $this->cookieMetadataFactory
+            ->createPublicCookieMetadata();
+        $cookieMetadata->setDomain($this->sessionConfig->getCookieDomain());
+        $cookieMetadata->setPath($this->sessionConfig->getCookiePath());
+        $cookieMetadata->setDuration($this->sessionConfig->getCookieLifetime());
+
+        $this->cookieFormKey->set(
+            $formKey,
+            $cookieMetadata
+        );
+    }
+}

k4emic commented 6 years ago

Update: Our internal QA did not find any problems after the patch was applied

engcom-backlog-nickolas commented 6 years ago

Hello @k4emic, thank you for your report. We've acknowledged the issue and added to our backlog.

torreytsui commented 5 years ago

FYI, we have also replicated the same issue on 2.1.15

m2-assistant[bot] commented 5 years ago

Hi @NithyaBabuKesavan. Thank you for working on this issue. Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

[ ] 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
[ ] 2. Verify that the issue is reproducible on 2.3-develop branch
Details
- Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 3. Verify that the issue is reproducible on 2.2-develop branch.
Details
- Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x
[ ] 4. If the issue is not relevant or is not reproducible any more, feel free to close it.

github-jira-sync-bot commented 3 years ago

:white_check_mark: Jira issue https://jira.corp.magento.com/browse/AC-1030 is successfully created for this GitHub issue.

m2-assistant[bot] commented 3 years ago

:white_check_mark: Confirmed by @engcom-Alfa. Thank you for verifying the issue.
Issue Available: @engcom-Alfa, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

github-jira-sync-bot commented 3 years ago

:x: Cannot export the issue. This GitHub issue is already linked to Jira issue(s): https://jira.corp.magento.com/browse/AC-1030

magento / magento2