OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
867 stars 436 forks source link

Why "Enable Form Key Validation On Checkout" is set to Yes by default? #3218

Closed ADDISON74 closed 3 months ago

ADDISON74 commented 1 year ago

I cloned OpenMage 20.1.0-rc4 to do more test without Magento Sample Pack. After installing it from scratch when I accessed the Backend, I no longer saw the familiar message under the menu like before

Important: Formkey validation on checkout disabled. This may expose security risks. We strongly recommend to Enable Form Key Validation On Checkout in Admin / Security, to protect your own checkout process

form_key_2

When I checked the value, I found that it is now set to Yes by default. When did this happen? It would have been natural for the value to remain set to No precisely as a warning to modify the template in case a custom theme is used.

form_key

fballiano commented 1 year ago

it should come from https://github.com/OpenMage/magento-lts/pull/871 but I agree that it should be on, it is an important security feature :-)

ADDISON74 commented 1 year ago

I have some comments related to this issue:

  1. After installation and accessing the Backend, that warning message was extremely important because it leaves it up to the administrator to activate the option, only after checking that his templates are ready. As it is now, if it uses a theme other than RWD, without changes, being activated by default then it will obviously create problems.

  2. The fact that it is activated by default, the administrator will not get to read the Important label and understand that if his custom templates are not updated according to the change, then the checkout process will not be completed.

  3. The change should have been mentioned in the README because it creates a beautiful BC with the other custom themes.

My opinion is that we should revert that PR. as a strong argument I come from the fact that the displayed warning message causes me to take actions. The fact that the warning is no longer displayed, I consider it a serious problem.

fballiano commented 1 year ago

The configuration for the CSRF is here:

Screenshot 2023-04-29 alle 17 58 20

the fact that it's enabled by default is actually a very old commit:

Screenshot 2023-04-29 alle 18 00 30

at least if I'm not mistaken

fballiano commented 1 year ago

On new installs CSRF really should be enabled by default, since the default checkout works perfectly with CSRF. That setting can be altered by people who need to disable it but it shouldn't be encouraged. IMHO this bug report could be closed.

ADDISON74 commented 1 year ago

I agree, but we must insert an information in the README about this change and those who have not activated it so far to modify their custom templates (if they use them).

sreichel commented 1 year ago

Just an idea ... maybe its possible to search custom templates for missing formkey and show this hint depending on it?