-
Notifications
You must be signed in to change notification settings - Fork 150
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
Update Nginx configuration for Drupal, to bring it inline with best practice. Additional tests too. #2110
Conversation
…ractice. Additional tests too.
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.
minor query but LGTM and tests++ 🚀
## period. This includes directories used by version control systems such | ||
## as Subversion or Git to store control files. | ||
location ~ (^|/)\. { | ||
return 403; |
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.
should this be 404 to match the other directives above?
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.
Potentially, I was on the fence with this one.
@@ -90,34 +101,39 @@ server { | |||
deny all; | |||
access_log off; | |||
log_not_found off; | |||
return 404; |
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.
why do we need a specific 404 here? the deny all
already returns with an 403
? see http://nginx.master.drupal-example-9.test1.amazee.io/patches/asdf
## Disallow access to any dot files, but send the request to Drupal | ||
location ~* /\. { | ||
try_files /dev/null @drupal; |
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.
removing this will cause issues. Drupal allows URLs that start with a dot: http://nginx.master.drupal-example-9.test1.amazee.io/foobar/.dotfilesforethewin
your proposed regex: location ~ (^|/)\. {
would match this and block traffic to this.
Can we get the nginx-image parts of this split into a uselagoon/lagoon-images PR, and then the updated tests against the main branch? |
@seanhamlin do we still want to implement these changes to the drupal conf - if so we'll need to move this across to lagoon-images? |
Yes this PR is still relevant, but likely will require refactoring for the uselagoon repos. |
Checklist
Explain the details for making this change. What existing problem does the pull request solve?
Updates the Nginx configuration for Drupal based sites to bring it closer to https://www.nginx.com/resources/wiki/start/topics/recipes/drupal/
Closing issues
Closes #2109