- πΊπΈ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.
- Merge request !10255Access to Views argument lacks cacheable metadata β (Open) created by danflanagan8
- πΊπΈUnited States danflanagan8 St. Louis, US
I added a fail test that exposes the bug. Now we just need to fix it!
- πΊπΈ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. β .
- πΊπΈ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
Looks lik the LB failure is a known rando:
see π [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing Active or π [random test failure] Drupal\Tests\layout_builder\Functional\LayoutBuilderBlocksTest::testBlockPlaceholder Active
I think I have the Unit test fixed.
- πΊπΈ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.