diff --git a/datasette/views/database.py b/datasette/views/database.py index e02de657..cf7a6db3 100644 --- a/datasette/views/database.py +++ b/datasette/views/database.py @@ -16,6 +16,7 @@ from datasette.write_sql import QueryWriteRejected from datasette.utils import ( add_cors_headers, await_me_maybe, + error_body, call_with_supported_arguments, named_parameters as derive_named_parameters, format_bytes, @@ -607,11 +608,7 @@ class QueryView(View): "_json" ): return Response.json( - { - "ok": False, - "message": ex.message, - "redirect": None, - }, + dict(error_body([ex.message], 403), redirect=None), status=403, ) datasette.add_message(request, ex.message, datasette.ERROR) @@ -681,12 +678,17 @@ class QueryView(View): redirect_url = stored_query.on_error_redirect ok = False if should_return_json: + if ok: + return Response.json( + { + "ok": True, + "message": message, + "redirect": redirect_url, + } + ) return Response.json( - { - "ok": ok, - "message": message, - "redirect": redirect_url, - } + dict(error_body([message], 400), redirect=redirect_url), + status=400, ) else: datasette.add_message(request, message, message_type) diff --git a/docs/sql_queries.rst b/docs/sql_queries.rst index 371348fb..4c6e4426 100644 --- a/docs/sql_queries.rst +++ b/docs/sql_queries.rst @@ -657,7 +657,7 @@ There are three options for specifying that you would like the response to your - Include ``?_json=1`` in the URL that you POST to - Include ``"_json": 1`` in your JSON body, or ``&_json=1`` in your form encoded body -The JSON response will look like this: +A successful JSON response will look like this: .. code-block:: json @@ -667,7 +667,21 @@ The JSON response will look like this: "redirect": "/data/add_name" } -The ``"message"`` and ``"redirect"`` values here will take into account ``on_success_message``, ``on_success_message_sql``, ``on_success_redirect``, ``on_error_message`` and ``on_error_redirect``, if they have been set. +If the SQL fails to execute - for example a constraint violation - the response uses the :ref:`standard error format ` with a ``400`` status, plus the ``"redirect"`` key from the query configuration: + +.. code-block:: json + + { + "ok": false, + "error": "UNIQUE constraint failed: docs.id", + "errors": [ + "UNIQUE constraint failed: docs.id" + ], + "status": 400, + "redirect": null + } + +The ``"message"``, ``"error"`` and ``"redirect"`` values here take into account ``on_success_message``, ``on_success_message_sql``, ``on_success_redirect``, ``on_error_message`` and ``on_error_redirect``, if they have been set. .. _pagination: diff --git a/existing-api.md b/existing-api.md index 90a931d1..e0842804 100644 --- a/existing-api.md +++ b/existing-api.md @@ -1132,12 +1132,14 @@ queries. `_random_chars_`, `_cookie_`, `_header_` (underscores → hyphens). User-stored queries cannot contain magic parameters — they are a feature of config/trusted queries. -- **Response — 200 for both success and SQL failure** (only permission - rejection is 403): - `{"ok": true|false, "message": "...", "redirect": "..."|null}` — - `message` honors `on_success_message_sql` / `on_success_message` / - `on_error_message`, falling back to `"Query executed"` or - `"Query executed, N rows affected"`. +- **Response:** success → 200 + `{"ok": true, "message": "...", "redirect": "..."|null}` — `message` + honors `on_success_message_sql` / `on_success_message`, falling back to + `"Query executed"` or `"Query executed, N rows affected"`. SQL failure → + **400** canonical error (message honors `on_error_message`) plus a + `redirect` context key from `on_error_redirect`. Operation rejection + (`QueryWriteRejected`, e.g. VACUUM) → 403 canonical error plus + `redirect: null`. --- diff --git a/stable-api-recommendations.md b/stable-api-recommendations.md index d68bd32e..c878cb70 100644 --- a/stable-api-recommendations.md +++ b/stable-api-recommendations.md @@ -27,8 +27,8 @@ Findings are grouped by theme. Each carries a priority: > debug-endpoint shape is gone; `_shape=object` misuse now returns HTTP 400 > (part of §1b). Covered by `tests/test_error_shape.py` and documented in > the "Error responses" section of `docs/json_api.rst`. §1a (`Forbidden` → -> JSON) is now also implemented. Still open from this section's sub-items: -> the write canned-query 200 (§1b) and the §1c status outliers. +> JSON) and §1b (write canned-query 200) are now also implemented. Still +> open from this section's sub-items: the §1c status outliers. The API currently produces four distinct JSON error shapes depending on which internal layer generates the error: @@ -76,7 +76,12 @@ machine-readable answer. **Recommendation:** the default forbidden handler must return the canonical JSON error when the path ends in `.json` or the request prefers JSON, mirroring `handle_exception`. -### 1b. Errors that return HTTP 200 (P1) +### 1b. Errors that return HTTP 200 (P1) — ✅ IMPLEMENTED + +> **Status:** implemented. `_shape=object` misuse returns 400 (done with +> §1), and write canned-query SQL failures now return **400** with the +> canonical error shape (plus the `redirect` context key); the +> `QueryWriteRejected` 403 branch also uses the canonical shape. - `_shape=object` on a query or pk-less table → `{"ok": false, "error": "_shape=object is only available on tables"}` with **200** @@ -378,8 +383,8 @@ Two details make tiering urgent rather than optional: 1. ~~One canonical JSON error shape; retire the other three (§1).~~ ✅ Done. 2. ~~`Forbidden` → JSON 403 for JSON requests (§1a).~~ ✅ Done. -3. No `ok: false` with HTTP 200 (§1b: `_shape=object`, write canned-query - SQL errors). +3. ~~No `ok: false` with HTTP 200 (§1b: `_shape=object`, write canned-query + SQL errors).~~ ✅ Done. 4. ~~Wrap `/-/plugins`, `/-/databases`, `/-/actions` top-level arrays in objects (§2).~~ ✅ Done. 5. ~~Filter `/-/databases.json` by `view-database` or gate it behind diff --git a/tests/test_error_shape.py b/tests/test_error_shape.py index 7f12abc6..dd1fcfc9 100644 --- a/tests/test_error_shape.py +++ b/tests/test_error_shape.py @@ -232,3 +232,134 @@ async def test_forbidden_json_path_allowed_actor_still_works(ds_forbidden): response = await ds_forbidden.client.get("/data/docs.json", actor={"id": "root"}) assert response.status_code == 200 assert response.json()["ok"] is True + + +# Write canned queries: SQL failures must not return HTTP 200 + + +@pytest.fixture +def ds_write_query(tmp_path_factory): + db_directory = tmp_path_factory.mktemp("dbs") + db_path = str(db_directory / "data.db") + conn = sqlite3.connect(db_path) + conn.execute("vacuum") + conn.execute("create table docs (id integer primary key, title text)") + conn.close() + ds = Datasette( + [db_path], + config={ + "databases": { + "data": { + "queries": { + "add_doc": { + "sql": ( + "insert into docs (id, title)" " values (:id, :title)" + ), + "write": True, + }, + "add_doc_custom_error": { + "sql": ( + "insert into docs (id, title)" " values (:id, :title)" + ), + "write": True, + "on_error_message": "Custom error message", + "on_error_redirect": "/data", + }, + } + } + } + }, + ) + yield ds + ds.close() + + +@pytest.mark.asyncio +async def test_write_query_success_returns_200(ds_write_query): + response = await ds_write_query.client.post( + "/data/add_doc", + json={"id": 1, "title": "One"}, + headers={"Accept": "application/json"}, + ) + assert response.status_code == 200 + data = response.json() + assert data["ok"] is True + assert data["message"] == "Query executed, 1 row affected" + assert data["redirect"] is None + + +@pytest.mark.asyncio +async def test_write_query_sql_failure_returns_400(ds_write_query): + for _ in range(2): + response = await ds_write_query.client.post( + "/data/add_doc", + json={"id": 1, "title": "One"}, + headers={"Accept": "application/json"}, + ) + data = assert_canonical_error(response, 400) + assert "UNIQUE constraint failed" in data["error"] + # The redirect context key from the canned query flow is preserved + assert data["redirect"] is None + + +@pytest.mark.asyncio +async def test_write_query_failure_uses_on_error_message_and_redirect( + ds_write_query, +): + for _ in range(2): + response = await ds_write_query.client.post( + "/data/add_doc_custom_error", + json={"id": 1, "title": "One"}, + headers={"Accept": "application/json"}, + ) + data = assert_canonical_error(response, 400) + assert data["error"] == "Custom error message" + assert data["redirect"] == "/data" + + +@pytest.mark.asyncio +async def test_write_query_forbidden_is_canonical_403(ds_write_query): + # An untrusted write query run by an actor without execute-write-sql + # raises Forbidden, handled by the forbidden() hook + await ds_write_query.invoke_startup() + await ds_write_query.add_query( + "data", + name="untrusted_add", + sql="insert into docs (id, title) values (:id, :title)", + is_write=True, + is_trusted=False, + source="user", + owner_id="someone", + ) + response = await ds_write_query.client.post( + "/data/untrusted_add", + json={"id": 5, "title": "Five"}, + headers={"Accept": "application/json"}, + actor={"id": "someone"}, + ) + assert_canonical_error(response, 403) + + +@pytest.mark.asyncio +async def test_write_query_rejected_operation_is_canonical_403(ds_write_query): + # A rejected operation (VACUUM) raises QueryWriteRejected, handled by + # the dedicated branch in QueryView.post - root has execute-write-sql + ds_write_query.root_enabled = True + await ds_write_query.invoke_startup() + await ds_write_query.add_query( + "data", + name="vacuum_it", + sql="vacuum", + is_write=True, + is_trusted=False, + source="user", + owner_id="root", + ) + response = await ds_write_query.client.post( + "/data/vacuum_it", + json={}, + headers={"Accept": "application/json"}, + actor={"id": "root"}, + ) + data = assert_canonical_error(response, 403) + assert data["redirect"] is None diff --git a/tests/test_queries.py b/tests/test_queries.py index 6dfcc8b7..b79a9af4 100644 --- a/tests/test_queries.py +++ b/tests/test_queries.py @@ -3093,9 +3093,9 @@ async def test_untrusted_stored_write_query_rejects_virtual_table_control_insert ) assert denied_response.status_code == 403 - assert denied_response.json()["message"] == ( + assert denied_response.json()["errors"] == [ "Writes to virtual tables are not allowed in user-supplied SQL" - ) + ] assert ( await db.execute("select count(*) from docs where docs match 'hello'") ).first()[0] == 1