diff --git a/src/dorian/datachecks.py b/src/dorian/datachecks.py index 954ef49..99b7481 100644 --- a/src/dorian/datachecks.py +++ b/src/dorian/datachecks.py @@ -36,6 +36,7 @@ import sqlite3 from datetime import date from pathlib import Path +from urllib.parse import quote # reuse the checker's own loader and null predicate so suggestions are # computed by the same code that will later check them @@ -239,7 +240,9 @@ def _sqlite_table_suggestions( def _sqlite_suggestions(p: Path, path: str, columns: list[str] | None) -> list[dict]: try: - con = sqlite3.connect(f"file:{p}?mode=ro", uri=True) + # percent-encode the path so a '?'/'#'/'%' in the filename cannot inject URI + # query params that override mode=ro (mirrors the sealed C5 reconcile path) + con = sqlite3.connect(f"file:{quote(str(p))}?mode=ro", uri=True) try: tables = [ r[0] diff --git a/tests/test_c5.py b/tests/test_c5.py index 62df5e6..78887a1 100644 --- a/tests/test_c5.py +++ b/tests/test_c5.py @@ -236,6 +236,40 @@ def test_reconcile_sqlite_attach_write_escape_is_blocked(fixture_repo): assert not evil.exists() # read-only contract: no file created outside the repo +def _make_question_named_db(repo: Path) -> Path: + # an in-repo sqlite file whose NAME contains a URI '?'. Without percent-encoding the + # path, `file:{path}?mode=ro` truncates at the '?', opening the WRONG file (and a + # name like `a?mode=rwc` would inject mode=rwc, defeating mode=ro). + p = repo / "data" / "q?.db" + con = sqlite3.connect(p) # plain path (not a URI): the '?' is literal in the filename + con.execute("CREATE TABLE t (lot_id TEXT)") + con.executemany("INSERT INTO t VALUES (?)", [("L001",), ("L002",), ("L003",), ("L004",)]) + con.commit() + con.close() + return p + + +def test_reconcile_sqlite_question_mark_named_file_opens_the_right_file(fixture_repo): + # the percent-encoded path opens the '?'-named DB itself (4 rows == the csv), not a + # path-truncated phantom — reverting the encoding fix turns this PASS into an ERROR. + _make_question_named_db(fixture_repo) + prog = f"reconcile:csv:{LOTS}~~sqlite:data/q?.db::SELECT COUNT(*) FROM t" + assert run_c5(fixture_repo, prog).verdict is Verdict.PASS + + +def test_reconcile_sqlite_question_mark_named_file_stays_read_only(fixture_repo): + # mode=ro must hold even for a '?'-named file: a CREATE TABLE through the reconcile + # path is rejected and the DB is left unmodified (the '?' cannot inject mode=rwc). + db = _make_question_named_db(fixture_repo) + prog = f"reconcile:csv:{LOTS}~~sqlite:data/q?.db::CREATE TABLE evil (x)" + res = run_c5(fixture_repo, prog) + assert res.verdict is Verdict.ERROR + con = sqlite3.connect(db) + tables = {r[0] for r in con.execute("SELECT name FROM sqlite_master WHERE type = 'table'")} + con.close() + assert tables == {"t"} # no write leaked through; read-only enforced + + def test_reconcile_sqlite_pathological_query_is_error_quickly(fixture_repo, monkeypatch): """A recursive CTE is permitted by the read-only authorizer (SQLITE_RECURSIVE is a read op) and runs unbounded, so without a per-query bound it hangs the process even