Bug: Canonical URLs with www. are not matched correctly for static files on redirects
paulbrzeski opened this issue · 9 comments
- I have read the guidelines for Contributing to Roots Projects
- This request is not a duplicate of an existing issue
- I have read the Trellis docs and followed them (if applicable)
- I have seached the Roots Discourse for answers and followed them (if applicable)
- This is not a personal support request that should be posted on the Roots Discourse community
Description
What's wrong?
I have a couple of websites deployed by Trellis that seem to have problems in the generated Nginx configs, when going to the non www URL of a site and trying to access a static file I get a 404.
All my sites canonical URLs are prefixed with www and I suspect this is the underlying issue because examples I've seen all have the non www domain as canonical and www in redirects.
What have you tried?
- Checked logs, the 404 for the redirects is going to /var/log/nginx/error.log for the default file server instead of the actual site. This is a bit surprising given we have a default busting config in the generated config file.
- Audited configs on the server to make sure there wasn't some rogue override etc
- Replicated a non Trellis nginx config manually for a workaround with the www/non www issues, this worked but is not a long term solution given we use Trellis for all WP deployments
- Checked existing issues in Trellis including closed ones that seemed related, but the configs have moved on since those changes - #806 and #808
- Compared my configs to Trellis docs, noticed that every example on docs, on Roots Discourse and even StackOverflow all use the root domain in canonical (not the www domain)
What insights have you gained?
I'm guessing I have a non standard use that may not be supported but everything I've read indicates it should be supported hence lodging this ticket. If I'm mistake or have an error in my configs it'd be really handy to know :)
Possible solutions
Unsure, open to suggestions inc that I've missed something :)
Temporary workarounds
Overriding the generated Nginx config manually to at least fix www and non www URLs, but this is not scalable and risks being squished when someone else deploys via Trellis.
Steps To Reproduce
- Set the canonical URL of a site to use the www prefix and the root domain in redirects, e.g. "canonical: www.google.com \n redirects: - google.com"
- Using two tabs to check the WWW and non WWW URLs, navigate to a file in your uploads folder i.e. app/uploads/2021/12/iStock-12345-scaled.jpg
- If you get a 404 for one but a file for the other, you have successfully replicated the issue
- If you have other domain redirects, another thing that can be confirmed is that your 404 is appearing in /var/log/nginx/error.log. AFAIK this doesn't work for the www/non www ones though, just different domains
Expected Behavior
URLs for static assets to be redirected the same way they are for WP pages.
Actual Behavior
All the redirects result in a 404 and the ones with a different domain name appear to be handled by the built-in nginx config even though it's supposed to be disabled by no-default.conf.j2
Relevant Log Output
2021/12/09 05:22:39 [error] 384100#384100: *700982 open() "/etc/nginx/html/app/uploads/2021/09/123456.jpg" failed (2: No such file or directory), client: 15.16.93.18, server: domain.com, request: "GET /app/uploads/2021/09/123456.jpg HTTP/2.0", host: "domain.com.au"Trellis Version
1.9.0
Ansible Version
ansible 2.8.0
config file = /etc/ansible/ansible.cfg
configured module search path = ['/home/devops/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
ansible python module location = /usr/local/lib/python3.8/dist-packages/ansible
executable location = /usr/local/bin/ansible
python version = 3.8.10 (default, Sep 28 2021, 16:10:42) [GCC 9.3.0]Trellis CLI Version
0.9.2
I can't reproduce this unfortunately.
Config:
diff --git group_vars/development/wordpress_sites.yml group_vars/development/wordpress_sites.yml
index 90009265e2..311f3d4ea2 100644
--- group_vars/development/wordpress_sites.yml
+++ group_vars/development/wordpress_sites.yml
@@ -5,9 +5,9 @@
wordpress_sites:
example.com:
site_hosts:
- - canonical: example.test
+ - canonical: www.example.test
redirects:
- - www.example.test
+ - example.test
local_path: ../site # path targeting local Bedrock site directory (relative to Ansible root)
admin_email: admin@example.test
multisite:Static file (an upload):
Home page:
Thanks for having a look @swalkinshaw
Any chance you could share what was generated in the site config on your end?
If it's a match maybe there's another issue like my version of Nginx.
Hey @swalkinshaw, thank you for that! We've made some progress with the issue on our end.
We have a static file handler from this discussiuon ( #966) in the includes.d/all folder, which is breaking the non www redirects, I'm guessing because it's falling back to try_files instead of redirecting, somehow.
I've just noticed that the configs generated on our end, and I think in the example you provided, contain multiple instances of that includes_d block. This might work OK for some sites but in this instance we only want the includes_d block in the main server:443 block, not in any of the redirect ones.
Furthermore, we are trying to implement a site specific config using includes.d/%sitename%/rewrites.conf.j2 to redirect static files to some custom PHP code in Wordpress to ensure a user is logged in, has permissions to see that file, etc.
We are currently discussing removing these lines, I think it would make sense to remove them for others too, but I can't say for sure.
- https://github.com/roots/trellis/blob/master/roles/wordpress-setup/templates/wordpress-site.conf.j2#L262
- https://github.com/roots/trellis/blob/master/roles/wordpress-setup/templates/wordpress-site.conf.j2#L290
Not sure if it would be a common use case to want includes to apply to redirects. Where I work we probably only want them to apply to the main server {} block.
I'm not sure if this is a known problem. It might be worth offering another template or considering removing these extra lines if possible, it could affect others who are creating custom stuff.
Happy to submit a PR if that helps.
Cheers!
🤔 those were specifically added in #879
@tangrufus its been forever, but do you remember why you wanted/needed these?
Also see this comment on how to work around them: #879 (comment)
#879 was created to allow 3rd parties to inject Nginx config to redirect domains.
My use case: Allow https://github.com/TypistTech/trellis-cloudflare-origin-ca to work on redirect domains.
Perhaps we should add more includes.d-like folders?
Example:
includes.d: Includes on all domains (current behavior)includes.canonical.d: Includes on canonical domainsincludes.redirects.d: Includes on redirect domains
Yeah.. maybe. But if @paulbrzeski can work around this with child templates then maybe that's fine for now too.
The suggestion from @tangrufus improves the file structure by clarifying that there are different types of includes, rather than 1 for all.
I would hazard to guess that many WP developers aren't keen, nor confident, to debug Nginx configs via CLI so providing them a bit of direction in this way would be helpful.
My 2c anyway :)
p.s. We are using a fork of Trellis so we're unblocked, we're watching this thread to see what the plan is for the long term
That's fair, but every change/feature has a cost and I'm hesitant to add all that for something that's been reported once over ~5 years (that we know of).
But I'm glad you got it all fixed and thanks for the investigation