Problem/Motivation

This was originally reported to the security team who decided it could be public
If you have a piece of content which differs among anonymous users for some reason then in Drupal 8 and 9 you could just add the "session" cache context and it would be enough. Anonymous users had random session id, the cache context resolved to a random string and so the render cache ID derived from that differed too and so the render cache never actually hit.

But in Drupal 10, the session id is the empty string, the return value of the cache context is the encoded empty string which is a constant, the render cache ID is constant and so the render cache hits. This leads to information disclosure where the data of the first anonymous user to hit the site after a render cache clear will be served to everyone.

And yes, it can be argued it would've better to be explicit about what makes the content different between anonymous users but there was no need. Now there is. And the depreciation notice or error message does not make this self evident by far -- there's no mention of the session cache context in the first place.

Steps to reproduce

Proposed resolution

Disable caching when the session cache context is present but the session is not started. This matches the old behavior because in D8/D9 every page load got a new session id and so there was never a cache hit.

Remaining tasks

Write a test.

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3440398

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

larowlan created an issue. See original summary.

larowlan’s picture

ghost of drupal past’s picture

Issue summary: View changes

andypost’s picture

andypost’s picture

ghost of drupal past’s picture

The two should be merged into one, I am not partial which stays alive. That one has a test (yay!) but this has a bugfix of some importance:

    if (!$this->requestStack->getSession()->getId()) {
      $metadata->setCacheMaxAge(0);
    }

so either the test should be moved from there to here or the fix should be moved over there. Either case the test needs to be expanded to cover this fix.

ghost of drupal past’s picture

Wrong issue, please disregard. This newfangled process completely confuses me. Patches were way easier.

prudloff changed the visibility of the branch drupal-3442009 to hidden.

prudloff changed the visibility of the branch 3442009-oop-hooks-using to hidden.

prudloff’s picture

Status: Active » Postponed

I suppose we should merge #2915594: SessionCacheContext class should implement CacheContextInterface first then this issue can focus on adding the 0 max age fix when there is no session.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.