[BC] Added form key validation to Contacts form

[fballiano]

All credits for this PR go to @elidrissidev which created it originally in #2347. I think that PR should be merged but I made a slight modification, using the default "enable CSFR" instead of a new one.

This is BC because it requires all users to update their custom theme (if they customised their contact form template)

Related Pull Requests

#2347

Fixed Issues (if relevant)

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

Manual testing scenarios (*)

Explained in #2347

[fballiano]

@matteotestoni ;-)

[matteotestoni]

is they customised their contact form template

if they customised their contact form template

[fballiano]

is they customised their contact form template

if they customised their contact form template

typo fixed

[elidrissidev]

We may as well get rid of the separate Newsletter and Checkout flags while we're at it.

[fballiano]

agree, but maybe in a separate PR

[justinbeaty]

This is BC because it requires all users to update their custom theme (if they customised their contact form template)

Should it target next branch?

[fballiano]

I'm a bit confused now... we're now in a situation where things became more confusing and I don't know what should I port to other branches or not...

[fballiano]

btw this is a security concern that should be treated as such, and previously we released those same things in .patch versions

[justinbeaty]

Since next is v21, any breaking change should target there. But if we consider it a security concern I think we have the ability for breaking changes in both v19 and v20.

Actually I’m a bit confused now too because I’m not sure why we have a v20 branch already, unless main is supposed to be v21 and next v22. I have to check the RFC issue when I get to my desk in a bit.

[colinmollenhour]

Since next is v21, any breaking change should target there. But if we consider it a security concern I think we have the ability for breaking changes in both v19 and v20.

The deciding factor for PATCH is does it get a CVE? What risks does spamming the contact form entail?

• It could be nuisance to the store owner
• No real security risk?
• DOS risk perhaps?

Does anyone use this contact form? I always immediately disabled it, basically for these reasons although adding a form key doesn't really mitigate DOS risk, just makes it slightly harder.. 😆

Actually I’m a bit confused now too because I’m not sure why we have a v20 branch already, unless main is supposed to be v21 and next v22. I have to check the RFC issue when I get to my desk in a bit.

Ahh, good call, there is no need for a v20 yet! I'll delete it..

[fballiano]

@colinmollenhour in https://github.com/OpenMage/magento-lts/releases/tag/v20.0.19 we did exactly the same thing (add a formkey to a form) and it was considered as a security concern (it was the password reset instead of the contact form but..)

i'm ok with targeting next, at the end of the day we could also cancel 19.5 and 20.1 and release a 21.0 ;-) it would be time for that and would also match the big changes with composer etc...

[justinbeaty]

i'm ok with targeting next, at the end of the day we could also cancel 19.5 and 20.1 and release a 21.0 ;-) it would be time for that and would also match the big changes with composer etc...

I almost think that is a better idea because it makes it easier for people who are used to their workflow without the vendor dir / release zip.

But then we have zend in our repos for those older branches for the next 5 years. However I am not sure there will be much bug patching there.

[fballiano]

But then we have zend in our repos for those older branches for the next 5 years. However I am not sure there will be much bug patching there.

mmm it would be in v19 and in v20. v19 would be maintained for another 2 years but I don't think we've to maintain v20 at all, yes it could get "patches" but I hope we'll never have to do that.

[justinbeaty]

But then we have zend in our repos for those older branches for the next 5 years. However I am not sure there will be much bug patching there.

mmm it would be in v19 and in v20. v19 would be maintained for another 2 years but I don't think we've to maintain v20 at all, yes it could get "patches" but I hope we'll never have to do that.

You are right, v19 is only until 2025 (then I think we throw a party with some cake to celebrate its demise.)

And yes, we don't need to maintain v20, only backport CVEs. But since there will never be a commit modifying Zend in v21 (since there is no Zend in the v21 codebase), then there's nothing to backport.

The base branch was changed.

[elidrissidev]

Now that this PR is targeting next, v19 and main(v20) will remain without CSRF protection.

[fballiano]

😅 it's either one or the other, I don't have a preference since I think everybody should upgrade to v20 anyway and v19 should be dead and buried.

Since previously this same thing has been treated as a PATCH, I think this should too, let's vote on this for a couple of days and then move on :-)

[colinmollenhour]

I'll defer to the rest of the community but I think releasing a v21 soon is a good idea since v20 has so many changes over v19 as it is.

[fballiano]

@addison74 I think I fixed all the sentences as you suggested ;-)

[addison74]

This PR was initially created for the v19 branch (#2347), but now I see that it was integrated into the next branch only. If the reason is RFC then the rules we have are not exactly good. This PR should be at least in v20. I have been using it in production for a long time and I have reduced by 95% the bots abuse on the contact form.

[fballiano]

it has to be considered breaking change so we have to merge it in the next branch, that's what the RFC decided

[colinmollenhour]

This PR should be at least in v20.

I don't disagree, but we do need for there to be an objective standard and we need to follow it. In this case, the lack of formkey is more of a nuissance and not really a security issue, but it is nuanced.. There may be users who are not having spam issues who would experience an unexpected regression in functionality (maybe they are behind Cloudflare or it is a private store, etc.), thus harming their confidence in the OpenMage project.

You could:

1. Make a case that this is a more severe issue which merits a BC-breaking backport (probably in a new PR for main would be the appropriate place)
2. Propose some language updates for the "rules" to better handle these grey areas
3. Maintain some patches for the fixes that you want and let it remain as-is

Doing a quick search I came across this brief guide which is for maintaining patches in M2: https://devdocs.magento.com/guides/v2.3/comp-mgr/patching/composer.html

Here is a more in-depth one for Drupal, but should be the exact same process: https://davidjguru.github.io/blog/drupal-techniques-patching-modules-with-composer#4--creating-patches-for-drupal

I just wrote a quick blog post guide as well, maintianing your own patches is a lot easier than it used to be. :)
OpenMage/OpenMage.github.io#122

[sreichel]

I just wrote a quick blog post guide as well, maintianing your own patches is a lot easier than it used to be. :)
OpenMage/OpenMage.github.io#122

Nice 👍

Maybe you can add a section about symplify/vendor-patches?

[colinmollenhour]

Maybe you can add a section about symplify/vendor-patches?

Ahh, thanks, I was not aware of that tool, very nice. I added a section for it.