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:

  1. 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.
  2. All anonymous visitors now get that page without the flag link, whatever their IP.
  3. When the flood window expires, nothing invalidates the page. The flag_flood:* tag is only invalidated in register(), 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

  1. Enable the Internal Page Cache module.
  2. Give anonymous users the flag permission for a flag, and turn on flood control for it with a low limit (for example 1).
  3. As anonymous visitor A, flag a node so that the limit is reached.
  4. 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.
  5. 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

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:

Comments

anybody created an issue. See original summary.

anybody’s picture

Status: Needs work » Needs review

MR ready: when flood control denies access to an anonymous user, FlagFloodControlService::access() now triggers page_cache_kill_switch, so the denied page is not stored in the Internal Page Cache.

The new AnonymousFlagFloodPageCacheTest fails without the fix (the denied page is served with X-Drupal-Cache: HIT) and passes with it.

The only API change is a new KillSwitch constructor argument for the service. The service is autowired, and service constructors are internal, so there is no BC break.

anybody’s picture

While 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.

ivnish’s picture

LGTM

  • ivnish committed e84a7a45 on 5.3.x authored by anybody
    fix: #3638056 Page cache keeps flag links hidden for all anonymous users...

  • ivnish committed ce406f99 on 5.2.x authored by anybody
    fix: #3638056 Page cache keeps flag links hidden for all anonymous users...
ivnish’s picture

Status: Needs review » Fixed

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.

anybody’s picture

As 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.

anybody’s picture

Ah lol you just merged it :D Thanks!!