Problem/Motivation
Flood control (added in 5.2.0, #3112986: Use flood control API in flag) is checked at render time. Once an anonymous user's IP reaches the limit, FlagFloodControlService::access() returns a forbidden result, and the flag link is not rendered. To keep that state out of caches, the result relies on setCacheMaxAge(0) and an ip cache context.
Core's Internal Page Cache ignores both. It caches anonymous pages by URL only and keeps them until a cache tag is invalidated. So:
- An anonymous visitor whose IP has reached the limit requests a page that is not cached yet. The page is rendered without the flag link and stored in the page cache.
- All anonymous visitors now get that page without the flag link, whatever their IP.
- When the flood window expires, nothing invalidates the page. The
flag_flood:*tag is only invalidated inregister(), which a blocked visitor never reaches. The link stays hidden until something else clears the page, for example saving the entity or a cache rebuild.
The opposite case is harmless: a cached page that still has the link, served to a blocked visitor, is denied when the flag action runs.
Steps to reproduce
- Enable the Internal Page Cache module.
- Give anonymous users the flag permission for a flag, and turn on flood control for it with a low limit (for example 1).
- As anonymous visitor A, flag a node so that the limit is reached.
- Clear the caches. In a new browser without cookies (same IP), open another node with the flag link. The link is missing, which is expected for this IP.
- Open the same node from a different IP, or wait until the flood window has expired. The link is still missing, because the page comes from the page cache.
Proposed resolution
In FlagFloodControlService::access(), call the page_cache_kill_switch service when flood control denies access to an anonymous user. That keeps the denied page out of the Internal Page Cache. Dynamic Page Cache and render caching already respect the cacheability metadata.
Add a functional test with Page Cache enabled. A sessionless visitor who is denied must not prime the page cache for other visitors.
Remaining tasks
- MR with fix and test
- Review
User interface changes
None.
API changes
The FlagFloodControlService constructor gets a new KillSwitch argument. Service constructors are internal, so this is not a BC break.
Data model changes
None.
Drafted with the help of AI.
Issue fork flag-3638056
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:
Comments
Comment #3
anybodyMR ready: when flood control denies access to an anonymous user,
FlagFloodControlService::access()now triggerspage_cache_kill_switch, so the denied page is not stored in the Internal Page Cache.The new
AnonymousFlagFloodPageCacheTestfails without the fix (the denied page is served withX-Drupal-Cache: HIT) and passes with it.The only API change is a new
KillSwitchconstructor argument for the service. The service is autowired, and service constructors are internal, so there is no BC break.Comment #4
anybodyWhile I was a bit curious about this fix, the tests and test-only shows it seems to work. Would be great if a maintainer could take a final look on this improvement.
Comment #5
ivnishLGTM
Comment #8
ivnishComment #10
anybodyAs this only happens in the "deny" case I think it's pretty safe and doesn't affect caching too much. Thanks for the feedback @ivnish!
We're using it in production and it seems to work fine. We found this while working on #3636853: Provide a CAPTCHA integration point for (anonymous) flagging and unflagging which now protects our anonymous flagging successfully.
Comment #11
anybodyAh lol you just merged it :D Thanks!!