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.