From e8048e023fa22971647ae38bfdf24b21b1853ffd Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 14:35:54 +0000 Subject: [PATCH] Return 400 for write canned-query SQL failures POST to a write canned query previously returned HTTP 200 with {"ok": false, "message": ...} when the SQL failed to execute, so JSON clients (and anything that trusts HTTP status) recorded success for failed writes. SQL failures now return 400 with the canonical error shape plus the "redirect" context key from on_error_redirect; the QueryWriteRejected 403 branch uses the canonical shape too. Successful executions and the HTML flash-message flow are unchanged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GrHZSypDfMnym1tM5XJAFZ --- datasette/views/database.py | 22 +++--- docs/sql_queries.rst | 18 ++++- existing-api.md | 14 ++-- stable-api-recommendations.md | 15 ++-- tests/test_error_shape.py | 131 ++++++++++++++++++++++++++++++++++ tests/test_queries.py | 4 +- 6 files changed, 179 insertions(+), 25 deletions(-) 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