[1.0.x] Site Guardian

Thank you for applying!

Please read Review process for security advisory coverage: What to expect for more details and Security advisory coverage application checklist to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,>DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status will not be changed by this application; no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the branch to review and the project name.

To the reviewers

Please read How to review security advisory coverage applications, Application workflow, What to cover in an application review, and Tools to use for reviews.

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications, even to leave a comment similar to the following one. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• It is also preferable to wait before using a CLI tool to report what needs to be changed, especially because the comment left from Code Review Administrators suggests to use PHP_CodeSniffer. Before that, manual reviews should be done.
• Reviewers should not copy-paste the output of a CLI tool. They should use a CLI tool only once per application. When they do that, they should later verify the code has been correctly changed; this means, for example, that adding a documentation comment that is not correct just to avoid to get a warning/error is not a correct change that should be reported in a further comment.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues.

• What follows is a quick review of the project; it doesn't mean to be complete
• For each point, the review usually shows some lines that should be fixed (except in the case the point is about the full content of a file); it doesn't show all the lines that need to be changed for the same reason
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

    foreach ($project_list as $project) {
      $formatted_project_list .= '<p>' . $project['info']['name'] . ' - Version: ' . $project['info']['version'] . '</p>';
    }

Strings shown in the user interface must be translated.

    if (!$this->isActivated()) {

      \Drupal::logger('site_guardian')->warning("Site Guardian request received but Site Guardian API has not been activated.");

      return "Site Guardian has not been activated for this site. Please contact a site administrator.";
    }

Any dependency should be defined as service arguments, in the .services.yml file.

/**
 * Implements hook_uninstall().
 */
function site_guardian_uninstall() {
  $config = \Drupal::service('config.factory')->getEditable('site_guardian.settings');
  $config->delete('site_guardian_key')
    ->delete('site_guardian_activated')
    ->delete('site_guardian_notes')
    ->save();
}

I think that is already done from Drupal core because none of the core modules deletes the values for the configuration objects they use.

Hello @seeduardo,

Found coding standard issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml site_guardian 

FILE: /Users/projects/drupal-d10/modules/contrib/site_guardian/tests/src/Unit/SiteGuardianServiceUnitTest.php
------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 2 LINES
------------------------------------------------------------------------------------------
  10 | WARNING | Line exceeds 80 characters; contains 96 characters
  10 | WARNING | There must be no blank line following an inline comment
 101 | WARNING | Unused variable $projects.
------------------------------------------------------------------------------------------


FILE: /Users/projects/drupal-d10/modules/contrib/site_guardian/src/Controller/SiteGuardianController.php
------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 6 WARNINGS AFFECTING 6 LINES
------------------------------------------------------------------------------------------
  63 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  65 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  76 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 103 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 105 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 115 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
------------------------------------------------------------------------------------------


FILE: /Users/projects/drupal-d10/modules/contrib/site_guardian/src/Services/SiteGuardianService.php
------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 11 WARNINGS AFFECTING 11 LINES
------------------------------------------------------------------------------------------
  60 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  67 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  93 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  99 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 100 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 131 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 136 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 147 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 154 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 182 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 190 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
------------------------------------------------------------------------------------------

Time: 144ms; Memory: 10MB

Hello @seeduardo!

Can you specify what sets Site Guardian apart from similar modules such as DRD, Vitals, and Monitoring on the product page and/or README?

It looks like the site_guardian_key will be exported as part of the Drupal configuration export. Many modules do this, but you may want to consider explicitly flagging this in the README or on the product page so that site maintainers can take actions as needed. Many site owners prefer to keep config items like this in environment variable or even directly in settings.php as an override.

The access check happening in checkActivationAndSiteGuardianKey() is potentially susceptible to timing attacks. The key comparison should be done using hash_equals() to mitigate.

    if ($savedKey && (empty($site_guardian_key) || $site_guardian_key != $savedKey)) {

      \Drupal::logger('site_guardian')->warning("Site Guardian request received but Site Guardian API access is denied.");

      return "Access denied";
    }

Finally, you should consider taking advantage of the Flood API. It looks like the module may currently open a site up to a brute force attacks while attempting to guess the key. Limiting the number of failed access checks via the Flood API would be a great way to mitigate that.

BTW - I am the maintainer of the Vitals module, which suffered from the exact same issues I'm flagging here. You can see the protections that vitals uses to address these concerns here.

Please fix the appropriate PHPCS issues especially the error reported by PHPCS:

FILE: /site_guardian/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 8 WARNINGS AFFECTING 8 LINES
----------------------------------------------------------------------
 68 | WARNING | Line exceeds 80 characters; contains 232 characters
 69 | WARNING | Line exceeds 80 characters; contains 127 characters
 70 | WARNING | Line exceeds 80 characters; contains 113 characters
 71 | WARNING | Line exceeds 80 characters; contains 122 characters
 74 | WARNING | Line exceeds 80 characters; contains 90 characters
 75 | WARNING | Line exceeds 80 characters; contains 143 characters
 78 | WARNING | Line exceeds 80 characters; contains 108 characters
 79 | WARNING | Line exceeds 80 characters; contains 152 characters
----------------------------------------------------------------------


FILE: /site_guardian/src/Controller/SiteGuardianController.php
-----------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------
 118 | ERROR | [x] Multi-line function declarations must define one parameter per line
-----------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------

Thanks for the feedback @vinaymahale. We'll address at that one error.
The README.md files having long lines is good not bad, so we won't be making any of those changes obviously.

Actually, the Drupal coding standards say text lines must not exceed 80 characters, so longer lines is bad, not good.
Also, this is a seeduardo's application and only seeduardo can do commits for the time this application is open, since we are reviewing what seeduardo understands about writing code that follows the coding standards, correctly uses the Drupal API, and does not contain security issues. We do not review what all the project maintainers as team understand about those points.

No issues found
Changing status to RTBC!

• What follows is a quick review of the project; it doesn't mean to be complete
• For each point, the review usually shows some lines that should be fixed (except in the case the point is about the full content of a file); it doesn't show all the lines that need to be changed for the same reason
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

/src/Controller/SiteGuardianController.php

  /**
   * The flood service.
   */
  protected FloodInterface $flood;

  /**
   * The logger channel.
   */
  protected LoggerChannelInterface $logger;

  /**
   * The module list service.
   */
  protected ModuleExtensionList $moduleList;

Since the module defines itself as installable on Drupal 8 and Drupal 9, it cannot use features available on PHP 7.4.

  /**
   * Container interface.
   */
  public static function create(ContainerInterface $container) {

The documentation comment for methods inherited from a parent class or defined in an interface are different. Container interface. is not a description for a method.

  /**
   * Gets a list of endpoints provided by the module.
   */
  public function endpoints() {

The documentation for the return value is missing.

        $endpoint_links .=
          '<a href="' .
          $route->getPath() .
          '?site_guardian_key=' .
          $siteGuardianKey .
          '">' .
          $route->getDefault('_title') .
          '</a><br><div>' .
          $route->getDefault('_description') .
          '</div><br>';

Instead of building a link markup concatenating strings, the code should use the functions/methods Drupal make available.

src/Form/SiteGuardianForm.php

    $settings->set('site_guardian_key', $form_state->getValue('site_guardian_key'));
    $settings->set('site_guardian_activated', (int) boolval($form_state->getValue('site_guardian_activated')));
    $settings->set('site_guardian_notes', $form_state->getValue('site_guardian_notes'));
    $settings->save();

(Just as a side note) Chaining method calls is possible with the class instance stored in $settings.

I am changing priority as per Issue priorities.

FILE: src/Form/SiteGuardianForm.php

  /**
   * Class constructor.
   */
  public function __construct(SiteGuardianService $site_guardian) {

FILE: src/Controller/SiteGuardianController.php

  /**
   * Class constructor.
   */
  public function __construct(
    FloodInterface $flood,
    LoggerInterface $logger,
    RequestStack $request_stack,
    SiteGuardianService $site_guardian,
    string $site_guardian_key,
    SystemManager $systemManager,
    UpdateManagerInterface $update_manager
  ) {

FILE: src/Controller/SiteGuardianEndpointsController.php

  /**
   * Class constructor.
   */
  public function __construct(
    Connection $database,
    RouteProviderInterface $route_provider,
    SiteGuardianService $siteGuardianService
  ) {

The documentation comment for constructors is not mandatory anymore, If it is given, the description must be Constructs a new [class name] object. where [class name] includes the class namespace.

Function and method declarations are written on a single line.

Hi,

1. I'm not sure why the default configuration is not defined in the config/install folder, but instead set in the hook_install. The only value that is dynamic in that hook seems to be "site_guardian_key", which anyways I assume is must be set afterwards by a human.

2. src/Form/SiteGuardianForm.php:

  /**
   * Site Guardian object.
   */
  protected object $siteGuardian;

  public function __construct(SiteGuardianService $site_guardian) {
    $this->siteGuardian = $site_guardian;
  }

The type of the $siteGuardian is "object" instead of "SiteGuardianService" .

3. src/Form/SiteGuardianForm.php:

  public function __construct(SiteGuardianService $site_guardian) {
    $this->siteGuardian = $site_guardian;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    // Instantiates this form class.
    return new static(
    // Load the service required to construct this class.
      $container->get('site_guardian.SiteGuardianService')
    );
  }

The ConfigFormBase already implements __construct and create, and your Class overrides both. Construct should call the parent and keep the initial param, and create should return both arguments.

4. src/Controller/SiteGuardianController.php (personal preference, minor)

   // If access is allowed.
    if ($this->siteGuardian->accessAllowed($this->siteGuardianKey)) {

      // Clear flood prevention check if required.
      if (!$this->flood->isAllowed('site_guardian_check_attempt', 10)) {
        $this->flood->clear('site_guardian_check_attempt');
      }

      return AccessResult::allowed();
    }

    // If access is denied.
    else {

The "else" statement here is redundant.

manual review:
config schema is missing, see https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-...

Otherwise looks good to me.

Thanks for your contribution, seeduardo!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.