From 03b2c66f6312b8317d87eb4c1326977f6f63b26d Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Wed, 27 May 2026 15:17:10 -0700 Subject: [PATCH] Require full row mutation permissions for raw SQL Raw SQL insert and update statements can have broader effects than their SQLite authorizer callbacks reveal. INSERT OR REPLACE and UPDATE OR REPLACE can delete conflicting rows while only surfacing insert or update operations. Expand table insert and update operations to require insert-row, update-row, and delete-row together. Keep delete operations mapped to delete-row, and update the analysis UI/API to report and evaluate multiple required permissions for a single operation. Refs https://github.com/simonw/datasette/pull/2749#issuecomment-4559083539 --- datasette/stored_queries.py | 108 ++++++++++++----- datasette/views/query_helpers.py | 27 +++-- tests/test_queries.py | 200 ++++++++++++++++++++++++++++++- 3 files changed, 290 insertions(+), 45 deletions(-) diff --git a/datasette/stored_queries.py b/datasette/stored_queries.py index 4b0fe6a6..cf44a9ff 100644 --- a/datasette/stored_queries.py +++ b/datasette/stored_queries.py @@ -588,10 +588,25 @@ async def list_queries( ) -PermissionRequirement = tuple[str, Resource] +@dataclass(frozen=True) +class PermissionRequirement: + action: str + resource: Resource -def permission_for_operation(operation: Operation) -> PermissionRequirement | None: +def row_mutation_requirements( + database: str, table: str +) -> tuple[PermissionRequirement, ...]: + resource = TableResource(database=database, table=table) + return tuple( + PermissionRequirement(action=action, resource=resource) + for action in ("insert-row", "update-row", "delete-row") + ) + + +def permission_requirements_for_operation( + operation: Operation, +) -> tuple[PermissionRequirement, ...]: if ( operation.operation == "read" and operation.target_type == "table" @@ -599,31 +614,45 @@ def permission_for_operation(operation: Operation) -> PermissionRequirement | No and operation.table is not None ): return ( - "view-table", - TableResource(database=operation.database, table=operation.table), + PermissionRequirement( + action="view-table", + resource=TableResource( + database=operation.database, table=operation.table + ), + ), ) - write_actions = { - "insert": "insert-row", - "update": "update-row", - "delete": "delete-row", - } - action = write_actions.get(operation.operation) if ( - action + operation.operation in {"insert", "update"} + and operation.target_type == "table" + and operation.database is not None + and operation.table is not None + ): + return row_mutation_requirements( + database=operation.database, + table=operation.table, + ) + if ( + operation.operation == "delete" and operation.target_type == "table" and operation.database is not None and operation.table is not None ): return ( - action, - TableResource(database=operation.database, table=operation.table), + PermissionRequirement( + action="delete-row", + resource=TableResource( + database=operation.database, table=operation.table + ), + ), ) if operation.operation == "create" and operation.target_type == "table": if operation.database is None: - return None + return () return ( - "create-table", - DatabaseResource(database=operation.database), + PermissionRequirement( + action="create-table", + resource=DatabaseResource(database=operation.database), + ), ) if ( operation.operation == "alter" @@ -632,8 +661,12 @@ def permission_for_operation(operation: Operation) -> PermissionRequirement | No and operation.table is not None ): return ( - "alter-table", - TableResource(database=operation.database, table=operation.table), + PermissionRequirement( + action="alter-table", + resource=TableResource( + database=operation.database, table=operation.table + ), + ), ) if ( operation.operation == "drop" @@ -642,8 +675,12 @@ def permission_for_operation(operation: Operation) -> PermissionRequirement | No and operation.table is not None ): return ( - "drop-table", - TableResource(database=operation.database, table=operation.table), + PermissionRequirement( + action="drop-table", + resource=TableResource( + database=operation.database, table=operation.table + ), + ), ) if ( operation.operation in {"create", "drop"} @@ -652,10 +689,14 @@ def permission_for_operation(operation: Operation) -> PermissionRequirement | No and operation.table is not None ): return ( - "alter-table", - TableResource(database=operation.database, table=operation.table), + PermissionRequirement( + action="alter-table", + resource=TableResource( + database=operation.database, table=operation.table + ), + ), ) - return None + return () def operation_should_be_ignored(operation: Operation) -> bool: @@ -704,20 +745,23 @@ async def ensure_query_write_permissions( for operation in analysis.operations: if operation_should_be_ignored(operation): continue - permission = permission_for_operation(operation) - if permission is None: + permissions = permission_requirements_for_operation(operation) + if not permissions: raise Forbidden( "Unsupported SQL operation: {} {}".format( operation.operation, operation.target_type ) ) - action, resource = permission if operation.database != database: raise Forbidden("Writable queries may not access attached databases") - if not await datasette.allowed( - action=action, - resource=resource, - actor=actor, - ): - raise Forbidden(f"Permission denied: need {action} on {resource}") + for permission in permissions: + if not await datasette.allowed( + action=permission.action, + resource=permission.resource, + actor=actor, + ): + raise Forbidden( + f"Permission denied: need {permission.action} " + f"on {permission.resource}" + ) return analysis diff --git a/datasette/views/query_helpers.py b/datasette/views/query_helpers.py index 05a0d73e..7f3ef1bc 100644 --- a/datasette/views/query_helpers.py +++ b/datasette/views/query_helpers.py @@ -6,7 +6,7 @@ from datasette.stored_queries import ( StoredQuery, operation_is_write, operation_should_be_ignored, - permission_for_operation, + permission_requirements_for_operation, ) from datasette.utils import ( named_parameters as derive_named_parameters, @@ -216,8 +216,10 @@ def _display_operations(analysis: SQLAnalysis) -> list[Operation]: def _analysis_rows(analysis: SQLAnalysis) -> list[dict[str, object]]: rows = [] for operation in _display_operations(analysis): - permission = permission_for_operation(operation) - required_permission = permission[0] if permission else "" + permissions = permission_requirements_for_operation(operation) + required_permission = ", ".join( + permission.action for permission in permissions + ) rows.append( { "operation": operation.operation, @@ -236,14 +238,17 @@ async def _analysis_rows_with_permissions( rows = _analysis_rows(analysis) is_write = _analysis_is_write(analysis) for row, operation in zip(rows, _display_operations(analysis)): - permission = permission_for_operation(operation) - if permission: - action, resource = permission - row["allowed"] = await datasette.allowed( - action=action, - resource=resource, - actor=actor, - ) + permissions = permission_requirements_for_operation(operation) + if permissions: + row["allowed"] = True + for permission in permissions: + if not await datasette.allowed( + action=permission.action, + resource=permission.resource, + actor=actor, + ): + row["allowed"] = False + break elif is_write: row["allowed"] = False else: diff --git a/tests/test_queries.py b/tests/test_queries.py index 97ec973f..fcd19d1c 100644 --- a/tests/test_queries.py +++ b/tests/test_queries.py @@ -508,6 +508,8 @@ async def test_analyze_write_query_requires_table_permissions(): "dogs": { "permissions": { "insert-row": {"id": "writer"}, + "update-row": {"id": "writer"}, + "delete-row": {"id": "writer"}, } } } @@ -1429,7 +1431,7 @@ async def test_execute_write_analyze_endpoint_uses_sql_only(): "operation": "insert", "database": "data", "table": "dogs", - "required_permission": "insert-row", + "required_permission": "insert-row, update-row, delete-row", "source": None, "allowed": True, } @@ -1627,6 +1629,40 @@ async def test_execute_write_post_requires_database_and_table_permissions(): } } } + missing_update_permission = await ds.client.post( + "/data/-/execute-write", + actor={"id": "writer"}, + json={ + "sql": "insert into dogs (name) values (:name)", + "params": {"name": "Cleo"}, + }, + ) + + assert missing_update_permission.status_code == 403 + assert missing_update_permission.json()["errors"] == [ + "Permission denied: need update-row on data/dogs" + ] + + ds.config["databases"]["data"]["tables"]["dogs"]["permissions"][ + "update-row" + ] = {"id": "writer"} + missing_delete_permission = await ds.client.post( + "/data/-/execute-write", + actor={"id": "writer"}, + json={ + "sql": "insert into dogs (name) values (:name)", + "params": {"name": "Cleo"}, + }, + ) + + assert missing_delete_permission.status_code == 403 + assert missing_delete_permission.json()["errors"] == [ + "Permission denied: need delete-row on data/dogs" + ] + + ds.config["databases"]["data"]["tables"]["dogs"]["permissions"][ + "delete-row" + ] = {"id": "writer"} allowed = await ds.client.post( "/data/-/execute-write", actor={"id": "writer"}, @@ -1643,6 +1679,156 @@ async def test_execute_write_post_requires_database_and_table_permissions(): assert (await db.execute("select name from dogs")).first()[0] == "Cleo" +@pytest.mark.asyncio +async def test_execute_write_insert_or_replace_requires_delete_row_permission(): + ds = Datasette( + memory=True, + default_deny=True, + config={ + "databases": { + "data": { + "permissions": { + "view-database": {"id": "writer"}, + "execute-write-sql": {"id": "writer"}, + }, + "tables": { + "users": { + "permissions": { + "insert-row": {"id": "writer"}, + "update-row": {"id": "writer"}, + "view-table": {"id": "writer"}, + } + } + }, + } + } + }, + ) + db = ds.add_memory_database("execute_write_insert_or_replace", name="data") + await db.execute_write( + "create table users (id integer primary key, email text unique)" + ) + await db.execute_write( + "insert into users (id, email) values " + "(1, 'a@example.com'), (2, 'b@example.com')" + ) + await ds.invoke_startup() + + denied_response = await ds.client.post( + "/data/-/execute-write", + actor={"id": "writer"}, + json={ + "sql": ( + "insert or replace into users(id, email) " + "values (3, 'b@example.com')" + ) + }, + ) + + assert denied_response.status_code == 403 + assert denied_response.json()["errors"] == [ + "Permission denied: need delete-row on data/users" + ] + assert (await db.execute("select id, email from users order by id")).dicts() == [ + {"id": 1, "email": "a@example.com"}, + {"id": 2, "email": "b@example.com"}, + ] + + +@pytest.mark.asyncio +async def test_execute_write_update_or_replace_requires_delete_row_permission(): + ds = Datasette( + memory=True, + default_deny=True, + config={ + "databases": { + "data": { + "permissions": { + "view-database": {"id": "writer"}, + "execute-write-sql": {"id": "writer"}, + }, + "tables": { + "users": { + "permissions": { + "insert-row": {"id": "writer"}, + "update-row": {"id": "writer"}, + "view-table": {"id": "writer"}, + } + } + }, + } + } + }, + ) + db = ds.add_memory_database("execute_write_update_or_replace", name="data") + await db.execute_write( + "create table users (id integer primary key, email text unique)" + ) + await db.execute_write( + "insert into users (id, email) values " + "(1, 'a@example.com'), (2, 'b@example.com')" + ) + await ds.invoke_startup() + + denied_response = await ds.client.post( + "/data/-/execute-write", + actor={"id": "writer"}, + json={"sql": "update or replace users set email = 'b@example.com' where id = 1"}, + ) + + assert denied_response.status_code == 403 + assert denied_response.json()["errors"] == [ + "Permission denied: need delete-row on data/users" + ] + assert (await db.execute("select id, email from users order by id")).dicts() == [ + {"id": 1, "email": "a@example.com"}, + {"id": 2, "email": "b@example.com"}, + ] + + +@pytest.mark.asyncio +async def test_execute_write_update_requires_insert_row_permission(): + ds = Datasette( + memory=True, + default_deny=True, + config={ + "databases": { + "data": { + "permissions": { + "view-database": {"id": "writer"}, + "execute-write-sql": {"id": "writer"}, + }, + "tables": { + "users": { + "permissions": { + "update-row": {"id": "writer"}, + "delete-row": {"id": "writer"}, + "view-table": {"id": "writer"}, + } + } + }, + } + } + }, + ) + db = ds.add_memory_database("execute_write_update_requires_insert", name="data") + await db.execute_write("create table users (id integer primary key, name text)") + await db.execute_write("insert into users (id, name) values (1, 'Alice')") + await ds.invoke_startup() + + denied_response = await ds.client.post( + "/data/-/execute-write", + actor={"id": "writer"}, + json={"sql": "update users set name = 'Alicia' where id = 1"}, + ) + + assert denied_response.status_code == 403 + assert denied_response.json()["errors"] == [ + "Permission denied: need insert-row on data/users" + ] + assert (await db.execute("select name from users where id = 1")).first()[0] == "Alice" + + @pytest.mark.asyncio async def test_execute_write_insert_select_requires_view_table_on_source(): ds = Datasette( @@ -1659,7 +1845,13 @@ async def test_execute_write_insert_select_requires_view_table_on_source(): "secret": { "permissions": {"view-table": {"id": "someone-else"}} }, - "public_log": {"permissions": {"insert-row": {"id": "writer"}}}, + "public_log": { + "permissions": { + "insert-row": {"id": "writer"}, + "update-row": {"id": "writer"}, + "delete-row": {"id": "writer"}, + } + }, }, } } @@ -1740,6 +1932,8 @@ async def test_execute_write_rejects_function_operations(): "dogs": { "permissions": { "insert-row": {"id": "writer"}, + "update-row": {"id": "writer"}, + "delete-row": {"id": "writer"}, } } }, @@ -2117,6 +2311,8 @@ async def test_user_writable_query_execution_rechecks_table_permissions(): "dogs": { "permissions": { "insert-row": {"id": "alice"}, + "update-row": {"id": "alice"}, + "delete-row": {"id": "alice"}, } } },