The TrustedHosts check only seems to analyze a sites main settings.php, but should also check any included/required settings.php. Some service providers automatically populate the trusted_host_patterns in environment-specific settings files based on domain names entered into their admin UI.

OR better yet, leverage or re-use code from the Drupal core check (see below) that checks for the trusted_hosts_pattern in the active environment, rather than inside settings files. This was previous suggested by @acrosman: https://www.drupal.org/project/security_review/issues/2855840#comment-12...

From https://cgit.drupalcode.org/drupal/tree/core/modules/system/system.insta...

  // See if trusted hostnames have been configured, and warn the user if they
  // are not set.
  if ($phase == 'runtime') {
    $trusted_host_patterns = Settings::get('trusted_host_patterns');
    if (empty($trusted_host_patterns)) {

Issue fork security_review-3008957

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

timwood created an issue. See original summary.

Ambient.Impact made their first commit to this issue’s fork.

Ambient.Impact opened merge request !1

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented
Related issues: +#3158859: Check for main host in another file

I've added a first draft of using \Drupal\Core\Site\Settings::get() to detect trusted_host_patterns regardless of where or how it's set. I have a few questions:

  1. Is there any security benefit to manually tokenizing the settings rather than using the core class?
  2. Since I've implemented using the core class as an alternative method of testing, it doesn't tokenize the settings file, so I'm just doing global $base_url; - is this okay or is there a better way to get this without tokenizing?
  3. Can using the core class be set as the default method, given that it solves a lot of edge cases and seems to be the preferred way to do this according to core?
ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Related issues: +#2581071: Remove checking base_url in TrustedHosts check

Wonder with the addition https://www.drupal.org/project/security_review/issues/2581071 if that affects this solution.

smustgrave’s picture

smustgrave commented
Version: 8.x-1.x-dev » 2.0.x-dev
smustgrave’s picture

smustgrave commented
Status: Needs work » Postponed

Postponing until https://www.drupal.org/project/security_review/issues/2581071 lands. But I think we can drop the settings for trusted all together and just use core.

So will need

  1. Drop the settings form
  2. Redo the existing check to use drupal core settings
  3. Delete schema and config settings
  4. Update hook to delete config for existing sites.

smustgrave credited prudloff.

smustgrave’s picture

smustgrave commented

Moved over credit from https://www.drupal.org/project/security_review/issues/3155650.

smustgrave opened merge request !28

smustgrave’s picture

smustgrave commented
Status: Postponed » Needs review

Was able to drop the settings form
Updated check to use Settings::get('trusted_host_patterns')
Deleted schema and config settings
Added update to hook to delete config value.

ednark’s picture

ednark commented
Status: Needs review » Reviewed & tested by the community

I have tested this and it functions as expected without errors.

Tested on 10.1.x-dev with a standard install
module drupal/security_review:2.0.x-dev@dev
branch 3008957-refactor-host

Test procedure:
Do not specify any trusted host patterns
run test to see "Trusted hosts are not set"

add trusted_host within main settings.php
run test to see "Trusted hosts are set"

remove trusted_host within main settings.php
add trusted host within secondary file included/required from settings.php
run test to see "Trusted hosts are set"

smustgrave’s picture

smustgrave commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

smustgrave closed merge request !1