Skip to content

fix(hide-internal-tables): filter sqlite_master in CREATE VIEW and CTAS bodies - #90

Merged
erans merged 5 commits into
mainfrom
worktree-issue-86-view-ctas
Aug 6, 2026
Merged

fix(hide-internal-tables): filter sqlite_master in CREATE VIEW and CTAS bodies#90
erans merged 5 commits into
mainfrom
worktree-issue-86-view-ctas

Conversation

@erans

@erans erans commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Closes #86.

Problem

--hide-internal-tables rewrote client sqlite_master references only in Statement::Query and Insert sources. A view or CTAS built over the catalog was stored unfiltered, and every later read went through it:

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 filtered it.

Change

Two arms added to the match in SqliteMasterFilter::translateCreateView's query and CreateTable's AS SELECT source. The visitor, predicate, alias handling, fail-open behavior, and both wire hook points are unchanged.

The rewrite stays an allowlist over statement kinds. UPDATE/DELETE remain wholly untouched, subqueries included: filtering there would change which rows a write touches rather than what a user sees. A denylist was rejected because Statement::Merge's target is itself a TableFactor, so a wholesale visit would emit invalid SQL whose error text pastes __pgsqlite_ back to the client — the regression #85 fixed.

Also fixed, found in review

  • CREATE TABLE t (cols) AS SELECT ... — the rewritten body defeated CreateTableTranslator's greedy \((.*)\) regex, producing invalid SQL that leaked the internal prefix into the client-facing error. The arm now skips statements carrying an explicit column list.
  • CREATE VIEW IF NOT EXISTS — unparseable by PostgreSqlDialect (PostgreSQL has no such form; SQLite does), so translate failed open and stored the view unfiltered. Now retries with SQLiteDialect.
  • temp.sqlite_master — accepted as a qualifier but dropped from the generated relation, so temp-catalog queries silently returned main's catalog. temp. is now left alone, as otherdb. already was.

Known trade-off

SQLite persists literal CREATE VIEW text, so a filtered view's stored DDL contains the predicate — visible in sqlite_master.sql and information_schema.views.view_definition. Documented in docs/configuration.md and asserted in the test suite. The alternative, an internal __pgsqlite_visible_master view, was rejected because it would make a pgsqlite-managed object load-bearing for user schema. CTAS is unaffected: SQLite stores the expanded column list, not the select.

Testing

  • 12 unit tests in src/translator/sqlite_master_filter.rs covering view/CTAS/materialized-view rewrites, the column-list guard, the dialect fallback, the temp. and attached-db qualifiers, and the MERGE allowlist boundary.
  • Wire-level tests in both flag states over both the simple and extended protocols, reproducing the issue verbatim.
  • Full suite: 191 test groups, 0 failures.

Follow-ups, not addressed here

  • CreateTableTranslator's greedy regex silently drops the AS SELECT from CREATE TABLE t (cols) AS SELECT ..., creating an empty table. Pre-existing and independent of this flag.
  • Positional test-name filters abort the integration test binaries, because config::CONFIG runs Config::parse() against the harness argv.
  • test_batch_delete_edge_cases is flaky under parallel load.

🤖 Generated with Claude Code

erans and others added 5 commits August 5, 2026 17:10
…CTAS

Closes the gap in #86: the sqlite_master filter rewrites only
Statement::Query and Insert sources, so a view or CTAS built over
sqlite_master reads the unfiltered catalog thereafter.

Records the two decisions taken while scoping: the filter predicate is
persisted inline into view definitions (rejecting an internal
__pgsqlite_visible_master view, which would make pgsqlite internals
load-bearing for user schema), and the rewrite stays an allowlist
(rejecting a denylist, which fails into invalid SQL that leaks the
internal prefix -- the #85 regression).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two tasks: point the existing visitor at CreateView's query and
CreateTable's AS SELECT source (five unit tests, TDD), then wire-level
regression tests reproducing the issue in both flag states.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…AS 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
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
Eight findings from a whole-branch review of the #86 work.

Critical: `CREATE TABLE t (name TEXT) AS SELECT ... FROM sqlite_master` is
valid PostgreSQL, and rewriting its body produced SQL that downstream
CreateTableTranslator's greedy CREATE_TABLE_REGEX then mangled — swallowing
the `AS SELECT` once a parenthesized subquery followed the column list. SQLite's
error embeds the whole statement text, so the client received a message
containing `__pgsqlite_`: exactly what the flag exists to prevent, and #85's
failure mode reintroduced. The CreateTable arm is now guarded on
`columns.is_empty()`, leaving that shape untouched. The regex is the root cause
and is deliberately left for a separate fix.

Important: `CREATE VIEW IF NOT EXISTS` is SQLite-only syntax that
PostgreSqlDialect cannot parse, so translate failed open and stored the view
unfiltered — issue #86 verbatim, one keyword away. The parse now retries with
SQLiteDialect. The `replaced == 0 => Cow::Borrowed` early return confines any
dialect rendering differences to statements that really named sqlite_master.

Important: `temp.sqlite_master` was accepted as a qualifier, but
FILTERED_RELATION_SQL hardcodes an unqualified `FROM sqlite_master`, so the
qualifier was silently dropped and the client got main's catalog instead of the
temp schema's — wrong rows, not merely unfiltered ones. Now left alone, exactly
like an ATTACHed database's.

The match remains an allowlist over statement kinds. `leaves_merge_untouched`
pins that boundary: Statement::Merge's target is itself a TableFactor, so a
denylist refactor would substitute a derived table for a MERGE target and emit
invalid SQL whose error text pastes the internal prefix back to the client.

Also adds extended-protocol DDL coverage (Parse/Bind/Execute is a physically
separate hook from the simple protocol, and most drivers send DDL that way), an
assertion recording the accepted trade-off that the filter predicate is baked
into the user's persisted view DDL, and documentation of all of the above in
docs/configuration.md and the design spec.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@erans
erans merged commit d1e88fe into main Aug 6, 2026
1 of 2 checks passed
@erans
erans deleted the worktree-issue-86-view-ctas branch August 6, 2026 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CREATE VIEW over sqlite_master bypasses --hide-internal-tables

1 participant