Errors when using table filters behind a proxy

[mattmalcher]

Using datasette==0.63 table filters do not respect the base_url setting as described here

To reproduce, go to:
https://datasette-apache-proxy-demo.datasette.io/prefix/fixtures/binary_data

Then use the table filter buttons.
The /prefix/ is dropped, resulting in URL not found:
https://datasette-apache-proxy-demo.datasette.io/fixtures/binary_data?_sort=rowid&rowid__exact=1

[simonw]

Great catch, thanks!

[simonw]

If you view source on that page the HTML looks correct:

<form class="filters" action="/prefix/fixtures/binary_data" method="get">

(I just added a test that confirms this too.)

But... it looks like the bug is in the redirection code.

https://datasette-apache-proxy-demo.datasette.io/prefix/fixtures/binary_data?_filter_column=rowid&_filter_op=exact&_filter_value=1&_sort=rowid returns the following:

location: /fixtures/binary_data?_sort=rowid&rowid__exact=1

Which is incorrect.

[simonw]

I tried adding this test but it passed! I expected it to fail:

def test_base_url_affects_filter_redirects(app_client_base_url_prefix):
    response = app_client_base_url_prefix.get(
        "/prefix/fixtures/binary_data?_filter_column=rowid&_filter_op=exact&_filter_value=1&_sort=rowid"
    )
    assert response.status == 302
    assert (
        response.headers["location"]
        == "/prefix/fixtures/binary_data?_sort=rowid&rowid__exact=1"
    )

[simonw]

Relevant code:

datasette/datasette/views/table.py

Lines 227 to 249 in bbaab3b

	# Handle ?_filter_column and redirect, if present
	redirect_params = filters_should_redirect(request.args)
	if redirect_params:
	return self.redirect(
	request,
	path_with_added_args(request, redirect_params),
	forward_querystring=False,
	)
	# If ?_sort_by_desc=on (from checkbox) redirect to _sort_desc=(_sort)
	if "_sort_by_desc" in request.args:
	return self.redirect(
	request,
	path_with_added_args(
	request,
	{
	"_sort_desc": request.args.get("_sort"),
	"_sort_by_desc": None,
	"_sort": None,
	},
	),
	forward_querystring=False,
	)

[simonw]

path_with_added_args(request, redirect_params) should be preserving the current path from the request.

datasette/datasette/utils/__init__.py

Lines 273 to 286 in bbaab3b

	def path_with_added_args(request, args, path=None):
	path = path or request.path
	if isinstance(args, dict):
	args = args.items()
	args_to_remove = {k for k, v in args if v is None}
	current = []
	for key, value in urllib.parse.parse_qsl(request.query_string):
	if key not in args_to_remove:
	current.append((key, value))
	current.extend([(key, value) for key, value in args if value is not None])
	query_string = urllib.parse.urlencode(current)
	if query_string:
	query_string = f"?{query_string}"
	return path + query_string

[simonw]

Is there a chance that it's Apache that's messing with that location: header here, not Datasette?

datasette/demos/apache-proxy/000-default.conf

Lines 7 to 13 in bbaab3b

	<VirtualHost *:80>
	ServerName localhost
	DocumentRoot /app/html
	ProxyPreserveHost On
	ProxyPass /prefix/ http://127.0.0.1:8001/
	Header add X-Proxied-By "Apache2 Debian"
	</VirtualHost>

[simonw]

https://httpd.apache.org/docs/2.4/mod/mod_proxy.html#proxypass includes this note:

Normally, mod_proxy will canonicalise ProxyPassed URLs. But this may be incompatible with some backends, particularly those that make use of PATH_INFO. The optional nocanon keyword suppresses this and passes the URL path "raw" to the backend.

[simonw]

I modified that config file to have this line instead:

    ProxyPass /prefix/ http://127.0.0.1:8001/ nocanon

And then deployed it by running:

flyctl deploy --build-arg DATASETTE_REF=main

This does NOT seem to have fixed the bug:

~ % curl -i 'https://datasette-apache-proxy-demo.datasette.io/prefix/fixtures/binary_data?_filter_column=rowid&_filter_op=exact&_filter_value=1&_sort=rowid'
HTTP/2 302 
date: Fri, 11 Nov 2022 06:40:01 GMT
server: Fly/b1863e2e7 (2022-11-09)
location: /fixtures/binary_data?_sort=rowid&rowid__exact=1
link: </fixtures/binary_data?_sort=rowid&rowid__exact=1>; rel=preload
content-type: text/plain
x-proxied-by: Apache2 Debian
via: 2 fly.io
fly-request-id: 01GHJPNCF51CJ626EWZEHK2CH9-sjc

https://datasette-apache-proxy-demo.datasette.io/prefix/-/versions seems to confirm that this is the latest deployed version (0.63), so it looks like the deploy worked.

[simonw]

https://datasette-apache-proxy-demo.datasette.io/prefix/-/asgi-scope is useful:

It confirms that /prefix/ is nowhere to be seen in the incoming request data:

 'path': '/-/asgi-scope',
 'query_string': b'',
 'raw_path': b'/-/asgi-scope',

[simonw]

Modifying that test to the following does indeed cause a failure:

def test_base_url_affects_filter_redirects(app_client_base_url_prefix):
    response = app_client_base_url_prefix.get(
        "/fixtures/binary_data?_filter_column=rowid&_filter_op=exact&_filter_value=1&_sort=rowid"
    )
    assert response.status == 302
    assert (
        response.headers["location"]
        == "/prefix/fixtures/binary_data?_sort=rowid&rowid__exact=1"
    )

[simonw]

This time deployed with:

cd demos/apache-proxy
fly deploy --build-arg DATASETTE_REF=8d9a957c6329d26cc1e417b5d6911640d74765eb

To ensure the exact commit with the fix.

And that fixed it!

% curl -i 'https://datasette-apache-proxy-demo.datasette.io/prefix/fixtures/binary_data?_filter_column=rowid&_filter_op=exact&_filter_value=1&_sort=rowid'
HTTP/2 302 
date: Fri, 11 Nov 2022 06:54:45 GMT
server: Fly/b1863e2e7 (2022-11-09)
location: /prefix/fixtures/binary_data?_sort=rowid&rowid__exact=1
link: </prefix/fixtures/binary_data?_sort=rowid&rowid__exact=1>; rel=preload
content-type: text/plain
x-proxied-by: Apache2 Debian
via: 2 fly.io
fly-request-id: 01GHJQGBSXBR7E53TY0EKMQ9PA-sjc

[simonw]

I released that fix in Datasette 0.63.1: https://docs.datasette.io/en/stable/changelog.html#v0-63-1

[mattmalcher]

Amazing - thank you for fixing and releasing that so quickly and for showing your process! <3