MC-30171: Add to Cart Form wrong Form Key in FPC #30961

engcom-Golf · 2020-11-18T07:55:27Z

Description (*)

Fixes behavior of late form key update by js script. The product could be added to the cart with a cached form_key value that causes an exception.

Related Pull Requests

Fixed Issues (if relevant)

Fixes Add to Cart Form wrong Form Key in FPC #13746

Manual testing scenarios (*)

Magento installed
Simple product created
Full page cache flushed
Open product view page, note down form key
Open the same page in other browser or incognito window
Open page source
Expected result:
form_key is the same as in step 4. It is a cached value
Open normal page view
Open DevTools in the browser
Enable Slow 3G throttling in the network tab
Refresh product page
Find form key element until page completely loaded
Expected result:
Form key value differs from the value on steps 4 and 6
Correct form key value is loaded instantly when the element is rendered

Questions or comments

Script logic in inline_form_key_provider.phtml can not be covered by any kind of automated tests. Just only manual testing.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

m2-assistant · 2020-11-18T07:55:30Z

Hi @engcom-Golf. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

engcom-Golf · 2020-11-18T07:56:23Z

@magento run all tests

ihor-sviziev · 2020-11-18T08:07:48Z

app/code/Magento/PageCache/view/frontend/templates/inline_form_key_provider.phtml

+
+use Magento\Framework\View\Helper\SecureHtmlRenderer;
+
+$scriptString = <<<HTML


his is not HTML, should be <<<JS probably.

this should be <<<script and close with script;

ihor-sviziev · 2020-11-18T08:08:32Z

app/code/Magento/PageCache/view/frontend/templates/inline_form_key_provider.phtml

+    function setFormKeyCookie(value) {
+        var expires,
+            secure,
+            date = new Date(),
+            isSecure = !!window.cookiesConfig && window.cookiesConfig.secure;
+        date.setTime(date.getTime() + 86400000);
+        expires = '; expires=' + date.toUTCString();
+        secure = isSecure ? '; secure' : '';
+
+        document.cookie = 'form_key=' + (value || '') + expires + secure + '; path=/';
+    }
+    function getFormKeyCookie() {
+        var nameEQ = 'form_key=',
+            cookieArr = document.cookie.split(';');
+        for (var i = 0; i < cookieArr.length; i++) {
+            var cookie = cookieArr[i];
+            while(cookie.charAt(0) == ' ') {
+                cookie = cookie.substring(1, cookie.length);
+            };
+            if (cookie.indexOf(nameEQ) == 0) {
+                return cookie.substring(nameEQ.length, cookie.length);
+            };
+        }
+        return null;
+    }
+    function generateCookieString() {
+        var result = '',
+            length = 16;
+            chars = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
+        while (length--) {
+           result += chars[Math.round(Math.random() * (chars.length - 1))];
+        }
+        return result;
+    }
+    var formKey = getFormKeyCookie();
+    if (!formKey) {
+        formKey = generateCookieString();
+        setFormKeyCookie(formKey);
+    }


Could you not expose all these functions to the global scope, but only the needed (window.formKey)?

As this code will be added to all pages - maybe it's better to include it as external JS file, so it will be at least cached between the pages OR maybe we can minify it for production mode?

engcom-Golf · 2020-11-18T15:07:49Z

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests

engcom-Golf · 2020-11-18T15:21:04Z

@magento run all tests

ihor-sviziev

Could you fix one small issue and squash your changes to single commit?

ihor-sviziev · 2020-11-18T19:00:20Z

app/code/Magento/PageCache/view/frontend/layout/default.xml

    <body>
        <referenceBlock name="head.components">
            <block class="Magento\Framework\View\Element\Js\Components" name="pagecache_page_head_components" template="Magento_PageCache::js/components.phtml"/>
        </referenceBlock>
+        <referenceBlock name="head.additional">


Seems like this block could be removed. Could you do that?

sure thing. accidentally missed it

moved logic from head to head.additional block to load asynchronously, won't remove

engcom-Golf · 2020-11-27T08:59:15Z

@magento run all tests

magento-engcom-team · 2020-11-27T09:02:19Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8489 has been created to process this Pull Request

engcom-Kilo · 2020-11-27T12:37:02Z

@magento import code to https://github.com/magento-tsg/magento2ce

magento-engcom-team · 2020-11-27T12:39:47Z

@engcom-Kilo the branch with code successfully imported intomagento-tsg/magento2ce repository. Branch name: imported-magento-magento2-30961.

engcom-Bravo · 2020-11-28T08:10:16Z

✔️ QA Passed

The result is the same like in the comment above #30961 (comment)

# Conflicts: # app/code/Magento/Checkout/Test/Mftf/Test/StoreFrontFreeShippingRecalculationAfterCouponCodeAddedTest.xml

engcom-Golf · 2020-12-01T07:41:01Z

@magento run all tests

zakdma · 2020-12-01T07:44:38Z

@magento import code to https://github.com/magento-tsg/magento2ce

magento-engcom-team · 2020-12-01T07:47:35Z

@zakdma the branch with code successfully imported intomagento-tsg/magento2ce repository. Branch name: imported-magento-magento2-30961.

m2-assistant · 2020-12-02T16:32:55Z

Hi @engcom-Golf, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team added Area: Frontend Component: PageCache Release Line: 2.4 labels Nov 18, 2020

m2-community-project bot added Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Nov 18, 2020

m2-community-project bot added this to Pending Review in High Priority Pull Requests Dashboard Nov 18, 2020

m2-community-project bot added Progress: pending review labels Nov 18, 2020

ihor-sviziev self-assigned this Nov 18, 2020

m2-community-project bot moved this from Pending Review to Review in Progress in High Priority Pull Requests Dashboard Nov 18, 2020

m2-community-project bot added Progress: review and removed Progress: pending review labels Nov 18, 2020

ihor-sviziev added the Award: bug fix label Nov 18, 2020

ihor-sviziev requested changes Nov 18, 2020

View reviewed changes

m2-community-project bot moved this from Review in Progress to Changes Requested in High Priority Pull Requests Dashboard Nov 18, 2020

m2-community-project bot added Progress: needs update and removed Progress: review labels Nov 18, 2020

ihor-sviziev added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: test coverage labels Nov 18, 2020

m2-community-project bot assigned mrtuvn Nov 18, 2020

m2-community-project bot moved this from Changes Requested to Review in Progress in High Priority Pull Requests Dashboard Nov 18, 2020

m2-community-project bot added Progress: review and removed Progress: needs update labels Nov 18, 2020

ihor-sviziev requested changes Nov 18, 2020

View reviewed changes

m2-community-project bot moved this from Review in Progress to Changes Requested in High Priority Pull Requests Dashboard Nov 18, 2020

m2-community-project bot removed the Progress: review label Nov 18, 2020

engcom-Golf force-pushed the MC-30171 branch from e2248a3 to 6f68dc5 Compare November 27, 2020 08:57

zakdma approved these changes Nov 27, 2020

View reviewed changes

m2-community-project bot moved this from Review in Progress to Ready for Testing in High Priority Pull Requests Dashboard Nov 27, 2020

m2-community-project bot added Progress: ready for testing and removed Progress: review labels Nov 27, 2020

Merge branch '2.4-develop' into MC-30171

a499cdf

ihor-sviziev approved these changes Nov 27, 2020

View reviewed changes

engcom-Bravo moved this from Ready for Testing to Testing in Progress in High Priority Pull Requests Dashboard Nov 28, 2020

engcom-Bravo moved this from Testing in Progress to Merge in Progress in High Priority Pull Requests Dashboard Nov 28, 2020

m2-community-project bot moved this from Merge in Progress to Testing in Progress in High Priority Pull Requests Dashboard Nov 28, 2020

m2-community-project bot added Progress: testing in progress and removed Progress: ready for testing labels Nov 28, 2020

m2-community-project bot moved this from Testing in Progress to Merge in Progress in High Priority Pull Requests Dashboard Nov 28, 2020

m2-community-project bot added the Progress: accept label Nov 28, 2020

ihor-sviziev removed the Progress: testing in progress label Nov 30, 2020

Merge remote-tracking branch 'magento/2.4-develop' into MC-30171

24d27a7

# Conflicts: # app/code/Magento/Checkout/Test/Mftf/Test/StoreFrontFreeShippingRecalculationAfterCouponCodeAddedTest.xml

magento-engcom-team merged commit be5db4a into magento:2.4-develop Dec 2, 2020

sidolov moved this from Merge in Progress to Recently Merged in High Priority Pull Requests Dashboard Dec 10, 2020

ihor-sviziev mentioned this pull request Jan 2, 2021

Add to Cart Form wrong Form Key in FPC #13746

Closed

magento-engcom-team removed this from Recently Merged in High Priority Pull Requests Dashboard Oct 14, 2021

makzef mentioned this pull request Dec 28, 2021

34686: fix form key value for toolbar post request #34892

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MC-30171: Add to Cart Form wrong Form Key in FPC #30961

MC-30171: Add to Cart Form wrong Form Key in FPC #30961

engcom-Golf commented Nov 18, 2020

m2-assistant bot commented Nov 18, 2020

engcom-Golf commented Nov 18, 2020

ihor-sviziev Nov 18, 2020

mrtuvn Nov 18, 2020

ihor-sviziev Nov 18, 2020 •

edited

Loading

engcom-Golf commented Nov 18, 2020

engcom-Golf commented Nov 18, 2020

ihor-sviziev left a comment

ihor-sviziev Nov 18, 2020

engcom-Golf Nov 19, 2020

engcom-Golf Nov 26, 2020

engcom-Golf commented Nov 27, 2020

magento-engcom-team commented Nov 27, 2020

engcom-Kilo commented Nov 27, 2020

magento-engcom-team commented Nov 27, 2020

engcom-Bravo commented Nov 28, 2020

engcom-Golf commented Dec 1, 2020

zakdma commented Dec 1, 2020

magento-engcom-team commented Dec 1, 2020

m2-assistant bot commented Dec 2, 2020


		use Magento\Framework\View\Helper\SecureHtmlRenderer;

		$scriptString = <<<HTML

MC-30171: Add to Cart Form wrong Form Key in FPC #30961

MC-30171: Add to Cart Form wrong Form Key in FPC #30961

Conversation

engcom-Golf commented Nov 18, 2020

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

m2-assistant bot commented Nov 18, 2020

engcom-Golf commented Nov 18, 2020

ihor-sviziev Nov 18, 2020

Choose a reason for hiding this comment

mrtuvn Nov 18, 2020

Choose a reason for hiding this comment

ihor-sviziev Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

engcom-Golf commented Nov 18, 2020

engcom-Golf commented Nov 18, 2020

ihor-sviziev left a comment

Choose a reason for hiding this comment

ihor-sviziev Nov 18, 2020

Choose a reason for hiding this comment

engcom-Golf Nov 19, 2020

Choose a reason for hiding this comment

engcom-Golf Nov 26, 2020

Choose a reason for hiding this comment

engcom-Golf commented Nov 27, 2020

magento-engcom-team commented Nov 27, 2020

engcom-Kilo commented Nov 27, 2020

magento-engcom-team commented Nov 27, 2020

engcom-Bravo commented Nov 28, 2020

engcom-Golf commented Dec 1, 2020

zakdma commented Dec 1, 2020

magento-engcom-team commented Dec 1, 2020

m2-assistant bot commented Dec 2, 2020

ihor-sviziev Nov 18, 2020 •

edited

Loading