Shield middleware invokes hooks before modules are loaded, corrupting module_implements cache

Created on 26 April 2022, over 2 years ago
Updated 30 April 2024, 8 months ago

Problem/Motivation

In its middleware, shield is invoking hooks before modules have been loaded, corrupting the module_implements cache. There's at least one instance of this, but there may be others. The known instance has to do with Shield using entity_type.manager in its middleware:

https://git.drupalcode.org/project/shield/-/blob/8.x-1.x/src/ShieldMiddl...

Per 🐛 ModuleHandler skips all hook implementations when invoked before the module files have been loaded Needs review :

A more concrete example is a middleware service using the entity_type.manager. Most of the times the entity type information is retrieved from cache (stored in the discovery cache bin). When this cache however is missing, hooks like hook_entity_type_build() and hook_entity_type_alter() need to be invoked at this early stage. The modules files however haven't been loaded yet, making ModuleHanlder::verifyImplementations() skip all the hook implementations (silently!). The entity definition cache is then incomplete resulting in various hard to debug (next to impossible) exceptions like 🐛 The "paragraph" entity type did not specify a translation handler. Closed: won't fix .

"The "paragraph" entity type did not specify a translation handler." is a common error associated with this issue that can be a critical issue for some.

Steps to reproduce

TBD

There are steps for a similar issue with access_filter 🐛 The "paragraph" entity type did not specify a translation handler. Closed: won't fix . Could probably tweak them to use shield, paragraphs, key, and content_translation modules (with shield configured to use key for authentication, since that's where entity manager is used). Can't confirm right now due to simplytest.me issues.

Proposed resolution

The most direct solution is to appropriate the #3207813 patch and insert ModuleHandler::loadAll somewhere in Shield's middleware.

Per catch's comment here 🐛 ModuleHandler skips all hook implementations when invoked before the module files have been loaded Needs review :

The short answer is that middlewares shouldn't be invoking hooks, there might be a later kernel event listener that would allow you to achieve the same thing.

But that's probably a bigger refactor.

Remaining tasks

Steps to reproduce, patch, test

User interface changes

N/A

API changes

N/A

Data model changes

N/A

🐛 Bug report
Status

Needs work

Version

1.6

Component

Code

Created by

🇺🇸United States charginghawk

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

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇸🇮Slovenia joco_sp

    I found this issue on another one -> The "paragraph" entity type did not specify a translation handler. 🐛 The "paragraph" entity type did not specify a translation handler. Closed: won't fix .

    The patches on both issues didn't help. Uninstalling Shield also triggered the error. And also after uninstalling it, the error was there.

    What helped was the patch from the comment #27 on this issue ModuleHandler skips all hook implementations when invoked before the module files have been loaded 🐛 ModuleHandler skips all hook implementations when invoked before the module files have been loaded Needs review . I don't know how much the issues are related, but it solved the problem in my case. At least for now :) If it appears I'll report it.

  • 🇨🇳China weynhamz Ningbo

    A peculiar issue arose during one of our projects that was exclusive to the Acquia cloud. After monitoring and debugging for weeks, we finally identified the root cause of the problem which led us to this issue.

    Our homepage, which uses the Layout Builder, would randomly lose its entire content section without any errors in the logs. There was no discernible pattern of its occurrence. However, we were able to confirm that a corrupted module_implements cache was the culprit. We eventually connected the dots and identified the problem.

    Since our website was on Acquia, Memcache was configured and enabled by default. It was also used for cache bins such as `bootstrap`, `discovery`, and `config`. The `entity_type` cache item is located within the `discovery` bin. For a production server shared with multiple sites, Memcache evicts items. Our problem occurred when the `entity_type` cache item was evicted, and we had the key module configured with the Shield.

    To reproduce this issue, simply ensure that Shield is configured with the Key module, and then execute the following command with drush:

    ```
    drush ev '\Drupal::cache("discovery")->delete("entity_type");'
    ```

    Upon the next refresh, the `module_implements` cache will become corrupted. We temporarily fixed by adding the following lines at the beginning of the `handle()` method of the `ShieldMiddleware`.

    ```
    // @see https://www.drupal.org/project/shield/issues/3277210 🐛 Shield middleware invokes hooks before modules are loaded, corrupting module_implements cache Needs work
    // Prevent corrupting the module_implements cache.
    $this->moduleHandler->loadAll();
    ```

    Although this could be better resolved by invoking the low-level API of the Key module.

  • I spent several weeks debugging this issue before I noticed that the module_implements cache in the bootstrap bin was getting corrupt somehow...

    I had to add `$this->moduleHandler->loadAll();` right at the start of the handle() function as weynhamz described above - it wasn't effective when placed under `case 'key':`.

    Patch file for this is attached.

  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update over 1 year ago
    9 pass
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 5.7
    last update over 1 year ago
    9 pass
  • 🇨🇦Canada joseph.olstad

    Can someone please explain /elaborate on comment #5?

    I personally would not recommend using this module in a production environment and I expect that this module is most likely uninstalled or functionality disabled in production environments. With that said, I don't like to tell people how to use modules.

    The heavyhandedness of this module should probably be expected as it's doing something that normally people would use Apache to do on the web server level by using configuration in a .conf file.

    With that said, at Drupalcon 2023 there was mention of the testing pipeline perhaps getting performance metrics added to core testing and eventually contrib testing. It would be interesting to know what the performance penalty would be for this patch before actually considering it.

    A clarification/elaboration of what is being asked for in comment #5 would perhaps help guide things forward.

  • 🇺🇦Ukraine vaza18

    Thank weynhamz a lot for steps to reproduce.
    One addition, which helped us to reproduce the issue on any environment even locally:

    drush cr
    drush ev '\Drupal::cache("discovery")->delete("entity_type");'
    

    Next page refresh becomes broken.

  • 🇧🇾Belarus Yury N

    It is not just modules. When using JSON API we get error
    Undefined constant "Drupal\Core\Entity\SAVED_UPDATED"
    Which is not resolved by adding `$this->moduleHandler->loadAll();` but requires require_once(__DIR__ . '/../../../../core/includes/common.inc');

  • 🇫🇷France vbouchet

    Returning back to the D7 version, it was using hook_boot() in place of the http_middleware. Looking at the change records ( https://www.drupal.org/node/1909596 ), it seems using an event_subscriber on KernelEvents::REQUEST.

    I did a small PoC on my local and it seems working.

    What I am not really clear yet is the potential impacts with the various http_middleware that are currently coming after ShieldMiddleware (mainly PageCache) but which will come before the ShieldSubscriber once moved to event_subscriber.

    I am interested in getting some feedback to find a proper solution to it because the plan for the 2.x branch of Shield is to move the Auth method and Exceptions into plugins (so it is easier for each project to extend if needed). However, having plugins will very likely make the corrupted cache issue to arise more often with the current implementation. That is why the 2.x branch is blocked from some time now.

  • 🇺🇸United States kevinquillen

    This sounds very much like what we are seeing with this issue:

    https://www.drupal.org/project/drupal/issues/3277728 🐛 Call to undefined method isLayoutBuilderEnabled() Postponed: needs info

Production build 0.71.5 2024