Would like to see the security_review module integrated with the site_audit module.Clos

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

codi’s picture

Status: Active » Needs review
FileSize
0 bytes

Patch attached.

FluxSauce’s picture

Neat! 0 bytes :-(

Does this require security_review to be installed as a module, or can it be installed like a drush command?

coltrane’s picture

Security Review module does not have to be installed to run the drush checks. See http://drupalcode.org/project/security_review.git/blob/refs/heads/7.x-1.... for how to use.

codi’s picture

Sorry about the 0 bytes. Here's the real one.

FluxSauce’s picture

coltrane - rad, thank you!

codi - Thanks for the patch, good start! However, crashed out-of-the-box.

require_once(Report/Security.php): failed to open stream: No such file or directory site_audit.drush.inc:10 [warning]

Fixed that by using the correct class name:

  $report = new SiteAuditReportSecurityReview();

Then I ran the report...

drush asec
Security Review: 0%
  Admin Permissions
                                                                                   [error]
  Base URL
                                                                                   [error]
  Error Reporting
                                                                                   [error]
  Executable PHP
                                                                                   [error]
  Failed Logins
                                                                                   [error]
  Field
                                                                                   [error]
  Input Formats
                                                                                   [error]
  Private Files
                                                                                   [error]
  Query Errors
                                                                                   [error]
  Temporary Files
                                                                                   [error]
  Untrusted PHP
                                                                                   [error]
  Upload Extensions
                                                                                   [error]

That doesn't seem right, same goes for the detail view:

drush asec --detail
Security Review: 0%
  Admin Permissions: Run configuration security checks on your Drupal site.

  Base URL: Run configuration security checks on your Drupal site.

  Error Reporting: Run configuration security checks on your Drupal site.

  Executable PHP: Run configuration security checks on your Drupal site.

  Failed Logins: Run configuration security checks on your Drupal site.

  Field: Run configuration security checks on your Drupal site.

  Input Formats: Run configuration security checks on your Drupal site.

  Private Files: Run configuration security checks on your Drupal site.

  Query Errors: Run configuration security checks on your Drupal site.

  Temporary Files: Run configuration security checks on your Drupal site.

  Untrusted PHP: Run configuration security checks on your Drupal site.

  Upload Extensions: Run configuration security checks on your Drupal site.

A couple things beyond the obvious;

  • Create a new check to see if the security review module is even available. If not, abort and don't run the rest of the checks. See the logic used in Views Enabled.
  • $result['object'][0]['result'] is super redundant. Why not just set the registry value to $result['object'][0]['result'] ?
  • Use a custom getResultInfo and getDescription for each check. Is there way to get that out of security_review without hard coding it?
  • Don't report problems, like security_review missing in getResultInfo - in every single check; that's not info. Only do that in the enabled check
  • Put in a check to see if the report was requested in HTML mode. If not HTML, don't render as HTML.
  • if ($result === FALSE) return SiteAuditCheckAbstract::AUDIT_CHECK_SCORE_INFO; - this should be broken up over multiple lines (see Drupal coding standards), and the only time you should use that logic is in an enabled check (pass if it's enabled, info if it's not).
  • If there's a failure, display the failure (didn't see it in HTML mode either)

Tested using PHP 5.3, Drush 5.9 on a Drupal 7.23 site and security_review 7.x-1.1 and site_audit 1.5.

FluxSauce’s picture

Status: Needs review » Needs work
FluxSauce’s picture

Hi Codi, have you gotten a chance to take a look at this? I'd love to see this in Site Audit.

  • Commit 1ea7d35 on 7.x-1.x by FluxSauce:
    Issue #2245239 and Issue #2129415 - laying groundwork to allow other...
FluxSauce’s picture

FYI, I'm going to treat this as a meta issue now; I've added the support for custom checks and reports into the dev branch and provided a patch in #2279283: Integrate with site_audit - progress is being made!

codi’s picture

Awesome, thanks for all the hard work Flux.

  • FluxSauce committed 1ea7d35 on 8.x-2.x
    Issue #2245239 and Issue #2129415 - laying groundwork to allow other...
FluxSauce’s picture

Issue summary: View changes
Status: Needs work » Closed (fixed)

Closing this one, work is completely in the security_review queue now. Almost ready to merge!