Skip to content

Conversation

@nicolasddev
Copy link

@nicolasddev nicolasddev commented Apr 8, 2020

Description (*)

This fix ensures that form keys are updated on frontend pages as soon as the form_key input field is loaded in order to prevent delays resulting in the "Invalid Form Key. Please refresh the page" error.

A script tag with a unique ID is being added and executed inline directly after each form_key input ensuring its value is updated as soon as possible.

An internally-developed extension using the same approach is running on one of our customer's production server preventing this issue from occurring. The issue was more frequently encountered on long category pages due to the fact that the HTML had to be downloaded in multiple packets.

This fix is provided by MageMontreal, a Magento Business Solution Partner.

Fixed Issue

Manual testing scenarios (*)

  1. Create a long category page with several visible products
  2. Make sure FPC is enabled and visit the page once with a browser
  3. Using another browser or incognito, visit the same page which is now cached
  4. Try adding a product to cart without waiting for the page to load

Comments

Before applying this fix, when a long page or a page requiring several resources was opened, the form keys on the page were not updated immediately as the below script hadn't been executed yet.

Script: Magento/PageCache/view/frontend/web/js/page-cache.js

As this fix makes sure that the form keys are each updated inline as soon as the content has been downloaded by the browser, the issue doesn't occur anymore.

Note: Uses pure JavaScript to ensure that the scripts are executed without having to wait for RequireJS or any dependencies.

Functional Requirement

Form key input fields must be integrated to the form through the Magento\Framework\View\Element\FormKey block which is best practice, in order to benefit from this fix/improvement.

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

m2-assistant bot commented Apr 8, 2020

Hi @nicolasddev. 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

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

@ghost ghost added Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Priority: P3 May be fixed according to the position in the backlog. labels Jun 9, 2020
@ihor-sviziev ihor-sviziev self-assigned this Sep 4, 2020
@ihor-sviziev
Copy link
Contributor

Hi @nicolasddev,
First of all thank you so much for your contribution and I'm really sorry that this PR was not processed for long time. Now I'm going to review it and will try to move it forward.

@magento run all tests

{
public function afterToHtml(\Magento\Framework\View\Element\FormKey $subject, $result)
{
return $result.$this->getInlineJavaScript();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this shouldn't happen in case if FPC is disabled. Am I correct?

Comment on lines +10 to +21
<script>
var formKey = (function () {

var value = "; " + document.cookie;
var parts = value.split("; " + 'form_key' + "=");

if (parts.length == 2) {
return parts.pop().split(";").shift();
}

})();
</script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. As Magento introduced CSP - we need to use something like this:

    echo /* @noEscape */ $secureRenderer->renderTag('script', [], $scriptString, false);

  2. Seems like case with missing form_key cookie isn't covered here

    if (!formKey) {
    formKey = generateRandomString(this.options.allowedCharacters, this.options.length);
    $.mage.cookies.set('form_key', formKey, options);
    }

  3. I think we need to remove following code as we basically replacing it with inline implementation

    domReady(function () {
    $('body')
    .formKey();
    });

{
$uniqueId = uniqid();

return '<script id="'.$uniqueId.'">document.getElementById("'.$uniqueId.'").previousSibling.value = formKey;</script>';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Magento introduced CSP, we need to replace this code with something like this:

echo /* @noEscape */ $secureRenderer->renderTag('script', [], $scriptString, false);

@gabrieldagama
Copy link
Contributor

The risk was set tomedium due to: the suggested changes may affect other areas/modules.

@ihor-sviziev
Copy link
Contributor

Hi @nicolasddev,
Will you be able to update your PR?

@sidolov
Copy link
Contributor

sidolov commented Sep 23, 2020

@nicolasddev I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@sidolov sidolov closed this Sep 23, 2020
@m2-assistant
Copy link

m2-assistant bot commented Sep 23, 2020

Hi @nicolasddev, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Frontend Component: PageCache Priority: P3 May be fixed according to the position in the backlog. Progress: needs update Release Line: 2.4 Risk: medium Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add to Cart Form wrong Form Key in FPC

5 participants