Contrib.social

Use of hook_js_alter breaks core JS cache

Open on Drupal.org →
Created on 13 March 2023, almost 2 years ago

Problem/Motivation

We discovered this in the 3.x version, but it still applies in 4.x

If a restricted user hits a cacheable page before it has loaded for an unrestricted user, Drupal's JS array cache gets truncated.

This happens because restrict_ip_js_alter removes any JS other than jQuery and its own. Presumably, this might be a security feature but it causes unpredictable bugs,

Steps to reproduce

Install a module or a theme that adds custom JS to the site -- a menu JS like Big Menu is a good one.

  • Enable page caching.
  • Test the site as a non-restricted but anonymous user and ensure the JS loads.
  • Clear the cache.
  • Load a page as a restricted user -- get redirected to the login prompt.
  • Load the same page as an anonymous but non-restricted user (someone from a blessed IP)
  • JS will fail to load properly.
  • Site footer JS will stop loading at the jQuery include.

It does not matter if JS aggregation is enabled or not.

You can look in the cache_datatable for the JS cache for debugging purposes.

Proposed resolution

What's happening:

  1. Drupal\Core\Asset\AssetResolver::getJsAssets() is running and then caching its results.
  2. restrict_ip_js_alter() is removing non-jQuery from those results before they are cached.
  3. Subsequent page calls for anon uses retrieve the wrong JS from cache

Remaining tasks

I see two possible fixes here:

  1. Remove the hook entirely -- simplest fix but it theoretically might open a security issue by "leaking" sensitive JS.
  2. Move the JS alter function from the hook to a theme preprocess -- where it can be removed after loadi9ng from cache and only for this module's template. This would be more security safe.

User interface changes

None.

API changes

None.

Data model changes

None.

🐛 Bug report
Status

Active

Version

4.0

Component

Code

Created by

🇺🇸United States agentrickard Georgia (US)

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @agentrickard
  • Comment almost 2 years ago →
    🇺🇸United States agentrickard Georgia (US)
  • Comment almost 2 years ago →
    🇩🇪Germany Anybody Porta Westfalica
  • Comment almost 2 years ago →
    🇺🇸United States agentrickard Georgia (US)

    I think the only question here is "Do we care about the potential security risk of JS leaking during the redirect process?"

    I fell like that's a pretty low risk, but it does provide direction on how to solve this issue.

  • Comment 30 days ago →
    🇩🇪Germany Anybody Porta Westfalica

    @agentrickard plans to provide a MR to solve this? I'm not using this module much, just became maintainer because there were no active maintainers and I wanted to ensure compatibility with latest D11 versions.

    As this is marked as Major, I think it should definitely be resolved.

    Do we care about the potential security risk of JS leaking during the redirect process?

    I also don't see a high risk here, but we introduce some kind of new bug then anyway. Perhaps there's a way to solve both cleanly?

  • Comment 27 days ago →
    🇺🇸United States smustgrave
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024