[Theme BC] Add form key validation to Contacts form

[elidrissidev]

Add form key validation when submitting Contacts form.

Description (*)

Similar to #1866, this PR adds form key validation to the Contacts form to prevent bots from sending spam requests to your email address. This option is disabled by default as it is a breaking change for custom themes.

With this change, the hideit field in contacts form is now obsolete, so it may be removed in a future PR if it's safe to do so.

Fixed Issues (if relevant)

1. Fixes Contact Form - Some bots can use the controller behind the form #1911

Manual testing scenarios (*)

1. Go to System -> Configuration -> General -> Contacts -> Security and enable form key validation.
2. Try submitting the form by issuing an HTTP request directly, if everything is fine the request should not go through.

$ curl '<magento_host>/contacts/index/post' -d 'name=John+Doe&email=bot@example.org&comment=test'

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[fballiano]

Since in "core" we have use_form_key:

wouldn't it be better to use the same?

I was also thinking that we had a single flag to enable csfr for all openmage:

shouldn't we use that single one instead of creating a flag for each module?

[elidrissidev]

Yes it would be better to have one global flag, but a separate one is added for backwards-compatibility because if users have system/csrf/use_form_key enabled, their contacts form won't work if they don't apply the change to their custom themes. Same thing for the newsletter.

[fballiano]

I thought so but at the same time now they update and they don't have the new security feature enabled so I don't know. Also the idea of having new configuration options for the same thing it's not what I'd like the most :-\

[elidrissidev]

I'm not a big fan of it either but I can't think of another way to fix this issue. We could remove these separate flags in 20.0(or superior versions) and keep only the global one. There's also a separate flag for the checkout I think.

[elidrissidev]

I think a major reason why many features, especially security related, go unnoticed is because users don't have an easy way of knowing about them. It is easy for such changes to be lost in the changelog next to tens of other changes. Using the notifications for this purpose would be good, but I remember seeing opposing opinions about that in some thread.

[addison74]

The chunk bellow was introduced by the Magento team in the past in /app/code/core/Mage/Checkout/etc/system.xml

        <admin>
            <groups>
                <security>
                    <fields>
                        <validate_formkey_checkout translate="label">
                            <label>Enable Form Key Validation On Checkout</label>
                            <frontend_type>select</frontend_type>
                            <source_model>adminhtml/system_config_source_yesno</source_model>
                            <sort_order>4</sort_order>
                            <comment><![CDATA[<strong style="color:red">Important!</strong> Enabling this option means
                            that your custom templates used in checkout process contain form_key output.
                            Otherwise checkout may not work.]]></comment>
                            <show_in_default>1</show_in_default>
                        </validate_formkey_checkout>
                    </fields>
                </security>
            </groups>
        </admin>

Here is the visual result in Backend

These are my comments:

1. For this PR but also for the Newsletter ([Theme BC] Add formkey validation to Newsletter subscribe action #1866) I would have insert the options in the Admin/Security section and not in Contacts and Newsletter. It is a personal opinion for grouping them in one place and better visibility.

2. I would have used validate_formkey_checkout, validate_formkey_contacts, validate_formkey_newsletter as fields and Enable Form Key Validation On Checkout, On Newsletter, On Contacts as labels. It is a personal opinion to keep the Magento team's initial implementation way.

I haven't tested yet if I can send the form using the curl command, in both situations Enabled/Disabled.

[UPDATE]
Both changes when Enabled (Newsletter and Contacts) do not allow sending the forms using the curl command.

The one for checkout could be moved to Checkout section as well. Admin/Security is not a proper place for it.

[addison74]

@elidrissidev - Thank you for work. PR works without issues and blocks the abusive use of the POST action in the controller. I implemented it in production on a server where 50 messages were received daily through the contact form, all with sexual content.

1. Before approving it I would suggest small format changes to respect the previous implementations of the Magento team. validate_formkey should replace enable_form_key. The changes are in the following 3 files.

In the file /app/code/core/Mage/Contacts/controller/IndexController.php

const XML_CSRF_USE_FLAG_CONFIG_PATH = 'contacts/security/validate_formkey';

In the file /app/code/core/Mage/Contacts/etc/config.xml

            <security>
                <validate_formkey>0</validate_formkey>
            </security>

In the file /app/code/core/Mage/Contacts/etc/system.xml

                <security translate="label">
                    <label>Security</label>
                    <sort_order>60</sort_order>
                    <show_in_default>1</show_in_default>
                    <show_in_website>1</show_in_website>
                    <show_in_store>1</show_in_store>
                    <fields>
                        <validate_formkey translate="label comment">
                            <label>Enable Form Key Validation</label>
                            <frontend_type>select</frontend_type>
                            <source_model>adminhtml/system_config_source_yesno</source_model>
                            <sort_order>1</sort_order>
                            <show_in_default>1</show_in_default>
                            <comment><![CDATA[<strong style="color:red">Important!</strong> Enabling this option means that your custom templates used for contact form must contain <code>form_key</code> block output. Otherwise the contact form will not work.]]></comment>
                        </validate_formkey>
                    </fields>
                </security>

2. In my previous comment I referred to CSRF for Checkout and I suggested placing the recent Backend options for Newsletter and Contacts in /Admin/System/Security. Analyzing in detail I can say that the Magento team made a mistake by placing the option in this section, it should have been placed in Checkout section. In conclusion, you chose to place the options in the correct sections. Maybe in the future we will move that option to the section it belongs to.

3. Perhaps we should also inform through a notification that formkeys must be activated for Newsletter and Contact, as happened for Checkout in the past. If administrators do not activate these options in OpenMage stores, without a reCaptcha protection, they become SPAM deliverers. The OM team discussed about this issue and enabled it by default. More details here Default setting for validate_formkey_checkout => 1 #869 and magento-lts-869 Default setting for validate_formkey_checkout => 1 #871.

4. The HoneySpam extension does not prevent abuse of the controller action, this extension only introduces a bait field in the form and if it is filled out it considers the visitor as bot (in the contact form there is already a field named hideit, but absolutely useless in my opinion when the formkey is used). The fishing percentage of HoneySpam is up to 40%, therefore it is useful and it should be installed, but most of the newsletter subscriptions and requests for information through contact form are made by bots that use the controller directly.

[addison74]

Maybe the formkey check in the controller should be better placed here:

        $post = $this->getRequest()->getPost();
        if ($post) {
            $translate = Mage::getSingleton('core/translate');
            /* @var Mage_Core_Model_Translate $translate */
            $translate->setTranslateInline(false);
            try {
                $postObject = new Varien_Object();
                $postObject->setData($post);

                $error = false;

		if (!$this->_validateFormKey()) {
	            Mage::throwException($this->__('Contacts CSRF validation has failed.'));
                    $this->_redirectReferer();
                   return;
        	}

In this way, when the formkey validation is not done, a line is written in the log file. You can find out how many unsuccessful attempts there were and you can move on with the banning of IPs.

[UPDATE]
Here is the exception caught in the log file in production

2022-08-01T16:58:16+00:00 DEBUG (7): Exception message: Contacts CSRF validation has failed.
Trace: #0 /home/XXXXXXX/public_html/app/code/core/Mage/Newsletter/controllers/SubscriberController.php(53): Mage::throwException('Contacts CSRF...')
#1 /home/XXXXXXX/public_html/app/code/core/Mage/Core/Controller/Varien/Action.php(418): Mage_Newsletter_SubscriberController->newAction()
#2 /home/XXXXXXX/public_html/app/code/core/Mage/Core/Controller/Varien/Router/Standard.php(254): Mage_Core_Controller_Varien_Action->dispatch('new')
#3 /home/XXXXXXX/public_html/app/code/core/Mage/Core/Controller/Varien/Front.php(172): Mage_Core_Controller_Varien_Router_Standard->match(Object(Mage_Core_Controller_Request_Http))
#4 /home/XXXXXXX/public_html/app/code/core/Mage/Core/Model/App.php(365): Mage_Core_Controller_Varien_Front->dispatch()
#5 /home/XXXXXXX/public_html/app/Mage.php(683): Mage_Core_Model_App->run(Array)
#6 /home/XXXXXXX/public_html/index.php(83): Mage::run('', 'store')
#7 {main}

[elidrissidev]

Thank you for sharing your remarks @addison74, I'll add them in the following days.

[fballiano]

I don’t know much about fail2ban and this kind of softwares buy is it possible to write a log file that’s already readable by fail2ban for an easy config?

[addison74]

Fail2Ban reads log files and based on the filters you set it will create jails for IPs. The two log files in OpenMage cannot be processed as long as there is no information about the IP, but in the case of this PR it is useful to find out when the CSRF was not validated and you can easily find the IP, or you can have an evaluation of the non-validations. If the level is high, then you need solutions such as reCaptcha. If you are interested I can give you an example of how Fail2Ban works with HoneySpam, which has a log and blocks all IPs that fill in the bait field, but we do it in Discussions.

[elidrissidev]

I rebased and applied requested changes.

[addison74]

I have to test it, but the various situations that can occur are much better analyzed now. If they are found in the log file then it is a useful information. An increasing number of reports means that the website is under attack and then it is necessary to implement a better level of protection, such as reCaptcha.

[fballiano]

since we were talking about #2557 will we already merge the configuration key?

[elidrissidev]

Merging the configs for CSRF validation will create 2 breaking changes, so I say we leave it till the next major version.

[fballiano]

If we've to break something, let's brake it only once :-D

[elidrissidev]

I agree, but this PR is meant to be backwards-compatible, hence why targeting 1.9.4.x.

[fballiano]

I agree, but this PR is meant to be backwards-compatible, hence why targeting 1.9.4.x.

also v19 will have a major because we're introducing php7.3 requirement, so it will happen for both v19 and v20

[elidrissidev]

I agree, but this PR is meant to be backwards-compatible, hence why targeting 1.9.4.x.

also v19 will have a major because we're introducing php7.3 requirement, so it will happen for both v19 and v20

I think you mean minor version (the middle number), because I was thinking v21 😅

[elidrissidev]

Rebased and applied the changes to translations.

[addison74]

Thank you. Please press the [Resolve conversation] button on the conversations initiated by me and which were resolved by you.

[luigifab]

There is a little error in CSV.
This is not: "<strong style="color:red">Important!</strong>...
This is: "<strong style=""color:red"">Important!</strong>...
Fully working for us.

[elidrissidev]

Good catch @luigifab! will fix soon.

[addison74]

After this PR is merged it must be modified according to the same format as #1866. The value in the config_data table will remain orphaned, but at least we have the same format for both forms.

[fballiano]

since we're in a RC phase I'd much rather prefer using the global config value instead of creating another one, this is the same approach that was taken for https://github.com/OpenMage/magento-lts/releases/tag/v19.4.22 and I think we should do the same here.

[elidrissidev]

Quoting from the new release RFC:

PATCH:
...

• security/stability: if it doesn't warrant a CVE, it doesn't get a PATCH

[fballiano]

That only means that we don’t have to part 19.4 and we can be good on 19.5, that’s why i say we can enable it with the global flag instead of a custom one

[matteotestoni]

This changes are very welcome.
When is possible merge changes and integrated anti spam for contacts?

[fballiano]

@matteotestoni agree, but I think we shouldn't have a specific flag to enable this feature because otherwise I think many people won't enable it.
and in a previous release we treated the same kind of thing as a security concerned and enabled by default for everybody, which i think we should do the same here.

[elidrissidev]

Since the new release RFC is accepted, I guess we should leave this until the next major version so we consolidate Contacts form and Newsletter under the global CSRF config.

[matteotestoni]

Thank you for closing. By now openmage followed by me have spam from contacts without a solution in OpenMage.
I think i need to find another platform

Amasty google recaptcha is not compatible with openmage and other extension are no longer supported by many vendors such as mageworx, amasty and many others.

[fballiano]

I'd convert this one to use the global flag and release it for 20.1.0 or we directly release 21.0, which would be about time anyway.

[matteotestoni]

20 is the right version. Is the version with break compatibility issues. In addition, a database of changes to vendor extensions is needed to make them compatible with Openmage

[fballiano]

actually when we break compatibility with the current (which would be v20) we should release a new major v21...

[elidrissidev]

Thank you for closing. By now openmage followed by me have spam from contacts without a solution in OpenMage. I think i need to find another platform

Amasty google recaptcha is not compatible with openmage and other extension are no longer supported by many vendors such as mageworx, amasty and many others.

My only concern is breaking the rules defined by the new RFC this early on since this issue doesn't have a proper CVE to justify merging it in a MINOR or PATCH release.

[fballiano]

we may not have a CVE but it's still a security concern