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.
Comment | File | Size | Author |
---|---|---|---|
#5 | 2192149-security-review-notice-5.patch | 2.22 KB | coltrane |
#4 | 2192149-security-review-notice.patch | 1.12 KB | coltrane |
#3 | security_review.ensure_in_checklist.patch | 976 bytes | iaha |
Comments
Comment #1
iaha CreditAttribution: iaha commentedComment #3
iaha CreditAttribution: iaha commentedTrying again - need to make sure the .inc file is loaded before security_review_get_checklist().
Comment #4
coltraneThanks @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.
Comment #5
coltraneThis 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.
Comment #6
coltraneCommitted http://drupalcode.org/project/security_review.git/commit/f79bc0c