Disallow edits of dangerous decsription_html/on_success_message_sql

Refs https://github.com/simonw/datasette/pull/2741#issuecomment-4549891578

datasette/views/query_helpers.py

@@ -18,14 +18,12 @@ _query_fields = {
     "sql",
     "title",
     "description",
-    "description_html",
     "hide_sql",
     "fragment",
     "parameters",
     "params",
     "is_private",
     "on_success_message",
-    "on_success_message_sql",
     "on_success_redirect",
     "on_error_message",
     "on_error_redirect",
@@ -35,7 +33,6 @@ _query_create_fields = _query_fields | {"name", "mode", "csrftoken"}
 _query_update_fields = _query_fields
 _query_write_fields = {
     "on_success_message",
-    "on_success_message_sql",
     "on_success_redirect",
     "on_error_message",
     "on_error_redirect",
@@ -441,7 +438,9 @@ def _apply_query_data_types(data):
 async def _prepare_query_create(datasette, request, db, data):
     invalid_keys = set(data) - _query_create_fields
     if invalid_keys:
-        raise QueryValidationError("Invalid keys: {}".format(", ".join(invalid_keys)))
+        raise QueryValidationError(
+            "Invalid keys: {}".format(", ".join(sorted(invalid_keys)))
+        )
 
     data = _apply_query_data_types(data)
     name = data.get("name")
@@ -467,7 +466,6 @@ async def _prepare_query_create(datasette, request, db, data):
         "sql": data["sql"],
         "title": data.get("title"),
         "description": data.get("description"),
-        "description_html": data.get("description_html"),
         "hide_sql": _as_bool(data.get("hide_sql")),
         "fragment": data.get("fragment"),
         "parameters": parameters,
@@ -477,7 +475,6 @@ async def _prepare_query_create(datasette, request, db, data):
         "source": "user",
         "owner_id": _actor_id(request.actor),
         "on_success_message": data.get("on_success_message"),
-        "on_success_message_sql": data.get("on_success_message_sql"),
         "on_success_redirect": data.get("on_success_redirect"),
         "on_error_message": data.get("on_error_message"),
         "on_error_redirect": data.get("on_error_redirect"),
@@ -488,7 +485,9 @@ async def _prepare_query_create(datasette, request, db, data):
 async def _prepare_query_update(datasette, request, db, existing, update):
     invalid_keys = set(update) - _query_update_fields
     if invalid_keys:
-        raise QueryValidationError("Invalid keys: {}".format(", ".join(invalid_keys)))
+        raise QueryValidationError(
+            "Invalid keys: {}".format(", ".join(sorted(invalid_keys)))
+        )
 
     update = _apply_query_data_types(update)
     sql = update.get("sql", existing["sql"])
@@ -519,14 +518,12 @@ async def _prepare_query_update(datasette, request, db, existing, update):
         "sql": sql,
         "title": update.get("title"),
         "description": update.get("description"),
-        "description_html": update.get("description_html"),
         "hide_sql": update.get("hide_sql"),
         "fragment": update.get("fragment"),
         "parameters": parameters,
         "is_write": query_is_write,
         "is_private": update.get("is_private"),
         "on_success_message": update.get("on_success_message"),
-        "on_success_message_sql": update.get("on_success_message_sql"),
         "on_success_redirect": update.get("on_success_redirect"),
         "on_error_message": update.get("on_error_message"),
         "on_error_redirect": update.get("on_error_redirect"),

tests/test_queries.py

@@ -186,7 +186,9 @@ async def test_config_queries_imported_to_internal_table():
                         "configured": {
                             "sql": "select :name as name",
                             "title": "Configured query",
+                            "description_html": "<p>Configured HTML</p>",
                             "params": ["name"],
+                            "on_success_message_sql": "select 'Hello ' || :name",
                         }
                     }
                 }
@@ -202,7 +204,7 @@ async def test_config_queries_imported_to_internal_table():
         "sql": "select :name as name",
         "title": "Configured query",
         "description": None,
-        "description_html": None,
+        "description_html": "<p>Configured HTML</p>",
         "hide_sql": False,
         "fragment": None,
         "params": ["name"],
@@ -213,7 +215,7 @@ async def test_config_queries_imported_to_internal_table():
         "source": "config",
         "owner_id": None,
         "on_success_message": None,
-        "on_success_message_sql": None,
+        "on_success_message_sql": "select 'Hello ' || :name",
         "on_success_redirect": None,
         "on_error_message": None,
         "on_error_redirect": None,
@@ -887,6 +889,45 @@ async def test_query_store_api_rejects_is_trusted():
     assert response.json()["errors"] == ["Invalid keys: is_trusted"]
 
 
+@pytest.mark.asyncio
+async def test_query_store_rejects_config_only_fields():
+    ds = Datasette(memory=True, default_deny=True)
+    ds.root_enabled = True
+    ds.add_memory_database("query_config_only_fields_api", name="data")
+    await ds.invoke_startup()
+
+    response = await ds.client.post(
+        "/data/-/queries/store",
+        actor={"id": "root"},
+        json={
+            "query": {
+                "name": "unsafe",
+                "sql": "select 1",
+                "description_html": "<script>window.XSS=1</script>",
+                "on_success_message_sql": "select 'secret'",
+            }
+        },
+    )
+    form_response = await ds.client.post(
+        "/data/-/queries/store",
+        actor={"id": "root"},
+        data={
+            "name": "unsafe_form",
+            "sql": "select 1",
+            "description_html": "<script>window.XSS=1</script>",
+        },
+    )
+
+    assert response.status_code == 400
+    assert response.json()["errors"] == [
+        "Invalid keys: description_html, on_success_message_sql"
+    ]
+    assert form_response.status_code == 400
+    assert "Invalid keys: description_html" in form_response.text
+    assert await ds.get_query("data", "unsafe") is None
+    assert await ds.get_query("data", "unsafe_form") is None
+
+
 @pytest.mark.asyncio
 async def test_query_store_api_creates_writable_query():
     ds = Datasette(memory=True, default_deny=True)
@@ -959,6 +1000,42 @@ async def test_query_update_and_delete_api():
     assert await ds.get_query("data", "editable") is None
 
 
+@pytest.mark.asyncio
+async def test_query_update_api_rejects_config_only_fields():
+    ds = Datasette(memory=True, default_deny=True)
+    ds.root_enabled = True
+    db = ds.add_memory_database("query_update_config_only_fields", name="data")
+    await db.execute_write("create table dogs (id integer primary key, name text)")
+    await ds.invoke_startup()
+    await ds.add_query(
+        "data",
+        "editable",
+        "insert into dogs (name) values (:name)",
+        is_write=True,
+        source="user",
+        owner_id="root",
+    )
+
+    response = await ds.client.post(
+        "/data/editable/-/update",
+        actor={"id": "root"},
+        json={
+            "update": {
+                "description_html": "<script>window.XSS=1</script>",
+                "on_success_message_sql": "select 'secret'",
+            }
+        },
+    )
+
+    assert response.status_code == 400
+    assert response.json()["errors"] == [
+        "Invalid keys: description_html, on_success_message_sql"
+    ]
+    query = await ds.get_query("data", "editable")
+    assert query["description_html"] is None
+    assert query["on_success_message_sql"] is None
+
+
 @pytest.mark.asyncio
 async def test_query_update_api_rejects_trusted_queries_but_internal_update_allowed():
     ds = Datasette(