Inconsistent URL decoding with X-Accel-Redirect depending on whether the original request URL was url-encoded

[ThiefMaster]

I'm using nginx and x-accel-redirect in what's probably a slightly creative way to proxy file downloads from S3 in order to not show the very ugly and temporary S3 URL to my users. This works fine, but I noticed that in a few very rare cases the requests fail (usually due to an invalid S3 signature).

In any case, I managed to reproduce it without S3 and it seem to happen because depending on whether the URL the user accesses contains url-encoded charaters or not, the URL encoding behavior in the internal request changes.

This is a problem of course, because it means I cannot reliable know whether I need to URL-encode the path included in my x-accel-redirect header or not unless I use a horrible workaround of checking whether the current request url contains a % character.

I believe this behavior is so strange that it can only be a bug, but if there are any workarounds to get consistent behavior (so I can either always url-encode the path in my x-accel-redirect header or never do so) please let me know.

I'm attaching a minimal test case to reproduce this. The nginx version used there was the one that's stable on my Gentoo system, but I'm also including a Dockerfile which can be used to test against the nginx:latest image on Docker hub in this Git repo: ​https://github.com/ThiefMaster/nginx-bug-repro

If you want to run the test case locally:
Put the two files in some folder, create a Python virtualenv, activate it and install flask in there (pip install flask).

Then run the flask app with flask run -p 12345 -h 0.0.0.0 and nginx with nginx -c $(realpath nginx.conf) -e /dev/stdout.

nginx.conf:

daemon off;
worker_processes 2;

pid /tmp/nginx/nginx.pid;
error_log /dev/stdout info;


events {
        worker_connections 1024;
        use epoll;
}

http {
	client_body_temp_path /tmp/nginx/client;
	proxy_temp_path /tmp/nginx/proxy;
	fastcgi_temp_path /tmp/nginx/fcgi;
	uwsgi_temp_path /tmp/nginx/uwsgi;

        log_format short '$remote_addr [$time_local] "$request" $status $bytes_sent';

	server {
                listen 8000 default_server;
                listen [::]:8000 default_server;
                access_log /dev/stdout short;

		location ~ ^/_internal/(https?)/([^/]+)/(.+)$ {
			internal;
			set $download_protocol $1;
			set $download_host $2;
			set $download_path $3;
			set $download_url $download_protocol://$download_host/$download_path;
			resolver 127.0.0.1;
			proxy_pass $download_url$is_args$args;
		}

		location / {
			proxy_pass http://127.0.0.1:12345;
		}
		root /var/empty;
        }
}

app.py:

from urllib.parse import quote
from flask import Flask, url_for, request


app = Flask(__name__)


@app.route('/')
def index():
    return f'''
        WITHOUT explicitly encoding x-accel-redirect url:<br>
        <a href="{url_for('test_without')}">Test [main url without encoding, target without encoding]</a><br>
        <a href="{url_for('test_without', enc_target=1)}">Test [main url without encoding, target with encoding]</a> 💣<br>
        <br>
        <a href="{url_for('test_with')}">Test [main url with encoding, target without encoding]</a><br>
        <a href="{url_for('test_with', enc_target=1)}">Test [main url with encoding, target with encoding]</a><br>
        <br><br>
        WITH explicitly encoding x-accel-redirect url:<br>
        <a href="{url_for('test_without', enc_xar=1)}">Test [main url without encoding, target without encoding]</a><br>
        <a href="{url_for('test_without', enc_xar=1, enc_target=1)}">Test [main url without encoding, target with encoding]</a><br>
        <br>
        <a href="{url_for('test_with', enc_xar=1)}">Test [main url with encoding, target without encoding]</a><br>
        <a href="{url_for('test_with', enc_xar=1, enc_target=1)}">Test [main url with encoding, target with encoding]</a> ⚠
    '''


def _test():
    endpoint = 'target_with' if request.args.get('enc_target') == '1' else 'target_without'
    path = url_for(endpoint)
    internal_url = f'/_internal/http/{request.host}{path}'
    print(f'Redirecting internally to {internal_url}')
    response = app.make_response('')
    if request.args.get('enc_xar') == '1':
        internal_url = quote(internal_url)
    response.headers['X-Accel-Redirect'] = internal_url
    return response


@app.route('/test-without/')
def test_without():
    return _test()


@app.route('/test with/')
def test_with():
    return _test()


def _target():
    return f"target called url={request.base_url}"


@app.route('/target-without')
def target_without():
    return _target()


@app.route('/target with')
def target_with():
    return _target()


@app.errorhandler(404)
def notfound(exc):
    return f'NOT FOUND: {request.base_url}'

[Maxim Dounin]

This looks like a duplicate of #348: excessive URI encoding is done by set ... $1; if there were escaped characters in the original request URI. As a workaround, consider using named captures to avoid this extra escaping.