-
Notifications
You must be signed in to change notification settings - Fork 824
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
ENH Add a warning if allowed hosts is not set. #11612
Conversation
* In some cases, this could be used to halt execution if configuration critical to operation | ||
* has not been set. | ||
*/ | ||
protected function validateConfiguration(): void |
There was a problem hiding this comment.
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.
c1f8fe7
to
2cd160b
Compare
Adds ability to "wildcard" allow all hosts, which disables the logging. Adds much needed test coverage.
2cd160b
to
9b0307a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well
There was a problem hiding this 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'
Looks like you're missing the ---
after: requestprocessors
---
SilverStripe\Core\Injector\Injector:
SilverStripe\Control\Middleware\AllowedHostsMiddleware:
properties:
AllowedHosts:
- 'example.com'
- 'www.example.com'
- 'subdomain.example.com' |
Yup that fixed it |
Also adds ability to "wildcard" allow all hosts, which disables the logging, and some much needed test coverage.
Issue
SS_ALLOWED_HOSTS
has not been set #11610