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

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

Open
k4emic opened this issue Jun 11, 2018 · 9 comments
Labels
Area: Framework Component: Framework/Cache help wanted Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: ready for dev Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Severity: S2 Major restrictions or short-term circumventions are required until a fix is available.

Comments

@k4emic
Copy link

k4emic commented Jun 11, 2018

Preconditions

  1. Magento 2.2.3/2.2.4
  2. PHP 7.0.28

Steps to reproduce

  1. Create a customer account and take any steps to make the account valid
  2. Log out, clear application cache or change browser
  3. Add any simple product to the cart
  4. Watch the following expressions
    4.1. jQuery('[name=form_key]').val()
    4.2. jQuery.cookie('form_key')
    4.3. window.checkoutConfig.formKey
  5. Proceed to checkout
  6. In the billing step, login using the account you previously created
  7. 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

  1. window.checkoutConfig.formKey should match current formkey

Actual result

  1. 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.
@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Jun 11, 2018
@k4emic
Copy link
Author

k4emic commented Jun 11, 2018

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

k4emic commented Jun 12, 2018

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

k4emic commented Jun 20, 2018

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

@engcom-backlog-nickolas engcom-backlog-nickolas added Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release help wanted Component: Framework/Cache labels Jul 10, 2018
@engcom-backlog-nickolas

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

@engcom-backlog-nickolas engcom-backlog-nickolas removed their assignment Jul 10, 2018
@torreytsui
Copy link
Contributor

FYI, we have also replicated the same issue on 2.1.15

@nittyy nittyy self-assigned this May 4, 2019
@m2-assistant
Copy link

m2-assistant bot commented May 4, 2019

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.

@nittyy nittyy removed their assignment May 4, 2019
@ghost ghost removed Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development labels Oct 20, 2020
@magento-engcom-team magento-engcom-team added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. labels Nov 30, 2020
@engcom-Alfa engcom-Alfa added Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Area: Framework Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed and removed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed labels Aug 25, 2021
@github-jira-sync-bot
Copy link

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

@m2-assistant
Copy link

m2-assistant bot commented Aug 25, 2021

✅ 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.

@engcom-Alfa engcom-Alfa added Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed and removed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed labels Aug 25, 2021
@github-jira-sync-bot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Framework Component: Framework/Cache help wanted Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: ready for dev Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Severity: S2 Major restrictions or short-term circumventions are required until a fix is available.
Projects
Status: Ready for Development
Development

No branches or pull requests

8 participants