[1.0.x] Site Guardian

@apaderno - many thanks for your comments and assessments, and apologies that it's taken so long to get back to you, but we just wanted to let you know that we are working through the recommendations you made. Hopefully they will all be done soon!

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() are 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.

@apaderno, @Rassoni, @daceej - thanks to you all for your comments. The Site Guardian team raised tickets for them all and we have now been through the process of making all the changes necessary, please do refer to the SG issue queue as required. I therefore submit this security advisory coverage application for further review here once again, now that the above issues have been addressed. Many thanks in advance!

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 line 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.

Thank you all @vinaymahale, @dunx and @apaderno. The PHPCS issues reported in this thread have now been addressed, as well as others noted by using more stringent set of standards (interesting exercise to compare these).

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 → .

Many thanks once again @vinaymahale and @apaderno - I have now made changes to the project in line with recommendations, as well as a couple of related/corollary changes.

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.

Many thanks for your comments @vishal.kadam. You will note as at comment #17 here, that PHPCS has been stringently run on the Site Guardian code, and according to stricter standards than normally required. None of the issues you mention have ever come to light that way, and running the sniffer again just now, they still don't. Nevertheless, I agree that the class constructor docblocks you mention are contemporarily extraneous, and not necessary in our case anyway, so I have removed them altogether.

However, I was more confused by your comment that 'Function and method declarations are written on a single line.' Are you still referring to the class constructors from which you pasted code? If so, I'd have to say that the coding standards are more ambiguous on this matter than you are.

For instance, I could stick all the arguments for the SiteGuardianController constructor (as per your second code block example) onto one line, but then PHPCS would indeed throw up an error of exceeding the 80 character limit. The coding standards on function declarations → actually don't mention the single line issue you're talking about at all, and whilst it is true that the standards section on line length → does nominally permit longer function definitions to exceed 80 characters, it doesn't say they should or have to - it is left to interpretation.

Above, in comment #16 here, @apaderno is far more stringent than this: while Drupal.org's wording states "In general, all lines of code should not be longer than 80 characters.", he instead, in this very project application, writes that "the Drupal coding standards say text lines must not exceed 80 characters, so longer lines is bad, not good" (my emphasis added), which would certainly contradict what I interpret as your somewhat tacit prescription of an exceedingly long class constructor declaration with all its arguments on a single line.

Indeed, you'll note that there are instances in Core itself where function declarations are not just spread across multiple lines but also decidedly more convoluted than mine, for example in the AttributeClassDiscovery plugin, whose constructor declaration (at time of writing) starts at line 45 but runs down to line 49, and it's arguable that perhaps it's best that it does.

For all these reasons, and above all else for what I see as plain old readability's sake, I'm electing to leave any multi-line constructor declarations mentioned in your comments just as they are.

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.

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