diff --git a/Cargo.lock b/Cargo.lock index 50c9227c..8dd1ac51 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2045,6 +2045,18 @@ dependencies = [ "log", "recursive", "serde", + "sqlparser_derive", +] + +[[package]] +name = "sqlparser_derive" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da5fc6819faabb412da764b99d3b713bb55083c11e7e0c00144d386cd6a1939c" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.106", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 17376e8a..477a7556 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ postgres-protocol = "0.6.8" rusqlite = { version = "0.36.0", features = ["bundled", "uuid", "serde_json", "functions", "collation", "vtab", "column_decltype"] } # SQL parsing -sqlparser = { version = "0.57.0", features = ["serde"] } +sqlparser = { version = "0.57.0", features = ["serde", "visitor"] } # Types uuid = { version = "1.11.0", features = ["v4", "serde"] } diff --git a/README.md b/README.md index a97037b2..8282bee6 100644 --- a/README.md +++ b/README.md @@ -135,9 +135,10 @@ const client = new Client({ ```bash # Basic options pgsqlite \ - --database # SQLite database file (default: sqlite.db) - --port # PostgreSQL port (default: 5432) - --in-memory # Use in-memory database + --database # SQLite database file (default: sqlite.db) + --port # PostgreSQL port (default: 5432) + --in-memory # Use in-memory database + --hide-internal-tables # Hide pgsqlite's __pgsqlite_* tables from sqlite_master listings # Security pgsqlite \ diff --git a/docs/configuration.md b/docs/configuration.md index 253c412a..f44796cf 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -22,6 +22,7 @@ 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`. | ### SSL/TLS Configuration diff --git a/docs/superpowers/plans/2026-08-05-hide-internal-tables.md b/docs/superpowers/plans/2026-08-05-hide-internal-tables.md new file mode 100644 index 00000000..0feb53e4 --- /dev/null +++ b/docs/superpowers/plans/2026-08-05-hide-internal-tables.md @@ -0,0 +1,834 @@ +# Hide Internal `__pgsqlite_*` Tables 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:** Add an opt-in `--hide-internal-tables` flag that removes pgsqlite's `__pgsqlite_*` bookkeeping tables and their indexes from client `sqlite_master` / `sqlite_schema` queries. + +**Architecture:** A pure translator parses the client's SQL with sqlparser and replaces every `sqlite_master`/`sqlite_schema` relation with an equivalent filtered derived table, leaving the client's own projections, predicates, and joins byte-for-byte untouched. The translator is invoked from the two wire-protocol entry points only — never from `DbHandler` — so pgsqlite's own bookkeeping probes still see the real catalog. A process-global `AtomicBool`, set once from `Config` at startup, gates the call. + +**Tech Stack:** Rust, clap (config), sqlparser 0.57 with the `visitor` feature, tokio-postgres (integration tests). + +**Spec:** `docs/superpowers/specs/2026-08-05-hide-internal-tables-design.md` + +## Global Constraints + +- **Default off.** With no flag and no env var, every `sqlite_master` query must behave exactly as it does today. Every task that touches behaviour needs a test proving the default is unchanged. +- **Never filter pgsqlite's own queries.** The translator must not be reachable from `DbHandler::process_query` (`src/session/db_handler.rs:485`). `migration/runner.rs:61,302,310,339`, `metadata/enum_metadata.rs:203,341,389`, `rewriter/enum_rewriter.rs:26`, `cache/lazy_schema_loader.rs:124`, and `cache/schema.rs:117` all probe `sqlite_master` for `__pgsqlite_*` names; filtering them would make pgsqlite re-run its migrations on every start. +- **Fail open.** A parse failure or an unrecognized AST shape returns the query unchanged. Never turn a hide-failure into a client-visible error. +- **Use `substr(name, 1, 11) <> '__pgsqlite_'`, never `LIKE '__pgsqlite_%'`.** In `LIKE`, `_` is a single-character wildcard, so the `LIKE` form also matches unrelated names such as `abpgsqliteX`. Existing repository code uses the `LIKE` form; do not copy it. +- **Scope is `__pgsqlite_*` only.** Do not filter the materialized `pg_*` / `information_schema_*` relations, and do not touch `PRAGMA table_list`. Both are deliberate exclusions recorded in the spec. +- **Never dereference `CONFIG` from test code.** `config::CONFIG` calls `Config::parse()` on the test binary's argv; running `cargo test -- --nocapture` then aborts the process with `error: unexpected argument '--nocapture'`. This is verified behaviour, and it is the reason Task 1 introduces an atomic rather than reading `CONFIG` at the call sites. +- **Pre-commit checklist** (from `CLAUDE.md`) before every commit: `cargo check`, `cargo clippy`, `cargo build`, `cargo test`. + +## File Structure + +| File | Responsibility | +| --- | --- | +| `src/config.rs` (modify) | The `hide_internal_tables` CLI/env flag, plus the process-global toggle and its accessors | +| `src/main.rs` (modify) | Copies the parsed flag into the global toggle at startup | +| `Cargo.toml` (modify) | Enables sqlparser's `visitor` feature | +| `src/translator/sqlite_master_filter.rs` (create) | The pure rewrite: text gate, AST walk, relation substitution. No config, no I/O | +| `src/translator/mod.rs` (modify) | Exports `SqliteMasterFilter` | +| `src/query/executor.rs` (modify) | Simple-protocol hook, inside `preprocess_query` | +| `src/query/extended.rs` (modify) | Extended-protocol hook, at the top of `handle_parse` | +| `tests/sqlite_master_filter_enabled_test.rs` (create) | Wire-level behaviour with the flag on. Own binary, so the global toggle cannot race another test | +| `tests/sqlite_master_filter_disabled_test.rs` (create) | Wire-level proof that the default is unchanged. Own binary | +| `docs/configuration.md`, `README.md` (modify) | User-facing documentation of the flag | + +--- + +### Task 1: Flag and process-global toggle + +**Files:** +- Modify: `src/config.rs` (add arg after `no_tcp` at :26; add statics and accessors after the `CONFIG` block at :221) +- Modify: `src/main.rs:30` +- Test: `src/config.rs` (new `#[cfg(test)] mod tests`) + +**Interfaces:** +- Consumes: nothing. +- Produces: `pgsqlite::config::hide_internal_tables() -> bool` and `pgsqlite::config::set_hide_internal_tables(bool)`. Task 3 and Task 4 call the getter; the integration tests call the setter. + +- [ ] **Step 1: Write the failing test** + +Append to `src/config.rs`: + +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn hide_internal_tables_defaults_off_and_is_settable() { + // Note: this test must never touch CONFIG — Config::parse() would run + // against the test binary's argv and abort the process. + assert!(!hide_internal_tables()); + set_hide_internal_tables(true); + assert!(hide_internal_tables()); + set_hide_internal_tables(false); + assert!(!hide_internal_tables()); + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cargo test --lib config::tests::hide_internal_tables_defaults_off_and_is_settable` +Expected: FAIL to compile — `cannot find function 'hide_internal_tables' in this scope`. + +- [ ] **Step 3: Write minimal implementation** + +Add the CLI argument to `struct Config`, immediately after the `no_tcp` field (`src/config.rs:25-26`): + +```rust + #[arg(long, env = "PGSQLITE_HIDE_INTERNAL_TABLES", help = "Hide pgsqlite's internal __pgsqlite_* tables from client sqlite_master queries")] + pub hide_internal_tables: bool, +``` + +Add the toggle at the end of `src/config.rs`, after the `lazy_static!` block: + +```rust +use std::sync::atomic::{AtomicBool, Ordering}; + +/// Process-global mirror of `Config::hide_internal_tables`, set once at startup. +/// +/// The wire-protocol hooks read this instead of `CONFIG` because dereferencing +/// `CONFIG` calls `Config::parse()` on the current process argv, which aborts +/// any test binary invoked with harness arguments such as `--nocapture`. +static HIDE_INTERNAL_TABLES: AtomicBool = AtomicBool::new(false); + +/// Set the global "hide internal tables" toggle. Called once from `main`. +pub fn set_hide_internal_tables(enabled: bool) { + HIDE_INTERNAL_TABLES.store(enabled, Ordering::Relaxed); +} + +/// Whether client `sqlite_master` queries should have `__pgsqlite_*` objects filtered out. +pub fn hide_internal_tables() -> bool { + HIDE_INTERNAL_TABLES.load(Ordering::Relaxed) +} +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cargo test --lib config::tests::hide_internal_tables_defaults_off_and_is_settable` +Expected: PASS. + +- [ ] **Step 5: Wire the flag into startup** + +In `src/main.rs`, immediately after `let config = Config::load();` (:30): + +```rust + pgsqlite::config::set_hide_internal_tables(config.hide_internal_tables); +``` + +- [ ] **Step 6: Verify the flag is exposed** + +Run: `cargo run --quiet -- --help 2>&1 | grep -A 1 hide-internal-tables` +Expected: the flag and its help text appear in the usage output. + +- [ ] **Step 7: Commit** + +```bash +cargo check && cargo clippy && cargo build && cargo test --lib +git add src/config.rs src/main.rs +git commit -m "feat(config): add --hide-internal-tables flag and global toggle (#80)" +``` + +--- + +### Task 2: The `SqliteMasterFilter` translator + +**Files:** +- Modify: `Cargo.toml:24` +- Create: `src/translator/sqlite_master_filter.rs` +- Modify: `src/translator/mod.rs` (add `mod` near :15, `pub use` near :44) +- Test: `src/translator/sqlite_master_filter.rs` (inline `#[cfg(test)] mod tests`) + +**Interfaces:** +- Consumes: nothing from Task 1. This module is pure — it never reads config, so its tests are safe. +- Produces: `SqliteMasterFilter::translate(query: &str) -> Cow<'_, str>`. Returns `Cow::Borrowed` unchanged when there is nothing to do (no textual mention, parse failure, no relation matched) and `Cow::Owned` with the rewritten SQL otherwise. Tasks 3 and 4 call this. *(The idempotence guard originally listed here was removed during execution — see Notes for the reviewer.)* + +- [ ] **Step 1: Enable the sqlparser visitor feature** + +In `Cargo.toml`, change line 24 from: + +```toml +sqlparser = { version = "0.57.0", features = ["serde"] } +``` + +to: + +```toml +sqlparser = { version = "0.57.0", features = ["serde", "visitor"] } +``` + +This pulls in the `sqlparser_derive` proc-macro crate, which generates the `VisitMut` impls used below. + +- [ ] **Step 2: Write the failing tests** + +Create `src/translator/sqlite_master_filter.rs` containing only this test module for now: + +```rust +#[cfg(test)] +mod tests { + use super::*; + + /// The filtered relation as sqlparser renders it back out (note: uppercase SUBSTR). + const FILTERED: &str = "(SELECT * FROM sqlite_master WHERE SUBSTR(name, 1, 11) <> '__pgsqlite_' AND (tbl_name IS NULL OR SUBSTR(tbl_name, 1, 11) <> '__pgsqlite_'))"; + + fn rewritten(query: &str) -> String { + SqliteMasterFilter::translate(query).into_owned() + } + + #[test] + fn rewrites_bare_relation_and_keeps_client_predicate() { + let out = rewritten("SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY name"); + assert_eq!( + out, + format!("SELECT name FROM {FILTERED} AS sqlite_master WHERE type = 'table' ORDER BY name") + ); + } + + #[test] + fn rewrites_aggregate() { + let out = rewritten("SELECT count(*) FROM sqlite_master"); + assert_eq!(out, format!("SELECT count(*) FROM {FILTERED} AS sqlite_master")); + } + + #[test] + fn preserves_client_alias_in_join() { + let out = rewritten("SELECT m.name FROM sqlite_schema m JOIN foo f ON f.n = m.name"); + assert_eq!( + out, + format!("SELECT m.name FROM {FILTERED} AS m JOIN foo AS f ON f.n = m.name") + ); + } + + #[test] + fn rewrites_schema_qualified_relation() { + let out = rewritten("SELECT name FROM main.sqlite_master"); + assert_eq!(out, format!("SELECT name FROM {FILTERED} AS sqlite_master")); + } + + #[test] + fn rewrites_inside_cte() { + let out = rewritten("WITH t AS (SELECT name FROM sqlite_master) SELECT * FROM t"); + assert_eq!( + out, + format!("WITH t AS (SELECT name FROM {FILTERED} AS sqlite_master) SELECT * FROM t") + ); + } + + #[test] + fn rewrites_inside_exists_subquery() { + let out = rewritten("SELECT * FROM foo WHERE EXISTS (SELECT 1 FROM sqlite_master WHERE name = foo.n)"); + assert_eq!( + out, + format!("SELECT * FROM foo WHERE EXISTS (SELECT 1 FROM {FILTERED} AS sqlite_master WHERE name = foo.n)") + ); + } + + #[test] + fn rewrites_select_sql_projection() { + let out = rewritten("SELECT sql FROM sqlite_master WHERE type = 'table'"); + assert_eq!( + out, + format!("SELECT sql FROM {FILTERED} AS sqlite_master WHERE type = 'table'") + ); + } + + #[test] + fn does_not_corrupt_complex_predicates() { + // This is the shape that broke the earlier query-rewriting attempt (PR #82). + let out = rewritten(r"SELECT name FROM sqlite_master WHERE name NOT IN ('a', 'b') AND name LIKE 'x%' ESCAPE '\'"); + assert_eq!( + out, + format!(r"SELECT name FROM {FILTERED} AS sqlite_master WHERE name NOT IN ('a', 'b') AND name LIKE 'x%' ESCAPE '\'") + ); + } + + #[test] + fn leaves_unrelated_queries_borrowed() { + assert!(matches!( + SqliteMasterFilter::translate("SELECT * FROM customers"), + Cow::Borrowed(_) + )); + } + + #[test] + fn leaves_unparseable_input_borrowed() { + assert!(matches!( + SqliteMasterFilter::translate("SELECT FROM WHERE sqlite_master ((("), + Cow::Borrowed(_) + )); + } + + #[test] + fn leaves_explicit_internal_lookups_alone() { + // "Hidden from listings, still queryable by name" — a client that names an + // internal table explicitly is asking for it, and rewriting would also + // double-apply the filter when both protocol hooks run. + let q = "SELECT name FROM sqlite_master WHERE name = '__pgsqlite_schema'"; + assert!(matches!(SqliteMasterFilter::translate(q), Cow::Borrowed(_))); + } + + #[test] + fn does_not_match_unrelated_table_named_like_a_wildcard_match() { + // 'abpgsqliteX' matches LIKE '__pgsqlite_%' but not substr(name,1,11). + // Guard that we generate the substr form, not the LIKE form. + let out = rewritten("SELECT name FROM sqlite_master"); + assert!(out.contains("SUBSTR(name, 1, 11) <> '__pgsqlite_'")); + assert!(!out.contains("LIKE '__pgsqlite_")); + } +} +``` + +- [ ] **Step 3: Run tests to verify they fail** + +Run: `cargo test --lib sqlite_master_filter` +Expected: FAIL to compile — `cannot find type 'SqliteMasterFilter'`. + +- [ ] **Step 4: Write the implementation** + +Prepend to `src/translator/sqlite_master_filter.rs`, above the test module: + +```rust +use std::borrow::Cow; +use std::ops::ControlFlow; + +use sqlparser::ast::{ + Ident, ObjectNamePart, Statement, TableAlias, TableFactor, VisitMut, VisitorMut, +}; +use sqlparser::dialect::PostgreSqlDialect; +use sqlparser::parser::Parser; +use tracing::debug; + +/// The relation a client `sqlite_master` reference is replaced with. +/// +/// `substr(...)` rather than `LIKE '__pgsqlite_%'` because `_` is a +/// single-character wildcard in LIKE, which would also match unrelated names. +/// The `tbl_name` half is what hides the index rows whose own names carry no +/// `__pgsqlite_` prefix (`idx_enum_values_label`, `sqlite_autoindex___pgsqlite_schema_1`). +const FILTERED_RELATION_SQL: &str = "SELECT * FROM sqlite_master \ + WHERE substr(name, 1, 11) <> '__pgsqlite_' \ + AND (tbl_name IS NULL OR substr(tbl_name, 1, 11) <> '__pgsqlite_')"; + +/// Rewrites client references to `sqlite_master` / `sqlite_schema` so that +/// pgsqlite's own `__pgsqlite_*` objects are not listed. +/// +/// This type is deliberately pure: it does not read configuration. Callers gate +/// it on `crate::config::hide_internal_tables()`, and it must only ever be +/// invoked on queries that arrived from a client over the wire. +pub struct SqliteMasterFilter; + +impl SqliteMasterFilter { + /// Cheap allocation-free gate: is there any point parsing this query? + pub fn needs_translation(query: &str) -> bool { + contains_ignore_ascii_case(query, "sqlite_master") + || contains_ignore_ascii_case(query, "sqlite_schema") + } + + /// Returns the rewritten query, or the input unchanged if there is nothing + /// to do or the query cannot be handled. Never returns an error: failing to + /// hide a row is cosmetic, rejecting a client's query is not. + pub fn translate(query: &str) -> Cow<'_, str> { + if !Self::needs_translation(query) { + return Cow::Borrowed(query); + } + + // REMOVED DURING EXECUTION — do not implement this block. It let a + // trailing SQL comment bypass filtering entirely. See "Notes for the + // reviewer" at the end of this plan for the ruling and reasoning. + // + // if query.contains("__pgsqlite_") { + // return Cow::Borrowed(query); + // } + + 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); + } + }; + + let mut visitor = RelationReplacer { replaced: 0 }; + for statement in &mut statements { + let _ = statement.visit(&mut visitor); + } + + if visitor.replaced == 0 { + return Cow::Borrowed(query); + } + + let rewritten = statements + .iter() + .map(|s| s.to_string()) + .collect::>() + .join("; "); + debug!("sqlite_master filter: {query} -> {rewritten}"); + Cow::Owned(rewritten) + } +} + +struct RelationReplacer { + replaced: usize, +} + +impl VisitorMut for RelationReplacer { + type Break = (); + + /// Replacement happens in `post_visit` rather than `pre_visit`: the visitor + /// descends into a node's children between the two, so replacing in + /// `pre_visit` would recurse into the `sqlite_master` reference inside the + /// relation we just substituted, forever. + fn post_visit_table_factor(&mut self, table_factor: &mut TableFactor) -> ControlFlow<()> { + let TableFactor::Table { name, alias, .. } = table_factor else { + return ControlFlow::Continue(()); + }; + + let relation = match name.0.last() { + Some(ObjectNamePart::Identifier(ident)) => ident.value.to_ascii_lowercase(), + _ => return ControlFlow::Continue(()), + }; + if relation != "sqlite_master" && relation != "sqlite_schema" { + return ControlFlow::Continue(()); + } + + // Only `main.` and `temp.` qualify the SQLite catalog. Anything else + // (e.g. an attached database) is left alone. + 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" { + return ControlFlow::Continue(()); + } + } + _ => return ControlFlow::Continue(()), + } + } + + // Keep the client's own alias if it had one, so `m.name` still resolves; + // otherwise alias to the spelling the client used, so `sqlite_schema.name` does. + let effective_alias = alias.clone().unwrap_or(TableAlias { + name: Ident::new(relation), + columns: vec![], + }); + + let Ok(statements) = Parser::parse_sql(&PostgreSqlDialect {}, FILTERED_RELATION_SQL) else { + return ControlFlow::Continue(()); + }; + let Some(Statement::Query(subquery)) = statements.into_iter().next() else { + return ControlFlow::Continue(()); + }; + + *table_factor = TableFactor::Derived { + lateral: false, + subquery, + alias: Some(effective_alias), + }; + self.replaced += 1; + ControlFlow::Continue(()) + } +} + +/// Allocation-free case-insensitive substring test. This runs on every client +/// query while the flag is on, so it must not allocate. +fn contains_ignore_ascii_case(haystack: &str, needle: &str) -> bool { + let haystack = haystack.as_bytes(); + let needle = needle.as_bytes(); + if needle.len() > haystack.len() { + return false; + } + haystack + .windows(needle.len()) + .any(|window| window.eq_ignore_ascii_case(needle)) +} +``` + +- [ ] **Step 5: Export the translator** + +In `src/translator/mod.rs`, add alongside the other module declarations (near :15): + +```rust +mod sqlite_master_filter; +``` + +and alongside the other re-exports (near :44): + +```rust +pub use sqlite_master_filter::SqliteMasterFilter; +``` + +- [ ] **Step 6: Run tests to verify they pass** + +Run: `cargo test --lib sqlite_master_filter` +Expected: PASS, 12 tests. + +- [ ] **Step 7: Commit** + +```bash +cargo check && cargo clippy && cargo build && cargo test --lib +git add Cargo.toml Cargo.lock src/translator/sqlite_master_filter.rs src/translator/mod.rs +git commit -m "feat(translator): add SqliteMasterFilter relation substitution (#80)" +``` + +--- + +### Task 3: Simple-protocol hook and wire-level tests + +**Files:** +- Modify: `src/query/executor.rs:33-40` (`preprocess_query`) +- Create: `tests/sqlite_master_filter_enabled_test.rs` +- Create: `tests/sqlite_master_filter_disabled_test.rs` + +**Interfaces:** +- Consumes: `crate::config::hide_internal_tables()` (Task 1), `crate::translator::SqliteMasterFilter::translate` (Task 2). +- Produces: filtered behaviour on the simple query protocol, exercised via `tokio_postgres::Client::simple_query`. Task 4 extends both test files with extended-protocol cases. + +Each test file is its own binary, so `set_hide_internal_tables(true)` in one cannot race the other. Do not merge them into a single file. + +- [ ] **Step 1: Write the failing tests** + +Create `tests/sqlite_master_filter_enabled_test.rs`: + +```rust +mod common; +use common::*; +use tokio_postgres::SimpleQueryMessage; + +async fn table_names(client: &tokio_postgres::Client, sql: &str) -> Vec { + client + .simple_query(sql) + .await + .unwrap() + .into_iter() + .filter_map(|m| match m { + SimpleQueryMessage::Row(row) => row.get(0).map(str::to_string), + _ => None, + }) + .collect() +} + +#[tokio::test] +async fn hides_internal_objects_from_simple_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?; + db.execute("CREATE INDEX idx_customers_name ON customers(name)").await?; + // Matches LIKE '__pgsqlite_%' but not substr(name, 1, 11) — must stay visible. + db.execute("CREATE TABLE abpgsqliteX (id INTEGER PRIMARY KEY)").await?; + Ok(()) + }) + }) + .await; + + let names = table_names( + &server.client, + "SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY name", + ) + .await; + assert!( + !names.iter().any(|n| n.starts_with("__pgsqlite_")), + "internal tables leaked: {names:?}" + ); + assert!(names.iter().any(|n| n == "customers"), "user table missing: {names:?}"); + assert!(names.iter().any(|n| n == "abpgsqliteX"), "LIKE-wildcard false positive: {names:?}"); + + // Indexes owned by internal tables carry no __pgsqlite_ prefix of their own + // and must be caught via tbl_name. + let indexes = table_names( + &server.client, + "SELECT name FROM sqlite_master WHERE type = 'index' ORDER BY name", + ) + .await; + assert!( + !indexes.iter().any(|n| n.starts_with("idx_enum_") || n.contains("__pgsqlite_")), + "internal indexes leaked: {indexes:?}" + ); + assert!( + indexes.iter().any(|n| n == "idx_customers_name"), + "user index missing: {indexes:?}" + ); + + // The DDL projection must not dump internal CREATE TABLE statements. + let ddl = table_names(&server.client, "SELECT sql FROM sqlite_master WHERE type = 'table'").await; + assert!( + !ddl.iter().any(|s| s.contains("__pgsqlite_")), + "internal DDL leaked" + ); + + // sqlite_schema is an alias for the same relation. + let via_alias = table_names( + &server.client, + "SELECT name FROM sqlite_schema WHERE type = 'table' ORDER BY name", + ) + .await; + assert!(!via_alias.iter().any(|n| n.starts_with("__pgsqlite_"))); + + // Aggregates must count only visible rows. + let counts = table_names(&server.client, "SELECT count(*) FROM sqlite_master WHERE type = 'table'").await; + assert_eq!(counts.len(), 1); + let visible: usize = counts[0].parse().unwrap(); + assert_eq!(visible, names.len(), "count disagrees with the listing"); + + pgsqlite::config::set_hide_internal_tables(false); +} +``` + +Create `tests/sqlite_master_filter_disabled_test.rs`: + +```rust +mod common; +use common::*; +use tokio_postgres::SimpleQueryMessage; + +async fn table_names(client: &tokio_postgres::Client, sql: &str) -> Vec { + client + .simple_query(sql) + .await + .unwrap() + .into_iter() + .filter_map(|m| match m { + SimpleQueryMessage::Row(row) => row.get(0).map(str::to_string), + _ => None, + }) + .collect() +} + +#[tokio::test] +async fn default_still_shows_internal_objects() { + // 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; + + let names = table_names( + &server.client, + "SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY name", + ) + .await; + assert!( + names.iter().any(|n| n == "__pgsqlite_schema"), + "default behaviour changed — internal tables should still be visible: {names:?}" + ); + assert!(names.iter().any(|n| n == "customers")); +} +``` + +- [ ] **Step 2: Run tests to verify the enabled one fails** + +Run: `cargo test --test sqlite_master_filter_enabled_test` +Expected: FAIL — `internal tables leaked: [...]`, because nothing calls the translator yet. + +Run: `cargo test --test sqlite_master_filter_disabled_test` +Expected: PASS already — it documents current behaviour. + +- [ ] **Step 3: Write the implementation** + +Replace `preprocess_query` in `src/query/executor.rs:33-40` with: + +```rust +fn preprocess_query(query: &str) -> String { + let query: Cow<'_, str> = if PG_SHOW_ALL_SETTINGS_PATTERN.is_match(query) { + Cow::Owned(PG_SHOW_ALL_SETTINGS_PATTERN.replace_all(query, "pg_settings").to_string()) + } else { + Cow::Borrowed(query) + }; + + if crate::config::hide_internal_tables() { + crate::translator::SqliteMasterFilter::translate(&query).into_owned() + } else { + query.into_owned() + } +} +``` + +`Cow` is already imported in this file (it is the return type of `create_command_tag` at :151). `preprocess_query` is called from `execute_single_statement` at :281, ahead of the SELECT branch at :326 and ahead of the wire-protocol cache, so the cache is keyed on the rewritten text. + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cargo test --test sqlite_master_filter_enabled_test --test sqlite_master_filter_disabled_test` +Expected: PASS, both. + +- [ ] **Step 5: Verify pgsqlite's own bookkeeping still works** + +The risk this guards is migrations re-running because an internal probe stopped seeing `__pgsqlite_metadata`. + +Run: `cargo test migration` +Expected: PASS. + +Run: `cargo test --test sqlite_master_filter_enabled_test` +Expected: PASS. Do **not** add `-- --nocapture` to any integration test in this repository: creating a session initializes lazy statics that dereference `config::CONFIG` (`src/session/state.rs:15`, `src/cache/statement_pool.rs:35`, `src/cache/execution.rs:130`, `src/cache/result_cache.rs:246`), and `CONFIG` runs `Config::parse()` against the test binary's argv, which rejects `--nocapture` and aborts the process. + +The end-to-end proof that internal probes are unaffected is the two-restart manual run in Task 4, Step 6: the second server start opens a database whose migrations are already applied, with the flag on. + +- [ ] **Step 6: Commit** + +```bash +cargo check && cargo clippy && cargo build && cargo test +git add src/query/executor.rs tests/sqlite_master_filter_enabled_test.rs tests/sqlite_master_filter_disabled_test.rs +git commit -m "feat(query): filter sqlite_master on the simple protocol (#80)" +``` + +--- + +### Task 4: Extended-protocol hook and documentation + +**Files:** +- Modify: `src/query/extended.rs:82` (`handle_parse`) +- Modify: `tests/sqlite_master_filter_enabled_test.rs` +- Modify: `tests/sqlite_master_filter_disabled_test.rs` +- Modify: `docs/configuration.md:15-25` (Server Options table) +- Modify: `README.md:135-140` (Essential Options block) + +**Interfaces:** +- Consumes: `crate::config::hide_internal_tables()` (Task 1), `crate::translator::SqliteMasterFilter::translate` (Task 2). +- Produces: the completed feature. Nothing depends on this task. + +`tokio_postgres::Client::query` uses Parse/Bind/Execute, so it exercises this hook, while `simple_query` from Task 3 exercises the other one. Both are needed for full coverage. + +- [ ] **Step 1: Write the failing tests** + +Append to `tests/sqlite_master_filter_enabled_test.rs`: + +```rust +#[tokio::test] +async fn hides_internal_objects_from_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 orders (id INTEGER PRIMARY KEY, total TEXT)").await?; + Ok(()) + }) + }) + .await; + + // client.query() goes through Parse/Bind/Execute, not simple query. + let rows = server + .client + .query("SELECT name FROM sqlite_master WHERE type = 'table' 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 over extended protocol: {names:?}" + ); + assert!(names.iter().any(|n| n == "orders"), "user table missing: {names:?}"); + + pgsqlite::config::set_hide_internal_tables(false); +} +``` + +Append to `tests/sqlite_master_filter_disabled_test.rs`: + +```rust +#[tokio::test] +async fn default_still_shows_internal_objects_on_extended_protocol() { + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE orders (id INTEGER PRIMARY KEY)").await?; + Ok(()) + }) + }) + .await; + + let rows = server + .client + .query("SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY name", &[]) + .await + .unwrap(); + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + assert!( + names.iter().any(|n| n == "__pgsqlite_schema"), + "default behaviour changed on extended protocol: {names:?}" + ); +} +``` + +- [ ] **Step 2: Run tests to verify the enabled one fails** + +Run: `cargo test --test sqlite_master_filter_enabled_test hides_internal_objects_from_extended_protocol` +Expected: FAIL — `internal tables leaked over extended protocol: [...]`. + +- [ ] **Step 3: Write the implementation** + +In `src/query/extended.rs`, at the very top of `handle_parse`'s body — before the `info!("PARSE: Starting parse...")` line at :93, so that the prepared-statement cache at :96 stores and compares the rewritten text: + +```rust + let query = if crate::config::hide_internal_tables() { + crate::translator::SqliteMasterFilter::translate(&query).into_owned() + } else { + query + }; +``` + +This shadows the owned `query: String` parameter. It must sit outside the `#[cfg(not(feature = "unified_processor"))]` translation block at :420-450, so the filter is never feature-gated. + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cargo test --test sqlite_master_filter_enabled_test --test sqlite_master_filter_disabled_test` +Expected: PASS, 4 tests total. + +- [ ] **Step 5: Document the flag** + +In `docs/configuration.md`, add a row to the Server Options table after the `No TCP` row (:25): + +```markdown +| 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`. | +``` + +In `README.md`, add to the "Basic options" block (after `--in-memory` at :140): + +```bash + --hide-internal-tables # Hide pgsqlite's __pgsqlite_* tables from sqlite_master listings +``` + +- [ ] **Step 6: Full verification** + +Run: `cargo check && cargo clippy && cargo build && cargo test` +Expected: no errors, no new warnings, all tests pass. + +Then reproduce the issue's own scenario manually, both ways: + +```bash +cargo build +rm -f /tmp/t.sqlite +./target/debug/pgsqlite --database /tmp/t.sqlite --port 5599 & +psql "host=127.0.0.1 port=5599 user=postgres dbname=main gssencmode=disable" -c "CREATE TABLE customers (id INTEGER PRIMARY KEY, name TEXT);" +psql "host=127.0.0.1 port=5599 user=postgres dbname=main gssencmode=disable" -tA -c "SELECT name FROM sqlite_master WHERE type='table' ORDER BY name;" +# Expected: internal tables present (default off) +kill %1 + +./target/debug/pgsqlite --database /tmp/t.sqlite --port 5599 --hide-internal-tables & +psql "host=127.0.0.1 port=5599 user=postgres dbname=main gssencmode=disable" -tA -c "SELECT name FROM sqlite_master WHERE type='table' ORDER BY name;" +# Expected: only "customers" +psql "host=127.0.0.1 port=5599 user=postgres dbname=main gssencmode=disable" -tA -c "SELECT COUNT(*) FROM __pgsqlite_schema;" +# Expected: a number — hidden from listings, still queryable by name +kill %1 +``` + +- [ ] **Step 7: Commit** + +```bash +git add src/query/extended.rs tests/sqlite_master_filter_enabled_test.rs tests/sqlite_master_filter_disabled_test.rs docs/configuration.md README.md +git commit -m "feat(query): filter sqlite_master on the extended protocol, document flag (#80)" +``` + +--- + +## Notes for the reviewer + +- **`\dt` and `information_schema.tables`** still list the materialized `pg_*` / `information_schema_*` relations, and `\dt` reports "Did not find any tables" on a database that has one. Both are pre-existing and out of scope; file separately. +- **PR #83** implements the same issue by filtering result rows. This plan supersedes it. When closing, the reasoning is in the spec's "Mechanism" section: row filtering cannot see `count(*)`, joins, subqueries, or `SELECT sql`, and fails open silently on all of them. +- **The idempotence guard was removed during execution.** Task 2's plan text specified an early return when the query already contained `__pgsqlite_`. Review found it disabled filtering for any query containing that literal anywhere, including a comment (`SELECT name FROM sqlite_master -- __pgsqlite_`). Ruling: removed. It was not load-bearing — `SELECT * FROM __pgsqlite_schema` never reaches it, since that query has no `sqlite_master` reference for `needs_translation` to match, so "hidden from listings, still queryable by name" holds without it. Double application, if both hooks ever fire on one query, nests one derived table inside another: identical rows, bounded at two passes. diff --git a/docs/superpowers/specs/2026-08-05-hide-internal-tables-design.md b/docs/superpowers/specs/2026-08-05-hide-internal-tables-design.md new file mode 100644 index 00000000..0b428500 --- /dev/null +++ b/docs/superpowers/specs/2026-08-05-hide-internal-tables-design.md @@ -0,0 +1,178 @@ +# Hide internal `__pgsqlite_*` tables behind a flag + +Issue: [#80](https://github.com/erans/pgsqlite/issues/80) +Date: 2026-08-05 +Status: Approved, ready for implementation planning + +## Problem + +pgsqlite stores its bookkeeping in tables named `__pgsqlite_*`. Every catalog +query pgsqlite synthesizes itself already filters them out, but a +`sqlite_master` (or `sqlite_schema`) query written directly by a client is +passed through verbatim. Schema browsers, ORMs, and any tool that lists the +native SQLite catalog therefore see pgsqlite's internals mixed in with the +user's own tables. The reporter runs a hosted SQLite product on top of pgsqlite +and has had users conclude their data was corrupted. + +### Observed leak (v0.0.22, fresh database with one user table) + +A client `SELECT type, name FROM sqlite_master` returns 37 internal rows +alongside `customers`: + +| Kind | Count | Examples | +| --- | --- | --- | +| `__pgsqlite_*` tables | 15 | `__pgsqlite_schema`, `__pgsqlite_migrations`, `__pgsqlite_enum_values` | +| Indexes without the prefix | 8 | `idx_enum_values_label`, `idx_comments_lookup` | +| Implicit unique indexes | 14 | `sqlite_autoindex___pgsqlite_schema_1` | + +The last two groups matter: a `name LIKE '__pgsqlite_%'` test matches only 15 of +the 37 rows. All 37 are caught by additionally testing `tbl_name`, which every +index row in `sqlite_master` carries. + +## Scope + +Hidden: `__pgsqlite_*` tables and the index rows owned by them. Nothing else. + +Not hidden, and not addressed by this work: + +- The materialized `pg_*` and `information_schema_*` relations (4 real tables, + 24 views) that pgsqlite creates. Filtering those is a broader policy question + because a user may legitimately name a table `pg_something`. +- `PRAGMA table_list`, which returns the internal tables to a client that asks + for it. It is a fixed-shape result needing a different mechanism (row + filtering), and the tools causing the reported confusion read `sqlite_master`. + +"Hidden" means hidden from listings only. `SELECT * FROM __pgsqlite_schema` +continues to work when the table is named explicitly, so a live deployment +remains debuggable over the wire and pgsqlite's own access is unaffected. + +## Two problems found while scoping, filed separately + +Neither is caused by this change and neither is fixed by it. + +1. `\dt` reports "Did not find any tables" on a database that has a user table. +2. `information_schema.tables` lists all 27 materialized `pg_*` / + `information_schema_*` relations next to the user's tables. + +## Flag + +```rust +#[arg(long, env = "PGSQLITE_HIDE_INTERNAL_TABLES", + help = "Hide pgsqlite's internal __pgsqlite_* tables from client sqlite_master queries")] +pub hide_internal_tables: bool, +``` + +Added to `src/config.rs` alongside the existing options and read through the +existing `CONFIG` lazy_static. Default false: internal tables stay visible +unless the operator opts in. Server-wide and fixed at startup; there is no +per-session override, because pgsqlite has no GUC/`SET` plumbing today +(`__pgsqlite_session_settings` exists as a migration artifact but nothing reads +it, and no `Statement::SetVariable` handler exists), and building it is a +feature in its own right. + +## Mechanism: relation substitution + +A new `src/translator/sqlite_master_filter.rs` exposes + +```rust +pub fn translate(query: &str) -> Cow<'_, str> +``` + +following the shape of the existing `SchemaPrefixTranslator`. + +**Gate, cheapest test first.** Return the input borrowed if the flag is off, or +if the lowercased query contains neither `sqlite_master` nor `sqlite_schema`. +Only past that gate is the query parsed. + +**Rewrite.** Parse with `PostgreSqlDialect`. Walk the statement for every +`TableFactor::Table` naming `sqlite_master` or `sqlite_schema`, bare or +qualified with `main` or `temp`, wherever it appears: joins, subqueries, CTEs, +set operations, `EXISTS`. Replace each such relation with a derived table: + +```sql +(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 +``` + +If the client gave the relation its own alias, that alias is preserved instead +so qualified references such as `m.name` keep resolving. + +Two details behind the predicate: + +- `substr(...) <> '__pgsqlite_'` rather than `LIKE '__pgsqlite_%'`. In `LIKE`, + `_` is a single-character wildcard, so the `LIKE` form also matches unrelated + names. Existing code in the repository uses the `LIKE` form; new code should + not. +- The `tbl_name` half is what hides the 22 index rows whose own names carry no + `__pgsqlite_` prefix. + +**Why substitution rather than the alternatives.** The client's WHERE clause, +projections, and joins are never modified, so there is no predicate to splice +and nothing to corrupt — the failure mode that sank the earlier query-rewriting +attempt (PR #82) on large `NOT IN` lists and `ESCAPE` clauses. Because the +filtering happens inside the relation, `count(*)`, joins, subqueries, `EXISTS`, +and `SELECT sql FROM sqlite_master` are all correct with no extra handling. + +The alternative considered and rejected was filtering result rows after running +the query verbatim (PR #83). It cannot corrupt SQL and is already written, but +it sees only what the client projected, needs a probe query per call to map +`name` to `tbl_name`, and silently returns unfiltered results for `count(*)`, +joins, subqueries, and `SELECT sql FROM sqlite_master`. A schema browser +displaying DDL is an ordinary thing to do and would dump every internal +`CREATE TABLE` unfiltered. For a flag whose contract is "hidden", silent partial +coverage is the wrong trade. + +Also rejected: moving the internal tables into an ATTACHed sidecar database. +It is the only approach that also holds when someone opens the file directly +with `sqlite3`, but it makes a pgsqlite database two files instead of one and +requires a large migration. + +## Hook points + +The rewrite applies to client queries only. + +| Protocol | Location | +| --- | --- | +| Simple | `src/query/executor.rs:281`, in or immediately after `preprocess_query` within `execute_single_statement` — ahead of the SELECT branch at :326 and ahead of the wire-protocol cache | +| Extended | `src/query/extended.rs`, top of `handle_parse` (:82), before the `#[cfg(not(feature = "unified_processor"))]` translation block at :420-450 so the filter is not feature-gated | + +The filter must not be placed at `DbHandler::process_query` +(`src/session/db_handler.rs:485`), despite that being the single chokepoint all +queries pass through. pgsqlite's own code probes `sqlite_master` for its +internal tables in at least `migration/runner.rs:61,302,310,339`, +`metadata/enum_metadata.rs:203,341,389`, `rewriter/enum_rewriter.rs:26`, +`cache/lazy_schema_loader.rs:124`, and `cache/schema.rs:117`. Filtering there +would make `SELECT 1 FROM sqlite_master WHERE name='__pgsqlite_metadata'` return +nothing and pgsqlite would re-run its migrations on every start. + +## Failure handling + +Fail open. A parse failure, or any AST shape the walker does not recognize, +returns the query unchanged rather than raising an error: failing to hide a row +is cosmetic, rejecting a client's query is not. + +The AST is re-rendered through sqlparser's `Display` only when a `sqlite_master` +reference was actually found and replaced; every other query is returned +borrowed and untouched. Round-tripping drops comments and may normalize exotic +syntax, which is acceptable for the narrow class of queries that reference +`sqlite_master`. + +## Tests + +Unit tests on the translator: + +- Rewrite shapes: bare relation, aliased relation, `main.`/`temp.` qualified, + join, subquery, CTE, `count(*)`, `SELECT sql`, `sqlite_schema` spelling. +- No-op cases: flag off, no `sqlite_master` reference, unparseable input. +- Predicate correctness: a user table named `abpgsqliteX`, which + `LIKE '__pgsqlite_%'` matches but `substr(name, 1, 11) = '__pgsqlite_'` does + not, is still returned. + +Wire-level integration tests, mirroring the issue's reproduction, run in both +flag states: + +- Flag on: none of the 37 internal rows appear; the user's table, its indexes, + and its views do; `SELECT count(*) FROM sqlite_master` counts only visible + rows; `SELECT sql FROM sqlite_master` returns no internal DDL. +- Flag off: all 37 internal rows still appear, proving the default is unchanged. diff --git a/src/config.rs b/src/config.rs index a80ea966..b24af98d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,5 +1,6 @@ use clap::Parser; use std::env; +use std::sync::atomic::{AtomicBool, Ordering}; #[derive(Parser, Debug, Clone)] #[command(name = "pgsqlite")] @@ -25,6 +26,9 @@ pub struct Config { #[arg(long, env = "PGSQLITE_NO_TCP", help = "Disable TCP listener and use only Unix socket")] pub no_tcp: bool, + #[arg(long, env = "PGSQLITE_HIDE_INTERNAL_TABLES", help = "Hide pgsqlite's internal __pgsqlite_* tables from client sqlite_master queries")] + pub hide_internal_tables: bool, + // Connection pool configuration #[arg(long, env = "PGSQLITE_USE_POOLING", help = "Enable connection pooling with read/write separation")] pub use_pooling: bool, @@ -220,4 +224,37 @@ impl Config { // Global configuration instance lazy_static::lazy_static! { pub static ref CONFIG: Config = Config::load(); +} + +/// Process-global mirror of `Config::hide_internal_tables`, set once at startup. +/// +/// The wire-protocol hooks read this instead of `CONFIG` because dereferencing +/// `CONFIG` calls `Config::parse()` on the current process argv, which aborts +/// any test binary invoked with harness arguments such as `--nocapture`. +static HIDE_INTERNAL_TABLES: AtomicBool = AtomicBool::new(false); + +/// Set the global "hide internal tables" toggle. Called once from `main`. +pub fn set_hide_internal_tables(enabled: bool) { + HIDE_INTERNAL_TABLES.store(enabled, Ordering::Relaxed); +} + +/// Whether client `sqlite_master` queries should have `__pgsqlite_*` objects filtered out. +pub fn hide_internal_tables() -> bool { + HIDE_INTERNAL_TABLES.load(Ordering::Relaxed) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn hide_internal_tables_defaults_off_and_is_settable() { + // Note: this test must never touch CONFIG — Config::parse() would run + // against the test binary's argv and abort the process. + assert!(!hide_internal_tables()); + set_hide_internal_tables(true); + assert!(hide_internal_tables()); + set_hide_internal_tables(false); + assert!(!hide_internal_tables()); + } } \ No newline at end of file diff --git a/src/main.rs b/src/main.rs index 753ca7f5..6da4a955 100644 --- a/src/main.rs +++ b/src/main.rs @@ -28,6 +28,7 @@ use pgsqlite::migration::MigrationRunner; #[tokio::main] async fn main() -> Result<()> { let config = Config::load(); + pgsqlite::config::set_hide_internal_tables(config.hide_internal_tables); // Initialize logging tracing_subscriber::fmt() diff --git a/src/query/executor.rs b/src/query/executor.rs index 79e97406..12fbb4b3 100644 --- a/src/query/executor.rs +++ b/src/query/executor.rs @@ -31,10 +31,16 @@ static SET_CONFIG_PATTERN: Lazy = Lazy::new(|| { }); fn preprocess_query(query: &str) -> String { - if PG_SHOW_ALL_SETTINGS_PATTERN.is_match(query) { - PG_SHOW_ALL_SETTINGS_PATTERN.replace_all(query, "pg_settings").to_string() + let query: Cow<'_, str> = if PG_SHOW_ALL_SETTINGS_PATTERN.is_match(query) { + Cow::Owned(PG_SHOW_ALL_SETTINGS_PATTERN.replace_all(query, "pg_settings").to_string()) } else { - query.to_string() + Cow::Borrowed(query) + }; + + if crate::config::hide_internal_tables() { + crate::translator::SqliteMasterFilter::translate(&query).into_owned() + } else { + query.into_owned() } } diff --git a/src/query/extended.rs b/src/query/extended.rs index 0ca2db03..cc0d9323 100644 --- a/src/query/extended.rs +++ b/src/query/extended.rs @@ -90,6 +90,11 @@ impl ExtendedQueryHandler { where T: tokio::io::AsyncRead + tokio::io::AsyncWrite + Unpin, { + let query = if crate::config::hide_internal_tables() { + crate::translator::SqliteMasterFilter::translate(&query).into_owned() + } else { + query + }; info!("PARSE: Starting parse for statement '{}', query: {}", name, query); // Fast path: Check if we already have this prepared statement // This avoids re-parsing the same query multiple times diff --git a/src/security/sql_injection_detector.rs b/src/security/sql_injection_detector.rs index 2d4cbeaf..b81d7f80 100644 --- a/src/security/sql_injection_detector.rs +++ b/src/security/sql_injection_detector.rs @@ -282,7 +282,17 @@ impl SqlInjectionDetector { } } TableFactor::Derived { subquery, .. } => { - self.analyze_query_statement(subquery, analysis, depth + 1, original_query)?; + // `--hide-internal-tables` rewrites a client `sqlite_master` + // reference into a derived table of our own making. That wrapper + // is pgsqlite's, not attacker-supplied nesting, so it must not + // push the relation inside it past the `depth > 1` rule above. + // Client-written subqueries still increment as before. + let nested_depth = if crate::translator::is_generated_filter_subquery(subquery) { + depth + } else { + depth + 1 + }; + self.analyze_query_statement(subquery, analysis, nested_depth, original_query)?; } _ => {} } @@ -608,4 +618,53 @@ mod tests { let result = detector.analyze_query("SELECT * FROM users UNION SELECT * FROM pg_user"); assert!(result.is_err()); } + + /// Regression: `--hide-internal-tables` rewrites client `sqlite_master` + /// queries into a derived table. That extra nesting used to trip the + /// `depth > 1` system-table rule, so an ordinary schema listing over the + /// extended protocol in text result format logged a HIGH-severity false + /// alert on its way through `execute_with_params` -> `validate_sql_security`. + #[test] + fn test_generated_sqlite_master_filter_is_not_an_injection() { + let detector = SqlInjectionDetector::new(); + + let listings = [ + "SELECT name FROM sqlite_master WHERE type='table' ORDER BY name", + "SELECT count(*) FROM sqlite_master", + "SELECT sql FROM sqlite_master WHERE type='table'", + "SELECT name FROM sqlite_schema WHERE type='index'", + ]; + + for listing in &listings { + // Build the rewritten form the exact same way the wire hooks do, + // rather than hardcoding it, so the two can never drift apart. + let rewritten = crate::translator::SqliteMasterFilter::translate(listing).into_owned(); + assert_ne!(&rewritten, listing, "expected {listing} to be rewritten"); + assert!( + detector.analyze_query(&rewritten).is_ok(), + "rewritten listing must be accepted: {rewritten}" + ); + } + } + + /// The fix above must not blunt the real check: nesting a system table + /// inside a subquery the *client* wrote is still suspicious. + #[test] + fn test_client_nested_system_table_access_is_still_rejected() { + let detector = SqlInjectionDetector::new(); + + let nested = [ + "SELECT * FROM (SELECT name FROM sqlite_master) z", + "SELECT * FROM (SELECT * FROM pg_user) z", + // Same wrapper shape as ours but a different predicate: not ours. + "SELECT * FROM (SELECT * FROM sqlite_master WHERE name <> 'x') z", + ]; + + for query in &nested { + assert!( + detector.analyze_query(query).is_err(), + "client-nested system table access must still be rejected: {query}" + ); + } + } } \ No newline at end of file diff --git a/src/translator/mod.rs b/src/translator/mod.rs index 96e3b3ad..fed666cb 100644 --- a/src/translator/mod.rs +++ b/src/translator/mod.rs @@ -28,6 +28,7 @@ mod function_parentheses_translator; mod catalog_function_translator; mod pg_table_is_visible_translator; mod session_identifier_translator; +mod sqlite_master_filter; pub use json_translator::JsonTranslator; pub use returning_translator::ReturningTranslator; @@ -56,4 +57,6 @@ pub use query_analyzer::{QueryAnalyzer, TranslationFlags}; pub use function_parentheses_translator::FunctionParenthesesTranslator; pub use catalog_function_translator::CatalogFunctionTranslator; pub use pg_table_is_visible_translator::PgTableIsVisibleTranslator; -pub use session_identifier_translator::SessionIdentifierTranslator; \ No newline at end of file +pub use session_identifier_translator::SessionIdentifierTranslator; +pub use sqlite_master_filter::SqliteMasterFilter; +pub(crate) use sqlite_master_filter::is_generated_filter_subquery; \ No newline at end of file diff --git a/src/translator/sqlite_master_filter.rs b/src/translator/sqlite_master_filter.rs new file mode 100644 index 00000000..67543fc2 --- /dev/null +++ b/src/translator/sqlite_master_filter.rs @@ -0,0 +1,397 @@ +use std::borrow::Cow; +use std::ops::ControlFlow; +use std::sync::LazyLock; + +use sqlparser::ast::{ + Ident, ObjectNamePart, Query, Statement, TableAlias, TableFactor, VisitMut, VisitorMut, +}; +use sqlparser::dialect::PostgreSqlDialect; +use sqlparser::parser::Parser; +use tracing::debug; + +/// The relation a client `sqlite_master` reference is replaced with. +/// +/// `substr(...)` rather than `LIKE '__pgsqlite_%'` because `_` is a +/// single-character wildcard in LIKE, which would also match unrelated names. +/// The `tbl_name` half is what hides the index rows whose own names carry no +/// `__pgsqlite_` prefix (`idx_enum_values_label`, `sqlite_autoindex___pgsqlite_schema_1`). +const FILTERED_RELATION_SQL: &str = "SELECT * FROM sqlite_master \ + WHERE substr(name, 1, 11) <> '__pgsqlite_' \ + AND (tbl_name IS NULL OR substr(tbl_name, 1, 11) <> '__pgsqlite_')"; + +/// [`FILTERED_RELATION_SQL`] parsed once, so each rewrite and each recognition +/// check reuses the same shape. +fn parse_filtered_relation() -> Option> { + let statements = Parser::parse_sql(&PostgreSqlDialect {}, FILTERED_RELATION_SQL).ok()?; + match statements.into_iter().next() { + Some(Statement::Query(query)) => Some(query), + _ => None, + } +} + +/// How sqlparser renders [`FILTERED_RELATION_SQL`] back out. Comparing against +/// this is what lets us recognize our own generated relation in a query that +/// has already been rewritten and re-parsed. +static FILTERED_RELATION_RENDERED: LazyLock = + LazyLock::new(|| parse_filtered_relation().map(|q| q.to_string()).unwrap_or_default()); + +/// Is `query` the subquery this translator substitutes for a client +/// `sqlite_master` reference? +/// +/// The SQL injection detector uses this so that pgsqlite's own wrapper is not +/// counted as attacker-supplied subquery nesting. Recognizing it cannot be +/// abused: a client that reproduces this exact subquery gets the filtered +/// relation, which is precisely what the flag exists to hand them. +pub(crate) fn is_generated_filter_subquery(query: &Query) -> bool { + let rendered = &*FILTERED_RELATION_RENDERED; + !rendered.is_empty() && query.to_string() == *rendered +} + +/// Rewrites client references to `sqlite_master` / `sqlite_schema` so that +/// pgsqlite's own `__pgsqlite_*` objects are not listed. +/// +/// This type is deliberately pure: it does not read configuration. Callers gate +/// it on `crate::config::hide_internal_tables()`, and it must only ever be +/// invoked on queries that arrived from a client over the wire. +pub struct SqliteMasterFilter; + +impl SqliteMasterFilter { + /// Cheap allocation-free gate: is there any point parsing this query? + fn needs_translation(query: &str) -> bool { + contains_ignore_ascii_case(query, "sqlite_master") + || contains_ignore_ascii_case(query, "sqlite_schema") + } + + /// Returns the rewritten query, or the input unchanged if there is nothing + /// to do or the query cannot be handled. Never returns an error: failing to + /// hide a row is cosmetic, rejecting a client's query is not. + pub fn translate(query: &str) -> Cow<'_, str> { + if !Self::needs_translation(query) { + return Cow::Borrowed(query); + } + + 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); + } + }; + + 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. + 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); + } + } + _ => {} + } + } + + if visitor.replaced == 0 { + return Cow::Borrowed(query); + } + + let rewritten = statements + .iter() + .map(|s| s.to_string()) + .collect::>() + .join("; "); + debug!("sqlite_master filter: {query} -> {rewritten}"); + Cow::Owned(rewritten) + } +} + +struct RelationReplacer { + replaced: usize, +} + +impl VisitorMut for RelationReplacer { + type Break = (); + + /// Replacement happens in `post_visit` rather than `pre_visit`: the visitor + /// descends into a node's children between the two, so replacing in + /// `pre_visit` would recurse into the `sqlite_master` reference inside the + /// relation we just substituted, forever. + fn post_visit_table_factor(&mut self, table_factor: &mut TableFactor) -> ControlFlow<()> { + let TableFactor::Table { name, alias, .. } = table_factor else { + return ControlFlow::Continue(()); + }; + + let relation = match name.0.last() { + Some(ObjectNamePart::Identifier(ident)) => ident.value.to_ascii_lowercase(), + _ => return ControlFlow::Continue(()), + }; + if relation != "sqlite_master" && relation != "sqlite_schema" { + return ControlFlow::Continue(()); + } + + // Only `main.` and `temp.` qualify the SQLite catalog. Anything else + // (e.g. an attached database) is left alone. + 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" { + return ControlFlow::Continue(()); + } + } + _ => return ControlFlow::Continue(()), + } + } + + // Keep the client's own alias if it had one, so `m.name` still resolves; + // otherwise alias to the spelling the client used, so `sqlite_schema.name` does. + let effective_alias = alias.clone().unwrap_or(TableAlias { + name: Ident::new(relation), + columns: vec![], + }); + + let Some(subquery) = parse_filtered_relation() else { + return ControlFlow::Continue(()); + }; + + *table_factor = TableFactor::Derived { + lateral: false, + subquery, + alias: Some(effective_alias), + }; + self.replaced += 1; + ControlFlow::Continue(()) + } +} + +/// Allocation-free case-insensitive substring test. This runs on every client +/// query while the flag is on, so it must not allocate. +fn contains_ignore_ascii_case(haystack: &str, needle: &str) -> bool { + let haystack = haystack.as_bytes(); + let needle = needle.as_bytes(); + if needle.len() > haystack.len() { + return false; + } + haystack + .windows(needle.len()) + .any(|window| window.eq_ignore_ascii_case(needle)) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The filtered relation as sqlparser renders it back out (note: uppercase SUBSTR). + const FILTERED: &str = "(SELECT * FROM sqlite_master WHERE SUBSTR(name, 1, 11) <> '__pgsqlite_' AND (tbl_name IS NULL OR SUBSTR(tbl_name, 1, 11) <> '__pgsqlite_'))"; + + fn rewritten(query: &str) -> String { + SqliteMasterFilter::translate(query).into_owned() + } + + #[test] + fn rewrites_bare_relation_and_keeps_client_predicate() { + let out = rewritten("SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY name"); + assert_eq!( + out, + format!("SELECT name FROM {FILTERED} AS sqlite_master WHERE type = 'table' ORDER BY name") + ); + } + + #[test] + fn rewrites_aggregate() { + let out = rewritten("SELECT count(*) FROM sqlite_master"); + assert_eq!(out, format!("SELECT count(*) FROM {FILTERED} AS sqlite_master")); + } + + #[test] + fn preserves_client_alias_in_join() { + let out = rewritten("SELECT m.name FROM sqlite_schema m JOIN foo f ON f.n = m.name"); + assert_eq!( + out, + format!("SELECT m.name FROM {FILTERED} AS m JOIN foo AS f ON f.n = m.name") + ); + } + + #[test] + fn rewrites_schema_qualified_relation() { + let out = rewritten("SELECT name FROM main.sqlite_master"); + assert_eq!(out, format!("SELECT name FROM {FILTERED} AS sqlite_master")); + } + + #[test] + fn rewrites_inside_cte() { + let out = rewritten("WITH t AS (SELECT name FROM sqlite_master) SELECT * FROM t"); + assert_eq!( + out, + format!("WITH t AS (SELECT name FROM {FILTERED} AS sqlite_master) SELECT * FROM t") + ); + } + + #[test] + fn rewrites_inside_exists_subquery() { + let out = rewritten("SELECT * FROM foo WHERE EXISTS (SELECT 1 FROM sqlite_master WHERE name = foo.n)"); + assert_eq!( + out, + format!("SELECT * FROM foo WHERE EXISTS (SELECT 1 FROM {FILTERED} AS sqlite_master WHERE name = foo.n)") + ); + } + + #[test] + fn rewrites_select_sql_projection() { + let out = rewritten("SELECT sql FROM sqlite_master WHERE type = 'table'"); + assert_eq!( + out, + format!("SELECT sql FROM {FILTERED} AS sqlite_master WHERE type = 'table'") + ); + } + + #[test] + fn does_not_corrupt_complex_predicates() { + // This is the shape that broke the earlier query-rewriting attempt (PR #82). + let out = rewritten(r"SELECT name FROM sqlite_master WHERE name NOT IN ('a', 'b') AND name LIKE 'x%' ESCAPE '\'"); + assert_eq!( + out, + format!(r"SELECT name FROM {FILTERED} AS sqlite_master WHERE name NOT IN ('a', 'b') AND name LIKE 'x%' ESCAPE '\'") + ); + } + + #[test] + fn leaves_unrelated_queries_borrowed() { + assert!(matches!( + SqliteMasterFilter::translate("SELECT * FROM customers"), + Cow::Borrowed(_) + )); + } + + #[test] + fn leaves_unparseable_input_borrowed() { + assert!(matches!( + SqliteMasterFilter::translate("SELECT FROM WHERE sqlite_master ((("), + Cow::Borrowed(_) + )); + } + + #[test] + fn filters_listing_queries_that_name_an_internal_table() { + // A listing query is filtered the same way regardless of what its WHERE + // clause happens to test for; naming an internal table in a predicate + // doesn't opt the query out of the rewrite. + let out = rewritten("SELECT name FROM sqlite_master WHERE name = '__pgsqlite_schema'"); + assert_eq!( + out, + format!("SELECT name FROM {FILTERED} AS sqlite_master WHERE name = '__pgsqlite_schema'") + ); + } + + #[test] + fn filters_despite_internal_prefix_in_a_comment() { + // Regression guard: the internal-prefix literal appearing anywhere in the + // query text (e.g. a trailing comment) must never disable the rewrite. + // sqlparser drops comments on the round-trip, so assert on the rewritten + // relation rather than on the comment surviving. + let out = rewritten("SELECT name FROM sqlite_master -- __pgsqlite_"); + assert!(out.contains(FILTERED)); + } + + #[test] + fn does_not_match_unrelated_table_named_like_a_wildcard_match() { + // 'abpgsqliteX' matches LIKE '__pgsqlite_%' but not substr(name,1,11). + // Guard that we generate the substr form, not the LIKE form. + let out = rewritten("SELECT name FROM sqlite_master"); + assert!(out.contains("SUBSTR(name, 1, 11) <> '__pgsqlite_'")); + assert!(!out.contains("LIKE '__pgsqlite_")); + } + + #[test] + fn leaves_attached_database_qualifier_alone() { + // Only `main.` and `temp.` name the SQLite catalog; `otherdb.sqlite_master` + // belongs to an ATTACHed database and is none of our business. + assert!(matches!( + SqliteMasterFilter::translate("SELECT name FROM otherdb.sqlite_master"), + Cow::Borrowed(_) + )); + assert_eq!( + rewritten("SELECT name FROM otherdb.sqlite_master"), + "SELECT name FROM otherdb.sqlite_master" + ); + } + + #[test] + fn leaves_delete_untouched() { + // Substituting a derived table for the DELETE target yields invalid SQL + // whose error text would leak `__pgsqlite_` to the client. Let SQLite + // reject the write itself, with its own clear diagnostic. + assert!(matches!( + SqliteMasterFilter::translate("DELETE FROM sqlite_master WHERE name = 'zzz'"), + Cow::Borrowed(_) + )); + assert_eq!( + rewritten("DELETE FROM sqlite_master WHERE name = 'zzz'"), + "DELETE FROM sqlite_master WHERE name = 'zzz'" + ); + } + + #[test] + fn leaves_update_untouched() { + assert!(matches!( + SqliteMasterFilter::translate("UPDATE sqlite_master SET name = 'x'"), + Cow::Borrowed(_) + )); + assert_eq!( + rewritten("UPDATE sqlite_master SET name = 'x'"), + "UPDATE sqlite_master SET name = 'x'" + ); + } + + #[test] + fn rewrites_insert_select_source() { + // The read half of an INSERT ... SELECT is still a listing. + let out = rewritten("INSERT INTO t SELECT name FROM sqlite_master"); + assert_eq!( + out, + format!("INSERT INTO t SELECT name FROM {FILTERED} AS sqlite_master") + ); + } + + #[test] + fn recognizes_its_own_generated_subquery() { + // What the SQL injection detector keys on. Parse the rewritten form back + // and confirm the derived table it contains is recognized as ours. + let out = rewritten("SELECT name FROM sqlite_master WHERE type = 'table'"); + let statements = Parser::parse_sql(&PostgreSqlDialect {}, &out).unwrap(); + let Some(Statement::Query(query)) = statements.into_iter().next() else { + panic!("expected a query"); + }; + let sqlparser::ast::SetExpr::Select(select) = &*query.body else { + panic!("expected a select"); + }; + let TableFactor::Derived { subquery, .. } = &select.from[0].relation else { + panic!("expected a derived table"); + }; + assert!(is_generated_filter_subquery(subquery)); + } + + #[test] + fn does_not_recognize_a_client_written_subquery() { + let statements = + Parser::parse_sql(&PostgreSqlDialect {}, "SELECT * FROM (SELECT name FROM sqlite_master) z") + .unwrap(); + let Some(Statement::Query(query)) = statements.into_iter().next() else { + panic!("expected a query"); + }; + let sqlparser::ast::SetExpr::Select(select) = &*query.body else { + panic!("expected a select"); + }; + let TableFactor::Derived { subquery, .. } = &select.from[0].relation else { + panic!("expected a derived table"); + }; + assert!(!is_generated_filter_subquery(subquery)); + } +} diff --git a/tests/sqlite_master_filter_disabled_test.rs b/tests/sqlite_master_filter_disabled_test.rs new file mode 100644 index 00000000..f9c5db3f --- /dev/null +++ b/tests/sqlite_master_filter_disabled_test.rs @@ -0,0 +1,62 @@ +mod common; +use common::*; +use tokio_postgres::SimpleQueryMessage; + +async fn table_names(client: &tokio_postgres::Client, sql: &str) -> Vec { + client + .simple_query(sql) + .await + .unwrap() + .into_iter() + .filter_map(|m| match m { + SimpleQueryMessage::Row(row) => row.get(0).map(str::to_string), + _ => None, + }) + .collect() +} + +#[tokio::test] +async fn default_still_shows_internal_objects() { + // 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; + + let names = table_names( + &server.client, + "SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY name", + ) + .await; + assert!( + names.iter().any(|n| n == "__pgsqlite_schema"), + "default behaviour changed — internal tables should still be visible: {names:?}" + ); + assert!(names.iter().any(|n| n == "customers")); +} + +#[tokio::test] +async fn default_still_shows_internal_objects_on_extended_protocol() { + let server = setup_test_server_with_init(|db| { + Box::pin(async move { + db.execute("CREATE TABLE orders (id INTEGER PRIMARY KEY)").await?; + Ok(()) + }) + }) + .await; + + let rows = server + .client + .query("SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY name", &[]) + .await + .unwrap(); + let names: Vec = rows.iter().map(|r| r.get::<_, String>(0)).collect(); + + assert!( + names.iter().any(|n| n == "__pgsqlite_schema"), + "default behaviour changed on extended protocol: {names:?}" + ); +} diff --git a/tests/sqlite_master_filter_enabled_test.rs b/tests/sqlite_master_filter_enabled_test.rs new file mode 100644 index 00000000..f0729d48 --- /dev/null +++ b/tests/sqlite_master_filter_enabled_test.rs @@ -0,0 +1,138 @@ +mod common; +use common::*; +use tokio_postgres::SimpleQueryMessage; + +async fn table_names(client: &tokio_postgres::Client, sql: &str) -> Vec { + client + .simple_query(sql) + .await + .unwrap() + .into_iter() + .filter_map(|m| match m { + SimpleQueryMessage::Row(row) => row.get(0).map(str::to_string), + _ => None, + }) + .collect() +} + +#[tokio::test] +async fn hides_internal_objects_from_simple_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?; + db.execute("CREATE INDEX idx_customers_name ON customers(name)").await?; + // Matches LIKE '__pgsqlite_%' but not substr(name, 1, 11) — must stay visible. + db.execute("CREATE TABLE abpgsqliteX (id INTEGER PRIMARY KEY)").await?; + Ok(()) + }) + }) + .await; + + let names = table_names( + &server.client, + "SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY name", + ) + .await; + assert!( + !names.iter().any(|n| n.starts_with("__pgsqlite_")), + "internal tables leaked: {names:?}" + ); + assert!(names.iter().any(|n| n == "customers"), "user table missing: {names:?}"); + assert!(names.iter().any(|n| n == "abpgsqliteX"), "LIKE-wildcard false positive: {names:?}"); + + // Indexes owned by internal tables carry no __pgsqlite_ prefix of their own + // and must be caught via tbl_name. + let indexes = table_names( + &server.client, + "SELECT name FROM sqlite_master WHERE type = 'index' ORDER BY name", + ) + .await; + assert!( + !indexes.iter().any(|n| n.starts_with("idx_enum_") || n.contains("__pgsqlite_")), + "internal indexes leaked: {indexes:?}" + ); + assert!( + indexes.iter().any(|n| n == "idx_customers_name"), + "user index missing: {indexes:?}" + ); + + // The DDL projection must not dump internal CREATE TABLE statements. + let ddl = table_names(&server.client, "SELECT sql FROM sqlite_master WHERE type = 'table'").await; + assert!( + !ddl.iter().any(|s| s.contains("__pgsqlite_")), + "internal DDL leaked" + ); + + // sqlite_schema is an alias for the same relation. + let via_alias = table_names( + &server.client, + "SELECT name FROM sqlite_schema WHERE type = 'table' ORDER BY name", + ) + .await; + assert!(!via_alias.iter().any(|n| n.starts_with("__pgsqlite_"))); + + // Aggregates must count only visible rows. + let counts = table_names(&server.client, "SELECT count(*) FROM sqlite_master WHERE type = 'table'").await; + assert_eq!(counts.len(), 1); + let visible: usize = counts[0].parse().unwrap(); + assert_eq!(visible, names.len(), "count disagrees with the listing"); +} + +#[tokio::test] +async fn hides_internal_objects_from_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 orders (id INTEGER PRIMARY KEY, total TEXT)").await?; + Ok(()) + }) + }) + .await; + + // client.query() goes through Parse/Bind/Execute, not simple query. + let rows = server + .client + .query("SELECT name FROM sqlite_master WHERE type = 'table' 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 over extended protocol: {names:?}" + ); + assert!(names.iter().any(|n| n == "orders"), "user table missing: {names:?}"); +} + +#[tokio::test] +async fn write_attempts_on_sqlite_master_keep_sqlites_own_error() { + pgsqlite::config::set_hide_internal_tables(true); + + let server = setup_test_server_with_init(|_| Box::pin(async move { Ok(()) })).await; + + // The rewrite is a read-context rewrite. Substituting a derived table for a + // DELETE/UPDATE target produces invalid SQL whose error text would paste + // `__pgsqlite_` in front of the very user the flag exists to shield. + for sql in [ + "DELETE FROM sqlite_master WHERE name = 'zzz'", + "UPDATE sqlite_master SET name = 'x'", + ] { + let err = server + .client + .simple_query(sql) + .await + .expect_err("writing to sqlite_master must fail"); + let text = err.to_string(); + assert!( + !text.contains("__pgsqlite_"), + "internal prefix leaked into a client-facing error for `{sql}`: {text}" + ); + assert!( + text.contains("may not be modified"), + "expected SQLite's own diagnostic for `{sql}`, got: {text}" + ); + } +} diff --git a/tests/ssl_test.rs b/tests/ssl_test.rs index 7f3c094f..23c69436 100644 --- a/tests/ssl_test.rs +++ b/tests/ssl_test.rs @@ -9,6 +9,7 @@ mod tests { fn test_certificate_generation() { let config = Config { database: ":memory:".to_string(), + hide_internal_tables: false, ssl: true, ssl_cert: None, ssl_key: None, @@ -84,6 +85,7 @@ mod tests { let config = Config { database: db_path.to_string_lossy().to_string(), + hide_internal_tables: false, ssl: true, ssl_cert: None, ssl_key: None, @@ -166,6 +168,7 @@ mod tests { fn test_ssl_disabled_for_unix_sockets() { let config = Config { database: "test.db".to_string(), + hide_internal_tables: false, ssl: true, ssl_cert: None, ssl_key: None,