See parent issue #3200985: [meta] Fix undesirable access checking on entity query usages for context and test coverage policy.

Views argument plugins titleQuery() methods usually use loadMultiple() to retreive relevant entities, so do not check access. Therefore entity queries in titleQuery() should use accessCheck(FALSE).

Fixes needed:
- core/modules/file/src/Plugin/views/argument/Fid.php titleQuery
- core/modules/node/src/Plugin/views/argument/Vid.php titleQuery

Issue fork drupal-3202052

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw opened merge request !393

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Status: Active » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Needs work
Issue tags: +Needs tests

It looks like missing tests to document expectations

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Status: Needs work » Needs review

@andypost see #3200985: [meta] Fix undesirable access checking on entity query usages for test coverage policy specific to this family of issues.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

I don't understand where these titles are used. If this is displayed to the user, isn't this a possible access bypass if they should not be allowed to see the title? They can use an existing view (or perhaps construct one), pass in an argument ID that they aren't allowed to see, and then get the title this way.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

I don't understand where these titles are used. If this is displayed to the user, isn't this a possible access bypass if they should not be allowed to see the title? They can use an existing view (or perhaps construct one), pass in an argument ID that they aren't allowed to see, and then get the title this way.

The titles used here go to the page title block and the HTML document title. So access to them depends is controlled by having access to the view. So yes, if you use any of core's id based contextual argument filters (see e.g. Drupal\node\Plugin\views\argument\Nid) then you're giving anyone with access to the view access to the label of the entity that is being filtered by id. I'm not sure why core thinks this is OK: maybe it's just an understood consequence, maybe it's a bug, maybe labels are not access-controlled.

In the case of core/modules/node/src/Plugin/views/argument/Vid.php this is fairly straightforward, it's just the label being exposed so our fix follows the pattern elsewhere in core.

In the case of core/modules/file/src/Plugin/views/argument/Fid.php it's more complicated, because we're exposing the filename not the label so that's going beyond what core does in the other argument plugins.

I don't know how to proceed here, I will ask for some input.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: +Needs subsystem maintainer review

Argument validation allows you set 'verify the user has access to the $entity_type' when you configure it - so that is switching on and off an entity access check, it's not relying on query access.

Having said that, I don't know about these specific argument plugins.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Other argument "ID" plugins using no access check because most of implementations now like $this->storage->loadMultiple($this->value) to get list of titles by IDs

Checked history of changes in this plugins

- #2628130: SQL error on revision export from view is where \Drupal\node\Plugin\views\argument\Vid::titleQuery() changed direct db query to revisions, as it initially not supposed to filter revisions (vids) and that's why it should be FALSE

- #2068343: Convert file SQL queries to the Entity Query API similar conversion has done for \Drupal\file\Plugin\views\argument\Fid::titleQuery() so also no reason to apply access here

I bet "Needs subsystem maintainer review" is not needed

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Meantime not sure this method is useful when there's a lot of revisions...
It's not clear why the method implementations loading entities just to get titles.
Moreover no translation support for labels/titles - probably it needs separate issue and views maintainers' opinion

  • catch committed c2241ab on 9.2.x 
    Issue #3202052 by jonathanshaw, andypost: EntityQuery accessCheck: Views...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs subsystem maintainer review

Yeah agreed with #9 and removing the subsystem maintainer review tag, this is the wrong place to check access, so we shouldn't be, and good to see the history too.

@andypost would you be able to create the follow-up?

Committed c2241ab and pushed to 9.2.x. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

@catch I filed follow-up #3209144: Improve loading entity titles in views\argument\*ID plugins but no idea how to tag it

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.