Fix nginx redirect for no trailing slash

[sarayourfriend]

Fixes

Fixes #728 by @rbadillap

Description

There are three main parts to this PR: one direct change, one related change, and another change to support testing.

1. The changes to docker-compose.yml add a new nginx service that uses the nginx image we use in production created using api/Dockerfile. This is necessary for testing the changes in this PR and indeed, for easily verifying any changes to api/nginx.conf.template.
2. The change to location /static to add a trailing slash fixes a small issue and potential (though unlikely) security/misconfiguration issue. Adding a trailing slash /static/ prevents the rule from matching /static-somethingelsewedidnotintend. Likewise, removing the unused /media location also prevents potentially obscure security issues. Best to just not allow anything we don't intend. I learned about this while researching this issue and reading about how the alias and root directives work. A side effect of this change is that testing now needs to occur against /static/admin because /static won't match any configured locations and 404 instead of redirecting.
3. Turning off absolute_redirects directly fixes the problem described in the issue. The behaviour in the issue was caused by nginx attempting to redirect /static with a missing trailing slash to /static/. However, because port_in_redirect was not off, it would inject the :8080 port into the location on the redirect. Hence, api.openverse.engineering/static redirecting to api.openverse.engineering:8080/static/. Turning off port_in_redirect would fix the issue in production, but introduces a difficulty with testing locally, because it would then strip the port, causing local testing requests to 0.0.0.0:50270/static to redirect to 0.0.0.0/static/ without the original port. I couldn't find a good approach for this that didn't require heaps of additional configuration. Turning off absolute redirects does the trick perfectly in a way that is testable locally and will work exactly the same in production. Relative redirects used to not be supported but are fine these days, which is why the default of the absolute redirect directive preserves previous behaviour to use absolute redirects.

Testing Instructions

Checkout this branch and run just api/up. This will cause the nginx image to build and be accessible as a proxy to Django at localhost:50270. Test getting static files by going to /admin and logging in and poking around. Next, test the redirect: curl --location -I http://localhost:50270/static/admin. Confirm the redirected location is relative (does not include the port) and matches /static/admin/ with a trailing slash. /static/admin/ will still give you a 403 forbidden because directory navigation is not enabled in nginx (intended). You can confirm the original behaviour by commenting out the new absolute_redirect rule in api/nginx.conf.template and running just dc build nginx && just api/up to get the new configuration. Run the curl command above again and you should see a redirect to http://0.0.0.0:8080/static/admin/ instead of just the path. You can also go ahead and try to port_in_redirect off approach to see the issues with it stripping all ports, rather than just not replacing the port with Nginx's (i.e., it won't preserve the port used for local requests against the docker-compose stack).

Any time you make configuration changes to nginx, you must run just dc build nginx to get the new configuration: it does not auto-refresh the configuration. Run just api/up and Docker compose will replace the running service with the new image and preserve everything else. This is generally a pretty fast process.

Checklist

• My pull request has a descriptive title (not a vague title likeUpdate index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• [N/A] I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.
• [N/A] I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[dhruvkb]

I'm slightly confused as to why nginx was created as a separate service and not merged into the existing proxy service. Also is this NGINX conf https://github.com/WordPress/openverse/blob/23d3972aad42aeed1e72fb604fe7e2b243f2a4c6/docker/nginx/templates/web.conf.template redundant?

My main concern is that two NGINX services with very similar names can be confusing, so if this is the only approach, a docs page explaining the differences and the rationale for having 2 of these might be a good idea.

[sarayourfriend]

At first I was going to merge the two, but the existing proxy one is used for testing HTTPS setup, which we don't use in production. If we wanted to merge the two then the production configuration would need to become more complex to support a minor use-case that we rarely, if ever, actually use. I'd like to avoid that.

But now that you've mentioned it, one thing we could do is map the HTTPS config in the docker-compose, because Nginx supports concatenating multiple configuration files as long as they're in /etc/nginx/conf.d. It would just take extra work to get that web.conf.template you linked ready to work alongside the configuration we use in production. I would prefer doing that in a separate issue, however, or removing the proxy service entirely. I've suggested that we could switch to using caddy to test local HTTPS because it would be a bit easier overall. Switching to that would be a different issue, however.

In short: we could try to combine the two, but it could take a bit of effort to make the two configurations work together without making the production configuration unnecessarily more complicated. Alternatively, we could take a different approach to testing HTTPS locally altogether.

[dhruvkb]

Makes sense, thanks for the explanation @sarayourfriend. LGTM! Btw, we have #949 to switch to Caddy so we can deal with the removal/replacement of proxy there.

[obulat]

I tried running the testing instructions locally, and everything worked as described.

[stacimc]

🚀 Thanks for the great testing instructions -- worked seamlessly for me 👍