In cases when a check might be included conditionally, the stored results will become out-of-sync with the checklist if the condition for including the check is TRUE at some point but later becomes FALSE. This causes an undefined index error.

Consider this code sample where a module named 'mymodule' extends secruity_review with a conditional check:

function mymodule_security_checks() {
  $checks = array();

  if (module_exists('foo')) {
    $checks['foo'] = array('...'),
  }
  
  return array('mymodule' => $checks);
}

If I run the security check with the 'foo' module enabled, then disable the 'foo' module and run security check again, I'll get an error about an undefined index 'foo' on the results page. This is because the report bases its output on the stored results and there is no check to see if any stored result has a matching item in the currently active checklist.

I'm attaching a patch for this.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

iaha’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, security_review.ensure_in_checklist.patch, failed testing.

iaha’s picture

Status: Needs work » Needs review
FileSize
976 bytes

Trying again - need to make sure the .inc file is loaded before security_review_get_checklist().

coltrane’s picture

Thanks @iaha, good point. While it's often good to fix issues at the source, I think its appropriate that security_review_get_stored_results() return stored tests regardless of if the check is currently available. Check outputs can handle what to do if a check is from an inactive module. Attached patch covers this just for the check report view.

coltrane’s picture

This patch adds reporting for inactive checks to the settings page.

Example:
"Inactive checks are being stored under namespaces: views. Enabling associated modules may allow these checks to be run again. Inactive checks must be manually removed or uninstalling and reinstalling Security Review will clear all stored checks."

http://monosnap.com/image/dV4DF0KhXYU9zu9ISDqPrHhTszM5vg.png for a screenshot.

coltrane’s picture

Status: Fixed » Closed (fixed)

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