Problem/Motivation

Found while digging into #3011426: Page cache size is unlimited when arbitrary query parameters are requested (page cache bloat from arbitrary query strings). SystemHooks::jsSettingsAlter() (hook_js_settings_alter) reads $request->query->all() and dumps it into drupalSettings.path.currentQuery on every page with a query string. No cache context declared for it anywhere. active-link.js is the only Drupal Core consumer (core/misc/active-link.js) of it. Uses JSON.stringify(path.currentQuery) to build a selector matched against each link's data-drupal-link-query attribute, to decide is-active state.

Confirmed on a vanilla umami install: view source of the same page with vs without a random query string, only real diff in the HTML is this value. No exposed filter, no custom code involved. Blocks any attempt (e.g. contrib #3610948: Automatically cache pages that don't vary by query args) at sharing page cache entries across query string variants, since the dependency on query strings as cache context isn't noted. It also risks e.g. a newsletter system having query string value containing PII getting cached e.g. by contrib. As noted in that issue, window.location has this info already.

Steps to reproduce

Visit any page with a query string, e.g. /node/1?foo=bar. View source, search drupalSettings, see path.currentQuery reflects it.

Proposed resolution

active-link.js builds current query from window.location.search (URLSearchParams) instead of drupalSettings.path.currentQuery. Needs to replicate ksort + Json::encode format used both in SystemHooks::jsSettingsAlter and LinkGenerator::generate() (data-drupal-link-query), so sort keys then JSON.stringify, same as PHP side does now. Once nothing reads currentQuery, file follow-up to deprecate/remove it.

Remaining tasks

Confirm JS JSON.stringify output matches PHP Json::encode byte for byte for typical query shapes (strings, nested arrays via foo[]=). Test coverage for active-link with query args. Make a change record so contrib knows not to rely on currentQuery.

User interface changes

None.

Introduced terminology

None.

API changes

None, JS behavior only.

Data model changes

None.

Release notes snippet

active-link.js now reads the current query string from window.location instead of drupalSettings.path.currentQuery.

Issue fork drupal-3620504

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

scott_euser created an issue. See original summary.

scott_euser opened merge request !16950

scott_euser’s picture

scott_euser commented
Assigned: Unassigned » scott_euser
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

one thing I'm not sure about is whether there are any special cases around ajax and updating URLs, for example the new views ajax behavior. Maybe this is actually better, but I haven't thought it through.

scott_euser’s picture

scott_euser commented
Related issues: +#3620505: Deprecate drupalSettings.path.currentQuery

Follow-up issue added as a related issue

scott_euser’s picture

scott_euser commented
Related issues: +#2902769: active-link.js throws JS error if query string parameter contains a single quote, +#2525830: LinkGenerator does not generate is-active class when Russian characters are used in the query option

Added some related issues; when converting from currentQuery I think it makes sense to switch to a more reliable approach altogether. Will have a play and see if those two issues can be solved at the same time

scott_euser’s picture

scott_euser commented

one thing I'm not sure about is whether there are any special cases around ajax and updating URLs, for example the new views ajax behavior. Maybe this is actually better, but I haven't thought it through.

Hmmmm will try to explore it a bit

scott_euser’s picture

scott_euser commented

Okay I think its this in AjaxResponseAttachmentsProcessor.php

      // During Ajax requests basic path-specific settings are excluded from
      // new drupalSettings values. The original page where this request comes
      // from already has the right values. An Ajax request would update them
      // with values for the Ajax request and incorrectly override the page's
      // values.
      // @see system_js_settings_alter()
      unset($settings['path']);

So I think that means we don't need to worry about views ajax here

scott_euser’s picture

scott_euser commented
Assigned: scott_euser » Unassigned
Status: Active » Needs review

I think the php 8.6 warnings are expected and unrelated.

smustgrave’s picture

smustgrave commented
Issue tags: +Needs Review Queue Initiative, +Needs subsystem maintainer review

Left some comments on the MR.

@berdir mentions concern around ajax, wonder if we need any test for that or the fact our existing ones pass is enough? Also I do see the comment in #8.

Lets see if we can sub-maintainer to weigh in.

pwolanin’s picture

pwolanin commented

A little confused about the suggested impact of this change, since I thought random query strings pollute the page cache regardless of what's in the page content?

Trying to look back in history to see why it's in drupal settings. If I got back to 10.x, the current code more-or-less comes from:

commit 4ecc4ead39c67a3ea63bec6ce3510610cc4fde33
Author: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date:   Thu Dec 4 11:37:23 2014 +0000

    Issue #2382557 by Wim Leers: Change JS settings into a separate asset type

Going before that commit, it looks like the authenticated vs anonymous code paths differed and for anonymous it was using PHP to add the class (maybe JS not loaded for anonymous?). The query string was still always going into drupal settings via code in core/includes/common.inc and maybe there was an intent for it to be consistent between the PHP and JS implementations?

The code there comes from this commit which from the commit message looks to be trying to solve the same kind of problem as this current issue!

commit 5d4d6b1c80e3c91253bc015a7cbad1518e5b559d
Author: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date:   Thu Jan 23 18:04:41 2014 +0000

    Issue #1979468 by Wim Leers, nod_, pwolanin, longwave, thedavidmeister, jessebeach, larowlan | catch: .active from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.

https://www.drupal.org/node/1979468

Apparently I worked on that patch long ago...

So, assuming we are currently relying on JS to set the active class for both anonymous and authenticated users, taking the query string out of the window location seems reasonable if it will improve caching.

scott_euser’s picture

scott_euser commented

Thanks for having a look @pwolanin - yes at the moment random query strings pollute the page cache, but it would allow:

  1. Contrib's bit less performance page cache to safely vary automatically without risk of eg a query string containing an email address from a newsletter system getting cached by this code and then showing to another anonymous visitor (#3610948: Automatically cache pages that don't vary by query args).
  2. Things like @berdir is talking about here https://www.drupal.org/project/drupal/issues/3011426#comment-16745092

But I think regardless of whether we can land anything like either of those, we shouldn't be caching the query string in something that does not vary by query string, or the example of the email address coming from some newsletter system can get cached when it shouldn't get cached becomes a possibility I believe.

On anonymous vs authentication: only authenticated users load active-link.js (see BareHtmlPageRenderer). Anonymous users still get the class in PHP via ActiveLinkResponseFilter, which reads the request query. So this MR only changes the authenticated JS path.

Re #10 @smustgrave, thanks I addressed the comments. I am unsure on whether there is anything more to test on, open to suggestions.

Thanks all!