Writable canned queries with magic parameters fail if POST body is empty

[simonw]

When I try to use the new ?_json=1 feature from #880 with magic parameters from #842 I get this error:

Incorrect number of bindings supplied. The current statement uses 1, and there are 0 supplied

[simonw]

This is so weird. In the test I wrote for this the following passed:

response = magic_parameters_client.post("/data/runme_post?_json=1", {}, csrftoken_from=True)

But without the csrftoken_from=True parameter it failed with the bindings error:

response = magic_parameters_client.post("/data/runme_post?_json=1", {})

Here's the test I wrote:

def test_magic_parameters_json_body(magic_parameters_client):
    magic_parameters_client.ds._metadata["databases"]["data"]["queries"]["runme_post"][
        "sql"
    ] = "insert into logs (line) values (:_header_host)"
    response = magic_parameters_client.post("/data/runme_post?_json=1", {}, csrftoken_from=True)
    assert response.status == 200
    assert response.json["ok"], response.json
    post_actual = magic_parameters_client.get(
        "/data/logs.json?_sort_desc=rowid&_shape=array"
    ).json[0]["line"]

[simonw]

So the mystery here is why does omitting csrftoken_from=True break the MagicParameters mechanism?

[simonw]

Relevant code:

datasette/datasette/views/database.py

Lines 222 to 236 in 853c5fc

	# Should we return JSON?
	should_return_json = (
	request.headers.get("accept") == "application/json"
	or request.args.get("_json")
	or params.get("_json")
	)
	if canned_query:
	params_for_query = MagicParameters(params, request, self.ds)
	else:
	params_for_query = params
	ok = None
	try:
	cursor = await self.ds.databases[database].execute_write(
	sql, params_for_query, block=True
	)

This issue may not be about _json=1 interacting with magic parameters after all.

[simonw]

Is the bug here that magic parameters are incompatible with CSRF-exempt requests (e.g. request with no cookies)?

[simonw]

Hunch: I think the asgi-csrf middleware may be consuming the request body and failing to restore it.

[simonw]

New (failing) test:

@pytest.mark.parametrize("use_csrf", [True, False])
@pytest.mark.parametrize("return_json", [True, False])
def test_magic_parameters_csrf_json(magic_parameters_client, use_csrf, return_json):
    magic_parameters_client.ds._metadata["databases"]["data"]["queries"]["runme_post"][
        "sql"
    ] = "insert into logs (line) values (:_header_host)"
    qs = ""
    if return_json:
        qs = "?_json=1"
    response = magic_parameters_client.post(
        "/data/runme_post{}".format(qs),
        {},
        csrftoken_from=use_csrf or None,
        allow_redirects=False,
    )
    if return_json:
        assert response.status == 200
        assert response.json["ok"], response.json
    else:
        assert response.status == 302
        messages = magic_parameters_client.ds.unsign(
            response.cookies["ds_messages"], "messages"
        )
        assert [["Query executed, 1 row affected", 1]] == messages
    post_actual = magic_parameters_client.get(
        "/data/logs.json?_sort_desc=rowid&_shape=array"
    ).json[0]["line"]
    assert post_actual == "localhost"

It passes twice, fails twice - failures are for the ones where use_csrf is False.

[simonw]

While I'm running the above test, in the rounds that work the receive() awaitable returns {'type': 'http.request', 'body': b'csrftoken=IlpwUGlSMFVVa3Z3ZlVoamQi.uY2U1tF4i0M-5M6x34vnBCmJgr0'}

In the rounds that fails it returns {'type': 'http.request'}

So it looks like the csrftoken_from=True parameter may be helping just by ensuring the body key is present and not missing. I wonder if it would work if a body of b'' was present there?

[simonw]

Yes! The tests all pass if I update the test function to do this:

    response = magic_parameters_client.post(
        "/data/runme_post{}".format(qs),
        {"ignore_me": "1"},
        csrftoken_from=use_csrf or None,
        allow_redirects=False,
    )

So the bug only occurs if the POST body is completely empty.

[simonw]

So the problem actually occurs when the MagicParameters class wraps an empty dictionary.

Relevant code:

datasette/datasette/views/database.py

Lines 228 to 236 in 853c5fc

	if canned_query:
	params_for_query = MagicParameters(params, request, self.ds)
	else:
	params_for_query = params
	ok = None
	try:
	cursor = await self.ds.databases[database].execute_write(
	sql, params_for_query, block=True
	)

And:

datasette/datasette/views/database.py

Lines 364 to 383 in 853c5fc

	class MagicParameters(dict):
	def __init__(self, data, request, datasette):
	super().__init__(data)
	self._request = request
	self._magics = dict(
	itertools.chain.from_iterable(
	pm.hook.register_magic_parameters(datasette=datasette)
	)
	)
	def __getitem__(self, key):
	if key.startswith("_") and key.count("_") >= 2:
	prefix, suffix = key[1:].split("_", 1)
	if prefix in self._magics:
	try:
	return self._magics[prefix](suffix, self._request)
	except KeyError:
	return super().__getitem__(key)
	else:
	return super().__getitem__(key)

I'm passing a special magic parameters dictionary for the Python sqlite3 module to look up parameters in. When that dictionary is {} a __len__ check is performed on that dictionary, the result comes back as 0 and as a result it assumes there are no parameters.

I tracked down the relevant C code:

https://github.com/python/cpython/blob/81715808716198471fbca0a3db42ac408468dbc5/Modules/_sqlite/statement.c#L218-L237

    Py_BEGIN_ALLOW_THREADS
    num_params_needed = sqlite3_bind_parameter_count(self->st);
    Py_END_ALLOW_THREADS

    if (PyTuple_CheckExact(parameters) || PyList_CheckExact(parameters) || (!PyDict_Check(parameters) && PySequence_Check(parameters))) {
        /* parameters passed as sequence */
        if (PyTuple_CheckExact(parameters)) {
            num_params = PyTuple_GET_SIZE(parameters);
        } else if (PyList_CheckExact(parameters)) {
            num_params = PyList_GET_SIZE(parameters);
        } else {
            num_params = PySequence_Size(parameters);
        }
        if (num_params != num_params_needed) {
            PyErr_Format(pysqlite_ProgrammingError,
                         "Incorrect number of bindings supplied. The current "
                         "statement uses %d, and there are %zd supplied.",
                         num_params_needed, num_params);
            return;
        }

It looks to me like this should fail if the number of keys known to be in the dictionary differs from the number of named parameters in the query. But if those numbers fail to match it still works as far as I can tell - it's only dictionary length of 0 that is causing the problems.

[simonw]

I wish I could call https://www.sqlite.org/c3ref/bind_parameter_count.html and https://www.sqlite.org/c3ref/bind_parameter_name.html from Python.

Might be possible to do that using ctypes - see this example code: https://mail.python.org/pipermail//pypy-commit/2013-February/071372.html

            param_count = lib.sqlite3_bind_parameter_count(self.statement)
            for idx in range(1, param_count + 1):
                param_name = lib.sqlite3_bind_parameter_name(self.statement,
                                                             idx)

[simonw]

I think the easiest fix is for me to ensure that calls to __len__ on the MagicParameters class always return at least 1.