Bug: Canonical URLs with www. are not matched correctly for static files on redirects

paulbrzeski commented 2 years ago

[X] I have read the guidelines for Contributing to Roots Projects
[X] This request is not a duplicate of an existing issue
[X] I have read the Trellis docs and followed them (if applicable)
[X] I have seached the Roots Discourse for answers and followed them (if applicable)
[X] 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

swalkinshaw commented 2 years ago

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:

paulbrzeski commented 2 years ago

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.

swalkinshaw commented 2 years ago

Yep, here it is https://gist.github.com/swalkinshaw/f6ac3bbf8200564b52ff04dc6faac446

paulbrzeski commented 2 years ago

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 ( https://github.com/roots/trellis/pull/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.

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!

swalkinshaw commented 2 years ago

🤔 those were specifically added in https://github.com/roots/trellis/pull/879

@TangRufus its been forever, but do you remember why you wanted/needed these?

Also see this comment on how to work around them: https://github.com/roots/trellis/pull/879#issuecomment-327935139

tangrufus commented 2 years ago

https://github.com/roots/trellis/pull/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 domains
includes.redirects.d: Includes on redirect domains

swalkinshaw commented 2 years ago

Yeah.. maybe. But if @paulbrzeski can work around this with child templates then maybe that's fine for now too.

paulbrzeski commented 2 years ago

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

swalkinshaw commented 2 years ago

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

roots / trellis