Fix for GHSA-h547-rmjf-5m2m

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('"', '""'))
 

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 <https://github.com/dpfkdlemtp>`__. `GHSA-h547-rmjf-5m2m <https://github.com/simonw/datasette/security/advisories/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 <TableAlterView>` now rolls back schema changes when a :ref:`write_wrapper <plugin_hook_write_wrapper>` raises after the write. (:issue:`2924`, :pr:`2925`)
 - The :ref:`extra_template_vars() <plugin_hook_extra_template_vars>` plugin hook can now return a function or awaitable that resolves to ``None`` when no extra variables are needed. (:issue:`2005`)

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(

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"'),