Problem/Motivation

I'm currently working on swapping out 404/403 pages from using nodes to a static HTML response for performance reasons. While doing this one of our tests started to fail that hit a taxonomy term page as an anonymous user, ensured that the response was a 404, then hit it as an admin and ensured it was a 200. The code this was testing was a hook_entity_access hook which denied access to view terms of specific vocabularies based on a permission.

The problem that I'm seeing is that the initial 404 is cached in dynamic_page_cache and has no cache contexts around user permissions even though the cache headers report there should be a user.permissions cache context on the response.

The taxonomy term page is delivered via a view with the "Content: Has taxonomy term ID" contextual filter. The contextual filter has the "Validate user has access to the Taxonomy term" option checked so when a user visits a term page, the view will then check the user has view access to that term, triggering our access hook.

This coincidentally worked fine without static 404 handling because we were using core's settings to serve a node page, therefore the cache contexts were added to the response from that node route which include the user.permissions context.

However, that doesn't seem to be done with this Views page because \Drupal\views\Plugin\views\argument_validator\Entity::validateEntity just returns FALSE based on $entity->access() and doesn't bubble up any cacheable metadata.

The codepath then sets $this->view->build_info['fail'] to TRUE and Drupal\views\Plugin\views\display\PathPluginBase::execute throws a NotFoundHttpException

I can explicitly add user.permissions to my response from my custom static 404 handler to fix it, but I don't have this same issue with any other types of pages, only these views pages set up with this particular access flow.

Proposed resolution

Somehow bubble up the cacheable data?

Issue fork drupal-3091671

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

acbramley created an issue. See original summary.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.9 was released on November 6 and is the final full bugfix release for the Drupal 8.7.x series. Drupal 8.7.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.8.0 on December 4, 2019. (Drupal 8.8.0-beta1 is available for testing.)

Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

I ran into a similar situation recently. It can technically result in information disclosure, but the security team decided this is an issue we can handle in public. Here's how I reported the issue to security (issue 180326):

Views entity argument_validator does not properly cache access, leading to the possibility of information disclosure.

There's already a public issue from a few years ago that mentions this: https://www.drupal.org/project/drupal/issues/3091671

Here's an example of how this can result in a user seeing a View they shouldn't see. This is a weak example in that none of the content itself is off limits, but it demonstrates the main issue well enough.

1. Do a standard install
2. Create an article that has one published tag.
3. Create a page View of articles that shows titles at /validator-test. Add a contextual filter for " Content: Tags (field_tags)" with the validator configured as "Taxonomy term" with the box "Validate user has access to the Taxonomy term" checked. Take the "Access Denied" action if validation fails.
4. As anonymous user, go to /validator-test/1 where you should see the article listed.
5. As admin, unpublish the tag you created in step 2.
6. As anonymous user, re-load /validator-test/1 which you should no longer have access to. But you do!
7. Clear cache and reload as /validator-test/1 anonymous user. Now you correctly lack access.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Related issues: +#3427374: taxonomy_tid ViewsArgumentDefault plugin doesn't add cache tags

On that security issue, @lendude had some nice comments that I want to put here:

The main issue I see here is getting the correct tags and contexts if a term (or the same seems to go for any entity) doesn't validate. We can add the term tag but the actual validation/access check is done using \Drupal\views\Plugin\views\argument_validator\Entity::validateEntity, which only returns a boolean and not a \Drupal\Core\Access\AccessResult. So we lose all the relevant cache information from the access check.
Ah. Just read @acbramley IS on https://www.drupal.org/project/drupal/issues/3091671 and that seems to have the same conclusion :)

So adding the Term tags will solve the problem outlined here, but I don't think it would solve all the issues

Not too familiar with the Context definitions, but that also doesn't seem to contain anything relevant to access checks right?

Also, another potential issue we need to consider, if we do this on the argument_validator plugin level, the cache information will only bubble up to the config since all cache metadata is stored in the View config, so information about specific term IDs/access will not be available at that point.

I replied with:

Thanks, @Lendude. I think it's #7 that makes this really hard :(

In fact, the "fix" I put in for https://www.drupal.org/project/drupal/issues/3427374 is at once completely accurate and completely worthless since the cacheability is only calculated when the View is saved.

It seems like we need to calculate the argument/argument_default/argument_validator cacheability at runtime somewhere.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Aha! There is a way to get runtime cacheability for argument-related plugins! I knew I had done something that worked before. That was in the contrib space for #3240332: Cache tags not added to View. Luckily, my patch has a great comment that leads us to the core issue: #2853592: Cacheability metadata can't be set from within argument default handlers.

That technique seems really weird, but it could be used in the absence of a better api.

danflanagan8 opened merge request !10255

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Active » Needs work

I added a fail test that exposes the bug. Now we just need to fix it!

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
Related issues: +#2853592: Cacheability metadata can't be set from within argument default handlers.

I pushed up a fix that uses the same kind of funky technique as #2853592: Cacheability metadata can't be set from within argument default handlers..

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs review » Needs work

There were a couple failures:

Layout Builder Blocks (Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocks)
┐
├ Behat\Mink\Exception\ResponseTextException: The text "Placeholder for the "The block label" block" appears in the text of this page, but it should not.
│
│ /builds/issue/drupal-3091671/vendor/behat/mink/src/WebAssert.php:907
│ /builds/issue/drupal-3091671/vendor/behat/mink/src/WebAssert.php:312
│ /builds/issue/drupal-3091671/core/modules/layout_builder/tests/src/Functional/LayoutBuilderBlocksTest.php:219
┴

and

Drupal\Tests\views\Unit\Plugin\argument_validator\EntityTest 0 passes 1s 1 exceptions
FATAL Drupal\Tests\views\Unit\Plugin\argument_validator\EntityTest: test runner returned a non-zero error code (2).
Drupal\Tests\views\Unit\Plugin\argument_validator\EntityTest 0 passes 1 fails

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review

Looks lik the LB failure is a known rando:

see #3477586: [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing or #3486986: [random test failure] Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocksTest::testBlockPlaceholder

I think I have the Unit test fixed.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Left some comments on the MR.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented

Resolved the threads, can the issue summary get some quick eyes. Is that the proposed solution correct? Can the steps to reproduce section be added back.

Going to leave in review for additional eyes.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Will keep an eye out for this one.

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.