Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH Add a warning if allowed hosts is not set. #11612

Merged

Conversation

GuySartorelli
Copy link
Member

Also adds ability to "wildcard" allow all hosts, which disables the logging, and some much needed test coverage.

Issue

* In some cases, this could be used to halt execution if configuration critical to operation
* has not been set.
*/
protected function validateConfiguration(): void
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to do this as a protected method because the fact that bootConfigs() is called from BaseKernel is an implementation detail of that kernel - other kernels could be implemented to intentionally avoid calling that method, so this allows them a way to still perform this validation.

This also provides a clear place for this sort of validation to be done for other config in the future, if we find a need to do so.

@GuySartorelli GuySartorelli force-pushed the pulls/5/warn-allowed-hosts branch 3 times, most recently from c1f8fe7 to 2cd160b Compare February 13, 2025 01:40
Adds ability to "wildcard" allow all hosts, which disables the logging.
Adds much needed test coverage.
@GuySartorelli GuySartorelli force-pushed the pulls/5/warn-allowed-hosts branch from 2cd160b to 9b0307a Compare February 13, 2025 20:23
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually noticed that the non-env method of setting this doesn't appear to work correctly - with the following config I get warnings logged and I'm able to access my site which doesn't have an example.com style domain

I've copied the config from the docs for the logging, and from the docs PR for the allowed hosts

SilverStripe\Core\Injector\Injector:
  Psr\Log\LoggerInterface:
    calls:
      LogFileHandler: [ pushHandler, [ '%$LogFileHandler' ] ]
  LogFileHandler:
    class: Monolog\Handler\StreamHandler
    constructor:
      - "/var/www/silverstripe.log"
      - "info"
  SilverStripe\Control\Middleware\AllowedHostsMiddleware:
    properties:
      AllowedHosts:
        - 'example.com'
        - 'www.example.com'
        - 'subdomain.example.com'

@GuySartorelli
Copy link
Member Author

Looks like you're missing the after directive

---
after: requestprocessors
---
SilverStripe\Core\Injector\Injector:
  SilverStripe\Control\Middleware\AllowedHostsMiddleware:
    properties:
      AllowedHosts:
        - 'example.com'
        - 'www.example.com'
        - 'subdomain.example.com'

@emteknetnz
Copy link
Member

Yup that fixed it

@emteknetnz emteknetnz merged commit 5fa5a0c into silverstripe:5 Feb 16, 2025
17 checks passed
@emteknetnz emteknetnz deleted the pulls/5/warn-allowed-hosts branch February 16, 2025 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants