Seems to me that I can legit set the trusted_host_patterns variable in settings.local.php and that should be processed by this module and not flag an error? If not, no worries, but seems like an unexpected result.

Seen at: /admin/reports/security-review/help/security_review/trusted_hosts
Neither $base_url nor trusted_host_patterns is set.

Comments

NikLP created an issue. See original summary.

niklp’s picture

niklp commented

I concur this is a bug!

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented

Since core tests for this, could that test be leveraged instead of one specific to this module? Or does that go against an intentional choice?

niklp’s picture

niklp commented

I think given that settings.local.php is, let's say, "recognised at a core level" that core should probably check for the existence of that file and process that the same way that it does settings.php.

Given the level of abstraction of settings/config etc it seems a bit much to ask us to put Real Things in settings.php any more.

Feels like this might need to be a core issue? Or maybe it already is one...?!

banviktor’s picture

banviktor commented
Status: Active » Closed (works as designed)

Yes it can be set in settings.local.php (though it needs to be set at least once in settings.php to be recognized, which is weird but not the point), but why do that without setting it in settings.php in the first place?
The file settings.local.php is designed to hold settings overrides for development/staging/testing environments, while this security feature is most useful in production. This works as intended both in core and security_review.

ronald van belzen’s picture

ronald van belzen commented

Yet, the status report is perfectly capable of detecting that trusted host patterns are set in this situation. Why not enhance this module? Display the correct result and advice to set the trusted host patterns in the settings.php.

It can be argued that this is not a bug, but I would suggest to at least re-open this issue as a feature request.

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented

#5 suggests an intentional choice to not allow the module to use the same detection pattern core uses, but doesn't explain why using a different pattern is a good idea. Just because there is a design assumption that people set production values in settings.php and override them in settings.local.php doesn't mean that's more secure. I know there are site owners that prefer to keep anything that chances in the environment out of git, including trusted host settings, because it forces you to make sure each instance of the site is configured correctly. I'm not sure they are right, but I don't think they are wrong either.

It seems like using the same test core uses resolves the frustrations here, and reducing the chance that people are ignoring a warning. Arguably anytime people ignore security warnings because they are false positives that makes them more likely to ignore warnings that are legitimate.

banviktor’s picture

banviktor commented
Category: Bug report » Feature request
Status: Closed (works as designed) » Active

Reopening as feature request

smustgrave’s picture

smustgrave commented
Status: Active » Closed (duplicate)
Related issues: +#3008957: Refactor trusted hosts check

Closing as duplicate of https://www.drupal.org/project/security_review/issues/3008957

If anyone disagrees please reopen explaining how they are different.