From 0e15ca78a482b9639b9ffa482df2ffc559ba5154 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Wed, 5 Aug 2026 16:40:54 -0700 Subject: [PATCH 1/5] docs: design for extending --hide-internal-tables to CREATE VIEW and CTAS Closes the gap in #86: the sqlite_master filter rewrites only Statement::Query and Insert sources, so a view or CTAS built over sqlite_master reads the unfiltered catalog thereafter. Records the two decisions taken while scoping: the filter predicate is persisted inline into view definitions (rejecting an internal __pgsqlite_visible_master view, which would make pgsqlite internals load-bearing for user schema), and the rewrite stays an allowlist (rejecting a denylist, which fails into invalid SQL that leaks the internal prefix -- the #85 regression). Co-Authored-By: Claude Opus 5 (1M context) --- ...5-view-ctas-sqlite-master-filter-design.md | 208 ++++++++++++++++++ 1 file changed, 208 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-05-view-ctas-sqlite-master-filter-design.md diff --git a/docs/superpowers/specs/2026-08-05-view-ctas-sqlite-master-filter-design.md b/docs/superpowers/specs/2026-08-05-view-ctas-sqlite-master-filter-design.md new file mode 100644 index 0000000..1fbab5c --- /dev/null +++ b/docs/superpowers/specs/2026-08-05-view-ctas-sqlite-master-filter-design.md @@ -0,0 +1,208 @@ +# Extend `--hide-internal-tables` to `CREATE VIEW` and `CREATE TABLE ... AS SELECT` + +Issue: [#86](https://github.com/erans/pgsqlite/issues/86) +Follow-up to: [#80](https://github.com/erans/pgsqlite/issues/80) (the flag), [#85](https://github.com/erans/pgsqlite/issues/85) (narrowing to read contexts) +Date: 2026-08-05 +Status: Approved, ready for implementation planning + +## Problem + +`SqliteMasterFilter::translate` rewrites client `sqlite_master` references in two +statement positions only: `Statement::Query`, and the `source` of +`Statement::Insert`. A view body is neither, so with the flag on: + +```sql +CREATE VIEW v AS SELECT name FROM sqlite_master; +SELECT * FROM v; -- unfiltered catalog, including __pgsqlite_* rows +``` + +The second statement never mentions `sqlite_master`, so nothing filters it. The +view's stored definition references the raw catalog, and SQLite expands that +definition on every read. + +`CREATE TABLE t AS SELECT ... FROM sqlite_master` has the same gap. + +The restriction to those two positions was deliberate. Before #85, +`DELETE FROM sqlite_master WHERE name='zzz'` was rewritten into syntactically +invalid SQL, and the resulting SQLite error pasted the internal prefix straight +back to the client: + +``` +SQLite error: near "(": syntax error in DELETE FROM (SELECT * FROM sqlite_master WHERE SUBSTR(name, 1, 11) <> '__pgsqlite_' ... +``` + +Narrowing to read contexts fixed that but dropped view and CTAS bodies along +with the writes. + +### Severity + +Not a security hole. Per the #80 design, hiding is listing-only and +`SELECT * FROM __pgsqlite_schema` keeps working by name, so nothing becomes +reachable that was not already. The flag exists so end users do not stumble onto +internal tables and conclude their data is corrupt, and a schema browser that +creates views defeats that. + +## Change + +Two arms added to the `match` in `SqliteMasterFilter::translate` +(`src/translator/sqlite_master_filter.rs:88`): + +```rust +Statement::CreateView { query, .. } => { + let _ = query.visit(&mut visitor); +} +Statement::CreateTable(create_table) => { + if let Some(query) = create_table.query.as_mut() { + let _ = query.visit(&mut visitor); + } +} +``` + +Nothing else moves. The visitor, the `substr(...)` predicate, the alias +handling, the cheap substring gate, the fail-open behavior, and both wire hook +points are unchanged. This only widens which AST positions the existing visitor +is pointed at. + +`CREATE MATERIALIZED VIEW` and `CREATE TEMP VIEW` are covered for free: same +`Statement::CreateView` variant, same `query` field. + +The `_ => {}` arm keeps its existing comment, extended with the rule that +governs this and any future statement kind: + +> Rewrite where the filtered rows are read back; do not rewrite where filtering +> would change which rows a write touches. + +## Decisions + +### The filter predicate is persisted into view definitions + +SQLite stores the literal `CREATE VIEW` statement text in `sqlite_master.sql`. +Rewriting the body means a client reading the definition back sees: + +```sql +CREATE VIEW v AS SELECT name FROM (SELECT * FROM sqlite_master + WHERE SUBSTR(name, 1, 11) <> '__pgsqlite_' + AND (tbl_name IS NULL OR SUBSTR(tbl_name, 1, 11) <> '__pgsqlite_')) AS sqlite_master +``` + +That is the flag's own purpose working against itself: the user gets back a +definition full of `__pgsqlite_`. Accepted anyway. + +The alternative considered was adding a migration for an internal +`__pgsqlite_visible_master` view and rewriting `sqlite_master` to that name, +which keeps the stored DDL short. Rejected: it makes a pgsqlite-managed object +load-bearing for *user* schema. A future migration that renames or rebuilds it +silently breaks the user's view, and anyone who opens the database file with +plain `sqlite3` after moving off pgsqlite inherits a view referencing an +internal name. The inline predicate is ordinary SQL that works anywhere with no +pgsqlite present. It also needs no migration, no new schema object that exists +even when the flag is off, and no second shape for +`is_generated_filter_subquery` to recognize. + +There is precedent for not returning view text verbatim: PostgreSQL's +`pg_get_viewdef` returns a normalized rewrite, not what the user typed. + +Not doing the rewrite at all and fixing CTAS only was also rejected — it leaves +the issue's own reproduction open. + +### The rewrite stays an allowlist + +Two wider scopes were considered and rejected. + +**Also visiting the read-only fields of `UPDATE`/`DELETE`** (`selection`, +`assignments`, `using`, `returning`, never the target relation) is cheap and +carries no invalid-SQL risk, because it is still an allowlist. Rejected because +it changes what a write *does* rather than what a user *sees*: filtering +`DELETE FROM my_log WHERE table_name IN (SELECT name FROM sqlite_master)` +silently deletes fewer rows than the user's SQL says. That is a worse outcome +than an unfiltered listing and is not what the flag promises. + +**Inverting to a denylist** — visit every statement wholesale, skip only known +write targets — is future-proof against new statement kinds, and was rejected on +failure-mode asymmetry. An allowlist that misses a statement kind fails into +*not filtered*, which is cosmetic and can be filed as a follow-up. A denylist +that misses a write target fails into *invalid SQL whose error text leaks the +internal prefix*, which is #85 verbatim. sqlparser 0.57 already carries a live +landmine for this: `Statement::Merge`'s target is itself a `TableFactor`, so the +visitor would substitute a derived table for a MERGE target. The module's +existing doc comment states the same principle — "failing to hide a row is +cosmetic, rejecting a client's query is not." + +### Views bake in the filter permanently + +Because the rewrite happens at creation time and SQLite persists the text, a +view created while the flag is on keeps filtering after the operator turns the +flag *off*; symmetrically, a view created while the flag was off keeps leaking +after it is turned on. This is inherent to rewriting at creation time. The +alternative — resolving and rewriting view bodies at read time — is a much +larger mechanism, and the flag is a startup-fixed server-wide setting, so +flipping it on a live database is already a rare event. + +### CTAS leaves no trace + +SQLite stores the expanded column list for `CREATE TABLE t AS SELECT ...` +(`CREATE TABLE t(name TEXT)`), not the select. So a CTAS is a clean one-shot +filtered copy with no visible DDL change, and the concern above applies to views +only. + +## What does not change + +**The SQL injection detector needs no changes.** `analyze_statement` +(`src/security/sql_injection_detector.rs:126`) handles `Statement::CreateTable` +in the DDL arm without recursing into it, and `Statement::CreateView` falls +through to `_ => {}`. Neither descends into the body, so the derived table +spliced into a view or CTAS source cannot trip the `depth > 1` system-table rule +that required the `is_generated_filter_subquery` escape hatch for the plain +`SELECT` path in #85. That hatch stays exactly as it is, serving the query path. + +**Hook points.** Unchanged: `preprocess_query` in `src/query/executor.rs:40` for +the simple protocol, `handle_parse` in `src/query/extended.rs:94` for the +extended protocol. DDL reaches both. + +**`EXPLAIN` and `DECLARE ... CURSOR`** remain excluded. pgsqlite has no handler +for either. + +## Error handling + +Unchanged — fail open. The new arms introduce no new failure path: + +- Unparseable `CREATE VIEW` returns the input borrowed; the view leaks. Cosmetic, + and consistent with every other path in this module. +- `visitor.replaced == 0` returns the input borrowed. This matters more than + before: `CREATE TABLE sqlite_master_backup (id INT)` passes the cheap + substring gate, parses to a `CreateTable` with `query: None`, replaces + nothing, and must come back untouched. The new arm must not perturb ordinary + DDL. + +## Tests + +### Unit — `src/translator/sqlite_master_filter.rs` + +Alongside the existing tests, reusing the `FILTERED` constant: + +| Test | Asserts | +| --- | --- | +| `rewrites_create_view_body` | `CREATE VIEW v AS SELECT name FROM sqlite_master` — body carries the derived table | +| `rewrites_create_table_as_select_source` | CTAS source rewritten | +| `rewrites_materialized_view` | `CREATE MATERIALIZED VIEW` covered by the same arm | +| `leaves_create_table_without_query_borrowed` | `CREATE TABLE sqlite_master_backup (id INT)` returns `Cow::Borrowed` | +| `leaves_create_view_over_attached_db_borrowed` | `otherdb.sqlite_master` in a view body untouched | + +### Wire-level + +The issue's reproduction, in both flag states. + +`tests/sqlite_master_filter_enabled_test.rs`: + +- Create `v` over `sqlite_master`, then `SELECT * FROM v` — no `__pgsqlite_*` + rows, `customers` present. This is the test that would have caught the bug. +- Same for a CTAS snapshot table. + +`tests/sqlite_master_filter_disabled_test.rs`: + +- Same view with the flag off — internal rows still present, proving the default + is unchanged. + +No test asserts on the view's persisted `sql` text. That is the one thing +knowingly accepted as ugly, and pinning sqlparser's exact rendering into an +assertion makes the test fail on every sqlparser upgrade for no signal. From e2d79f380ca271eccc3b972c5c989c5236988f84 Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Wed, 5 Aug 2026 16:51:41 -0700 Subject: [PATCH 2/5] docs: implementation plan for #86 view/CTAS sqlite_master filtering Two tasks: point the existing visitor at CreateView's query and CreateTable's AS SELECT source (five unit tests, TDD), then wire-level regression tests reproducing the issue in both flag states. Co-Authored-By: Claude Opus 5 (1M context) --- ...26-08-05-view-ctas-sqlite-master-filter.md | 349 ++++++++++++++++++ 1 file changed, 349 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-05-view-ctas-sqlite-master-filter.md diff --git a/docs/superpowers/plans/2026-08-05-view-ctas-sqlite-master-filter.md b/docs/superpowers/plans/2026-08-05-view-ctas-sqlite-master-filter.md new file mode 100644 index 0000000..e1267d4 --- /dev/null +++ b/docs/superpowers/plans/2026-08-05-view-ctas-sqlite-master-filter.md @@ -0,0 +1,349 @@ +# Extend `--hide-internal-tables` to `CREATE VIEW` and CTAS — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make `--hide-internal-tables` also filter `sqlite_master` references inside `CREATE VIEW ... AS SELECT` and `CREATE TABLE ... AS SELECT`, closing [#86](https://github.com/erans/pgsqlite/issues/86). + +**Architecture:** `SqliteMasterFilter::translate` already owns the whole mechanism — a cheap substring gate, a `VisitorMut` that swaps each `sqlite_master` `TableFactor::Table` for a filtered derived table, and a `match` that decides which statement positions the visitor is pointed at. This change adds two arms to that `match` and nothing else. The visitor, predicate, alias handling, fail-open behavior, and both wire hook points are untouched. + +**Tech Stack:** Rust, `sqlparser` 0.57 (`ast::VisitMut` / `VisitorMut`, `PostgreSqlDialect`), `tokio-postgres` for wire-level tests. + +**Spec:** `docs/superpowers/specs/2026-08-05-view-ctas-sqlite-master-filter-design.md` + +## Global Constraints + +- **Allowlist only.** Never convert the `match` in `translate` to a denylist that visits statements wholesale. `Statement::Merge`'s target is itself a `TableFactor`, and substituting a derived table there produces invalid SQL whose error text leaks `__pgsqlite_` to the client — the #85 regression. +- **Do not touch `Statement::Update` or `Statement::Delete`,** including their subquery positions. Filtering there would change which rows a write touches. +- **Fail open.** Never return an error from `translate`. Unhandled shape, parse failure, or nothing replaced ⇒ return the input `Cow::Borrowed`. +- **No changes to `src/security/sql_injection_detector.rs`.** `analyze_statement` does not descend into `CreateTable` or `CreateView` bodies, so the spliced derived table cannot trip the `depth > 1` rule. +- **Pre-commit checklist (from `CLAUDE.md`), all four, before every commit:** `cargo check` (no errors/warnings), `cargo clippy`, `cargo build`, `cargo test`. +- Prefix filter is `substr(name, 1, 11) <> '__pgsqlite_'`, never `LIKE '__pgsqlite_%'`. + +--- + +### Task 1: Point the visitor at view and CTAS bodies + +**Files:** +- Modify: `src/translator/sqlite_master_filter.rs:88-99` (the `match statement` block) +- Test: `src/translator/sqlite_master_filter.rs` (the `mod tests` block at the bottom of the same file) + +**Interfaces:** +- Consumes: existing private items in this module — `RelationReplacer` (the `VisitorMut`), the `rewritten(&str) -> String` test helper, and the `FILTERED` test constant holding the rendered derived table. +- Produces: no signature changes. `pub fn translate(query: &str) -> Cow<'_, str>` keeps its exact shape; only which inputs it rewrites changes. + +- [ ] **Step 1: Write the five failing tests** + +Append to the `mod tests` block in `src/translator/sqlite_master_filter.rs`, after `rewrites_insert_select_source`: + +```rust + #[test] + fn rewrites_create_view_body() { + // The gap in #86: a view body is read back on every SELECT against the + // view, and the client's later `SELECT * FROM v` never mentions + // sqlite_master, so this is the only chance to filter it. + let out = rewritten("CREATE VIEW v AS SELECT name FROM sqlite_master"); + assert_eq!( + out, + format!("CREATE VIEW v AS SELECT name FROM {FILTERED} AS sqlite_master") + ); + } + + #[test] + fn rewrites_create_table_as_select_source() { + let out = rewritten("CREATE TABLE snapshot AS SELECT name FROM sqlite_master"); + assert_eq!( + out, + format!("CREATE TABLE snapshot AS SELECT name FROM {FILTERED} AS sqlite_master") + ); + } + + #[test] + fn rewrites_materialized_view() { + // Same Statement::CreateView variant, same `query` field: free. + let out = rewritten("CREATE MATERIALIZED VIEW mv AS SELECT name FROM sqlite_master"); + assert_eq!( + out, + format!("CREATE MATERIALIZED VIEW mv AS SELECT name FROM {FILTERED} AS sqlite_master") + ); + } + + #[test] + fn leaves_create_table_without_query_borrowed() { + // Passes the cheap substring gate on the *table name*, parses to a + // CreateTable with `query: None`, replaces nothing, and must come back + // untouched. Guards the new arm against perturbing ordinary DDL. + assert!(matches!( + SqliteMasterFilter::translate("CREATE TABLE sqlite_master_backup (id INT)"), + Cow::Borrowed(_) + )); + } + + #[test] + fn leaves_create_view_over_attached_db_borrowed() { + // Only `main.` and `temp.` name the SQLite catalog. + assert!(matches!( + SqliteMasterFilter::translate( + "CREATE VIEW v AS SELECT name FROM otherdb.sqlite_master" + ), + Cow::Borrowed(_) + )); + } +``` + +- [ ] **Step 2: Run the tests to verify the right ones fail** + +Run: `cargo test --lib sqlite_master_filter -- --nocapture` + +Expected: exactly three failures — `rewrites_create_view_body`, `rewrites_create_table_as_select_source`, `rewrites_materialized_view`. Each fails on `assert_eq!` because the output still contains the bare `FROM sqlite_master` instead of the derived table. + +The other two (`leaves_create_table_without_query_borrowed`, `leaves_create_view_over_attached_db_borrowed`) **pass already** — they are regression guards for the new arm, not drivers of it. If either one fails at this point, something else is wrong; stop and investigate rather than proceeding. + +- [ ] **Step 3: Add the two match arms** + +In `src/translator/sqlite_master_filter.rs`, replace the `match statement { ... }` block (currently lines 88-99) with: + +```rust + // Rewrite where the filtered rows are read back; never where + // filtering would change which rows a write touches. + // + // `UPDATE`/`DELETE` name their target relation with the same + // `TableFactor::Table`, and substituting a derived table there + // produces syntactically invalid SQL whose error text would leak + // the `__pgsqlite_` prefix straight back to the client. Writes to + // `sqlite_master` are SQLite's to reject, unmodified. Their + // subqueries are left alone too: filtering a `DELETE ... WHERE + // name IN (SELECT ... FROM sqlite_master)` would silently delete + // fewer rows than the client's SQL says. + // + // This stays an allowlist. A statement kind we miss is merely + // unfiltered; a write target we fail to recognize is invalid SQL. + match statement { + Statement::Query(query) => { + let _ = query.visit(&mut visitor); + } + Statement::Insert(insert) => { + if let Some(source) = insert.source.as_mut() { + let _ = source.visit(&mut visitor); + } + } + // SQLite persists the literal CREATE VIEW text and expands it + // on every read, so creation time is the only chance to filter. + // Covers MATERIALIZED and TEMP views: same variant, same field. + Statement::CreateView { query, .. } => { + let _ = query.visit(&mut visitor); + } + // `CREATE TABLE ... AS SELECT`. `query` is `None` for ordinary + // CREATE TABLE, which is then left untouched. + Statement::CreateTable(create_table) => { + if let Some(query) = create_table.query.as_mut() { + let _ = query.visit(&mut visitor); + } + } + _ => {} + } +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cargo test --lib sqlite_master_filter` + +Expected: PASS, all tests in the module including the pre-existing ones. `leaves_delete_untouched` and `leaves_update_untouched` passing is the check that the allowlist stayed narrow. + +- [ ] **Step 5: Run the pre-commit checklist** + +```bash +cargo check && cargo clippy && cargo build && cargo test +``` + +Expected: no errors, no new warnings, all tests pass. + +- [ ] **Step 6: Commit** + +```bash +git add src/translator/sqlite_master_filter.rs +git commit -m "fix(hide-internal-tables): filter sqlite_master in CREATE VIEW and CTAS bodies + +A view or CTAS built over sqlite_master read the unfiltered catalog: the +rewrite covered only Statement::Query and Insert sources, and the client's +later SELECT against the view never mentions sqlite_master, so nothing +filtered it. + +Points the existing visitor at CreateView's query and CreateTable's AS +SELECT source. Stays an allowlist -- UPDATE/DELETE remain wholly untouched, +including their subqueries. + +Refs #86" +``` + +--- + +### Task 2: Wire-level regression tests in both flag states + +**Files:** +- Modify: `tests/sqlite_master_filter_enabled_test.rs` (append two tests) +- Modify: `tests/sqlite_master_filter_disabled_test.rs` (append one test) + +**Interfaces:** +- Consumes: `SqliteMasterFilter::translate` behavior from Task 1, reached over the wire through `preprocess_query` (`src/query/executor.rs:287`). Both test files already define a local `table_names(&tokio_postgres::Client, &str) -> Vec` helper that runs `simple_query` and collects column 0; reuse it, do not redefine it. +- Produces: nothing consumed by later tasks. + +These are separate test *binaries* on purpose: `pgsqlite::config::set_hide_internal_tables(true)` sets process-global state, so the flag-off assertions have to live in a binary that never calls it. + +- [ ] **Step 1: Write the failing flag-on tests** + +Append to `tests/sqlite_master_filter_enabled_test.rs`: + +```rust +#[tokio::test] +async fn hides_internal_objects_through_a_view() { + pgsqlite::config::set_hide_internal_tables(true); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }) + .await; + + // The issue's reproduction. The SELECT below never mentions sqlite_master, + // so the view body is the only place the filter can be applied. + server + .client + .simple_query("CREATE VIEW schema_names AS SELECT name FROM sqlite_master") + .await + .expect("CREATE VIEW over sqlite_master should succeed"); + + let names = table_names(&server.client, "SELECT name FROM schema_names ORDER BY name").await; + assert!( + !names.iter().any(|n| n.starts_with("__pgsqlite_")), + "internal tables leaked through a view: {names:?}" + ); + assert!(names.iter().any(|n| n == "customers"), "user table missing: {names:?}"); +} + +#[tokio::test] +async fn hides_internal_objects_through_create_table_as_select() { + pgsqlite::config::set_hide_internal_tables(true); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE orders (id INTEGER PRIMARY KEY, total TEXT)").await?; + Ok(()) + }) + }) + .await; + + server + .client + .simple_query("CREATE TABLE catalog_snapshot AS SELECT name FROM sqlite_master") + .await + .expect("CTAS over sqlite_master should succeed"); + + let names = table_names( + &server.client, + "SELECT name FROM catalog_snapshot ORDER BY name", + ) + .await; + assert!( + !names.iter().any(|n| n.starts_with("__pgsqlite_")), + "internal tables leaked into a CTAS snapshot: {names:?}" + ); + assert!(names.iter().any(|n| n == "orders"), "user table missing: {names:?}"); +} +``` + +- [ ] **Step 2: Run them to verify they fail against pre-Task-1 code** + +Because Task 1 is already committed at this point, these would pass immediately. To confirm they actually exercise the fix rather than passing vacuously, run them once against the pre-fix translator. This assumes Task 1's commit is `HEAD` and nothing else has been committed since; verify with `git log --oneline -1` first. + +```bash +git checkout HEAD~1 -- src/translator/sqlite_master_filter.rs +cargo test --test sqlite_master_filter_enabled_test hides_internal_objects_through +``` + +Expected: both FAIL with `internal tables leaked ...` listing `__pgsqlite_*` names. + +Then restore the fix: + +```bash +git checkout HEAD -- src/translator/sqlite_master_filter.rs +git status --short src/translator/sqlite_master_filter.rs # must print nothing +``` + +If `CREATE VIEW` or the CTAS instead fails with a pgsqlite error unrelated to filtering, stop — that is a separate defect and must be reported, not worked around. + +- [ ] **Step 3: Run them against the fixed code** + +Run: `cargo test --test sqlite_master_filter_enabled_test` + +Expected: PASS, including the three pre-existing tests in that file. + +- [ ] **Step 4: Write the flag-off test** + +Append to `tests/sqlite_master_filter_disabled_test.rs`: + +```rust +#[tokio::test] +async fn default_still_shows_internal_objects_through_a_view() { + // No call to set_hide_internal_tables — this asserts the shipped default. + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }) + .await; + + server + .client + .simple_query("CREATE VIEW schema_names AS SELECT name FROM sqlite_master") + .await + .expect("CREATE VIEW over sqlite_master should succeed"); + + let names = table_names(&server.client, "SELECT name FROM schema_names ORDER BY name").await; + assert!( + names.iter().any(|n| n == "__pgsqlite_schema"), + "default behaviour changed — a view over sqlite_master should still show internals: {names:?}" + ); + assert!(names.iter().any(|n| n == "customers")); +} +``` + +- [ ] **Step 5: Run it** + +Run: `cargo test --test sqlite_master_filter_disabled_test` + +Expected: PASS. This proves the fix is gated on the flag and the default is unchanged. + +- [ ] **Step 6: Run the pre-commit checklist** + +```bash +cargo check && cargo clippy && cargo build && cargo test +``` + +Expected: no errors, no new warnings, full suite passes. + +- [ ] **Step 7: Commit** + +```bash +git add tests/sqlite_master_filter_enabled_test.rs tests/sqlite_master_filter_disabled_test.rs +git commit -m "test(hide-internal-tables): cover views and CTAS over sqlite_master + +Wire-level reproduction from #86 in both flag states: with the flag on, a +view and a CTAS built over sqlite_master return no __pgsqlite_* rows; with +it off, they still do. + +Closes #86" +``` + +--- + +## Out of scope + +Deliberately not done, per the spec — do not add these: + +- Filtering `sqlite_master` subqueries inside `UPDATE`/`DELETE`. +- An internal `__pgsqlite_visible_master` view to keep the persisted view DDL short. Rejected because it makes a pgsqlite-managed object load-bearing for user schema. +- Any test asserting on a view's persisted `sql` text. It pins sqlparser's exact rendering and would break on every upgrade for no signal. +- Un-filtering views created while the flag was on, after the operator turns it off. Baking is inherent to rewriting at creation time and is accepted. From aac01746771ad1b29d6d44acdc359561ee3b287a Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Wed, 5 Aug 2026 17:18:48 -0700 Subject: [PATCH 3/5] fix(hide-internal-tables): filter sqlite_master in CREATE VIEW and CTAS bodies A view or CTAS built over sqlite_master read the unfiltered catalog: the rewrite covered only Statement::Query and Insert sources, and the client's later SELECT against the view never mentions sqlite_master, so nothing filtered it. Points the existing visitor at CreateView's query and CreateTable's AS SELECT source. Stays an allowlist -- UPDATE/DELETE remain wholly untouched, including their subqueries. Refs #86 --- src/translator/sqlite_master_filter.rs | 85 ++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 5 deletions(-) diff --git a/src/translator/sqlite_master_filter.rs b/src/translator/sqlite_master_filter.rs index 67543fc..32de9ba 100644 --- a/src/translator/sqlite_master_filter.rs +++ b/src/translator/sqlite_master_filter.rs @@ -80,11 +80,20 @@ impl SqliteMasterFilter { let mut visitor = RelationReplacer { replaced: 0 }; for statement in &mut statements { - // Read contexts only. `UPDATE`/`DELETE` name their target relation - // with the same `TableFactor::Table`, and substituting a derived - // table there produces syntactically invalid SQL whose error text - // would leak the `__pgsqlite_` prefix straight back to the client. - // Writes to `sqlite_master` are SQLite's to reject, unmodified. + // Rewrite where the filtered rows are read back; never where + // filtering would change which rows a write touches. + // + // `UPDATE`/`DELETE` name their target relation with the same + // `TableFactor::Table`, and substituting a derived table there + // produces syntactically invalid SQL whose error text would leak + // the `__pgsqlite_` prefix straight back to the client. Writes to + // `sqlite_master` are SQLite's to reject, unmodified. Their + // subqueries are left alone too: filtering a `DELETE ... WHERE + // name IN (SELECT ... FROM sqlite_master)` would silently delete + // fewer rows than the client's SQL says. + // + // This stays an allowlist. A statement kind we miss is merely + // unfiltered; a write target we fail to recognize is invalid SQL. match statement { Statement::Query(query) => { let _ = query.visit(&mut visitor); @@ -94,6 +103,19 @@ impl SqliteMasterFilter { let _ = source.visit(&mut visitor); } } + // SQLite persists the literal CREATE VIEW text and expands it + // on every read, so creation time is the only chance to filter. + // Covers MATERIALIZED and TEMP views: same variant, same field. + Statement::CreateView { query, .. } => { + let _ = query.visit(&mut visitor); + } + // `CREATE TABLE ... AS SELECT`. `query` is `None` for ordinary + // CREATE TABLE, which is then left untouched. + Statement::CreateTable(create_table) => { + if let Some(query) = create_table.query.as_mut() { + let _ = query.visit(&mut visitor); + } + } _ => {} } } @@ -360,6 +382,59 @@ mod tests { ); } + #[test] + fn rewrites_create_view_body() { + // The gap in #86: a view body is read back on every SELECT against the + // view, and the client's later `SELECT * FROM v` never mentions + // sqlite_master, so this is the only chance to filter it. + let out = rewritten("CREATE VIEW v AS SELECT name FROM sqlite_master"); + assert_eq!( + out, + format!("CREATE VIEW v AS SELECT name FROM {FILTERED} AS sqlite_master") + ); + } + + #[test] + fn rewrites_create_table_as_select_source() { + let out = rewritten("CREATE TABLE snapshot AS SELECT name FROM sqlite_master"); + assert_eq!( + out, + format!("CREATE TABLE snapshot AS SELECT name FROM {FILTERED} AS sqlite_master") + ); + } + + #[test] + fn rewrites_materialized_view() { + // Same Statement::CreateView variant, same `query` field: free. + let out = rewritten("CREATE MATERIALIZED VIEW mv AS SELECT name FROM sqlite_master"); + assert_eq!( + out, + format!("CREATE MATERIALIZED VIEW mv AS SELECT name FROM {FILTERED} AS sqlite_master") + ); + } + + #[test] + fn leaves_create_table_without_query_borrowed() { + // Passes the cheap substring gate on the *table name*, parses to a + // CreateTable with `query: None`, replaces nothing, and must come back + // untouched. Guards the new arm against perturbing ordinary DDL. + assert!(matches!( + SqliteMasterFilter::translate("CREATE TABLE sqlite_master_backup (id INT)"), + Cow::Borrowed(_) + )); + } + + #[test] + fn leaves_create_view_over_attached_db_borrowed() { + // Only `main.` and `temp.` name the SQLite catalog. + assert!(matches!( + SqliteMasterFilter::translate( + "CREATE VIEW v AS SELECT name FROM otherdb.sqlite_master" + ), + Cow::Borrowed(_) + )); + } + #[test] fn recognizes_its_own_generated_subquery() { // What the SQL injection detector keys on. Parse the rewritten form back From 78e80742471b52b5db6530efc215a1528fb7f0cf Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Wed, 5 Aug 2026 17:29:25 -0700 Subject: [PATCH 4/5] test(hide-internal-tables): cover views and CTAS over sqlite_master Wire-level reproduction from #86 in both flag states: with the flag on, a view and a CTAS built over sqlite_master return no __pgsqlite_* rows; with it off, they still do. Closes #86 --- tests/sqlite_master_filter_disabled_test.rs | 25 +++++++++ tests/sqlite_master_filter_enabled_test.rs | 58 +++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/tests/sqlite_master_filter_disabled_test.rs b/tests/sqlite_master_filter_disabled_test.rs index f9c5db3..903bd62 100644 --- a/tests/sqlite_master_filter_disabled_test.rs +++ b/tests/sqlite_master_filter_disabled_test.rs @@ -60,3 +60,28 @@ async fn default_still_shows_internal_objects_on_extended_protocol() { "default behaviour changed on extended protocol: {names:?}" ); } + +#[tokio::test] +async fn default_still_shows_internal_objects_through_a_view() { + // No call to set_hide_internal_tables — this asserts the shipped default. + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }) + .await; + + server + .client + .simple_query("CREATE VIEW schema_names AS SELECT name FROM sqlite_master") + .await + .expect("CREATE VIEW over sqlite_master should succeed"); + + let names = table_names(&server.client, "SELECT name FROM schema_names ORDER BY name").await; + assert!( + names.iter().any(|n| n == "__pgsqlite_schema"), + "default behaviour changed — a view over sqlite_master should still show internals: {names:?}" + ); + assert!(names.iter().any(|n| n == "customers")); +} diff --git a/tests/sqlite_master_filter_enabled_test.rs b/tests/sqlite_master_filter_enabled_test.rs index f0729d4..2769398 100644 --- a/tests/sqlite_master_filter_enabled_test.rs +++ b/tests/sqlite_master_filter_enabled_test.rs @@ -136,3 +136,61 @@ async fn write_attempts_on_sqlite_master_keep_sqlites_own_error() { ); } } + +#[tokio::test] +async fn hides_internal_objects_through_a_view() { + pgsqlite::config::set_hide_internal_tables(true); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }) + .await; + + // The issue's reproduction. The SELECT below never mentions sqlite_master, + // so the view body is the only place the filter can be applied. + server + .client + .simple_query("CREATE VIEW schema_names AS SELECT name FROM sqlite_master") + .await + .expect("CREATE VIEW over sqlite_master should succeed"); + + let names = table_names(&server.client, "SELECT name FROM schema_names ORDER BY name").await; + assert!( + !names.iter().any(|n| n.starts_with("__pgsqlite_")), + "internal tables leaked through a view: {names:?}" + ); + assert!(names.iter().any(|n| n == "customers"), "user table missing: {names:?}"); +} + +#[tokio::test] +async fn hides_internal_objects_through_create_table_as_select() { + pgsqlite::config::set_hide_internal_tables(true); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE orders (id INTEGER PRIMARY KEY, total TEXT)").await?; + Ok(()) + }) + }) + .await; + + server + .client + .simple_query("CREATE TABLE catalog_snapshot AS SELECT name FROM sqlite_master") + .await + .expect("CTAS over sqlite_master should succeed"); + + let names = table_names( + &server.client, + "SELECT name FROM catalog_snapshot ORDER BY name", + ) + .await; + assert!( + !names.iter().any(|n| n.starts_with("__pgsqlite_")), + "internal tables leaked into a CTAS snapshot: {names:?}" + ); + assert!(names.iter().any(|n| n == "orders"), "user table missing: {names:?}"); +} From b023dde5bc33fc13c35451b1e1859c3915dd673d Mon Sep 17 00:00:00 2001 From: Eran Sandler Date: Wed, 5 Aug 2026 19:16:57 -0700 Subject: [PATCH 5/5] fix(hide-internal-tables): close review findings on view/CTAS filtering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight findings from a whole-branch review of the #86 work. Critical: `CREATE TABLE t (name TEXT) AS SELECT ... FROM sqlite_master` is valid PostgreSQL, and rewriting its body produced SQL that downstream CreateTableTranslator's greedy CREATE_TABLE_REGEX then mangled — swallowing the `AS SELECT` once a parenthesized subquery followed the column list. SQLite's error embeds the whole statement text, so the client received a message containing `__pgsqlite_`: exactly what the flag exists to prevent, and #85's failure mode reintroduced. The CreateTable arm is now guarded on `columns.is_empty()`, leaving that shape untouched. The regex is the root cause and is deliberately left for a separate fix. Important: `CREATE VIEW IF NOT EXISTS` is SQLite-only syntax that PostgreSqlDialect cannot parse, so translate failed open and stored the view unfiltered — issue #86 verbatim, one keyword away. The parse now retries with SQLiteDialect. The `replaced == 0 => Cow::Borrowed` early return confines any dialect rendering differences to statements that really named sqlite_master. Important: `temp.sqlite_master` was accepted as a qualifier, but FILTERED_RELATION_SQL hardcodes an unqualified `FROM sqlite_master`, so the qualifier was silently dropped and the client got main's catalog instead of the temp schema's — wrong rows, not merely unfiltered ones. Now left alone, exactly like an ATTACHed database's. The match remains an allowlist over statement kinds. `leaves_merge_untouched` pins that boundary: Statement::Merge's target is itself a TableFactor, so a denylist refactor would substitute a derived table for a MERGE target and emit invalid SQL whose error text pastes the internal prefix back to the client. Also adds extended-protocol DDL coverage (Parse/Bind/Execute is a physically separate hook from the simple protocol, and most drivers send DDL that way), an assertion recording the accepted trade-off that the filter predicate is baked into the user's persisted view DDL, and documentation of all of the above in docs/configuration.md and the design spec. Co-Authored-By: Claude Opus 5 (1M context) --- docs/configuration.md | 50 ++++++- ...5-view-ctas-sqlite-master-filter-design.md | 84 +++++++++++ src/translator/sqlite_master_filter.rs | 129 +++++++++++++++-- tests/sqlite_master_filter_enabled_test.rs | 131 ++++++++++++++++++ 4 files changed, 381 insertions(+), 13 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index f44796c..9a935ba 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -22,7 +22,55 @@ pgsqlite can be configured through: | In-Memory | `--in-memory` | `PGSQLITE_IN_MEMORY` | `false` | Use in-memory SQLite database | | Socket Directory | `--socket-dir` | `PGSQLITE_SOCKET_DIR` | `/tmp` | Directory for Unix domain socket | | No TCP | `--no-tcp` | `PGSQLITE_NO_TCP` | `false` | Disable TCP listener, use only Unix socket | -| Hide Internal Tables | `--hide-internal-tables` | `PGSQLITE_HIDE_INTERNAL_TABLES` | `false` | Hide pgsqlite's internal `__pgsqlite_*` tables and their indexes from client `sqlite_master` / `sqlite_schema` queries. The tables remain queryable when named explicitly. Does not affect the materialized `pg_*` / `information_schema_*` relations or `PRAGMA table_list`. | +| Hide Internal Tables | `--hide-internal-tables` | `PGSQLITE_HIDE_INTERNAL_TABLES` | `false` | Hide pgsqlite's internal `__pgsqlite_*` tables and their indexes from client `sqlite_master` / `sqlite_schema` reads. See [notes below](#hide-internal-tables). | + +#### Hide Internal Tables + +`--hide-internal-tables` rewrites client references to `sqlite_master` / +`sqlite_schema` into a filtered relation, so pgsqlite's own `__pgsqlite_*` +bookkeeping objects and the indexes they own are not listed. + +**Where the rewrite applies.** Read contexts only: + +- plain `SELECT` (including CTEs, subqueries and joins) +- the `SELECT` source of an `INSERT ... SELECT` +- the body of `CREATE VIEW` (including `CREATE MATERIALIZED VIEW`, `CREATE TEMP + VIEW`, and `CREATE VIEW IF NOT EXISTS`) +- the `AS SELECT` source of `CREATE TABLE ... AS SELECT` + +**Where it does not apply.** `UPDATE` and `DELETE` are never rewritten, including +their subqueries — filtering there would silently change which rows a write +touches, and SQLite rejects writes to `sqlite_master` on its own anyway. +References qualified with another database (`otherdb.sqlite_master`) or with +`temp.` are left alone, because those name a different relation than the one the +filter substitutes for. `CREATE TABLE t (col ...) AS SELECT ...` — a CTAS with an +explicit column list — is also skipped. In all of these cases the client simply +sees the unfiltered catalog; the flag never rejects a query it cannot handle. + +**The filter predicate is persisted into your view definitions.** This is a +knowingly accepted trade-off, and it is the one surprising consequence. SQLite +stores the literal `CREATE VIEW` text, so a view created while this flag is on +has the filter baked into its stored DDL: + +```sql +CREATE VIEW v AS SELECT name FROM (SELECT * FROM sqlite_master + WHERE SUBSTR(name, 1, 11) <> '__pgsqlite_' + AND (tbl_name IS NULL OR SUBSTR(tbl_name, 1, 11) <> '__pgsqlite_')) AS sqlite_master +``` + +That text is what you get back from `sqlite_master.sql` and from +`information_schema.views.view_definition` — so a definition intended to hide the +internal prefix ends up displaying it. Two further consequences follow from the +rewrite happening at creation time: a view created while the flag is on keeps +filtering after the flag is turned off, and a view created while the flag was off +keeps listing internal objects after it is turned on. `CREATE TABLE ... AS +SELECT` is unaffected — SQLite stores the expanded column list, not the select — +so it is a clean one-shot filtered copy. + +**Scope.** Hiding is listing-only, not access control. The internal tables remain +queryable when named explicitly (`SELECT * FROM __pgsqlite_schema`), and the flag +does not affect the materialized `pg_*` / `information_schema_*` relations or +`PRAGMA table_list`. ### SSL/TLS Configuration diff --git a/docs/superpowers/specs/2026-08-05-view-ctas-sqlite-master-filter-design.md b/docs/superpowers/specs/2026-08-05-view-ctas-sqlite-master-filter-design.md index 1fbab5c..aed38e6 100644 --- a/docs/superpowers/specs/2026-08-05-view-ctas-sqlite-master-filter-design.md +++ b/docs/superpowers/specs/2026-08-05-view-ctas-sqlite-master-filter-design.md @@ -206,3 +206,87 @@ The issue's reproduction, in both flag states. No test asserts on the view's persisted `sql` text. That is the one thing knowingly accepted as ugly, and pinning sqlparser's exact rendering into an assertion makes the test fail on every sqlparser upgrade for no signal. + +## Findings from final review + +Three corrections landed after implementation, in response to a whole-branch +review. The shipped code differs from the "Change" section above in these ways. + +### CTAS with an explicit column list is skipped + +`CREATE TABLE t (name TEXT) AS SELECT name FROM sqlite_master` is valid +PostgreSQL, and the `CreateTable` arm rewrote its body into valid SQL — but the +rewritten DDL then reaches `CreateTableTranslator`, whose greedy +`CREATE_TABLE_REGEX` (`src/translator/create_table_translator.rs:10`) swallows +the `AS SELECT` clause once a parenthesized subquery follows the column list. +rusqlite embeds the whole statement text in its error and pgsqlite propagates it +verbatim, so the client received: + +``` +SQLite error: near "SELECT": syntax error in CREATE TABLE typed_snapshot +(name TEXT AS SELECT name FROM (SELECT * FROM sqlite_master WHERE +SUBSTR(name, 1, 11) <> '__pgsqlite_' ... +``` + +That is #85's failure mode reintroduced: the flag exists to keep the internal +prefix away from the user, and this pasted it into their face. The arm is now +guarded on `create_table.columns.is_empty()`, so the shape is left borrowed and +behaves exactly as with the flag off. + +The root cause is the regex, not the filter. Fixing `CreateTableTranslator` was +deliberately kept out of this branch — it is a separate, wider blast radius — +and is to be filed separately. Note that the underlying translator bug is +pre-existing and independent of this flag: with `--hide-internal-tables` off, the +same statement silently creates an *empty* `typed_snapshot`, the `AS SELECT` +having been dropped. + +### The parse retries with `SQLiteDialect` + +`translate` parsed with `PostgreSqlDialect` only. PostgreSQL has no +`CREATE VIEW IF NOT EXISTS`; SQLite does, so that shape failed to parse, failed +open, and stored the view unfiltered — issue #86 verbatim, one keyword away. +Verified over the wire: the resulting view listed all 15 `__pgsqlite_*` tables +plus internal indexes. + +On `PostgreSqlDialect` failure the parse now retries with +`sqlparser::dialect::SQLiteDialect` before giving up. The existing +`replaced == 0 ⇒ Cow::Borrowed` early return confines any rendering differences +between the two dialects to statements that genuinely referenced +`sqlite_master`, so ordinary SQLite-only syntax is unaffected. + +(`CREATE TABLE IF NOT EXISTS ... AS SELECT` parses under both dialects; the gap +was specific to views.) + +### `temp.` is no longer accepted as a qualifier + +`post_visit_table_factor` accepted `temp` alongside `main`, but +`FILTERED_RELATION_SQL` hardcodes an unqualified `FROM sqlite_master`, so the +qualifier was silently dropped. `temp.sqlite_master` is a *different* relation in +SQLite, listing only temp objects — so the rewrite returned main's catalog and +omitted the client's actual temp objects. Wrong rows, not merely unfiltered ones. + +Pre-existing, but this branch would have newly baked the wrong substitution into +persisted view DDL. `temp.`-qualified references are now left alone, exactly as +`otherdb.sqlite_master` already was. Rewriting them correctly would mean +parameterizing `FILTERED_RELATION_SQL` on the qualifier; not worth it for a +relation that contains no `__pgsqlite_*` objects to hide in the first place. + +### Tests added + +Beyond the table above: + +| Test | Asserts | +| --- | --- | +| `leaves_create_table_as_select_with_column_list_borrowed` | the guarded CTAS shape returns `Cow::Borrowed` | +| `rewrites_create_view_if_not_exists` | the `SQLiteDialect` retry catches the SQLite-only keyword | +| `leaves_temp_qualified_relation_borrowed` | `temp.`-qualified references untouched, in both `SELECT` and `CREATE VIEW` | +| `leaves_merge_untouched` | the allowlist boundary the design names as the landmine for any denylist refactor | +| `ctas_with_explicit_column_list_never_leaks_internal_prefix` (wire) | no client-facing error text contains `__pgsqlite_` | +| `hides_internal_objects_through_a_create_view_if_not_exists` (wire) | the SQLite-only keyword is filtered end to end | +| `hides_internal_objects_through_a_view_over_extended_protocol` (wire) | DDL through Parse/Bind/Execute, a physically separate hook from the simple protocol | + +The "no test asserts on the view's persisted `sql` text" decision above is +softened, not reversed: `hides_internal_objects_through_a_view` now asserts only +that the stored DDL contains `__pgsqlite_` and `substr` (case-insensitively), so +the accepted trade-off is recorded where a future reader will hit it. sqlparser's +exact rendering is still not pinned. diff --git a/src/translator/sqlite_master_filter.rs b/src/translator/sqlite_master_filter.rs index 32de9ba..ea36b5c 100644 --- a/src/translator/sqlite_master_filter.rs +++ b/src/translator/sqlite_master_filter.rs @@ -5,7 +5,7 @@ use std::sync::LazyLock; use sqlparser::ast::{ Ident, ObjectNamePart, Query, Statement, TableAlias, TableFactor, VisitMut, VisitorMut, }; -use sqlparser::dialect::PostgreSqlDialect; +use sqlparser::dialect::{PostgreSqlDialect, SQLiteDialect}; use sqlparser::parser::Parser; use tracing::debug; @@ -72,10 +72,22 @@ impl SqliteMasterFilter { let mut statements = match Parser::parse_sql(&PostgreSqlDialect {}, query) { Ok(statements) => statements, - Err(e) => { - debug!("sqlite_master filter: parse failed, passing through: {e}"); - return Cow::Borrowed(query); - } + // PostgreSQL has no `CREATE VIEW IF NOT EXISTS`; SQLite does, and a + // client talking to pgsqlite may well use it. Without this retry the + // parse fails, we fail open, and the view is stored unfiltered — + // issue #86 verbatim, one keyword away. The `replaced == 0` early + // return below confines any rendering differences between the two + // dialects to statements that genuinely referenced `sqlite_master`. + Err(pg_err) => match Parser::parse_sql(&SQLiteDialect {}, query) { + Ok(statements) => statements, + Err(sqlite_err) => { + debug!( + "sqlite_master filter: parse failed, passing through: \ + postgres dialect: {pg_err}; sqlite dialect: {sqlite_err}" + ); + return Cow::Borrowed(query); + } + }, }; let mut visitor = RelationReplacer { replaced: 0 }; @@ -111,7 +123,17 @@ impl SqliteMasterFilter { } // `CREATE TABLE ... AS SELECT`. `query` is `None` for ordinary // CREATE TABLE, which is then left untouched. - Statement::CreateTable(create_table) => { + // + // A CTAS carrying an explicit column list + // (`CREATE TABLE t (name TEXT) AS SELECT ...`) is skipped: the + // rewritten DDL is valid SQL, but downstream + // `CreateTableTranslator`'s greedy CREATE_TABLE_REGEX swallows + // the `AS SELECT` clause once a parenthesized subquery follows + // the column list, and the resulting SQLite error embeds the + // whole statement text — pasting `__pgsqlite_` back to the very + // client the flag exists to shield. Leaving it borrowed means + // the statement behaves exactly as it does with the flag off. + Statement::CreateTable(create_table) if create_table.columns.is_empty() => { if let Some(query) = create_table.query.as_mut() { let _ = query.visit(&mut visitor); } @@ -158,13 +180,15 @@ impl VisitorMut for RelationReplacer { return ControlFlow::Continue(()); } - // Only `main.` and `temp.` qualify the SQLite catalog. Anything else - // (e.g. an attached database) is left alone. + // Only `main.` names the catalog this filter substitutes for. `temp.` + // is a *different* relation listing only temp objects, and + // FILTERED_RELATION_SQL hardcodes an unqualified `FROM sqlite_master`, + // so rewriting it would silently drop the qualifier and hand back + // main's catalog. Left alone, exactly like an ATTACHed database's. if name.0.len() > 1 { match name.0.first() { Some(ObjectNamePart::Identifier(qualifier)) => { - let qualifier = qualifier.value.to_ascii_lowercase(); - if qualifier != "main" && qualifier != "temp" { + if !qualifier.value.eq_ignore_ascii_case("main") { return ControlFlow::Continue(()); } } @@ -333,7 +357,7 @@ mod tests { #[test] fn leaves_attached_database_qualifier_alone() { - // Only `main.` and `temp.` name the SQLite catalog; `otherdb.sqlite_master` + // Only `main.` names the catalog we substitute for; `otherdb.sqlite_master` // belongs to an ATTACHed database and is none of our business. assert!(matches!( SqliteMasterFilter::translate("SELECT name FROM otherdb.sqlite_master"), @@ -345,6 +369,25 @@ mod tests { ); } + #[test] + fn leaves_temp_qualified_relation_borrowed() { + // `temp.sqlite_master` is a *different* relation, listing only temp + // objects. FILTERED_RELATION_SQL hardcodes an unqualified + // `FROM sqlite_master`, so rewriting would drop the qualifier and hand + // the client main's catalog instead — wrong rows, not merely unfiltered + // ones. Leave it alone. + for sql in [ + "SELECT name FROM temp.sqlite_master", + "SELECT name FROM TEMP.sqlite_schema", + "CREATE VIEW v AS SELECT name FROM temp.sqlite_master", + ] { + assert!( + matches!(SqliteMasterFilter::translate(sql), Cow::Borrowed(_)), + "temp-qualified reference was rewritten: {sql}" + ); + } + } + #[test] fn leaves_delete_untouched() { // Substituting a derived table for the DELETE target yields invalid SQL @@ -426,7 +469,7 @@ mod tests { #[test] fn leaves_create_view_over_attached_db_borrowed() { - // Only `main.` and `temp.` name the SQLite catalog. + // Only `main.` names the catalog we substitute for. assert!(matches!( SqliteMasterFilter::translate( "CREATE VIEW v AS SELECT name FROM otherdb.sqlite_master" @@ -435,6 +478,68 @@ mod tests { )); } + #[test] + fn rewrites_create_view_if_not_exists() { + // PostgreSQL has no `CREATE VIEW IF NOT EXISTS`, so the PostgreSqlDialect + // parse fails and we used to fail open — storing the view unfiltered, + // which is issue #86 verbatim one keyword away. The SQLiteDialect retry + // is what catches it. + let out = rewritten("CREATE VIEW IF NOT EXISTS v AS SELECT name FROM sqlite_master"); + assert!( + out.contains("SUBSTR(name, 1, 11) <> '__pgsqlite_'"), + "CREATE VIEW IF NOT EXISTS was not filtered: {out}" + ); + assert!(!out.contains("LIKE '__pgsqlite_")); + } + + #[test] + fn leaves_create_table_as_select_with_column_list_borrowed() { + // `CREATE TABLE t (name TEXT) AS SELECT ...` is valid PostgreSQL, and + // the rewrite of its body is valid SQL — but the rewritten DDL then + // meets CreateTableTranslator's greedy CREATE_TABLE_REGEX, which + // swallows the `AS SELECT` once a parenthesized subquery follows the + // column list. The broken statement's text is echoed back in SQLite's + // error, leaking `__pgsqlite_` to the client. Skipping the rewrite + // makes the statement behave exactly as with the flag off. + for sql in [ + "CREATE TABLE t (name TEXT) AS SELECT name FROM sqlite_master", + "CREATE TABLE IF NOT EXISTS t (name TEXT) AS SELECT name FROM sqlite_master", + ] { + assert!( + matches!(SqliteMasterFilter::translate(sql), Cow::Borrowed(_)), + "CTAS with an explicit column list was rewritten: {sql}" + ); + } + } + + #[test] + fn leaves_merge_untouched() { + // The allowlist boundary. `Statement::Merge`'s target is itself a + // `TableFactor`, so any future refactor to a denylist that visits + // statements wholesale would substitute a derived table for the MERGE + // target and emit invalid SQL whose error text pastes `__pgsqlite_` + // back to the client. This test pins the boundary. + // + // Both forms below parse under sqlparser 0.57's PostgreSqlDialect, so + // today this test really does exercise the allowlist rather than the + // fail-open path. That is not guaranteed forever: if a future sqlparser + // stopped parsing this syntax the assertion would still hold, but only + // via fail-open, and the test would quietly become weaker evidence than + // it looks. It is a regression guard against a denylist refactor, not + // proof that MERGE parses. + for sql in [ + "MERGE INTO sqlite_master USING src ON src.name = sqlite_master.name \ + WHEN MATCHED THEN UPDATE SET tbl_name = src.tbl_name", + "MERGE INTO t USING sqlite_master AS s ON s.name = t.name \ + WHEN MATCHED THEN UPDATE SET n = s.name", + ] { + assert!( + matches!(SqliteMasterFilter::translate(sql), Cow::Borrowed(_)), + "MERGE was rewritten: {sql}" + ); + } + } + #[test] fn recognizes_its_own_generated_subquery() { // What the SQL injection detector keys on. Parse the rewritten form back diff --git a/tests/sqlite_master_filter_enabled_test.rs b/tests/sqlite_master_filter_enabled_test.rs index 2769398..c0fa3b1 100644 --- a/tests/sqlite_master_filter_enabled_test.rs +++ b/tests/sqlite_master_filter_enabled_test.rs @@ -163,6 +163,87 @@ async fn hides_internal_objects_through_a_view() { "internal tables leaked through a view: {names:?}" ); assert!(names.iter().any(|n| n == "customers"), "user table missing: {names:?}"); + + // Accepted trade-off, asserted so a future reader finds it recorded rather + // than discovering it as a surprise: SQLite persists the literal CREATE VIEW + // text, so rewriting the body bakes the filter predicate — internal prefix + // and all — into the *user's own* DDL. It surfaces here and in + // information_schema.views.view_definition. Deliberately a weak assertion: + // sqlparser's exact rendering is not pinned, only that the predicate is in + // there somewhere. + let view_ddl = table_names(&server.client, "SELECT sql FROM sqlite_master WHERE type = 'view'").await; + let stored = view_ddl.join("\n").to_ascii_lowercase(); + assert!( + stored.contains("__pgsqlite_") && stored.contains("substr"), + "the filter predicate is expected to be baked into the persisted view DDL \ + (see docs/configuration.md); got: {view_ddl:?}" + ); +} + +#[tokio::test] +async fn hides_internal_objects_through_a_view_over_extended_protocol() { + pgsqlite::config::set_hide_internal_tables(true); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }) + .await; + + // The extended-protocol hook (src/query/extended.rs) is a physically + // separate call site from the simple-protocol one, and many drivers send + // DDL through Parse/Bind/Execute by default. client.execute()/client.query() + // take that path; simple_query() does not. + server + .client + .execute("CREATE VIEW ext_schema_names AS SELECT name FROM sqlite_master", &[]) + .await + .expect("CREATE VIEW over sqlite_master should succeed on the extended protocol"); + + let rows = server + .client + .query("SELECT name FROM ext_schema_names ORDER BY name", &[]) + .await + .unwrap(); + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + assert!( + !names.iter().any(|n| n.starts_with("__pgsqlite_")), + "internal tables leaked through a view created over the extended protocol: {names:?}" + ); + assert!(names.iter().any(|n| n == "customers"), "user table missing: {names:?}"); +} + +#[tokio::test] +async fn hides_internal_objects_through_a_create_view_if_not_exists() { + pgsqlite::config::set_hide_internal_tables(true); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT)").await?; + Ok(()) + }) + }) + .await; + + // PostgreSQL has no `CREATE VIEW IF NOT EXISTS`; SQLite does. The filter + // parsed with PostgreSqlDialect only, so this shape failed to parse, failed + // open, and stored the view unfiltered — issue #86 verbatim, one keyword + // away. The SQLiteDialect retry closes it. + server + .client + .simple_query("CREATE VIEW IF NOT EXISTS maybe_names AS SELECT name FROM sqlite_master") + .await + .expect("CREATE VIEW IF NOT EXISTS over sqlite_master should succeed"); + + let names = table_names(&server.client, "SELECT name FROM maybe_names ORDER BY name").await; + assert!( + !names.iter().any(|n| n.starts_with("__pgsqlite_")), + "internal tables leaked through CREATE VIEW IF NOT EXISTS: {names:?}" + ); + assert!(names.iter().any(|n| n == "customers"), "user table missing: {names:?}"); } #[tokio::test] @@ -194,3 +275,53 @@ async fn hides_internal_objects_through_create_table_as_select() { ); assert!(names.iter().any(|n| n == "orders"), "user table missing: {names:?}"); } + +#[tokio::test] +async fn ctas_with_explicit_column_list_never_leaks_internal_prefix() { + pgsqlite::config::set_hide_internal_tables(true); + + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE orders (id INTEGER PRIMARY KEY, total TEXT)").await?; + Ok(()) + }) + }) + .await; + + // `CREATE TABLE t (name TEXT) AS SELECT ...` is valid PostgreSQL. Rewriting + // its body produced valid SQL, but downstream CreateTableTranslator's greedy + // CREATE_TABLE_REGEX swallowed the `AS SELECT` once a parenthesized subquery + // followed the column list; rusqlite embeds the whole statement in its error + // and pgsqlite propagates it verbatim, so the client got a message + // containing `__pgsqlite_` — the exact opposite of what the flag promises. + // + // The filter now skips this shape, so it behaves as with the flag off. This + // test does not care whether the statement succeeds or fails; it only + // requires that nothing pastes the internal prefix at the client. + // + // Verified against the unguarded code, which fails this assertion with: + // SQLite error: near "SELECT": syntax error in CREATE TABLE + // typed_snapshot (name TEXT AS SELECT name FROM (SELECT * FROM + // sqlite_master WHERE SUBSTR(name, 1, 11) <> '__pgsqlite_' ... + let sql = "CREATE TABLE typed_snapshot (name TEXT) AS SELECT name FROM sqlite_master"; + if let Err(err) = server.client.simple_query(sql).await { + let text = err.to_string(); + assert!( + !text.contains("__pgsqlite_"), + "internal prefix leaked into a client-facing error for `{sql}`: {text}" + ); + return; + } + + // If it succeeded, the table must still not expose internal rows. Note that + // CreateTableTranslator's greedy CREATE_TABLE_REGEX drops the `AS SELECT` + // clause outright for this shape, so the table comes out empty. That is a + // pre-existing bug in the translator, unrelated to this flag and out of + // scope here (it reproduces with --hide-internal-tables off); this test + // only pins that we do not make it *worse* by leaking the prefix. + let names = table_names(&server.client, "SELECT name FROM typed_snapshot ORDER BY name").await; + assert!( + !names.iter().any(|n| n.contains("__pgsqlite_")), + "internal tables leaked into a typed CTAS snapshot: {names:?}" + ); +}