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

Update Nginx configuration for Drupal, to bring it inline with best practice. Additional tests too. #2110

Closed
wants to merge 2 commits into from

Conversation

seanhamlin
Copy link
Contributor

Checklist

  • Affected Issues have been mentioned in the Closing issues section
  • Documentation has been written/updated
  • PR title is ready for changelog and subsystem label(s) applied

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

@seanhamlin seanhamlin added the 6-images-testing Base Images & Testing subsystem label Jul 27, 2020
Copy link
Member

@smlx smlx left a 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;
Copy link
Member

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?

Copy link
Contributor Author

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;
Copy link
Contributor

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

Comment on lines -39 to -41
## Disallow access to any dot files, but send the request to Drupal
location ~* /\. {
try_files /dev/null @drupal;
Copy link
Contributor

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.

@tobybellwood
Copy link
Member

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?

@tobybellwood tobybellwood added lagoon-one and removed 6-images-testing Base Images & Testing subsystem labels May 24, 2021
@tobybellwood
Copy link
Member

@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?

@seanhamlin
Copy link
Contributor Author

Yes this PR is still relevant, but likely will require refactoring for the uselagoon repos.

tobybellwood added a commit to uselagoon/lagoon-images that referenced this pull request Mar 8, 2022
@tobybellwood tobybellwood deleted the nginx-conf-update-drupal branch March 17, 2022 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redirect /index.php/* to /* in Drupal Nginx images
4 participants