From 040e42ddca047a2e616d412b733fd36de233d2b2 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sun, 24 May 2026 22:58:50 -0700 Subject: [PATCH] Enforce query ownership and remove canned query hook Refs #2735 --- datasette/default_permissions/__init__.py | 16 ----- datasette/default_permissions/defaults.py | 30 ++++++++ datasette/hookspecs.py | 5 -- datasette/views/database.py | 12 ++++ tests/fixtures.py | 2 - tests/plugins/my_plugin.py | 5 -- tests/plugins/my_plugin_2.py | 26 +++---- tests/test_canned_queries.py | 34 +++++---- tests/test_permissions.py | 40 ++++------- tests/test_plugins.py | 27 ++------ tests/test_queries.py | 84 +++++++++++++++++++++++ 11 files changed, 182 insertions(+), 99 deletions(-) diff --git a/datasette/default_permissions/__init__.py b/datasette/default_permissions/__init__.py index 5a53dbe7..a9f2d8bd 100644 --- a/datasette/default_permissions/__init__.py +++ b/datasette/default_permissions/__init__.py @@ -17,13 +17,6 @@ UNION/INTERSECT operations. The order of evaluation is: from __future__ import annotations -from typing import TYPE_CHECKING - -if TYPE_CHECKING: - from datasette.app import Datasette - -from datasette import hookimpl - # Re-export all hooks and public utilities from .restrictions import ( actor_restrictions_sql as actor_restrictions_sql, @@ -38,12 +31,3 @@ from .defaults import ( default_query_permissions_sql as default_query_permissions_sql, DEFAULT_ALLOW_ACTIONS as DEFAULT_ALLOW_ACTIONS, ) - - -@hookimpl -def canned_queries(datasette: "Datasette", database: str, actor) -> dict: - """Return canned queries defined in datasette.yaml configuration.""" - queries = ( - ((datasette.config or {}).get("databases") or {}).get(database) or {} - ).get("queries") or {} - return queries diff --git a/datasette/default_permissions/defaults.py b/datasette/default_permissions/defaults.py index 2613c4f4..9737de96 100644 --- a/datasette/default_permissions/defaults.py +++ b/datasette/default_permissions/defaults.py @@ -74,6 +74,22 @@ async def default_query_permissions_sql( actor: Optional[dict], action: str, ) -> Optional[PermissionSQL]: + actor_id = actor.get("id") if isinstance(actor, dict) else None + + if action in {"update-query", "delete-query"}: + if actor_id is None: + return None + return PermissionSQL( + sql=""" + SELECT database_name AS parent, name AS child, 1 AS allow, + 'query owner' AS reason + FROM queries + WHERE source = 'user' + AND owner_id = :query_owner_id + """, + params={"query_owner_id": actor_id}, + ) + if action != "view-query": return None @@ -98,6 +114,19 @@ async def default_query_permissions_sql( AND source IN ('config', 'plugin') """ + user_writable_sql = "" + if actor_id is not None: + params["query_owner_id"] = actor_id + user_writable_sql = """ + UNION ALL + SELECT database_name AS parent, name AS child, 1 AS allow, + 'query owner' AS reason + FROM queries + WHERE is_write = 1 + AND source = 'user' + AND owner_id = :query_owner_id + """ + return PermissionSQL( sql=f""" WITH execute_sql_allowed AS ( @@ -118,6 +147,7 @@ async def default_query_permissions_sql( WHERE q.is_write = 0 AND q.published = 0 {trusted_writable_sql} + {user_writable_sql} """, params=params, ) diff --git a/datasette/hookspecs.py b/datasette/hookspecs.py index cf95abcb..a4067eaa 100644 --- a/datasette/hookspecs.py +++ b/datasette/hookspecs.py @@ -137,11 +137,6 @@ def permission_resources_sql(datasette, actor, action): """ -@hookspec -def canned_queries(datasette, database, actor): - """Return a dictionary of canned query definitions or an awaitable function that returns them""" - - @hookspec def register_magic_parameters(datasette): """Return a list of (name, function) magic parameter functions""" diff --git a/datasette/views/database.py b/datasette/views/database.py index f40c434c..2cdaab9f 100644 --- a/datasette/views/database.py +++ b/datasette/views/database.py @@ -945,6 +945,18 @@ class QueryView(View): # That should not have happened raise DatasetteError("Unexpected table found on POST", status=404) + if not await datasette.allowed( + action="view-query", + resource=QueryResource(database=db.name, query=canned_query["name"]), + actor=request.actor, + ): + raise Forbidden("You do not have permission to view this query") + + if canned_query.get("write") and canned_query.get("source") == "user": + await datasette.ensure_query_write_permissions( + db.name, canned_query["sql"], actor=request.actor + ) + # If database is immutable, return an error if not db.is_mutable: raise Forbidden("Database is immutable") diff --git a/tests/fixtures.py b/tests/fixtures.py index 71884294..8ab3633f 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -35,7 +35,6 @@ EXPECTED_PLUGINS = [ "hooks": [ "actor_from_request", "asgi_wrapper", - "canned_queries", "database_actions", "extra_body_script", "extra_css_urls", @@ -68,7 +67,6 @@ EXPECTED_PLUGINS = [ "hooks": [ "actor_from_request", "asgi_wrapper", - "canned_queries", "extra_js_urls", "extra_template_vars", "handle_exception", diff --git a/tests/plugins/my_plugin.py b/tests/plugins/my_plugin.py index 4e401c07..1dd9ed3e 100644 --- a/tests/plugins/my_plugin.py +++ b/tests/plugins/my_plugin.py @@ -314,11 +314,6 @@ def startup(datasette): _ = (Response, Forbidden, NotFound, hookimpl, actor_matches_allow) -@hookimpl -def canned_queries(datasette, database, actor): - return {"from_hook": f"select 1, '{actor['id'] if actor else 'null'}' as actor_id"} - - @hookimpl def register_magic_parameters(): from uuid import uuid4 diff --git a/tests/plugins/my_plugin_2.py b/tests/plugins/my_plugin_2.py index 9e8d9b2b..e3d3e760 100644 --- a/tests/plugins/my_plugin_2.py +++ b/tests/plugins/my_plugin_2.py @@ -139,20 +139,20 @@ def startup(datasette): datasette._startup_catalog_databases = [ row["database_name"] for row in catalog_rows ] - - return inner - - -@hookimpl -def canned_queries(datasette, database): - async def inner(): - return { - "from_async_hook": "select {}".format( - ( - await datasette.get_database(database).execute("select 1 + 1") - ).first()[0] + for database in datasette.databases: + await datasette.add_query( + database, + "from_hook", + "select 1, 'null' as actor_id", + source="plugin", + ) + result = await datasette.get_database(database).execute("select 1 + 1") + await datasette.add_query( + database, + "from_async_hook", + "select {}".format(result.first()[0]), + source="plugin", ) - } return inner diff --git a/tests/test_canned_queries.py b/tests/test_canned_queries.py index 5e36a87a..e06ad189 100644 --- a/tests/test_canned_queries.py +++ b/tests/test_canned_queries.py @@ -1,10 +1,16 @@ from bs4 import BeautifulSoup as Soup +from asgiref.sync import async_to_sync import json import pytest import re from .fixtures import make_app_client +def update_query(client, name, **kwargs): + async_to_sync(client.ds.invoke_startup)() + async_to_sync(client.ds.update_query)("data", name, **kwargs) + + @pytest.fixture def canned_write_client(tmpdir): template_dir = tmpdir / "canned_write_templates" @@ -153,9 +159,7 @@ def test_insert_error(canned_write_client): ) assert [["UNIQUE constraint failed: names.rowid", 3]] == messages # How about with a custom error message? - canned_write_client.ds.config["databases"]["data"]["queries"][ - "add_name_specify_id" - ]["on_error_message"] = "ERROR" + update_query(canned_write_client, "add_name_specify_id", on_error_message="ERROR") response = canned_write_client.post( "/data/add_name_specify_id", {"rowid": 1, "name": "Should fail"}, @@ -327,12 +331,16 @@ def magic_parameters_client(): ], ) def test_magic_parameters(magic_parameters_client, magic_parameter, expected_re): - magic_parameters_client.ds.config["databases"]["data"]["queries"]["runme_post"][ - "sql" - ] = f"insert into logs (line) values (:{magic_parameter})" - magic_parameters_client.ds.config["databases"]["data"]["queries"]["runme_get"][ - "sql" - ] = f"select :{magic_parameter} as result" + update_query( + magic_parameters_client, + "runme_post", + sql=f"insert into logs (line) values (:{magic_parameter})", + ) + update_query( + magic_parameters_client, + "runme_get", + sql=f"select :{magic_parameter} as result", + ) cookies = { "ds_actor": magic_parameters_client.actor_cookie({"id": "root"}), "foo": "bar", @@ -366,9 +374,11 @@ def test_magic_parameters(magic_parameters_client, magic_parameter, expected_re) @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.config["databases"]["data"]["queries"]["runme_post"][ - "sql" - ] = "insert into logs (line) values (:_header_host)" + update_query( + magic_parameters_client, + "runme_post", + sql="insert into logs (line) values (:_header_host)", + ) qs = "" if return_json: qs = "?_json=1" diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 8166532f..04800ed3 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -1,4 +1,5 @@ import collections +from asgiref.sync import async_to_sync from datasette.app import Datasette from datasette.cli import cli from datasette.default_permissions import restrictions_allow_action @@ -609,6 +610,10 @@ def test_padlocks_on_database_page(cascade_app_client): previous_config = cascade_app_client.ds.config try: cascade_app_client.ds.config = config + async_to_sync(cascade_app_client.ds.invoke_startup)() + async_to_sync(cascade_app_client.ds.add_query)( + "fixtures", "query_two", "select 2", source="config" + ) response = cascade_app_client.get( "/fixtures", cookies={"ds_actor": cascade_app_client.actor_cookie({"id": "test"})}, @@ -624,6 +629,7 @@ def test_padlocks_on_database_page(cascade_app_client): assert ">simple_view" in response.text finally: cascade_app_client.ds.config = previous_config + async_to_sync(cascade_app_client.ds.remove_query)("fixtures", "query_two") @pytest.mark.asyncio @@ -954,39 +960,20 @@ async def test_permissions_in_config( @pytest.mark.asyncio -async def test_allowed_resources_view_query_includes_actor_specific_canned_queries(): - """ - Actor-specific canned queries should be listed by allowed_resources("view-query"). - - This test is intentionally explicit about the previous bug: - - the canned query only exists for actor "alice" - - the permission rule only allows actor "alice" to view it - - allowed() succeeds for that specific query resource - - allowed_resources("view-query", actor) must include the same query - - Before the fix, QueryResource.resources_sql() called canned_queries(..., actor=None), - so the query was omitted from resource enumeration and allowed_resources() returned - an empty list even though allowed() returned True. - """ +async def test_allowed_resources_view_query_includes_actor_specific_query_permissions(): from datasette import hookimpl from datasette.permissions import PermissionSQL from datasette.resources import QueryResource - class ActorSpecificQueryPlugin: - __name__ = "ActorSpecificQueryPlugin" - - @hookimpl - def canned_queries(self, datasette, database, actor): - if database == "testdb" and actor and actor.get("id") == "alice": - return {"user_only": {"sql": "select 1 as n"}} - return {} + class ActorSpecificQueryPermissionPlugin: + __name__ = "ActorSpecificQueryPermissionPlugin" @hookimpl def permission_resources_sql(self, datasette, actor, action): if action == "view-query" and actor and actor.get("id") == "alice": return PermissionSQL(sql=""" SELECT 'testdb' AS parent, 'user_only' AS child, 1 AS allow, - 'alice can view her actor-specific canned query' AS reason + 'alice can view this query' AS reason """) return None @@ -994,9 +981,10 @@ async def test_allowed_resources_view_query_includes_actor_specific_canned_queri await ds.invoke_startup() ds.add_memory_database("testdb") await ds._refresh_schemas() + await ds.add_query("testdb", "user_only", "select 1 as n") - plugin = ActorSpecificQueryPlugin() - ds.pm.register(plugin, name="actor_specific_query_plugin") + plugin = ActorSpecificQueryPermissionPlugin() + ds.pm.register(plugin, name="actor_specific_query_permission_plugin") try: actor = {"id": "alice"} @@ -1012,7 +1000,7 @@ async def test_allowed_resources_view_query_includes_actor_specific_canned_queri ("testdb", "user_only") ] finally: - ds.pm.unregister(name="actor_specific_query_plugin") + ds.pm.unregister(name="actor_specific_query_permission_plugin") @pytest.mark.asyncio diff --git a/tests/test_plugins.py b/tests/test_plugins.py index c5b9aef0..b5a13ae5 100644 --- a/tests/test_plugins.py +++ b/tests/test_plugins.py @@ -885,40 +885,27 @@ async def test_hook_startup_catalog_populated(ds_client): @pytest.mark.asyncio -async def test_hook_canned_queries(ds_client): +async def test_plugin_startup_queries(ds_client): queries = (await ds_client.get("/fixtures.json")).json()["queries"] queries_by_name = {q["name"]: q for q in queries} - assert { - "sql": "select 2", - "name": "from_async_hook", - "private": False, - } == queries_by_name["from_async_hook"] - assert { - "sql": "select 1, 'null' as actor_id", - "name": "from_hook", - "private": False, - } == queries_by_name["from_hook"] + assert queries_by_name["from_async_hook"]["sql"] == "select 2" + assert queries_by_name["from_async_hook"]["private"] is False + assert queries_by_name["from_hook"]["sql"] == "select 1, 'null' as actor_id" + assert queries_by_name["from_hook"]["private"] is False @pytest.mark.asyncio -async def test_hook_canned_queries_non_async(ds_client): +async def test_plugin_startup_query_from_hook(ds_client): response = await ds_client.get("/fixtures/from_hook.json?_shape=array") assert [{"1": 1, "actor_id": "null"}] == response.json() @pytest.mark.asyncio -async def test_hook_canned_queries_async(ds_client): +async def test_plugin_startup_query_from_async_hook(ds_client): response = await ds_client.get("/fixtures/from_async_hook.json?_shape=array") assert [{"2": 2}] == response.json() -@pytest.mark.asyncio -async def test_hook_canned_queries_actor(ds_client): - assert ( - await ds_client.get("/fixtures/from_hook.json?_bot=1&_shape=array") - ).json() == [{"1": 1, "actor_id": "bot"}] - - def test_hook_register_magic_parameters(restore_working_directory): with make_app_client( extra_databases={"data.db": "create table logs (line text)"}, diff --git a/tests/test_queries.py b/tests/test_queries.py index 8e802b75..c6685d6c 100644 --- a/tests/test_queries.py +++ b/tests/test_queries.py @@ -490,3 +490,87 @@ async def test_create_query_ui_and_arbitrary_sql_save_link(): assert query_response.status_code == 200 assert "Save query" in query_response.text assert "/data/-/queries/-/create?sql=select+%2A+from+dogs" in query_response.text + + +@pytest.mark.asyncio +async def test_query_owner_gets_update_delete_and_writable_view_defaults(): + ds = Datasette(memory=True, default_deny=True) + ds.add_memory_database("query_owner_defaults", name="data") + await ds.invoke_startup() + await ds.add_query( + "data", + "insert_dog", + "insert into dogs (name) values (:name)", + is_write=True, + source="user", + owner_id="alice", + ) + + for action in ("view-query", "update-query", "delete-query"): + assert await ds.allowed( + action=action, + resource=QueryResource("data", "insert_dog"), + actor={"id": "alice"}, + ) + assert not await ds.allowed( + action=action, + resource=QueryResource("data", "insert_dog"), + actor={"id": "bob"}, + ) + + +@pytest.mark.asyncio +async def test_user_writable_query_execution_rechecks_table_permissions(): + ds = Datasette( + memory=True, + default_deny=True, + config={ + "databases": { + "data": { + "tables": { + "dogs": { + "permissions": { + "insert-row": {"id": "alice"}, + } + } + } + } + } + }, + ) + db = ds.add_memory_database("query_write_execution", name="data") + await db.execute_write("create table dogs (id integer primary key, name text)") + await ds.invoke_startup() + await ds.add_query( + "data", + "insert_dog", + "insert into dogs (name) values (:name)", + is_write=True, + source="user", + owner_id="alice", + ) + await ds.add_query( + "data", + "insert_cat", + "insert into dogs (name) values (:name)", + is_write=True, + source="user", + owner_id="bob", + ) + + allowed_response = await ds.client.post( + "/data/insert_dog?_json=1", + actor={"id": "alice"}, + data={"name": "Cleo"}, + ) + denied_response = await ds.client.post( + "/data/insert_cat?_json=1", + actor={"id": "bob"}, + data={"name": "Milo"}, + ) + + assert allowed_response.status_code == 200 + assert allowed_response.json()["ok"] is True + assert denied_response.status_code == 403 + rows = (await db.execute("select name from dogs")).dicts() + assert rows == [{"name": "Cleo"}]