mirror of
https://github.com/simonw/datasette.git
synced 2026-07-10 01:24:45 +02:00
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GrHZSypDfMnym1tM5XJAFZ
This commit is contained in:
Claude
parent
ae10a99811
commit
e8048e023f
6 changed files with 179 additions and 25 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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 <json_api_errors>` 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:
|
||||
|
||||
|
|
|
|||
|
|
@ -1132,12 +1132,14 @@ queries.
|
|||
`_random_chars_<N>`, `_cookie_<name>`, `_header_<name>` (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`.
|
||||
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue