[1.0.x] Secure Password Reset Log

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.

• New releases are not necessary for these applications, which could require changes that are not backward-compatible. Not creating new releases avoids any possible issue.
• Please do not change the branch to review once reviews started, except in the case the used branch needs to be deleted.
• If you have not done it yet, enable GitLab CI for the project, and fix what reported from the phpcs job. This help to fix most of what reviewers would report.
• For the time this application is open, only your commits are allowed. No other people, including other maintainers/co-maintainers can make commits.
• 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 won't be changed by this application.
• Nobody else will get the permission to opt projects into security advisory policy. If there are other maintainers/co-maintainers who will to get that permission, they need to apply with a different module.
• 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 project name and the branch to review.

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. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool only once per application. The configuration used for these tools needs to be the same configuration used by GitLab CI, stored in the GitLab Templates repository.
• 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.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that does not follow the coding standards, contains possible security issue, or does not correctly use the Drupal API
• The single review points are not ordered, not even by importance

secure_password_reset_log.module

For a new module that aims to be compatible with Drupal 10/11, it is expected it implements hooks as class methods as described in Support for object oriented hook implementations using autowired services.
It requires increasing the minimum required Drupal 10 version, but that is not an issue, since not all the Drupal 10 releases are currently supported.

/**
 * Implements hook_views_api().
 */
function secure_password_reset_log_views_api() {
  return [
    'api' => 4.0,
    'path' => 'src/Plugin/views/field', // Directory where DeleteLink.php is located
  ];
}

Drupal core does not use that hook, which is also not necessary, as view plugins are discovered like any other Drupal plugin.

src/Controller/LogController.php

Since that class uses two methods from the parent class, it does not need to use ControllerBase as parent class. Controllers do not need to have a parent class; as long as they implement \Drupal\Core\DependencyInjection\ContainerInjectionInterface, they are fine.

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
          $container->get('database'),
          $container->get('flood'),
          $container->get('date.formatter'),
          $container->get('page_cache_kill_switch')
      );
  }

With AutowireTrait, that code is no longer necessary.

src/EventSubscriber/PasswordResetSubscriber.php

With Drupal 11, classes should use property promotion.

      $users = \Drupal::entityTypeManager()
        ->getStorage('user')
        ->loadByProperties(['mail' => $email]);

Any dependency needs to be obtained via dependency injection.

1. FILE: README.md

The README file is missing the required section - Requirements.

2. FILE: secure_password_reset_log.module

/**
 * @file
 * Primary module hooks for secure_password_reset_log module.
 */

Drupal does not have primary and secondary hooks. Instead of that, it is preferable to use the usual description: “Hook implementations for the [module name] module”, where [module name] is the name of the module given in its .info.yml file.

3. FILE: src/Controller/LogController.php

  /**
   * The database connection.
   *
   * @var \Drupal\Core\Database\Connection
   */
  protected Connection $database;

  /**
   * The flood control service.
   *
   * @var \Drupal\Core\Flood\FloodInterface
   */
  protected FloodInterface $flood;

  /**
   * The date formatter service.
   *
   * @var \Drupal\Core\Datetime\DateFormatterInterface
   */
  protected DateFormatterInterface $dateFormatter;

  /**
   * The page cache kill switch.
   *
   * @var \Drupal\Core\PageCache\ResponsePolicy\KillSwitch
   */
  protected KillSwitch $killSwitch;

  /**
   * Constructs a LogController object.
   *
   * @param \Drupal\Core\Database\Connection $database
   *   The database connection.
   * @param \Drupal\Core\Flood\FloodInterface $flood
   *   The flood control service.
   * @param \Drupal\Core\Datetime\DateFormatterInterface $dateFormatter
   *   The date formatter service.
   * @param \Drupal\Core\PageCache\ResponsePolicy\KillSwitch $killSwitch
   *   The page cache kill switch.
   */
  public function __construct(Connection $database, FloodInterface $flood, DateFormatterInterface $dateFormatter, KillSwitch $killSwitch) {
    $this->database = $database;
    $this->flood = $flood;
    $this->dateFormatter = $dateFormatter;
    $this->killSwitch = $killSwitch;
  }

FILE: src/EventSubscriber/PasswordResetSubscriber.php

  /**
   * The current user service.
   *
   * @var \Drupal\Core\Session\AccountProxyInterface
   */
  protected AccountProxyInterface $currentUser;

  /**
   * The database connection.
   *
   * @var \Drupal\Core\Database\Connection
   */
  protected Connection $database;

  /**
   * The flood control service.
   *
   * @var \Drupal\Core\Flood\FloodInterface
   */
  protected FloodInterface $flood;

  /**
   * The config factory service.
   *
   * @var \Drupal\Core\Config\ConfigFactoryInterface
   */
  protected ConfigFactoryInterface $configFactory;

  /**
   * Constructs a new PasswordResetSubscriber object.
   *
   * @param \Drupal\Core\Session\AccountProxyInterface $current_user
   *   The current user service.
   * @param \Drupal\Core\Database\Connection $database
   *   The database connection.
   * @param \Drupal\Core\Flood\FloodInterface $flood
   *   The flood control service.
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The config factory service.
   * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
   *   The string translation service.
   */
  public function __construct(
    AccountProxyInterface $current_user,
    Connection $database,
    FloodInterface $flood,
    ConfigFactoryInterface $config_factory,
    TranslationInterface $string_translation,
  ) {
    $this->currentUser = $current_user;
    $this->database = $database;
    $this->flood = $flood;
    $this->configFactory = $config_factory;
    $this->setStringTranslation($string_translation);
  }

FILE: src/Form/ClearLogsForm.php

  /**
   * Database connection.
   *
   * @var \Drupal\Core\Database\Connection
   */
  protected Connection $database;

  /**
   * Constructs the ClearLogsForm.
   */
  public function __construct(Connection $database) {
    $this->database = $database;
  }

FILE: src/Form/DeleteLogForm.php

  /**
   * Database connection.
   *
   * @var \Drupal\Core\Database\Connection
   */
  protected Connection $database;

  /**
   * Constructs the DeleteLogForm.
   */
  public function __construct(Connection $database) {
    $this->database = $database;
  }

FILE: src/Plugin/views/field/DeleteLogLink.php

  /**
   * The current user account service.
   *
   * @var \Drupal\Core\Session\AccountInterface
   */
  protected $currentUser;

  /**
   * Constructs a new DeleteLogLink object.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin ID for the plugin instance.
   * @param mixed $plugin_definition
   *   The plugin definition.
   * @param \Drupal\Core\Session\AccountInterface $current_user
   *   The current user account service.
   */
  public function __construct(array $configuration, $plugin_id, $plugin_definition, AccountInterface $current_user) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->currentUser = $current_user;
  }

New modules, which are compatible with Drupal 10 and higher versions are expected to include type declarations in property definitions, and use constructor property promotion.

4. FILE: src/Plugin/views/field/DeleteLogLink.php

* @ViewsField("secure_password_reset_log_delete_link")

FILE: src/Plugin/views/field/UserStatus.php

* @ViewsField("secure_password_reset_log_user_status")

Projects that are compatible with Drupal 10 or higher versions should use attributes instead of annotations.

5. Fix the warnings/errors reported by PHP_CodeSniffer.

Note: I would suggest enabling GitLab CI for the project, follow the Drupal Association .gitlab-ci.yml template and fix the PHP_CodeSniffer errors/warnings it reports.

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

FILE: secure_password_reset_log/secure_password_reset_log.info.yml
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 7 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: secure_password_reset_log/README.md
-------------------------------------------------------------------------
FOUND 1 ERROR AND 3 WARNINGS AFFECTING 4 LINES
-------------------------------------------------------------------------
   9 | WARNING | [ ] Line exceeds 80 characters; contains 266 characters
  11 | WARNING | [ ] Line exceeds 80 characters; contains 168 characters
  33 | WARNING | [ ] Line exceeds 80 characters; contains 113 characters
 157 | ERROR   | [x] Expected 1 newline at end of file; 0 found
-------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------

FILE: secure_password_reset_log/config/install/secure_password_reset_log.settings.yml
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 2 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: secure_password_reset_log/src/Form/ClearLogsForm.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 70 | WARNING | Possible useless method overriding detected
--------------------------------------------------------------------------------

FILE: secure_password_reset_log/src/EventSubscriber/PasswordResetSubscriber.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------------
 119 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead
 179 | ERROR   | [x] Expected 1 space before "?"; 2 found
 182 | WARNING | [ ] \Drupal calls should be avoided in classes, use dependency injection instead
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: secure_password_reset_log/secure_password_reset_log.module
--------------------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 14 | WARNING | [ ] Line exceeds 80 characters; contains 84 characters
 14 | ERROR   | [x] Comments may not appear after statements
 14 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: secure_password_reset_log/secure_password_reset_log.routing.yml
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 39 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: secure_password_reset_log/secure_password_reset_log.install
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------------
 1 | ERROR   | [x] Missing file doc comment
 3 | WARNING | [x] Unused use statement
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

I am changing priority as per Issue priorities.

Hi, here's a partial review from taking a quick look at the code:

- Missing schema for the configuration object provided by the module in config/install.
- New modules, which are compatible with Drupal 10 and higher versions are expected to include type declarations in property definitions, and use property promotion. Update all classes.
- The module is not dependent on the core views module, therefore it should not provide a views configuration file in config/install, but instead it should be moved to config/optional. Unless the views module is required and it becomes a dependency.
- The Kernel test doesn't bring anything beneficial, it's redundant. Update the tests to actually test the code.
- Is the use of $this->killSwitch->trigger(); really needed?
- With Drupal 10 and Drupal 11, there is no longer need to use #default_value for each form element, when the parent class is ConfigFormBase: It is sufficient to use #config_target, as in the following code:

$form['security']['standalone_url'] = [
      '#prefix' => '<hr>',
      '#type' => 'checkbox',
      '#title' => $this->t('Standalone media URL'),
      '#config_target' => 'media.settings:standalone_url',
      '#description' => $this->t("Allow users to access @media-entities at /media/{id}.", ['@media-entities' => $this->entityTypeManager->getDefinition('media')->getPluralLabel()]),
    ];

- Ensure all string are translatable everywhere. For example, the following code doesn't comply:

'#markup' => '<span style="color:green;font-weight:bold;">✔ Existing</span>',

**Review of secure_password_reset_log (1.0.x branch, commit 512a79a)**

This is a re-review. I checked all the issues raised by avpaderno, vishal.kadam, and bbu23. Some were addressed, but several remain unfixed and I found additional issues. Setting back to **Needs work**.

---

### Blockers

1. **`.DS_Store` committed to repo** -- `src/Plugin/.DS_Store` is a macOS artifact tracked by git. Remove it (`git rm`) and add to `.gitignore`.

2. **Config schema in wrong directory** -- The schema file is at `schema/secure_password_reset_log.schema.yml` but Drupal expects `config/schema/secure_password_reset_log.schema.yml`. It will not be discovered in its current location. This was flagged in the previous review as "missing config schema" -- the file was created but placed incorrectly.

3. **Views config has NO access control** -- In `config/optional/views.view.password_reset_logs.yml`, the access section is:
```yaml
access:
type: none
options: { }
```
This means **anonymous users** can view the password reset logs at `/password-reset-logs`, which contain emails, IPs, user IDs, and reset attempt details. This is a serious security issue. Set `type: perm` with permission `view secure password reset logs`.

4. **`is_non_existing_user` field in views data references non-existent column** -- In `secure_password_reset_log.views.inc` line 80, the views data defines `$data[$table]['is_non_existing_user']` but the database schema column is `user_exists`. This will cause Views queries to fail.

5. **Translated strings stored in database** -- In `PasswordResetSubscriber.php`, `$this->t()` results are passed to `logEvent()` and stored in the DB `details` column. Store English strings in the DB and translate at display time.

6. **DeleteLogForm clears ALL flood records** -- When deleting a single log entry, `DeleteLogForm::submitForm()` runs `$this->database->delete('flood')->condition('event', 'password_reset_attempt')->execute()` which clears flood protection for **all** IPs, not just the one associated with the deleted log. This undermines flood protection.

---

### Previous feedback not fully addressed

7. **Property promotion** -- Previous reviewers requested constructor property promotion. `LogController` uses it, but `ClearLogsForm`, `DeleteLogForm`, and `DeleteLogLink` still use the old pattern:
```php
protected $database;
public function __construct(Connection $database) {
$this->database = $database;
}
```

8. **OOP hooks** -- `hook_views_data()` is still in a procedural `.views.inc` file. Previous feedback asked for OOP hooks with `#[Hook]` attributes.

9. **No tests** -- The module has no tests at all (no unit, kernel, or functional tests). For a security module handling flood control and sensitive log data, tests are expected.

---

### Additional issues found

10. **Dead code in LogController** -- The `delete()` and `clearAll()` methods are unused. The routes `secure_password_reset_log.delete` and `secure_password_reset_log.clear_all` both point to form classes, not these controller methods. These methods also reference a non-existent route `secure_password_reset_log.view`. Remove them.

11. **ClearLogsForm bypasses Flood API** -- Directly queries the `flood` table instead of using `FloodInterface::clear()`. The `flood` table is an implementation detail.

12. **Stale comments in PasswordResetSubscriber** (lines 85-89) -- Comments say "In a real scenario, you should inject the entityTypeManager" and "For a quick fix without changing the constructor" but the entity type manager IS already injected. These are copy-paste leftovers.

13. **Stale @see references** -- Both constants reference `\Drupal\secure_password_reset_log\EventSubscriber\SecurePasswordResetEventSubscriber` which does not exist. The actual class is `PasswordResetSubscriber`.

14. **Inconsistent constant visibility** -- `DEFAULT_MAX_ATTEMPTS` has `public` visibility but `DEFAULT_WINDOW` does not. Both should be explicit.

15. **UserStatus views plugin uses inline styles** -- `#markup` with inline `style` attributes instead of CSS classes and proper render arrays. Also contains unicode symbols in `$this->t()` translation strings.

16. **Bare Response for 429** -- `PasswordResetSubscriber` returns `new Response('...', 429)` bypassing Drupal's theming. Use `TooManyRequestsHttpException` instead.

17. **LogController has no pagination** -- Hard-coded `range(0, 50)` with no pager. Use `PagerSelectExtender`.

18. **README formatting** -- Maintainer link syntax is broken: `[zeeshan_khan]https://www.drupal.org/u/zeeshan_khan` (missing parentheses). "SUPPORT" and "LICENSE" headers are missing `##` markers.

@oivanov since u are using AI to help drafting your feedback, u could at least make an effort to format the output from Markdown to html or plain text so that people can easily read it...

me personally - I prefer Markdown to everything else, so I make an effort not to have to deal with HTML (definitely) or plain text (when I can).
If markdown is not acceptable - we can add that to the guidance. Let me know if you want me to do that

great job! All 6 critical blockers and all 3 major issues have been addressed. This is a solid improvement. I see some minor issues still left -

1. ViewsHooks uses bare t() function — src/Hook/ViewsHooks.php uses the global t() function (lines 24-29, etc.) instead of $this->t() with StringTranslationTrait. Since this is now an OOP hook class, it should use the trait.
2. @see on constants are self-referential — Both DEFAULT_MAX_ATTEMPTS and DEFAULT_WINDOW have @see pointing to the class they're defined in (PasswordResetSubscriber). Harmless but pointless — either remove them or point to where the constants are used.
3. FloodSettingsForm missing declare(strict_types=1) — Every other PHP file has it; this one doesn't. Inconsistency.
4. services.yml has redundant arguments with autowire: true — The event subscriber service has both autowire: true and explicit arguments: list. With autowiring, the arguments are unnecessary.

Those are cosmetic/minor, but we might as well address them since we're here :-)

Here is some feedback to improve the README.md

None of this is required for the security coverage application, but it does help users that are interested in using your module.

Review is based on drupal.org README template.

1. ALL CAPS headings: All ## headings use ALL CAPS (## REQUIREMENTS, ## INSTALLATION, etc.). The template requires sentence case: ## Requirements, ## Installation.
2. Too much sections: The README has 16 sections where ~8 is standard. Consolidate OVERVIEW, KEY FEATURES, and HOW IT WORKS into the introduction directly under the # heading. Remove sections that don't belong: LICENSE (handled by drupal.org), ROADMAP (use the issue queue), SUPPORT, UNINSTALLATION, and the duplicate DEPENDENCIES (already covered by Requirements).
3. Metadata block after title: The lines Drupal 11 Security Module, Author: Community Contribution, Package: Security look like structured data for automated parsing, not human-readable content. Remove them.
4. Wrong installation instructions: The README tells users to manually place the module in /modules/custom/. Contributed modules should be installed via Composer. Use the standard text: "Install as you would normally install a contributed Drupal module. Visit https://www.drupal.org/node/1897420 for further information."
5. Missing Markdown list formatting: Multiple sections (Key Features, How It Works, Security Considerations) list items as plain text without - dash markers. Use proper Markdown lists throughout.

A good reference to follow: Views GeoJSON README.

The readme is very good now

I am not so familiar with the exact process for these applications. But I understand from #539608: What to expect from the review process that I am allowed to set it RTBC if there are no more issues found.

Did you address all the reported issues?

I'll give other reviewers a chance to have another look and if there is nothing to report I will set it RTBC next week.

I re-reviewed issues that I'm familiar with. All issues from my previous reviews have been addressed:

Critical issues — fixed:
- .DS_Store removed from repo (added to .gitignore)
- Config schema moved to config/schema/
- Views access now properly requires 'view secure password reset logs' permission
- is_non_existing_user column renamed to user_exists throughout (schema, views data, subscriber)
- Database-stored strings no longer use $this->t()
- Single log deletion now clears flood only for that entry's IP, not all IPs

Major issues — fixed:
- Constructor property promotion used throughout
- OOP hooks via #[Hook] attribute (ViewsHooks)
- Tests added: 2 Functional (access control, flood protection) + 2 Kernel (schema, views data)

Follow-up issues — fixed:
- StringTranslationTrait added to ViewsHooks
- Self-referential @see annotations removed
- declare(strict_types=1) added to all PHP files
- Redundant service arguments removed (autowire)

Code looks clean, tests cover the critical paths. Setting to RTBC.

Thank you for your contribution and for your patience with the review process!

I am going to update your account so you can opt into security advisory coverage any project you create, including the projects you already created.

These are some recommended readings to help you with maintainership:

• Dries' post on Responsible maintainers
• Maintainership
• Git version control system
• Issue procedures and etiquette
• Maintaining and responding to issues for a project
• Release naming conventions.

You can find more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!
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.

I thank also all the reviewers for helping with these applications.