diff --git a/datasette/utils/__init__.py b/datasette/utils/__init__.py index d7407b98..669d04c7 100644 --- a/datasette/utils/__init__.py +++ b/datasette/utils/__init__.py @@ -568,7 +568,7 @@ def escape_css_string(s): def escape_sqlite(s): - if _boring_keyword_re.match(s) and (s.lower() not in reserved_words): + if _boring_keyword_re.fullmatch(s) and (s.lower() not in reserved_words): return s return '"{}"'.format(s.replace('"', '""')) diff --git a/docs/changelog.rst b/docs/changelog.rst index c431007c..b4b19ad9 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -26,6 +26,7 @@ Datasette plugins can now use **background tasks** to run code independent of th Bug fixes ~~~~~~~~~ +- Fixed a security issue where a trailing newline in a requested table name could bypass table permissions and expose private rows. Thanks for the report, `dpfkdlemtp `__. `GHSA-h547-rmjf-5m2m `__ - Column facets now show the remove-filter link for filters using ``column__exact=value``, as well as ``column=value``. (:issue:`1695`) - The :ref:`alter-table API ` now rolls back schema changes when a :ref:`write_wrapper ` raises after the write. (:issue:`2924`, :pr:`2925`) - The :ref:`extra_template_vars() ` plugin hook can now return a function or awaitable that resolves to ``None`` when no extra variables are needed. (:issue:`2005`) diff --git a/tests/test_api_write.py b/tests/test_api_write.py index fea70819..50bf1d67 100644 --- a/tests/test_api_write.py +++ b/tests/test_api_write.py @@ -56,6 +56,105 @@ def _headers(token): } +@pytest.mark.asyncio +@pytest.mark.parametrize("operation", ["read", "read_row", "rename"]) +async def test_trailing_lf_table_permissions(tmp_path, operation): + # SQLite treats "secret" and "secret\n" as different table names. Permission + # checks and SQL execution must agree on which table a request targets. + db_path = tmp_path / "data.db" + conn = sqlite3.connect(str(db_path)) + conn.executescript( + "create table secret (id integer primary key, value text);" + "insert into secret values (1, 'private');" + ) + conn.close() + # Allow builder to create and use tables generally, but explicitly deny + # access to the existing secret table below. Disable arbitrary SQL access. + grants = { + action: {"id": "builder"} + for action in ( + "view-database", + "create-table", + "view-table", + "insert-row", + "alter-table", + ) + } + ds = Datasette( + [str(db_path)], + default_deny=True, + settings={"default_allow_sql": False}, + config={ + "permissions": {"view-instance": {"id": "builder"}}, + "databases": { + "data": { + "permissions": grants, + "tables": { + "secret": { + "permissions": { + "view-table": False, + "insert-row": False, + "alter-table": False, + } + } + }, + } + }, + }, + ) + headers = _headers(write_token(ds, actor_id="builder")) + try: + # Establish that the protected table is inaccessible before creating + # a second table whose name differs only by a trailing line feed. + response = await ds.client.get("/data/secret.json", headers=headers) + assert response.status_code == 403 + response = await ds.client.get( + "/data/-/query.json?sql=select+*+from+secret", headers=headers + ) + assert response.status_code == 403 + # Distinct values let us detect if an operation targets secret + # instead of the newly created secret\n table. + response = await ds.client.post( + "/data/-/create", + json={"table": "secret\n", "row": {"id": 1, "value": "decoy"}, "pk": "id"}, + headers=headers, + ) + assert response.status_code == 201, response.text + # ~0A is Datasette's URL encoding for the line feed in the table name. + if operation in ("read", "read_row"): + # Both table and row endpoints must return only the permitted row. + path = "/1.json" if operation == "read_row" else ".json" + response = await ds.client.get( + "/data/secret~0A" + path + "?_shape=array", headers=headers + ) + assert response.status_code == 200, response.text + assert response.json() == [{"id": 1, "value": "decoy"}] + else: + # Renaming must move the permitted table, preserving its contents + # and removing its old name from the database. + response = await ds.client.post( + "/data/secret~0A/-/alter", + json={"operations": [{"op": "rename_table", "args": {"to": "moved"}}]}, + headers=headers, + ) + assert response.status_code == 200, response.text + db = ds.get_database("data") + assert ( + await db.execute('select value from "moved"') + ).single_value() == "decoy" + assert "secret\n" not in await db.table_names() + # Verify that the protected table and its data are unchanged, and that + # the API still denies access to it. + db = ds.get_database("data") + assert ( + await db.execute('select value from "secret"') + ).single_value() == "private" + response = await ds.client.get("/data/secret.json", headers=headers) + assert response.status_code == 403 + finally: + ds.close() + + def _insert_and_fetch_created(conn, table, insert_sql): cursor = conn.execute(insert_sql) return conn.execute( diff --git a/tests/test_utils.py b/tests/test_utils.py index a491b5a5..68d4a504 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -228,6 +228,8 @@ def test_detect_fts(open_quote, close_quote): "identifier,expected", ( ("plain", "plain"), + ("plain\n", '"plain\n"'), + ("select\n", '"select\n"'), ("select", '"select"'), ("has space", '"has space"'), ("has'quote", '"has\'quote"'),