Problem/Motivation

Since #3422431: Port views access batch to new plugin mechanism, the views_access check never finds any problem.
That's because in ViewsAccess::run(), the $findings variable is initialized for each view.

Steps to reproduce

Install the webform module.
The "Webform submissions: default" view display is not protected.
Run the views_access check : it does not find this problem.

Proposed resolution

The findings should be correctly stored in $sandbox['findings'].

Issue fork security_review-3463149

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

prudloff created an issue. See original summary.

prudloff opened merge request !75

prudloff’s picture

prudloff commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented

Can you check the setting if you're skipping default.

prudloff’s picture

prudloff commented

What do you mean? The setting is already checked here and the display will not be added to $sandbox['findings'] if the default is skipped.

smustgrave’s picture

smustgrave commented

There’s a setting form for ignoring default

prudloff’s picture

prudloff commented

Yes and $ignore_default contains the value from this setting and is then used in the loop to ignore the default display.
This logic is already there and I have not changed it at all. What are you asking me to do exactly?

smustgrave’s picture

smustgrave commented

Just making sure you had the setting checked in the form

prudloff’s picture

prudloff commented

Oh OK. No I don't have this option checked. We do want to check the default display.

I also tried with a non-default display to be sure and I still can reproduce the bug (and the patch fixes it).

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

So I tested by enabling the show default view setting
Edited the admin content default view to be unrestricted
Ran the check
It captured the finds as expected.

prudloff’s picture

prudloff commented

Is the content view the latest enabled view of the first batch of 5 by any chance?
foreach ($views as $view) { will loop on a batch of 5 views and only keep the findings of the latest in this batch. It will then loop on another batch and keep the latest findings in this batch, etc.

So you can by chance avoid the bug if your view is at the end of a batch.
A way to reproduce this is to unprotect the default display of 4-5 views and run the check: it will only display an error for some of the views.

smustgrave closed merge request !75

  • smustgrave committed b572d86b on 3.0.x 
    Issue #3463149 by prudloff: views_access check is broken
smustgrave’s picture

smustgrave commented
Status: Needs work » Fixed

That was it.

Status: Fixed » Closed (fixed)

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