[1.0.x] Smart 404

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.

1. FILE: README.md

The README file is missing the required sections - Installation and Configuration.

2. FILE: smart_404.libraries.yml

version: VERSION

VERSION is only used by Drupal core modules. Contributed modules should use a literal string that does not change with the Drupal core version a site is using.

3. FILE: smart_404.module

A new module that aims to be compatible with latest Drupal releases is expected to implement hooks as class methods as described in Support for object oriented hook implementations using autowired services.

4. FILE: src/Form/Smart404IgnoreForm.php and src/Form/Smart404SettingsForm.php

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['image_toolkit'] = [
      '#type' => 'radios',
      '#title' => $this->t('Select an image processing toolkit'),
      '#config_target' => 'system.image:toolkit',
      '#options' => [],
    ];

Using that code, it is no longer needed to save the configuration values in the form submission handler: The parent class will take care of that.

Reviewed 1.0.x (commit e5d08e1) on Drupal 11.

AUTOMATED REVIEW

phpcs (Drupal + DrupalPractice) is clean on all PHP/YAML — no errors. The only violations are 2 auto-fixable coding-standards errors in js/smart_404.admin.js (missing spaces around a ternary "?", line 34). Running phpcbf / prettier on that file fixes them.

MANUAL REVIEW — POSITIVES

- Clean dependency injection throughout src/; no \Drupal:: static calls in business logic.
- Database access goes entirely through the DB API (merge()->key(), select(), update(), delete() with condition()/fields()/execute()) — fully parameterized, no raw SQL.
- No stored XSS on the logged 404 paths: the overview form renders the path via Link::fromTextAndUrl() (auto-escaped title), the referer as a plain-string cell (Twig auto-escapes), and the detail controller escapes with a dedicated escapeHtml() helper inside #markup.
- Good structure (repository, normalizer, glob matcher, suggestion engine), tests present, README + CHANGELOG.

MINOR

- Smart404OverviewForm uses @parse_url($record->referer_last). The @ error-suppression operator is discouraged by Drupal standards; prefer checking the return value (parse_url() returns false on malformed input) without silencing.

Nice, well-organized module. Fix the two JS nits and it looks in good shape.

Following up on the @parse_url() fix in Smart404OverviewForm.php (comment #6/#7): the same pattern is still present in src/Logger/Smart404Logger.php, in sanitizeReferer():

$parsed = @parse_url($referer);

Same fix applies here: check for FALSE explicitly instead of suppressing with @.

Everything else I checked (DB API usage, dependency injection, hook implementations) looks solid.

The error control operator is used to suppress diagnostic errors that might be output; when no diagnostic error is output, it does not make sense to use the error control operator.
In this case, the used code and the following code are equivalent.

$parsed = parse_url($referer);
if ($parsed === FALSE || empty($parsed['host'])) {
  return '';
}

I am changing priority as per Issue priorities.

• 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

src/Controller/Smart404DetailController.php

With Drupal 10.2.x and higher versions, a controller class does not need to implement create(); using AutowireTrait, it just needs to implement the class constructor.

Since that class does not use methods from the parent class, or it uses a single method 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.

src/Hook/Smart404Hooks.php

  /**
   * Implements hook_runtime_requirements().
   *
   * Runs alongside the procedural smart_404_requirements() on Drupal 11.2,
   * and replaces it entirely on 11.3+; see the #[LegacyRequirementsHook]
   * attribute on that function. Both delegate to buildRequirements() so
   * they can never report different requirements depending on which one
   * core happens to invoke.
   *
   * @return array<string, mixed>
   *   The requirements array, keyed by requirement name.
   */
  #[Hook('runtime_requirements')]
  public function runtimeRequirements(): array {
    return self::buildRequirements($this->moduleHandler, $this->configFactory->get('smart_404.settings'));
  }

  /**
   * Builds this module's runtime requirements.
   *
   * Shared by runtimeRequirements() above and the procedural
   * smart_404_requirements() in smart_404.install, which core runs
   * instead of, or (on Drupal 11.2) alongside, this OOP hook depending on
   * the core version; see the #[LegacyRequirementsHook] attribute on that
   * function. A static method (not calling $this->t(), which needs an
   * instance) so the procedural function can call it without needing to
   * construct a Smart404Hooks object of its own.
   *
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $moduleHandler
   *   The module handler.
   * @param \Drupal\Core\Config\ImmutableConfig $config
   *   The smart_404.settings config object.
   *
   * @return array<string, mixed>
   *   The requirements array, keyed by requirement name.
   */
  public static function buildRequirements(ModuleHandlerInterface $moduleHandler, ImmutableConfig $config): array {
    $requirements = [];
    $search404Installed = $moduleHandler->moduleExists('search404');

    // RequirementSeverity does not exist yet on Drupal core versions older
    // than 11.2, which the procedural caller of this method still needs to
    // support; enum_exists() is a no-op guard on 11.2+, where it always
    // does.
    if ($config->get('search404_integration') && !$search404Installed) {
      $requirements['smart_404_search404'] = [
        'title' => t('Smart 404: Search 404 integration'),
        'value' => t('Module not found'),
        // Omissis
        'severity' => enum_exists(RequirementSeverity::class) ? RequirementSeverity::Warning : 1,
      ];
    }

    if (!$search404Installed && !$config->get('search404_integration')) {
      $requirements['smart_404_search404_suggest'] = [
        'title' => t('Smart 404: Search 404 integration'),
        'value' => t('Optional module not installed'),
        'description' => t('Install the <a href=":url">Search 404</a> module to show visitors helpful search results on 404 pages. Smart 404 can integrate with it automatically.', [
          ':url' => 'https://www.drupal.org/project/search404',
        ]),
        'severity' => enum_exists(RequirementSeverity::class) ? RequirementSeverity::Info : -1,
      ];
    }

    if (empty(Settings::get('trusted_host_patterns', []))) {
      $requirements['smart_404_trusted_host_patterns'] = [
        'title' => t('Smart 404: trusted host patterns'),
        'value' => t('Not configured'),
        // Omissis
        'severity' => enum_exists(RequirementSeverity::class) ? RequirementSeverity::Warning : 1,
      ];
    }

    return $requirements;
  }

What the documentation comment says is correct: Drupal 11.2.x invokes both hook_runtime_requirements() and hook_requirements(). In that case, the following code should return an empty array when $phase is 'runtime'.

#[LegacyRequirementsHook]
function smart_404_requirements(string $phase): array {
  if ($phase !== 'runtime') {
    return [];
  }

  return Smart404Hooks::buildRequirements(\Drupal::moduleHandler(), \Drupal::config('smart_404.settings'));
}

Still, that code does not work on Drupal 10.3.x, which does not invoke hook_runtime_requirements(). It does not even work with Drupal 11.1.x, with which the project is supposed to work.

src/Form/Smart404BulkRedirectForm.php

    if ($created > 0) {
      $this->messenger()->addStatus($this->formatPlural($created, '1 redirect created.', '@count redirects created.'));
    }
    if ($skipped > 0) {
      $this->messenger()->addWarning($this->formatPlural($skipped, '1 path skipped.', '@count paths skipped.'));
    }

@count needs to be used also for the singular string because there are languages where that is used also when English would use the plural string. For example, in Russian the translation for 1 log file and 101 log files are 1 файл журнала and 101 файл журнала, which literally are 1 log file and 101 log file.

Manual review of 1.0.x at 16282aa. I read the module in full rather than re-running the points already covered in comments 4 to 10, and concentrated on what has landed since 81f37bd on 26 July -- which is a lot: 63 files and 7944 added lines, most of it the test suite and the redirect-validation hardening. I first read the tree at 205f92b and re-checked everything below against 16282aa after the six commits of 28 and 29 August. Pipeline 940809 is green on this commit.

I did not find anything I would call a defect. Rather than say only that, here is what I actually tried to break, since a negative result is only worth something if you can see what was tested.

What I probed, and why it holds

• Open redirect through the destination field. RedirectDestinationValidator::isValidInternalPath() is the single choke point -- grep confirms Smart404RedirectCreator line 63 is its only non-test caller, and the form, the bulk form and the Drush command all go through the creator, so there is no second path in. The protocol-relative case, the leading backslash case, "://" anywhere rather than only at the start, raw control characters, and dot segments checked segment-wise on the parsed path rather than by substring, are each handled, and the docblock explains why Url::fromUserInput() alone is not enough for the "/\evil.com" variant. I could not construct a destination that survives all five checks and still leaves the site.
• Invalid UTF-8 reaching GlobMatcher. match() compiles with the "u" modifier and casts preg_match() to bool, so a subject that is not valid UTF-8 would make preg_match() return FALSE and be indistinguishable from "no match" -- an ignore pattern that silently stops matching. On PHP 8.3 I checked a raw Latin-1 byte and a bare 0xFF in a path: mb_strtolower() in PathNormalizer::normalize() substitutes them, so every subject that reaches the matcher is already valid UTF-8, and preg_match() returns 1 in each case. The docblock's claim that both call sites normalize first is what makes this safe, and it is accurate.
• Smart404IgnoreConfirmForm::buildForm() returning a RedirectResponse. This looked like it would break FormBuilder, which assigns $form['#form_id'] to the return value. It does not: FormBuilder::retrieveForm() checks for a Response and throws EnforcedResponseException before that line, so the pattern is one core explicitly supports.
• The privacy claims on the project page. Verified against smart_404.install rather than the README: the schema has no IP column, the only user-agent-derived column is the is_bot integer, and Smart404Logger::sanitizeReferer() keeps scheme plus host for an external referer and drops the query string even for an internal one. The claims match the table. The scheme allowlist added to sanitizeReferer() in 34ac45e is the same instinct applied to a value nothing currently renders -- refusing to store it rather than relying on that staying true.
• The permission split. "create smart_404 redirects" without the Redirect module's "administer redirects" is the part I expected to be the weak point, and the re-check of the source path against the routing table at creation time (getUrlIfValidWithoutAccessCheck(), not isValid(), so it is not skipped for a path the current user merely cannot see) closes the hijack of a path that has since become live. The permission description says out loud that this is a best-effort routing check and not a real request. Documenting the residual limit rather than implying none is the right call, and it is rarer than it should be.

Three small things

• FILE: src/Redirect/Smart404RedirectCreator.php. validate() runs its cheapest check last. By the time it reaches the in_array() against VALID_STATUS_CODES on line 225 it has already done two inbound path resolutions (each building a throwaway Request), a Redirect::generateHash(), a findMatchingRedirect() chain lookup, and a full routing-table probe. From the form the status code comes from a select and can only be 301 or 302, but drush smart404:redirect takes --status-code from the command line, so "drush smart404:redirect 12 /about-us --status-code=999" pays for all of that before being rejected. Moving the status-code check to the top of validate() costs nothing and changes no behaviour.
• FILE: src/Utility/GlobMatcher.php. match() defaults $caseInsensitive to FALSE, and the docblock then explains at length that FALSE would make any pattern containing an uppercase character silently match nothing, which is why both in-tree callers pass TRUE. The class is final and has no callers outside this module, so the default is a trap left for the next call site rather than a compatibility constraint. Defaulting to TRUE, or dropping the default so the parameter has to be passed, would remove it.
• FILE: src/SuggestionEngine/SuggestionEngine.php. The docblock extended in ce014aa answers the access-filtering question before a reviewer gets to ask it, and the first half is right: per-user filtering behind a shared cache would risk serving one user's filtered result to another, which is a worse bug than the alias exposure. The second half rules out the simpler published-only filter on cost -- resolving a path_alias row's internal path back to an entity generically means route-matching plus one entity load per row. That cost is real for the general case, but not for the shape that dominates the table: fetchCandidates() already selects pa.path, and a row whose path is "/node/N" can be filtered with a single join on node_field_data.status, with no entity load and no route matching. It would leave taxonomy terms, media and contrib entity types unfiltered, so it is a partial filter rather than the general one you turned down -- which may be exactly why you would still not want it. I raise it only because the cost figure is what carries the argument in that docblock, and for /node/N the cost is much lower than stated.

For what it is worth as an outside opinion: the density of "why this and not the obvious thing" comments is the most useful part of this codebase. Delegating to Redirect::generateHash() instead of re-implementing its normalization, and asking findMatchingRedirect() for the chained destination instead of analysing the new redirect statically, are both the right call for the same reason, and the comments say so.

Leaving the status at Needs review -- none of the three points above is a reason to move it to Needs work.

Hi,

I reviewed the 1.0.x branch of Smart 404.

git clone --branch 1.0.x https://git.drupalcode.org/project/smart_404.git

This is a manual read of the PHP, routing, permissions, and the admin JS, not a paste of PHPCS/PHPStan.

The project page and README match the code: aggregated 404 log, no raw IP, raw user-agent not stored (only an is_bot flag), external referers stored as domain only. Smart404Logger does that before Smart404Repository::upsert().

Permissions in smart_404.permissions.yml all have restrict access: true. That is justified. view smart 404 log can surface aliases that point at unpublished content (the permission description already says so). create smart 404 redirects can write Redirect entities without "Administer redirects", but RedirectDestinationValidator::isValidInternalPath() refuses //, backslash, ://, control characters, and . / .. segments, then runs the path through Url::fromUserInput(). I also checked the Ignore route: smart_404.ignore_single has _csrf_token: TRUE and requires both view and manage permissions.

SQL goes through the Database API (merge() / conditions). I did not find concatenated statements.

GlobMatcher turns admin glob patterns into a regex. That is only reachable with administer smart 404 settings, which is restricted. I am not asking for a change.

Branch 1.0.x, core_version_requirement: ^10.3 || ^11, enough PHP, no third-party library in the tree, Redirect is a proper composer/project dependency. README is detailed. Tests cover access, logging, and the redirect validator, including a javascript:// referer case.

I have no blocking findings. Thank you for the careful work on the open-redirect checks in particular.

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.