Skip to content

nginx: obscure internal rewrite URLs #991

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

Open
wants to merge 31 commits into
base: next
Choose a base branch
from

Conversation

alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Apr 28, 2025

TODO

  • consider if this change is helpful
  • if not, modify tests to test existing behaviour

Noted while working on #984:

Current setting proxy_redirect off prevents nginx from converting URLs like http://enketo:8005/v1 to external URLs.

This is not a feature that's required. Reverting to the default proxy_redirect config will:

  • simplify config
  • prevent some future leaking of internal docker URLs

See: https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_redirect

What has been done to verify that this works as intended?

  • added new passing tests: build
  • revert the proxy_redirect directive changes and checked the new tests now fail: build
  • reinstate the changes which fix the tests

Why is this the best possible solution? Were any other approaches considered?

This rule's meaning is subtle.

The change introduces inconsistency, in that redirects are only rewritten if they match the location block in which the proxy_pass is defined.

However, it doesn't seem like preserving internal URLs would ever by desirable, so reducing the occasions they can occur is probably helpful.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

No effect.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@alxndrsn alxndrsn changed the title nginx: add test for proxy_redirect config nginx: set proxy_redirect to default Apr 28, 2025
@alxndrsn alxndrsn changed the title nginx: set proxy_redirect to default nginx: obscure internal rewrite URLs Apr 28, 2025
@alxndrsn alxndrsn marked this pull request as ready for review April 28, 2025 10:13
@matthew-white
Copy link
Member

Just FYI, we're currently thinking that we should wait for .2 to try to merge this PR. We're hoping to start regression testing early next week. Let me know if that doesn't sound right!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants