From 92c7d4b60888c34660afc336bd04c7dfa6fd98f2 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Thu, 10 Sep 2026 15:46:12 -0700 Subject: [PATCH] Limit derived-table permissions to one source hop Simplify the solution to 5de0c1724e0490733d7fe5b99f389d17b279cdee - avoid contextvar. --- datasette/app.py | 88 ++++++++++++---------- datasette/utils/sqlite.py | 14 ++-- docs/authentication.rst | 9 ++- docs/plugins.rst | 9 +++ tests/test_api.py | 7 +- tests/test_fts_permissions.py | 121 ++++++++++++++++++++++++++++++- tests/test_html.py | 4 +- tests/test_internals_database.py | 31 +++++++- tests/test_pr76_fts_policy.py | 16 ++-- 9 files changed, 235 insertions(+), 64 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 76c48825..1f738a4f 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -308,12 +308,6 @@ DEFAULT_NOT_SET = object() ResourcesSQL = collections.namedtuple("ResourcesSQL", ("sql", "params")) -# Tracks recursive view-table inheritance checks and fails closed if a -# malicious or malformed schema declares a cycle of FTS content tables. -_derived_permission_stack = contextvars.ContextVar( - "derived_permission_stack", default=() -) - def _permission_cache_key(actor, action, parent, child): # Key on the full serialized actor so actors differing in any field @@ -1762,6 +1756,26 @@ class Datasette: ) return ResourcesSQL(sql, params) + async def _allowed_derived_table_source( + self, database, source, *, actor, dependencies + ): + """Check an immediate source, denying sources that are themselves derived.""" + if any( + TableResource.normalize_child(table) + == TableResource.normalize_child(source) + for table in dependencies + ): + return False + # The source has no dependency in this map. Evaluate its own permission + # and prerequisites without starting another dependency check. + verdicts = await self._allowed_many( + actions=["view-table"], + resource=TableResource(database, source), + actor=actor, + check_derived=False, + ) + return verdicts["view-table"] + async def _apply_derived_table_permissions_to_sql( self, sql, @@ -1776,12 +1790,17 @@ class Datasette: if parent in self.databases else ([] if parent is not None else list(self.databases.items())) ) - dependency_maps = await asyncio.gather( - *(db.derived_table_dependencies() for _, db in databases) + dependency_maps = dict( + zip( + (name for name, _ in databases), + await asyncio.gather( + *(db.derived_table_dependencies() for _, db in databases) + ), + ) ) dependencies = [ (database_name, child, source) - for (database_name, _), dependency_map in zip(databases, dependency_maps) + for database_name, dependency_map in dependency_maps.items() for child, source in dependency_map.items() ] if not dependencies: @@ -1792,10 +1811,11 @@ class Datasette: ) actor_verdicts = await asyncio.gather( *( - self.allowed( - action="view-table", - resource=TableResource(database_name, source), + self._allowed_derived_table_source( + database_name, + source, actor=actor, + dependencies=dependency_maps[database_name], ) for database_name, source in sources ) @@ -1806,10 +1826,11 @@ class Datasette: if include_is_private: anonymous_verdicts = await asyncio.gather( *( - self.allowed( - action="view-table", - resource=TableResource(database_name, source), + self._allowed_derived_table_source( + database_name, + source, actor=None, + dependencies=dependency_maps[database_name], ) for database_name, source in sources ) @@ -2066,6 +2087,12 @@ ORDER BY allowed.parent, allowed.child ) # {"edit-schema": True, "drop-table": True, "insert-row": False} """ + return await self._allowed_many( + actions=actions, resource=resource, actor=actor, check_derived=True + ) + + async def _allowed_many(self, *, actions, resource, actor, check_derived): + """Evaluate permissions, optionally applying the one-hop source policy.""" from datasette.permissions import ( _permission_check_cache, _skip_permission_checks, @@ -2119,40 +2146,27 @@ ORDER BY allowed.parent, allowed.child child=child, ) - # Automatically derived implementation tables cannot be more visible - # than the logical/content table they expose. Keep the requested - # table's own permission too: either side can make access private. if ( - "view-table" in to_check + check_derived + and "view-table" in to_check and raw.get("view-table") and isinstance(resource, TableResource) and parent in self.databases ): - dependency = await self.databases[parent].derived_table_dependencies() - dependency = next( + dependencies = await self.databases[parent].derived_table_dependencies() + source = next( ( source - for table, source in dependency.items() + for table, source in dependencies.items() if TableResource.normalize_child(table) == TableResource.normalize_child(child) ), None, ) - if dependency is not None: - stack = _derived_permission_stack.get() - dependency_key = (parent, dependency) - if dependency_key in stack or dependency == child: - raw["view-table"] = False - else: - token = _derived_permission_stack.set(stack + ((parent, child),)) - try: - raw["view-table"] = await self.allowed( - action="view-table", - resource=TableResource(parent, dependency), - actor=actor, - ) - finally: - _derived_permission_stack.reset(token) + if source is not None: + raw["view-table"] = await self._allowed_derived_table_source( + parent, source, actor=actor, dependencies=dependencies + ) def resolve(name): # final verdict = own rules AND verdict of also_requires chain diff --git a/datasette/utils/sqlite.py b/datasette/utils/sqlite.py index b4e81aeb..2ae1be9b 100644 --- a/datasette/utils/sqlite.py +++ b/datasette/utils/sqlite.py @@ -170,14 +170,14 @@ def sqlite_derived_table_dependencies( does not report which virtual table owns a shadow table or which table is named by an FTS ``content=`` option. Derive those relationships from ``sqlite_master`` DDL and the documented shadow-table suffixes. + + Database errors propagate: failed discovery must not be mistaken for an + empty dependency map and cached as permission to skip inheritance. """ schema_table = _sqlite_schema_table(schema) - try: - rows = conn.execute( - f"select name, sql from {schema_table} where type = 'table'" - ).fetchall() - except sqlite3.DatabaseError: - return {} + rows = conn.execute( + f"select name, sql from {schema_table} where type = 'table'" + ).fetchall() table_names = {row[0] for row in rows} # SQLite identifiers fold ASCII letters only. @@ -212,7 +212,7 @@ def sqlite_derived_table_dependencies( if source else None ) - # An unresolved source uses the existing cycle guard to deny access. + # An unresolved source is itself derived, so the one-hop policy denies it. dependencies[virtual_table] = source or virtual_table return dependencies diff --git a/docs/authentication.rst b/docs/authentication.rst index aea055e3..8957fe83 100644 --- a/docs/authentication.rst +++ b/docs/authentication.rst @@ -1382,10 +1382,11 @@ view-table Actor is allowed to view a table (or view) page, e.g. https://latest.datasette.io/fixtures/complex_foreign_keys -FTS vocabulary tables (``fts5vocab`` and ``fts4aux``) also require access to -their source FTS table and, for an external-content index, its content table. -The vocabulary table's own permission rules still apply. Vocabulary tables -whose source cannot be resolved within the same SQLite schema are denied. +Derived implementation tables require access to their immediate source: FTS and RTree shadow tables require access to their virtual table, external-content FTS tables require access to their content table, and FTS vocabulary tables (``fts5vocab`` and ``fts4aux``) require access to their FTS table. The derived table's own permission rules also apply. + +Access is always denied if the source table is itself derived, or if a vocabulary table's source cannot be identified. + +The same rules apply to individual permission checks and table listings, including whether they are private. If a database error prevents dependency discovery, the check or listing fails with an error instead of ignoring the dependencies. Failed discovery results are not cached, so later checks can retry. ``resource`` - ``datasette.resources.TableResource(database, table)`` ``database`` is the name of the database (string) diff --git a/docs/plugins.rst b/docs/plugins.rst index d32a9fe6..296ef55d 100644 --- a/docs/plugins.rst +++ b/docs/plugins.rst @@ -261,6 +261,15 @@ If you run ``datasette plugins --all`` it will include default plugins that ship "permission_resources_sql" ] }, + { + "name": "datasette.default_permissions.sqlite_statistics", + "static": false, + "templates": false, + "version": null, + "hooks": [ + "permission_resources_sql" + ] + }, { "name": "datasette.default_permissions.tokens", "static": false, diff --git a/tests/test_api.py b/tests/test_api.py index ac6b2cc0..690cb080 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -102,14 +102,11 @@ async def test_database_page(ds_client): "tags", } - # Expected hidden tables + # The external-content index is visible, but its shadow tables need a + # second dependency hop and are excluded by the one-hop permission policy. expected_hidden_tables = { "no_primary_key", "searchable_fts", - "searchable_fts_config", - "searchable_fts_data", - "searchable_fts_docsize", - "searchable_fts_idx", } # Verify all expected tables exist diff --git a/tests/test_fts_permissions.py b/tests/test_fts_permissions.py index c3e4292a..24a40cef 100644 --- a/tests/test_fts_permissions.py +++ b/tests/test_fts_permissions.py @@ -1,7 +1,121 @@ import pytest +from datasette import hookimpl from datasette.app import Datasette +from datasette.permissions import Action, PermissionSQL, _permission_check_cache from datasette.resources import DatabaseResource, TableResource +from datasette.utils.sqlite import sqlite3, sqlite_derived_table_dependencies + + +@pytest.mark.asyncio +@pytest.mark.parametrize("fts_module", ["fts4", "fts5"]) +@pytest.mark.parametrize("actor", [None, {"id": "root"}], ids=["anonymous", "root"]) +async def test_derived_permissions_allow_one_hop_but_deny_nested_sources( + fts_module, actor +): + class InspectPlugin: + @hookimpl + def register_actions(self): + return [ + Action( + name="inspect-derived", + description="Inspect a table", + resource_class=TableResource, + also_requires="view-table", + ) + ] + + @hookimpl + def permission_resources_sql(self, action): + if action == "inspect-derived": + return PermissionSQL( + sql="SELECT NULL AS parent, NULL AS child, 1 AS allow, 'inspect allowed' AS reason" + ) + + ds = Datasette(memory=True) + ds.pm.register(InspectPlugin(), name="inspect-derived-test") + db = ds.add_memory_database( + f"derived_one_hop_{fts_module}_{actor is not None}", name="data" + ) + await db.execute_write("create table Documents (body text)") + await db.execute_write( + f"create virtual table Search using {fts_module}(body, content='Documents')" + ) + await db.execute_write( + f"create virtual table Nested using {fts_module}(body, content='sEaRcH')" + ) + await ds.invoke_startup() + token = _permission_check_cache.set({}) + try: + # Both direct permissions are allowed, but a derived source makes its + # dependent unavailable even to an actor who can view the whole chain. + # Check and cache Search first so its cached grant cannot grant Nested. + for table, expected in ( + ("Documents", True), + ("Search", True), + ("Nested", False), + ("Search_docsize", False), + ): + for spelling in (table, table.upper(), table.lower()): + assert await ds.allowed_many( + actions=["view-table", "inspect-derived"], + resource=TableResource("data", spelling), + actor=actor, + ) == {"view-table": expected, "inspect-derived": expected} + + page = await ds.allowed_resources( + "view-table", actor, parent="data", include_is_private=True, limit=1000 + ) + allowed = {resource.child for resource in page.resources} + assert {"Documents", "Search"}.issubset(allowed) + assert "Nested" not in allowed + assert "Search_docsize" not in allowed + finally: + _permission_check_cache.reset(token) + ds.pm.unregister(name="inspect-derived-test") + ds.close() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("listing", [False, True], ids=["individual", "listing"]) +async def test_derived_permission_discovery_error_is_retried(monkeypatch, listing): + ds = Datasette(memory=True) + db = ds.add_memory_database(f"derived_discovery_error_{listing}", name="data") + await db.execute_write("create table documents (id integer primary key)") + await ds.invoke_startup() + + class UnavailableSchema: + def execute(self, sql): + raise sqlite3.DatabaseError("schema temporarily unavailable") + + async def check(): + if listing: + return await ds.allowed_resources("view-table", parent="data") + return await ds.allowed( + action="view-table", resource=TableResource("data", "documents") + ) + + token = _permission_check_cache.set({}) + try: + with monkeypatch.context() as patch: + patch.setattr( + "datasette.database.sqlite_derived_table_dependencies", + lambda conn: sqlite_derived_table_dependencies(UnavailableSchema()), + ) + with pytest.raises(sqlite3.DatabaseError, match="schema temporarily"): + await check() + + # Failed discovery must not cache an empty map or a permission grant. + assert db._cached_derived_table_dependencies is None + assert not _permission_check_cache.get() + result = await check() + if listing: + assert [resource.child for resource in result.resources] == ["documents"] + else: + assert result is True + assert db._cached_derived_table_dependencies is not None + finally: + _permission_check_cache.reset(token) @pytest.mark.asyncio @@ -243,8 +357,11 @@ async def test_derived_tables_propagate_private_flag_and_route_permissions(): resource.child: resource for resource in actor_page.resources } derived_names = set(await db.derived_table_dependencies()) - assert derived_names.issubset(actor_resources) - assert all(actor_resources[name].private for name in derived_names) + assert "secret_fts" in actor_resources + assert actor_resources["secret_fts"].private + # Shadow tables depend on the already-derived external-content FTS + # table, so they remain unavailable even to the permitted reader. + assert not (derived_names - {"secret_fts"}).intersection(actor_resources) anonymous_page = await ds.allowed_resources( "view-table", parent="data", limit=1000 diff --git a/tests/test_html.py b/tests/test_html.py index 6a7b4907..42cca701 100644 --- a/tests/test_html.py +++ b/tests/test_html.py @@ -36,8 +36,10 @@ def test_homepage(app_client_two_attached_databases): h2 = soup.select("h2")[0] assert "extra database" == h2.text.strip() counts_p, links_p = h2.find_all_next("p")[:2] + # Shadow tables of the external-content index are denied, so they do not + # contribute to the table or row totals. assert ( - "2 rows in 1 table, 5 rows in 4 hidden tables, 1 view" == counts_p.text.strip() + "2 rows in 1 table, 2 rows in 1 hidden table, 1 view" == counts_p.text.strip() ) # We should only show visible, not hidden tables here: table_links = [ diff --git a/tests/test_internals_database.py b/tests/test_internals_database.py index 2500fa22..4842d8b7 100644 --- a/tests/test_internals_database.py +++ b/tests/test_internals_database.py @@ -20,7 +20,11 @@ from datasette.database import ( _deliver_write_result, ) from datasette.utils import Column -from datasette.utils.sqlite import sqlite3, supports_returning +from datasette.utils.sqlite import ( + sqlite3, + sqlite_derived_table_dependencies, + supports_returning, +) requires_sqlite_returning = pytest.mark.skipif( not supports_returning(), reason="SQLite does not support RETURNING" @@ -39,6 +43,31 @@ async def test_execute(db): assert 15 == len(results) +@pytest.mark.asyncio +async def test_derived_dependency_cache_survives_failed_refresh(monkeypatch): + ds = Datasette(memory=True) + db = ds.add_memory_database(uuid.uuid4().hex, name="data") + await db.derived_table_dependencies() + previous_cache = db._cached_derived_table_dependencies + await db.execute_write("create table dependency_cache_refresh (id integer)") + + class UnavailableSchema: + def execute(self, sql): + raise sqlite3.DatabaseError("schema temporarily unavailable") + + with monkeypatch.context() as patch: + patch.setattr( + "datasette.database.sqlite_derived_table_dependencies", + lambda conn: sqlite_derived_table_dependencies(UnavailableSchema()), + ) + with pytest.raises(sqlite3.DatabaseError, match="schema temporarily"): + await db.derived_table_dependencies() + assert db._cached_derived_table_dependencies == previous_cache + + await db.derived_table_dependencies() + assert db._cached_derived_table_dependencies[0] != previous_cache[0] + + @pytest.mark.asyncio async def test_results_first(db): assert None is (await db.execute("select * from facetable where pk > 100")).first() diff --git a/tests/test_pr76_fts_policy.py b/tests/test_pr76_fts_policy.py index 848a4a8c..2d4086b3 100644 --- a/tests/test_pr76_fts_policy.py +++ b/tests/test_pr76_fts_policy.py @@ -35,17 +35,20 @@ def test_vocabulary_dependency_identity(module, arguments, vocab_name): @pytest.mark.asyncio @pytest.mark.parametrize("module", ["fts5", "fts4"]) +@pytest.mark.parametrize("external_content", [False, True], ids=["one-hop", "two-hop"]) @pytest.mark.parametrize( "source_allowed,vocab_allowed", [(False, True), (True, False), (True, True)] ) -async def test_vocabulary_transitive_permissions(module, source_allowed, vocab_allowed): +async def test_vocabulary_immediate_source_permissions( + module, external_content, source_allowed, vocab_allowed +): ds = Datasette( memory=True, config={ "databases": { "data": { "tables": { - "documents": { + "search": { "permissions": { "view-table": ( {"id": "reader"} if source_allowed else False @@ -60,9 +63,8 @@ async def test_vocabulary_transitive_permissions(module, source_allowed, vocab_a ) db = ds.add_memory_database(uuid.uuid4().hex, name="data") await db.execute_write("create table documents(body text)") - await db.execute_write( - f"create virtual table search using {module}(body, content='documents')" - ) + options = "body, content='documents'" if external_content else "body" + await db.execute_write(f"create virtual table search using {module}({options})") definition = ( "fts5vocab('SEARCH', 'row')" if module == "fts5" else "fts4aux('SEARCH')" ) @@ -70,7 +72,7 @@ async def test_vocabulary_transitive_permissions(module, source_allowed, vocab_a await ds.invoke_startup() try: actor = {"id": "reader"} - expected = source_allowed and vocab_allowed + expected = source_allowed and vocab_allowed and not external_content for name in ("words", "WORDS"): assert ( await ds.allowed( @@ -112,7 +114,7 @@ def test_cross_schema_vocabulary_is_unresolved(module, definition): conn.execute(f"create virtual table search using {module}(body)") conn.execute(f"create virtual table temp.words using {definition}") # Cross-schema ownership is not representable by the current map. - # The self-dependency invokes the permission layer's cycle denial. + # The source is itself derived, so the immediate-source policy denies it. assert ( sqlite_derived_table_dependencies(conn, schema="temp")["words"] == "words" )