Skip to content

fix nginx well-known redirects when behind another reverse proxy #6392

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 1 commit into
base: master
Choose a base branch
from

Conversation

icewind1991
Copy link
Member

if nginx is behind another reverse proxy that handles the ssl termination it currently returns the wrong (non-https) redirect for well-known urls.

This change causes the redirect urls to only contain the path

if nginx is behind another reverse proxy that handles the ssl termination it currently returns the wrong (non-https) redirect for well-known urls
@MichaIng
Copy link
Member

The same should be required then for location /remote, right?

As you say it contains "only the path", the query string is contained as well, i.e. all URL elements that are not explicitly part of the redirect directive are omitted? So return 301 /nextcloud/index.php$request_uri; will return /nextcloud/index.php$request_uri while by default Nginx adds (it's own) scheme and host so that e.g. http://<local_hostname>/nextcloud/index.php$request_uri could be returned when it's an instance behind a reverse proxy or load balancer in the same LAN, right?

That's actually very good to know as IMO not intuitive.

@icewind1991
Copy link
Member Author

icewind1991 commented Apr 15, 2021

I believe that the query string is not part of the redirect url (with both the current config and this PR), from how I understand the nginx config the absolute_redirect only effects the scheme, hostname and port

So return 301 /nextcloud/index.php$request_uri; will return /nextcloud/index.php$request_uri while by default Nginx adds (it's own) scheme and host so that e.g. http://<local_hostname>/nextcloud/index.php$request_uri could be returned when it's an instance behind a reverse proxy or load balancer in the same LAN, right?

Yes, by default nginx seems to transform the redirect into a full url when only the path is provided

@MichaIng
Copy link
Member

MichaIng commented Apr 15, 2021

I believe that the query string is not part of the redirect url (with both the current config and this PR)

That would be a problem. We just changed $uri to $request_uri to have the query strings preserved and at least the Apache2 Redirect and RewriteRule both do preserve the query string, so we would have failed to "clone" the .htaccess and would need to switch to Nginx's rewrite instead? I think it is important as we redirect all .well-known requests, even possible unknown ones, so cutting away the query string will definitely break some backends.

Apache2 Redirect btw appends the current servers scheme and hostname as well, so should suffer from the same issue with proxies: https://httpd.apache.org/docs/2.4/mod/mod_alias.html#redirect
The .htaccess uses RewriteRule with R=301 flag, where it is the same: https://httpd.apache.org/docs/2.4/rewrite/flags.html#flag_r
I'm wondering whether a reasonable proxy that terminates SSL would not as well adjust redirects sent back to the client?

Actually... a redirect MUST be an absolute URI according to the RFC:

So what we'd do with this change is creating an invalid redirect (Location header), even that all modern browsers seem to support it. That actually is a good reason for the above assumption, that a proxy should alter the redirect the same way it alters the client request before forwarding it, isn't it?

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.

2 participants