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/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. 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..aed38e6 --- /dev/null +++ b/docs/superpowers/specs/2026-08-05-view-ctas-sqlite-master-filter-design.md @@ -0,0 +1,292 @@ +# 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. + +## 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 67543fc..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,19 +72,40 @@ 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 }; 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 +115,29 @@ 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. + // + // 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); + } + } _ => {} } } @@ -136,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(()); } } @@ -311,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"), @@ -323,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 @@ -360,6 +425,121 @@ 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.` names the catalog we substitute for. + assert!(matches!( + SqliteMasterFilter::translate( + "CREATE VIEW v AS SELECT name FROM otherdb.sqlite_master" + ), + Cow::Borrowed(_) + )); + } + + #[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_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..c0fa3b1 100644 --- a/tests/sqlite_master_filter_enabled_test.rs +++ b/tests/sqlite_master_filter_enabled_test.rs @@ -136,3 +136,192 @@ 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:?}"); + + // 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] +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:?}"); +} + +#[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:?}" + ); +}