Validate user has access option in contextual filter does not bubble access cacheability for views page

Created on 1 November 2019, over 5 years ago
Updated 18 June 2024, 8 months ago

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?

πŸ› Bug report
Status

Active

Version

11.0 πŸ”₯

Component
ViewsΒ  β†’

Last updated 1 day ago

Created by

πŸ‡¦πŸ‡ΊAustralia acbramley

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

Merge Requests

Comments & Activities

Not all content is available!

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

  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

    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.

  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

    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 πŸ› taxonomy_tid ViewsArgumentDefault plugin doesn't add cache tags Fixed 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.

  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

    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 πŸ› Cache tags not added to View Fixed . 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.

  • Pipeline finished with Failed
    3 months ago
    Total: 264s
    #343685
  • Pipeline finished with Failed
    3 months ago
    Total: 4078s
    #343689
  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

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

  • Pipeline finished with Failed
    3 months ago
    Total: 215s
    #343859
  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

    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. β†’ .

  • Pipeline finished with Failed
    3 months ago
    Total: 609s
    #343870
  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

    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

  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US
  • Pipeline finished with Success
    3 months ago
    Total: 1722s
    #343904
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Left some comments on the MR.

  • πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US
  • Pipeline finished with Success
    3 months ago
    Total: 502s
    #359166
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Will keep an eye out for this one.

Production build 0.71.5 2024