[1.0.x] Protected Pages Extra

Thank you for applying!

Before giving links helpful to understand how the review process works, what to expect from a review, and what to do to avoid a review takes more time than needed, I would like to thank all the reviewers for the work they do.
These applications are volunters-driven, which also means it is not possible to predict when an application will be marked fixed and the applicant will get the permission to opt projects into security advisory policy. While we aim to make an application as quick as possible, it is also important for us that more people review the project used for an application. In this way, we make sure applications do not miss some important points that should be instead reported.
Applications are not meant to be complete debugging sessions that eliminate every existing bug, though. I apologize if sometimes applications seem to go into too-detailed reviews.

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. See also Policy on the use of AI when contributing to Drupal, which is valid when contributing to Drupal, either by committing code in a project, or by creating a merge request for an existing project.

The important notes are the following.

• For the purposes of this application, it is not necessary to create releases or pre-releases. It is better to make commits only on a branch, and possibly in the same branch used from the start.
• If you have not done it yet, you should enable GitLab CI for the project and fix the PHP_CodeSniffer errors/warnings it reports.
• 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; once this application is closed, you will be able to change the project status from Not covered to Opt into security advisory coverage. This is possible only 14 days after the project is created.
  
  Keep in mind that once the project is opted into security advisory coverage, only Security Team members may change coverage.
• Only the person who created the application will get the permission to opt projects into security advisory coverage. No other person will get the same permission from the same application; that applies also to co-maintainers/maintainers of the project used for the application.
• 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 project moderator before posting the first comment on newly created applications. Project moderators 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.
• 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.

Reviewed 1.0.x (commit 816ef97) on Drupal 11.

AUTOMATED REVIEW

phpcs (Drupal + DrupalPractice) across php/module/install/yml/twig: 0 errors, 0 warnings. The repo also ships its own phpcs.xml.dist, phpstan.neon.dist and phpunit.xml.dist — good to see.

MANUAL REVIEW — POSITIVES

- Full dependency injection; no \Drupal:: static calls anywhere in src/.
- Clean, modern architecture: HTTP StackMiddleware, a ProtectedPage config entity with a list builder and access checker, a service-provider override and cache/event subscribers — a proper replacement for the original procedural approach.
- Tests present (6 test files), README included, correct core_version_requirement (^11.1), and no hardcoded version/project keys in the .info.yml.

NOTES / QUESTIONS

- Since this is positioned as a refactor of the existing Protected Pages module, a short paragraph in the README (and on the project page) about the relationship and migration path from Protected Pages would help users and reviewers understand the scope.

This is a strong, clean application from a coding-standards and security standpoint. Nicely done.

Reviewed 1.0.x at commit 816ef97.

Automated checks

PHPCS (Drupal, DrupalPractice) across 32 files: 0 errors, 0 warnings, which matches abdelwahied's result in #4.

PHPStan with phpstan-drupal at the declared level 1 is clean apart from the ProtectedPagesSubscriberOverride artefacts your own config already excludes. At level 5 the production code produces exactly one hit, noted below.

Findings

1. Path prefix checks have no boundary

ProtectedPagesExtraMiddleware::handle() line 60 and ProtectedPagesExtraCacheSubscriber::onResponse() line 71 both use str_starts_with($path, '/admin'). ProtectedPageForm::validateForm() lines 279 and 284 do the same for /admin and for the login path.

Paths like /admin-guide, /administrators or /admin-tools satisfy that test without being admin paths, and /protected-page/login-help satisfies the login-path test. In the form this rejects legitimate paths. In the middleware and the response subscriber it skips the protection check.

Form validation is not a security boundary here. A protected_page entity created by config import, by an update hook, or in code never passes through validateForm(), and the schema regex allows such a path, so an entity of that shape would be silently unprotected at request time.

Suggested: $path === '/admin' || str_starts_with($path, '/admin/'), and the same treatment for the login path.

2. No flood control on password attempts

Nothing in the module uses the flood service. Neither the login form nor the ?password= check in the middleware limits guesses, per IP or per entity. The append-password mode makes brute forcing cheap, since it needs only plain GET requests with no form token and no session. Core's user module (the flood service plus user.flood config) is the obvious model. For a module whose entire job is a password gate, throttling plus a logged warning would be a real improvement.

3. matchPath() gives up on the first wildcard whose entity fails to load

ProtectedPagesExtraAccessChecker::matchPath() lines 143 to 149: inside the foreach, a matching pattern does return $entity instanceof ProtectedPageInterface ? $entity : NULL;. A load failure returns NULL immediately instead of continuing to the remaining patterns, so a later valid pattern is never consulted and the path is treated as unprotected. It is unlikely to fire, since wildcardIndex() only records ids it already loaded successfully, but the failure direction is open rather than closed. continue would be safer.

4. Unreachable branch in the migration

protected_pages_extra.install line 48: ProtectedPage::load($id) can never return non-NULL, because _protected_pages_extra_unique_id() already loops until the id is unused. So $skipped never increments and the migration message always reports "0 already existed".

Related, and what PHPStan flags at level 5: $changed in _protected_pages_extra_migrate_settings() is set TRUE at line 96 and never set FALSE, so the if (!$changed) at line 134 is dead code.

5. The append-password option deserves a warning

The checkbox description says what the option does but not what it costs. A password in a query string ends up in web server access logs, proxy logs, browser history, and Referer headers on outbound links. Your README security notes are thorough enough that this stands out by its absence.

6. Tooling

phpstan.neon.dist declares level 1. At level 5 the production code produces only finding 4, so raising it looks cheap.

The @phpstan-ignore bbd.naming.propertyNotCamelCase on ProtectedPage::$allow_append_password targets a sniff that lives in a private Composer repository, and reportUnmatchedIgnoredErrors: false is set project-wide to accommodate it. That weakens PHPStan for everyone else working on the module. The property name has to match the config key, which is ordinary for a config entity, and no public ruleset objects to it.

Two test-only type errors at level 5: SettingsConfigTranslationTest line 53 calls getLanguageConfigOverride() on LanguageManager (type against ConfigurableLanguageManagerInterface, or use language.config_factory_override), and line 96 calls toArray() on DataDefinitionInterface.

Things I checked and found correct

• The caching design holds up. page_cache middleware runs at priority 200 and yours at 30, so page_cache is outside yours and can serve a copy cached before the path became protected. The hook_ENTITY_TYPE_insert and _update calls to Cache::invalidateTags(['http_response']) are what close that, and http_response is core's own bulk lever for exactly this. page_cache's request policy also skips requests carrying a session, so unlocked visitors never populate it. I also checked whether a delete counterpart was missing and concluded it is not needed, since protected responses carry no-store and the login redirect is a plain RedirectResponse that PageCache::storeResponse() refuses.
• The destination handling in ProtectedPagesLoginForm::submitForm() is sound. Prepending the slash after ltrim() neutralises protocol-relative URLs and UrlHelper::isExternal() catches absolute ones.
• Password handling. Passwords are hashed with core's PasswordInterface, blank preserves the existing hash on both forms, and the migration copies the legacy hash rather than re-hashing it, so migrated passwords keep working.
• Pre-auth bypass check. userCanBypass() reading uid straight from the session mirrors what core's own SessionHandler does, so that is not a defect.
• Migration documentation. On avpaderno's point in #5, the README "Migrating from Protected Pages" section does cover entity migration, the settings mapping, permissions, and the decorator for running both modules side by side.

Summary

A strong candidate. The architecture is modern and consistent (config entity, hook attributes, dependency injection throughout, FullyValidatable schema constraints, ConfigTarget on the settings form), and the README is better than most contrib modules ever manage, particularly the caching and config-export sections.

I would want findings 1 and 2 addressed before coverage. The rest are minor.

I use Assisted AI but I review what it generates

As a side note, these applications do not require creating a new release. It is better to work on a branch without tagging new releases.