Problem/Motivation

When using this module, every single page that touches a form gets given the "ip" and "user.permissions" cache context, causing varying on those items in the cache, significantly reducing effectiveness of the dynamic page caches for all pages if you have just a simple cacheable form on every page such as a newsletter or search

In a site where there is eg a header or footer form, this can quickly overwhelm the dynamic cache table size.

Non-dynamic page cache and proxies are unaffected but if you use another module to integrate with those such as Varnish, and have modules using IP whitelisting elsewhere that integrate with those to vary the proxies - it then causes every page to miss cache

Additionally, when IP is in the mix, it varies by each IP whereas it should only have 2 variations: one for in and one for not in the list.

Steps to reproduce

Check X-Drupal-Cache-Contexts on any page with any form, even if the form is not protected

Proposed resolution

  1. Call formIsProtected BEFORE checking for bypass.
  2. Only add the config to the form #cache initially
  3. Add user.permissions only to protected forms
  4. If there is no configuration for IP whitelisting, do not add IP context, only add the IP context if there is an IP configuration
  5. If there is IP in context use a custom cache context to ensure only two variations in the cache rather than one for every IP (so near infinite)

Remaining tasks

-

User interface changes

None

API changes

None

Data model changes

None

Issue fork protect_form_flood_control-3555273

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

driskell created an issue. See original summary.

driskell opened merge request !6

pminf’s picture

pminf
Primary language German
Location Nuremburg (Germany), formerly Dresden
commented

I also stumbled upon this issue and wondering, why this cache context is necessary at all? Since the module does only validate submissions – where AFAIK cache isn't involved – and does not close forms for blocked IPs, we may get rid of the cache context declaration, without a downside.

The cache context has been implemented in the initial version 2021 and has not been changed since. @flocondetoile, what do you think about this today?

scott_euser made their first commit to this issue’s fork.

scott_euser’s picture

scott_euser commented
Status: Active » Reviewed & tested by the community

Tested this out and confirmed; this is a really serious bug; a large site has an out of control cache_dynamic_page as a result because it causes every page to vary by IP address.

Added test coverage, but marking the code itself as RTBC (hope that's okay). Fixed a phpcs issue that was introduced by this. Would be good to get this fixed and released as soon as possible.

scott_euser’s picture

scott_euser commented
Assigned: Unassigned » scott_euser
Priority: Major » Critical
Status: Reviewed & tested by the community » Needs work

Re #3 varying via IP its a good point, will see if I can add a custom cache context that would make this 2 cache entries (e.g those in and those out of allowlist) rather than near infinite combinations if an IP list is set.

Note that I did update #3373111: Fix pipeline linting errors including use of flagged words to use wording 'allowlist' throughout since "whitelist" is a banned term

scott_euser’s picture

scott_euser commented

Ah and switching to critical after discussion with a member of the security team since this can overwhelm cache tables with a huge volume potential leading to a denial of service, but agreed it could remain public.

scott_euser’s picture

scott_euser commented
Status: Needs work » Needs review

Okay followed cores approach to getting ip to vary cache context but instead made it very in 2 states, in or out of the list (so much more efficient caching for this context). Added test coverage, also to verify that it gets cleared if the allowlist changes.

scott_euser’s picture

scott_euser commented
Issue summary: View changes
driskell’s picture

driskell commented

@scotteuser Thanks - LGTM! A stark improvement on my original MR.

scott_euser’s picture

scott_euser commented
Assigned: scott_euser » Unassigned
Status: Needs review » Reviewed & tested by the community

Thanks for confirming, will move back to RTBC

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented

Wow, Nice job. Thanks a lot. Will review it asap.

  • flocondetoile committed c8e100ae on 1.0.x 
    fix: #3555273 Every form on the site gets user.permissions and ip cache...
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented
Status: Reviewed & tested by the community » Fixed

Comitted. Thanks.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented

@scott_euser I believe that #3373111: Fix pipeline linting errors including use of flagged words needs to be rerolled / rebase with this fix committed. If you can update it, i could review it quickly I think.

scott_euser’s picture

scott_euser commented

Thanks! Will have a look now