From 9ae2bfed6bcca7bb8de789e1d22bf288a81f4305 Mon Sep 17 00:00:00 2001 From: Lakshitha Deshan <85143235+notlakshitha@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:11:53 +0000 Subject: [PATCH 1/3] fix(query-safety): ask before a CTE that deletes, merges, or inserts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A PostgreSQL WITH is operated by its outer SELECT, so the confirmation gate only noticed UPDATE … SET inside the CTE list. DELETE, MERGE, and INSERT in that list now ask too. Those words are read from the statement's code, and only before the keyword that operates the statement, so a quoted or commented word still does not ask and a plain WITH … SELECT does not either. CockroachDB uses the same grammar. Closes #1862 --- docs/editor/query-optimization.md | 29 ++++++--- src/components/QuerySafetyDialog.tsx | 37 ++++++++++-- tests/components/QuerySafetyDialog.test.tsx | 66 ++++++++++++++++----- 3 files changed, 102 insertions(+), 30 deletions(-) diff --git a/docs/editor/query-optimization.md b/docs/editor/query-optimization.md index ef5200ed97..244fbc70e7 100644 --- a/docs/editor/query-optimization.md +++ b/docs/editor/query-optimization.md @@ -573,7 +573,7 @@ same reading the auto-limiter uses, so the two cannot disagree about where a sta | The `DO` of `ON CONFLICT DO NOTHING` or `CREATE RULE … DO INSTEAD`, which a word follows | No | | Any of those behind a comment (`-- note`, `/* note */`, MySQL's `# note`, several stacked) | Yes | | A `WITH` whose CTE list precedes one (`WITH x AS (…) DELETE FROM …`) | Yes | -| A `WITH` whose CTE list *contains* an `UPDATE … SET` (PostgreSQL's data-modifying CTE) | Yes | +| A `WITH` whose CTE list *contains* an `INSERT`, `UPDATE … SET`, `DELETE` or `MERGE` (PostgreSQL's data-modifying CTE; CockroachDB shares the grammar) | Yes | | A statement carrying a run the reader cannot resolve (`SELECT '\';` and anything after it, an unterminated comment, literal, name or array) | Yes, and the dialog says why | | Any of those behind a **nested** comment (`/* a /* b */ c */ DROP TABLE users`), on a dialect that nests — PostgreSQL, SQL Server, ClickHouse | Yes: the comment is read whole, so the statement's own keyword is the one that answers | | The same text on a dialect that does **not** nest — MySQL, MariaDB, SQLite, Oracle | No, because there the comment closed at the first `*/` and what follows it is text the engine rejects outright | @@ -601,6 +601,15 @@ not bound the rows a write commits — so anchoring this probe too would take th from a statement that really does write. It reads the statement's **code** rather than its text (`src/lib/sql/words.ts`), so unlike the pattern before it, a read that merely quotes `'UPDATE t SET x = 1'` or mentions it in a comment no longer prompts. + +`DELETE`, `MERGE` and `INSERT` are the other three words of that set (`DATA_MODIFYING` in +`src/lib/explain/select-prefix.ts`). They count only when the statement leads with `WITH`, the CTE +list can be read, and the word sits in that list — before the keyword that operates the statement — +so `WITH d AS (DELETE FROM t RETURNING *) SELECT * FROM d` asks, and so do the `MERGE … DELETE` and +`INSERT` forms. CockroachDB connections are the `postgres` type-id and share that grammar, so they +ask too. A plain `SELECT` whose code names one of those words does not ask, and neither does a +word that only appears in a string or a comment, or after the statement the list precedes. An +unclosed list still does not: the shape cannot be typed, and no dialect accepts the text. ┌──────────────────────────────────────────────────────────────────────────────┐ ### Text the reading cannot resolve asks, and says so ┌──────────────────────────────────────────────────────────────────────────────┐ @@ -663,23 +672,25 @@ own grammar (#295) and unresolvable only to a reader without it — and it lifts cannot read at all: Oracle's `q'{it's}'` (#292). It does nothing for the first class, per that bullet. ┌──────────────────────────────────────────────────────────────────────────────┐ -Two gaps are known and pinned by tests rather than claimed closed: +One gap is known and pinned by tests rather than claimed closed: ┌──────────────────────────────────────────────────────────────────────────────┐ -- **Only `UPDATE … SET` is looked for inside a read.** A write hidden in a CTE *body* under any - other keyword (`WITH gone AS (DELETE FROM t RETURNING *) SELECT * FROM gone`) does not prompt. - Widening the probe to `DELETE`/`INSERT`/`MERGE` would also make every read whose code names one - of them prompt, so the asymmetry is inherited from the vocabulary above rather than chosen here. - **A statement whose shape cannot be TYPED** — an unclosed CTE list such as `WITH t AS (DELETE FROM x` — does not prompt. This is the boundary of the rule above rather than an exception to it: every character was read, and what they spell is an incomplete statement rather than a run hiding what is written inside it. No dialect accepts the text, so the server - rejects it; and the keyword inside the unclosed list is a CTE-body write, which the gap above - already does not prompt for even when the list closes. Pinned by + rejects it. The same keyword inside a list that *does* close asks (#1862); this one does not, + because the list cannot be crossed and the probe refuses to guess. Pinned by `tests/components/QuerySafetyDialog.test.tsx` ("does not prompt when the statement's shape cannot be typed") so the boundary stays a decision — the shipped rule keys on unresolvable *runs*, and widening it to every statement the reader cannot type would prompt for an empty editor. ┌──────────────────────────────────────────────────────────────────────────────┐ -A third gap sat beside them and is closed (S1). A multi-statement script used to be read as one +A data-modifying CTE that `DELETE`s, `MERGE`s or `INSERT`s used to be a second gap beside that one: +only `UPDATE … SET` was looked for inside a read, so +`WITH gone AS (DELETE FROM t RETURNING *) SELECT * FROM gone` ran with no confirmation. Widening the +unanchored pair to those words would have made every read whose code names one of them prompt, so +the three are read only inside a `WITH` whose list can be typed. That gap is closed (#1862). +┌──────────────────────────────────────────────────────────────────────────────┐ +Another gap sat beside them and is closed (S1). A multi-statement script used to be read as one statement, so `SELECT 1; DROP TABLE users` did not prompt — its first keyword is the `SELECT` — while `SELECT 1; UPDATE t SET x = 1` did, because the unanchored probe happened to find it. The gate now splits the editor text with the same splitter and the same grammar the runner uses, and asks diff --git a/src/components/QuerySafetyDialog.tsx b/src/components/QuerySafetyDialog.tsx index cf76428895..60f66baf37 100644 --- a/src/components/QuerySafetyDialog.tsx +++ b/src/components/QuerySafetyDialog.tsx @@ -30,6 +30,7 @@ import { wordsFollow, } from "@/lib/sql/confirm-forms"; import { readsSqlText, resolveSqlGrammar, type SqlGrammar } from "@/lib/sql/grammar"; +import { readLeadingKeyword } from "@/lib/sql/leading-keyword"; import { readOperativeKeyword } from "@/lib/sql/operative-keyword"; import { hasUnterminatedSpan, readSqlSpan } from "@/lib/sql/spans"; import { splitStatements } from "@/lib/sql/statement-splitter"; @@ -612,6 +613,16 @@ const CONTROL_FLOW_KEYWORDS = new Set([ "WHEN", ]); +/** + * The data-modifying words a CTE body can carry besides `UPDATE`. + * + * The same set `DATA_MODIFYING` names in `src/lib/explain/select-prefix.ts` + * (`INSERT`, `UPDATE`, `DELETE`, `MERGE`), minus `UPDATE`: that one stays the + * `UPDATE … SET` pair below, because a bare `UPDATE` is a cursor lock as often + * as it is a write. These three need no following word. + */ +const CTE_DATA_MODIFYING = ["INSERT", "DELETE", "MERGE"] as const; + /** Whether a `confirmExemptForms` form led by `keyword` starts at the keyword that ends at `end`. */ function exemptFormAt(text: string, keyword: string, end: number, grammar: SqlGrammar): boolean { return (grammar.confirmExemptForms ?? []).some( @@ -772,11 +783,27 @@ export function writesUnderGrammar(text: string, grammar: SqlGrammar): boolean { return true; } - // A write the statement's own keyword does not report: PostgreSQL's data-modifying - // CTE is OPERATED by its SELECT (`WITH x AS (UPDATE … SET …) SELECT * FROM x`), so - // this probe stays unanchored deliberately. It reads the statement's CODE rather - // than its text, so a read that merely quotes or comments the two words - which - // the pattern before it treated as a write - no longer asks for a confirmation. + // A write the statement's own keyword does not report. PostgreSQL's data-modifying + // CTE is OPERATED by its SELECT (`WITH x AS (DELETE FROM t RETURNING *) SELECT * FROM x`), + // so the keyword test above answers for the SELECT and the write sits in the list. + // The words that can ride there are `DATA_MODIFYING` in `src/lib/explain/select-prefix.ts`: + // INSERT, UPDATE, DELETE and MERGE. UPDATE is the unanchored `UPDATE … SET` pair, which + // already asks for that form and for a later statement in a script. DELETE, MERGE and + // INSERT count only when the statement leads with WITH, its list can be typed, and the + // word sits in that list — before the keyword that operates the statement. A plain + // SELECT whose code names one of them does not ask. An unclosed list + // (`WITH t AS (DELETE FROM x`) stays silent: the shape cannot be crossed and no dialect + // accepts the text. A `#` the dialect-less reader cannot place stays silent too, which + // is the pinned reading of `WITH t AS ( #- … ) SELECT … ) DELETE`; naming MySQL makes + // the `DELETE` the operative keyword instead. Both probes read CODE (`findCodeWord`), + // so a quoted or commented word does not ask. CockroachDB connections are the postgres + // type-id and share this grammar, so the same statements ask there too (#1862). + if (readLeadingKeyword(text, grammar)?.keyword === "WITH" && reading !== null) { + for (const word of CTE_DATA_MODIFYING) { + const found = findCodeWord(text, word, 0, grammar); + if (found !== null && found.start < reading.start) return true; + } + } const update = findCodeWord(text, "UPDATE", 0, grammar); return update !== null && findCodeWord(text, "SET", update.end, grammar) !== null; } diff --git a/tests/components/QuerySafetyDialog.test.tsx b/tests/components/QuerySafetyDialog.test.tsx index 81e67e1897..40bede2237 100644 --- a/tests/components/QuerySafetyDialog.test.tsx +++ b/tests/components/QuerySafetyDialog.test.tsx @@ -1511,6 +1511,43 @@ describe("isDangerousQuery", () => { expect(isDangerousQuery("WITH moved AS (UPDATE t SET a = 1 RETURNING *) SELECT * FROM moved")).toBe(true); }); + /** + * `DELETE`, `MERGE` and `INSERT` are the rest of the data-modifying set the + * `UPDATE … SET` probe above already covers for `UPDATE` (#1862). They are read + * from the statement's code, and only when it leads with `WITH`, so the same + * words inside a string or a comment do not ask and a plain `WITH … SELECT` does + * not either. CockroachDB is the postgres type-id, so it shares the grammar. + */ + test.each<[string, string]>([ + ["a DELETE", "WITH d AS (DELETE FROM users RETURNING *) SELECT count(*) FROM d"], + [ + "a MERGE that deletes", + "WITH m AS (MERGE INTO users u USING (SELECT 1 id) s ON u.id=s.id WHEN MATCHED THEN DELETE RETURNING u.id) SELECT * FROM m", + ], + ["an INSERT", "WITH i AS (INSERT INTO users(id) VALUES (1) RETURNING id) SELECT * FROM i"], + ])("prompts for a data-modifying CTE that carries %s", (_label, query) => { + expect(isDangerousQuery(query, "postgres")).toBe(true); + }); + + test("prompts for a data-modifying CTE on cockroachdb, which shares the PostgreSQL grammar", () => { + expect(isDangerousQuery("WITH d AS (DELETE FROM users RETURNING *) SELECT count(*) FROM d", "postgres")).toBe(true); + }); + + test.each<[string, string]>([ + ["DELETE inside a string", "WITH d AS (SELECT 'DELETE FROM users') SELECT count(*) FROM d"], + [ + "MERGE and DELETE inside a comment", + "WITH m AS (SELECT 1 /* MERGE INTO users WHEN MATCHED THEN DELETE */) SELECT * FROM m", + ], + ["INSERT inside a line comment", "WITH i AS (SELECT 1) SELECT * FROM i -- INSERT INTO users VALUES (1)"], + ])("does not prompt for a WITH that only names %s", (_label, query) => { + expect(isDangerousQuery(query, "postgres")).toBe(false); + }); + + test("does not prompt for a plain WITH … SELECT", () => { + expect(isDangerousQuery("WITH cte AS (SELECT 1) SELECT * FROM cte", "postgres")).toBe(false); + }); + test("prompts for a destructive statement a CTE list only precedes", () => { expect(isDangerousQuery("WITH x AS (SELECT id FROM t) DELETE FROM t USING x WHERE t.id = x.id")).toBe(true); }); @@ -1534,32 +1571,29 @@ describe("isDangerousQuery", () => { * * This is the boundary of #297's rule, which is about text no reader can RESOLVE: * here every character was read, and the shape it spells is an incomplete - * statement rather than a run hiding what is written inside it. The keyword inside - * the unclosed list is a CTE-body write, which the row above pins as not prompting - * even when the list DOES close - so the answer is inherited from that gap, not - * from the reading stopping early. + * statement rather than a run hiding what is written inside it. The same keyword + * inside a list that DOES close asks (#1862). This one does not, because the list + * cannot be crossed and the probe refuses to guess. */ test("does not prompt when the statement's shape cannot be typed", () => { expect(isDangerousQuery("WITH t AS (DELETE FROM x")).toBe(false); }); /** - * The gaps this predicate does NOT close, pinned so each stays a decision and so - * `docs/editor/query-optimization.md` cannot drift from them. + * Answers pinned so a regression is silence, and so `docs/editor/query-optimization.md` + * cannot drift from them. * - * The write-inside-a-read probe looks for `UPDATE … SET` only, because widening it - * to the other write keywords would make every read whose code names one of them - * prompt. + * A `DELETE`, `MERGE` or `INSERT` inside a `WITH` whose list can be read asks + * (#1862). Those words stay out of the unanchored `UPDATE … SET` pair: widening + * that pair would make every read whose code names one of them prompt. * - * The gap this list used to record beside it - "the whole editor text is read as one - * statement, so a destructive statement later in a script is only caught by that same - * unanchored probe" - is closed (S1): the predicate now reads the SAME fragments - * `/api/db/multi-query` will run, so a later `DROP` is answered by its own fragment's - * keyword rather than by luck. The row below is kept, with the answer it now has, - * because a regression there is silence on a script's second statement. + * The multi-statement rows closed an older gap (S1): the predicate reads the SAME + * fragments `/api/db/multi-query` will run, so a later `DROP` is answered by its + * own fragment's keyword rather than by luck. A regression there is silence on a + * script's second statement. */ test.each<[string, string, boolean]>([ - ["a DELETE hidden in a CTE body", "WITH gone AS (DELETE FROM t RETURNING *) SELECT * FROM gone", false], + ["a DELETE hidden in a CTE body", "WITH gone AS (DELETE FROM t RETURNING *) SELECT * FROM gone", true], ["a DROP after a leading SELECT", "SELECT 1; DROP TABLE users", true], ["an UPDATE after a leading SELECT", "SELECT 1;\nUPDATE t SET x = 1", true], ])("answers %s with %p", (_label, query, expected) => { From c0cbcf00748f1b737536bf7a4e5c5b9e0391a6e4 Mon Sep 17 00:00:00 2001 From: Lakshitha Deshan <85143235+notlakshitha@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:36:26 +0000 Subject: [PATCH 2/3] fix(query-safety): ask before EXPLAIN ANALYZE runs a write On PostgreSQL, EXPLAIN ANALYZE executes the statement it explains. The confirmation gate read the operative keyword EXPLAIN and never looked past it, so a typed EXPLAIN ANALYZE DELETE or DROP ran with no dialog. The gate now strips ANALYZE, ANALYSE and VERBOSE, or a parenthesised option list, and when ANALYZE is on asks about the remaining statement. ANALYZE set to false, off or 0, and a plain EXPLAIN, only plan, so they do not ask. CockroachDB shares the postgres grammar. Closes #1863 --- docs/editor/query-optimization.md | 26 +++- docs/providers/postgres.md | 3 + src/components/QuerySafetyDialog.tsx | 142 +++++++++++++++++- .../db/providers/sql/postgres-sql-grammar.ts | 4 + src/lib/sql/grammar.ts | 13 ++ tests/components/QuerySafetyDialog.test.tsx | 78 ++++++++++ tests/unit/db/sql-grammar/golden/postgres.txt | 2 +- 7 files changed, 263 insertions(+), 5 deletions(-) diff --git a/docs/editor/query-optimization.md b/docs/editor/query-optimization.md index 244fbc70e7..ffb92bdefc 100644 --- a/docs/editor/query-optimization.md +++ b/docs/editor/query-optimization.md @@ -574,6 +574,7 @@ same reading the auto-limiter uses, so the two cannot disagree about where a sta | Any of those behind a comment (`-- note`, `/* note */`, MySQL's `# note`, several stacked) | Yes | | A `WITH` whose CTE list precedes one (`WITH x AS (…) DELETE FROM …`) | Yes | | A `WITH` whose CTE list *contains* an `INSERT`, `UPDATE … SET`, `DELETE` or `MERGE` (PostgreSQL's data-modifying CTE; CockroachDB shares the grammar) | Yes | +| `EXPLAIN ANALYZE` or `EXPLAIN (ANALYZE, …)` of a statement that asks, on PostgreSQL (CockroachDB shares the grammar). `ANALYZE` set to `false`, `off` or `0`, and a plain `EXPLAIN`, only plan | Yes, the same answer as the explained statement | | A statement carrying a run the reader cannot resolve (`SELECT '\';` and anything after it, an unterminated comment, literal, name or array) | Yes, and the dialog says why | | Any of those behind a **nested** comment (`/* a /* b */ c */ DROP TABLE users`), on a dialect that nests — PostgreSQL, SQL Server, ClickHouse | Yes: the comment is read whole, so the statement's own keyword is the one that answers | | The same text on a dialect that does **not** nest — MySQL, MariaDB, SQLite, Oracle | No, because there the comment closed at the first `*/` and what follows it is text the engine rejects outright | @@ -595,7 +596,8 @@ Writing a note above a destructive statement is ordinary, and it is exactly wher matters most. ┌──────────────────────────────────────────────────────────────────────────────┐ One probe stays deliberately unanchored: a statement whose code contains `UPDATE` followed by -`SET` asks for confirmation whatever its own keyword is. PostgreSQL's data-modifying CTE is +`SET` asks for confirmation whatever its own keyword is, except an `EXPLAIN` that will not execute +it (below). PostgreSQL's data-modifying CTE is *operated* by its `SELECT` — which is the honest answer, and the one the limiter needs so it does not bound the rows a write commits — so anchoring this probe too would take the last check away from a statement that really does write. It reads the statement's **code** rather than its text @@ -611,6 +613,17 @@ ask too. A plain `SELECT` whose code names one of those words does not ask, and word that only appears in a string or a comment, or after the statement the list precedes. An unclosed list still does not: the shape cannot be typed, and no dialect accepts the text. ┌──────────────────────────────────────────────────────────────────────────────┐ +On PostgreSQL, `EXPLAIN ANALYZE` executes the statement it explains. Typed into the editor, it used to +be read as an `EXPLAIN`, which is not a dangerous keyword, so `EXPLAIN ANALYZE DELETE FROM users` +deleted the rows with no confirmation (#1863). The gate strips a leading run of `ANALYZE`, `ANALYSE` +and `VERBOSE`, or a parenthesised option list, and when `ANALYZE` is enabled asks about the remaining +statement the way it would on its own — including a data-modifying CTE written under the `EXPLAIN`. +`ANALYZE` with no value is on; `true`, `on` and `1` are on; `false`, `off` and `0` are off. A plain +`EXPLAIN`, `EXPLAIN VERBOSE`, or `ANALYZE` set off only plans, so it does not ask, and an `UPDATE … SET` +inside that plan does not either. A word inside a string or a comment is not an option and is not the +statement. CockroachDB connections are the `postgres` type-id and share the grammar, so they ask too. +The grammar fact is `explainExecutesWithAnalyze`, absent on every other row. +┌──────────────────────────────────────────────────────────────────────────────┐ ### Text the reading cannot resolve asks, and says so ┌──────────────────────────────────────────────────────────────────────────────┐ Every other reader in `src/lib/sql/` errs toward **not acting** on text it cannot resolve, because @@ -690,6 +703,12 @@ only `UPDATE … SET` was looked for inside a read, so unanchored pair to those words would have made every read whose code names one of them prompt, so the three are read only inside a `WITH` whose list can be typed. That gap is closed (#1862). ┌──────────────────────────────────────────────────────────────────────────────┐ +`EXPLAIN ANALYZE` of a write was the same kind of gap: the operative keyword is `EXPLAIN`, so a +`DELETE` or `DROP` under it never asked, while `EXPLAIN (ANALYZE) UPDATE … SET` asked only because +the unanchored pair happened to see the `UPDATE`. The explained statement is what runs, so the gate +reads it as its own statement when `ANALYZE` is on, and stays silent when `ANALYZE` is off or absent +(#1863). Pinned by `tests/components/QuerySafetyDialog.test.tsx`. +┌──────────────────────────────────────────────────────────────────────────────┐ Another gap sat beside them and is closed (S1). A multi-statement script used to be read as one statement, so `SELECT 1; DROP TABLE users` did not prompt — its first keyword is the `SELECT` — while `SELECT 1; UPDATE t SET x = 1` did, because the unanchored probe happened to find it. The gate @@ -700,8 +719,9 @@ in a Mongo document or a Redis command separates nothing, so a fragment cut ther run"). ┌──────────────────────────────────────────────────────────────────────────────┐ Four runs bypass the gate on purpose, all on the standalone path -(`src/hooks/use-query-execution.ts`): an EXPLAIN run (see below), a Load-More page of a result the -user already has, a playground run (rolled back rather than committed), and anything carrying +(`src/hooks/use-query-execution.ts`): an EXPLAIN run built by the Explain button (see below — +`EXPLAIN ANALYZE` typed in the editor is an ordinary run and reaches the gate), a Load-More page of a +result the user already has, a playground run (rolled back rather than committed), and anything carrying `skipSafety` — which is the dialog's own "Execute Anyway" button and the inline row editor, whose `UPDATE` comes from a grid edit the user has already confirmed. The embedded adapter (`src/workspace/hooks/use-query-adapter.ts`) has only the force-execute bypass, so every other diff --git a/docs/providers/postgres.md b/docs/providers/postgres.md index 23fa3e5c9b..ef003ca3a0 100644 --- a/docs/providers/postgres.md +++ b/docs/providers/postgres.md @@ -1820,6 +1820,8 @@ Before this, the same file stopped at its first row with `ERROR: malformed array The editor's confirmation gate asks before the shared destructive keywords, `MERGE` among them ([query-optimization.md](../editor/query-optimization.md#destructive-statement-confirmation)). On this dialect it also asks before a `DO` block, whatever the block does: the block's code is a literal, `$$ ... $$` or `'...'`, which the gate does not read. The grammar row's `confirmCodeForms` names `DO` followed by a body and `DO LANGUAGE` ([`postgres-sql-grammar.ts`](../../src/lib/db/providers/sql/postgres-sql-grammar.ts)), so the `DO` of `INSERT ... ON CONFLICT DO NOTHING` and of `CREATE RULE ... DO INSTEAD`, which a word follows, does not ask. +`explainExecutesWithAnalyze` is set, because `EXPLAIN ANALYZE` (and `EXPLAIN (ANALYZE, …)`) runs the explained statement: the gate strips the option and asks about what remains when `ANALYZE` is on, and a plain `EXPLAIN` or `ANALYZE` set to `false`, `off` or `0` only plans, so it does not ask. +CockroachDB shares this type id and this grammar. --- @@ -1834,6 +1836,7 @@ Block comments nest, per the PostgreSQL manual (4.1.5 Comments, checked 2026-08- The row bound goes before Materialize's `AS OF [AT LEAST] `, which shares this type id (E2E pass of 2026-10-03/04, #1398): `SELECT * FROM ui_mv AS OF AT LEAST 0` with the bound appended is `Expected end of statement, found LIMIT`, and with the bound before the clause it returns rows. Completion quotes a catalog name by PostgreSQL's `quote_ident` rule, with the keyword set taken from PostgreSQL 18's `kwlist.h`: a name stays bare only when it is lower case and not a keyword other than an unreserved one, and a qualifier written in double quotes before the dot is read back with its doubled quotes undone. Only PostgreSQL completions introduce quoted table names; every other dialect quotes through the connection's capabilities. +`explainExecutesWithAnalyze` is `true`: `EXPLAIN ANALYZE` executes, and the confirmation gate reads the explained statement ([§5.8](#58-the-confirmation-gate)). `readsSqlText` is `true`: the editor text is SQL, and the provider extends `SQLBaseProvider`. Redaction is `sql` with `"` as its name quote: a redacted statement keeps its words and every name in that quote, and every literal becomes `?`. diff --git a/src/components/QuerySafetyDialog.tsx b/src/components/QuerySafetyDialog.tsx index 60f66baf37..286a3396db 100644 --- a/src/components/QuerySafetyDialog.tsx +++ b/src/components/QuerySafetyDialog.tsx @@ -34,7 +34,7 @@ import { readLeadingKeyword } from "@/lib/sql/leading-keyword"; import { readOperativeKeyword } from "@/lib/sql/operative-keyword"; import { hasUnterminatedSpan, readSqlSpan } from "@/lib/sql/spans"; import { splitStatements } from "@/lib/sql/statement-splitter"; -import { findCodeWord } from "@/lib/sql/words"; +import { findCodeWord, readSqlWord } from "@/lib/sql/words"; import type { DatabaseType } from "@/lib/types"; interface SafetyAnalysis { @@ -623,6 +623,21 @@ const CONTROL_FLOW_KEYWORDS = new Set([ */ const CTE_DATA_MODIFYING = ["INSERT", "DELETE", "MERGE"] as const; +/** + * The words the legacy `EXPLAIN [ANALYZE | ANALYSE] [VERBOSE]` spelling takes, in either order. + * `ANALYSE` is PostgreSQL's British spelling of the option that executes. + */ +const EXPLAIN_KEYWORD_OPTIONS = new Set(["ANALYZE", "ANALYSE", "VERBOSE"]); + +/** The option names that execute the explained statement when they are enabled. */ +const ANALYZE_OPTIONS = new Set(["ANALYZE", "ANALYSE"]); + +/** + * Parenthesised values that turn `ANALYZE` off. No value, and any other value (`true`, `on`, `1`), leaves it on: + * PostgreSQL's `defGetBoolean` reads an absent argument as true. + */ +const ANALYZE_OFF = new Set(["FALSE", "OFF", "0"]); + /** Whether a `confirmExemptForms` form led by `keyword` starts at the keyword that ends at `end`. */ function exemptFormAt(text: string, keyword: string, end: number, grammar: SqlGrammar): boolean { return (grammar.confirmExemptForms ?? []).some( @@ -727,6 +742,117 @@ function configKeyAsks( }); } +/** One token of a parenthesised EXPLAIN option list. */ +interface ExplainOptionToken { + /** A word is upper-cased; a string is its contents; a number is its digits. */ + text: string; + end: number; + kind: "word" | "number" | "comma" | "string" | "other"; +} + +/** + * The next option-list token at or after `from` and before `limit`, or null at the closing paren. + * + * Strings and comments are spans, so a word written inside one is not an option: `EXPLAIN ('ANALYZE')` does not + * execute, and `ANALYZE 'false'` is the off value rather than the word `false` somewhere later. + */ +function readExplainOptionToken( + text: string, + from: number, + limit: number, + grammar: SqlGrammar, +): ExplainOptionToken | null { + const i = pastHiddenText(text, from, grammar); + if (i >= limit) return null; + if (text[i] === ",") return { text: ",", end: i + 1, kind: "comma" }; + const word = readSqlWord(text, i); + if (word !== null) return { text: word.text, end: word.end, kind: "word" }; + if (text[i] >= "0" && text[i] <= "9") { + let end = i; + while (end < limit && text[end] >= "0" && text[end] <= "9") end++; + return { text: text.slice(i, end), end, kind: "number" }; + } + const span = readSqlSpan(text, i, grammar); + if (span?.kind === "string") return { text: text.slice(i + 1, span.end - 1), end: span.end, kind: "string" }; + const end = span === null ? i + 1 : span.end; + return { text: text.slice(i, end), end, kind: "other" }; +} + +/** + * The index past the `)` that closes the paren at `open`, or `-1` when it never closes. + * + * Literals and comments are skipped whole, so a `)` inside `EXPLAIN (ANALYZE ')')` cannot end the option list. + */ +function endOfParenthesised(text: string, open: number, grammar: SqlGrammar): number { + let depth = 0; + let i = open; + while (i < text.length) { + const span = readSqlSpan(text, i, grammar); + if (span !== null) { + i = span.end; + continue; + } + if (text[i] === "(") depth++; + else if (text[i] === ")") { + depth--; + if (depth === 0) return i + 1; + } + i++; + } + return -1; +} + +/** + * Whether a parenthesised option list enables ANALYZE. + * + * `false`, `off` and `0` turn that option off, and an option with no value is on. Any enabled `ANALYZE` executes, + * even when another is off, which asks rather than missing a `DELETE`. + */ +function explainOptionsExecute(text: string, open: number, close: number, grammar: SqlGrammar): boolean { + const limit = close - 1; + let execute = false; + let i = open + 1; + while (i < limit) { + const name = readExplainOptionToken(text, i, limit, grammar); + if (name === null) break; + i = name.end; + if (name.kind !== "word") continue; + const value = readExplainOptionToken(text, i, limit, grammar); + if (value !== null && value.kind !== "comma") i = value.end; + if (!ANALYZE_OPTIONS.has(name.text)) continue; + const setting = value === null || value.kind === "comma" ? null : value.text.toUpperCase(); + if (setting === null || !ANALYZE_OFF.has(setting)) execute = true; + } + return execute; +} + +/** + * The statement after `EXPLAIN`'s options when ANALYZE will execute it, or null when ANALYZE is absent or off. + * + * The legacy spelling is a run of `ANALYZE`/`ANALYSE`/`VERBOSE`. The parenthesised spelling is `EXPLAIN ( … )`. + * Anything else — a plain `EXPLAIN`, `EXPLAIN VERBOSE`, `ANALYZE false` — only plans, and null is what tells the + * caller not to ask. + */ +function stripExplainOptions(text: string, from: number, grammar: SqlGrammar): string | null { + const start = pastHiddenText(text, from, grammar); + if (text[start] === "(") { + const close = endOfParenthesised(text, start, grammar); + if (close < 0) return null; + return explainOptionsExecute(text, start, close, grammar) ? text.slice(close) : null; + } + + let at = start; + let analyze = false; + for (;;) { + const wordAt = pastHiddenText(text, at, grammar); + const word = readSqlWord(text, wordAt); + if (word === null || !EXPLAIN_KEYWORD_OPTIONS.has(word.text)) break; + if (ANALYZE_OPTIONS.has(word.text)) analyze = true; + at = word.end; + } + return analyze ? text.slice(at) : null; +} + /** * Whether ONE statement's own code asks for a confirmation, read under `grammar`. * @@ -741,6 +867,20 @@ function configKeyAsks( export function writesUnderGrammar(text: string, grammar: SqlGrammar): boolean { const reading = readOperativeKeyword(text, grammar); const keyword = reading?.keyword; + + // PostgreSQL's EXPLAIN executes the explained statement only when ANALYZE is on + // (`EXPLAIN ANALYZE DELETE`, `EXPLAIN (ANALYZE, BUFFERS) UPDATE`). The operative + // keyword is EXPLAIN, which is not itself dangerous, so without this the DELETE + // runs unconfirmed (#1863). A plain EXPLAIN, or ANALYZE set to false, off or 0, + // only plans: returning here, instead of falling through, is what keeps an + // `UPDATE … SET` inside a plan from asking. The remaining text is read as its own + // statement, so a data-modifying CTE under the EXPLAIN asks too. CockroachDB + // shares this grammar. + if (reading !== null && reading.keyword === "EXPLAIN" && grammar.explainExecutesWithAnalyze === true) { + const inner = stripExplainOptions(text, reading.end, grammar); + return inner !== null && writesUnderGrammar(inner, grammar); + } + if ( reading !== null && DANGEROUS_KEYWORDS.has(reading.keyword) && diff --git a/src/lib/db/providers/sql/postgres-sql-grammar.ts b/src/lib/db/providers/sql/postgres-sql-grammar.ts index 26e77dbd1b..0a34a7eade 100644 --- a/src/lib/db/providers/sql/postgres-sql-grammar.ts +++ b/src/lib/db/providers/sql/postgres-sql-grammar.ts @@ -61,6 +61,10 @@ const POSTGRES_GRAMMAR: SqlGrammar = { // asks whatever the block does; `DO LANGUAGE ` writes the language before the body. The `DO` of // `ON CONFLICT DO NOTHING | UPDATE` and of `CREATE RULE ... DO INSTEAD | ALSO` is followed by a word and does not ask. confirmCodeForms: [{ wordThenBody: "DO" }, ["DO", "LANGUAGE"]], + // `EXPLAIN` only plans. `EXPLAIN ANALYZE` (and the British `ANALYSE`) executes the explained statement, and so does + // a parenthesised `ANALYZE` whose value is absent or anything other than false, off or 0 (PostgreSQL manual, + // EXPLAIN). CockroachDB shares this type-id, so the same statements ask there (#1863). + explainExecutesWithAnalyze: true, }; /** How PostgreSQL statement text is read: grammar, SQL text, redaction, update target and completion. */ diff --git a/src/lib/sql/grammar.ts b/src/lib/sql/grammar.ts index d32489dbd1..8fd3c04304 100644 --- a/src/lib/sql/grammar.ts +++ b/src/lib/sql/grammar.ts @@ -371,6 +371,19 @@ export interface SqlGrammar { * key. Absent means none. */ readonly confirmConfigKeys?: readonly string[]; + /** + * Whether `EXPLAIN ANALYZE` executes the explained statement. + * + * True on PostgreSQL: `EXPLAIN` only plans unless `ANALYZE` (or the British `ANALYSE`) is enabled, and then it runs + * the statement. CockroachDB connections are the `postgres` type-id and share that grammar. Absent means false, and + * a plain `EXPLAIN` describes without running, so the confirmation gate must not treat the word `EXPLAIN` itself as + * a write and must not ask about a statement that is only planned. + * + * It is a fact the text cannot settle: `EXPLAIN ANALYZE DELETE FROM t` deletes the rows where the option executes + * and is a plan where it does not, so under the wrong reading the `DELETE` would run with no confirmation, or a + * plan would ask. + */ + readonly explainExecutesWithAnalyze?: boolean; } /** A form `SqlGrammar.confirmFormsWhen` looks for: a keyword sequence and the code word that makes it ask. */ diff --git a/tests/components/QuerySafetyDialog.test.tsx b/tests/components/QuerySafetyDialog.test.tsx index 40bede2237..54bc66029b 100644 --- a/tests/components/QuerySafetyDialog.test.tsx +++ b/tests/components/QuerySafetyDialog.test.tsx @@ -1552,6 +1552,84 @@ describe("isDangerousQuery", () => { expect(isDangerousQuery("WITH x AS (SELECT id FROM t) DELETE FROM t USING x WHERE t.id = x.id")).toBe(true); }); + /** + * PostgreSQL executes the statement under `EXPLAIN ANALYZE`, in the legacy spelling and in a parenthesised + * option list. The operative keyword is `EXPLAIN`, so the gate reads the statement that remains once the + * options are stripped (#1863). `ANALYZE` set to false, off or 0 only plans, and so does a plain `EXPLAIN`. + * CockroachDB is the postgres type-id and shares the grammar. A dialect that does not set + * `explainExecutesWithAnalyze` is unchanged: MySQL's `EXPLAIN ANALYZE` is outside this row. + */ + test.each<[string, string]>([ + ["a DELETE", "EXPLAIN ANALYZE DELETE FROM users"], + ["a DROP TABLE", "EXPLAIN ANALYZE DROP TABLE users"], + ["an UPDATE under a parenthesised option list", "EXPLAIN (ANALYZE, BUFFERS) UPDATE users SET id = 0"], + ["a DELETE under ANALYZE VERBOSE", "EXPLAIN ANALYZE VERBOSE DELETE FROM users"], + ["the British ANALYSE spelling", "EXPLAIN ANALYSE DELETE FROM users"], + ["ANALYZE with no space before the option list", "EXPLAIN(ANALYZE)DELETE FROM users"], + ["ANALYZE written in any case", "explain analyze delete from users"], + ["a comment between EXPLAIN and ANALYZE", "EXPLAIN /* plan */ ANALYZE DELETE FROM users"], + ["a note above the EXPLAIN", "-- profile\nEXPLAIN ANALYZE DELETE FROM users"], + ["ANALYZE after other options", "EXPLAIN (BUFFERS, ANALYZE) DELETE FROM users"], + ["ANALYZE true", "EXPLAIN (ANALYZE true) DELETE FROM users"], + ["ANALYZE on", "EXPLAIN (ANALYZE on) DELETE FROM users"], + ["ANALYZE 1", "EXPLAIN (ANALYZE 1) DELETE FROM users"], + ["ANALYZE 'true'", "EXPLAIN (ANALYZE 'true') DELETE FROM users"], + ["a parenthesised ANALYSE", "EXPLAIN (ANALYSE, VERBOSE) DROP TABLE users"], + ["FORMAT before ANALYZE", "EXPLAIN (FORMAT JSON, ANALYZE) DELETE FROM users"], + ["a data-modifying CTE under the EXPLAIN", "EXPLAIN ANALYZE WITH d AS (DELETE FROM t RETURNING *) SELECT * FROM d"], + ["a nested EXPLAIN ANALYZE", "EXPLAIN ANALYZE EXPLAIN ANALYZE DELETE FROM users"], + ["a DELETE in a later statement", "EXPLAIN ANALYZE SELECT 1; DELETE FROM users"], + ["a ) inside an option string, which does not close the list", "EXPLAIN (ANALYZE ')') DELETE FROM users"], + ["an equals sign, which is not the off spelling", "EXPLAIN (ANALYZE = false) DELETE FROM users"], + ["an off value that is only a comment", "EXPLAIN (ANALYZE /* false */) DELETE FROM users"], + ["ANALYZE on after an ANALYZE off", "EXPLAIN (ANALYZE false, ANALYZE) DELETE FROM users"], + ["a quoted name where a boolean would be", 'EXPLAIN (ANALYZE "off") DELETE FROM users'], + ])("prompts for EXPLAIN ANALYZE of %s", (_label, query) => { + expect(isDangerousQuery(query, "postgres")).toBe(true); + }); + + test("prompts for EXPLAIN ANALYZE on cockroachdb, which shares the PostgreSQL grammar", () => { + expect(isDangerousQuery("EXPLAIN ANALYZE DELETE FROM users", "postgres")).toBe(true); + expect(isDangerousQuery("EXPLAIN ANALYZE DROP TABLE users", "postgres")).toBe(true); + }); + + test.each<[string, string]>([ + ["ANALYZE false", "EXPLAIN (ANALYZE false) DELETE FROM users"], + ["ANALYZE off", "EXPLAIN (ANALYZE off) DELETE FROM users"], + ["ANALYZE 0", "EXPLAIN (ANALYZE 0) DELETE FROM users"], + ["ANALYZE 'false'", "EXPLAIN (ANALYZE 'false') DELETE FROM users"], + ["a plain EXPLAIN of a DELETE", "EXPLAIN DELETE FROM users"], + ["a plain EXPLAIN of an UPDATE … SET", "EXPLAIN UPDATE users SET id = 0"], + ["ANALYZE false around an UPDATE … SET", "EXPLAIN (ANALYZE false) UPDATE users SET id = 0"], + ["EXPLAIN ANALYZE of a SELECT", "EXPLAIN ANALYZE SELECT 1"], + ["EXPLAIN VERBOSE, which does not execute", "EXPLAIN VERBOSE DELETE FROM users"], + ["a parenthesised list with no ANALYZE", "EXPLAIN (BUFFERS) DELETE FROM users"], + ["FORMAT JSON and no ANALYZE", "EXPLAIN (FORMAT JSON) DELETE FROM users"], + ["ANALYZE only inside a comment", "EXPLAIN /* ANALYZE */ DELETE FROM users"], + ["a nested comment hiding ANALYZE", "EXPLAIN /* outer /* ANALYZE */ still */ DELETE FROM users"], + ["ANALYZE commented out inside the option list", "EXPLAIN (/* ANALYZE */ VERBOSE) DELETE FROM users"], + ["DROP inside the explained SELECT's string", "EXPLAIN ANALYZE SELECT 'DROP TABLE users'"], + ["DELETE inside a comment of the explained SELECT", "EXPLAIN ANALYZE SELECT 1 /* DELETE FROM users */"], + ["ANALYZE written as a string, not an option", "EXPLAIN 'ANALYZE DELETE FROM users'"], + ["the option name written as a string", "EXPLAIN ('ANALYZE') DELETE FROM users"], + ["an unclosed option list", "EXPLAIN (ANALYZE DELETE FROM users"], + ["ANALYZE with no statement after it", "EXPLAIN ANALYZE"], + ["a DO block that is only planned", "EXPLAIN DO $$ BEGIN DELETE FROM t; END $$"], + ["ANALYZE false around a DO block", "EXPLAIN (ANALYZE false) DO $$ BEGIN DELETE FROM t; END $$"], + ["a nested EXPLAIN that does not analyze", "EXPLAIN ANALYZE EXPLAIN DELETE FROM users"], + ])("does not prompt for %s", (_label, query) => { + expect(isDangerousQuery(query, "postgres")).toBe(false); + }); + + test("does not prompt for EXPLAIN ANALYZE on a dialect whose EXPLAIN does not execute", () => { + expect(isDangerousQuery("EXPLAIN ANALYZE DELETE FROM users", "mysql")).toBe(false); + expect(isDangerousQuery("EXPLAIN ANALYZE DELETE FROM users")).toBe(false); + }); + + test("prompts for a DO block EXPLAIN ANALYZE will execute", () => { + expect(isDangerousQuery("EXPLAIN ANALYZE DO $$ BEGIN DELETE FROM t; END $$", "postgres")).toBe(true); + }); + // ── Nothing to read ───────────────────────────────────────────────────── test.each<[string, string]>([ diff --git a/tests/unit/db/sql-grammar/golden/postgres.txt b/tests/unit/db/sql-grammar/golden/postgres.txt index 214a34e50f..815e0cd8b3 100644 --- a/tests/unit/db/sql-grammar/golden/postgres.txt +++ b/tests/unit/db/sql-grammar/golden/postgres.txt @@ -1,4 +1,4 @@ -resolveSqlGrammar("postgres") => {"hash":"code","bracket":"subscript","blockComment":"nesting","alternateQuoting":false,"doubleSlashComment":false,"backslashAlwaysEscapes":false,"script":{"blocks":"none","separatorLine":null,"unit":"statement"},"trailingLimitClauses":["/\\s+AS\\s+OF\\s+(?:AT\\s+LEAST\\s+)?(?:\\d+|'[^']*')\\s*$/i"],"confirmCodeForms":[{"wordThenBody":"DO"},["DO","LANGUAGE"]]} +resolveSqlGrammar("postgres") => {"hash":"code","bracket":"subscript","blockComment":"nesting","alternateQuoting":false,"doubleSlashComment":false,"backslashAlwaysEscapes":false,"script":{"blocks":"none","separatorLine":null,"unit":"statement"},"trailingLimitClauses":["/\\s+AS\\s+OF\\s+(?:AT\\s+LEAST\\s+)?(?:\\d+|'[^']*')\\s*$/i"],"confirmCodeForms":[{"wordThenBody":"DO"},["DO","LANGUAGE"]],"explainExecutesWithAnalyze":true} readsSqlText("postgres") => true hashRunIsAmbiguous(resolveSqlGrammar("postgres")) => false readSqlSpan walk("SELECT 1 # c\nFROM t") => [["whitespace",6,7],["whitespace",8,9],["whitespace",10,11],["whitespace",12,13],["whitespace",17,18]] From e78b419975a85a5396df333fcac967d6f486ca54 Mon Sep 17 00:00:00 2001 From: notlakshitha <85143235+notlakshitha@users.noreply.github.com> Date: Thu, 22 Oct 2026 08:55:14 +0000 Subject: [PATCH 3/3] fix(query-safety): treat only a real ANALYZE false as off EXPLAIN (ANALYZE 0x1) deletes on PostgreSQL, and so do 0b1, 0o1 and 0_1. The option reader stopped at the first non-digit, so each of those was the integer 0 and the confirmation never opened. ANALYZE is off only when the whole value is a boolean false PostgreSQL accepts (false, off, no, 0 and their prefixes) or an integer literal whose value is 0. Any other value, including a nonzero numeric literal, asks. --- docs/editor/query-optimization.md | 7 +- docs/providers/postgres.md | 2 +- src/components/QuerySafetyDialog.tsx | 208 ++++++++++++++++-- .../db/providers/sql/postgres-sql-grammar.ts | 6 +- tests/components/QuerySafetyDialog.test.tsx | 57 ++++- 5 files changed, 260 insertions(+), 20 deletions(-) diff --git a/docs/editor/query-optimization.md b/docs/editor/query-optimization.md index ffb92bdefc..1d5db55e7f 100644 --- a/docs/editor/query-optimization.md +++ b/docs/editor/query-optimization.md @@ -574,7 +574,7 @@ same reading the auto-limiter uses, so the two cannot disagree about where a sta | Any of those behind a comment (`-- note`, `/* note */`, MySQL's `# note`, several stacked) | Yes | | A `WITH` whose CTE list precedes one (`WITH x AS (…) DELETE FROM …`) | Yes | | A `WITH` whose CTE list *contains* an `INSERT`, `UPDATE … SET`, `DELETE` or `MERGE` (PostgreSQL's data-modifying CTE; CockroachDB shares the grammar) | Yes | -| `EXPLAIN ANALYZE` or `EXPLAIN (ANALYZE, …)` of a statement that asks, on PostgreSQL (CockroachDB shares the grammar). `ANALYZE` set to `false`, `off` or `0`, and a plain `EXPLAIN`, only plan | Yes, the same answer as the explained statement | +| `EXPLAIN ANALYZE` or `EXPLAIN (ANALYZE, …)` of a statement that asks, on PostgreSQL (CockroachDB shares the grammar). `ANALYZE` set to a boolean false (`false`, `off`, `no`, `0` and their prefixes) or an integer 0 (`0x0`, `0b0`, `0o0`, `0_0`), and a plain `EXPLAIN`, only plan. `0x1`, `0b1`, `0o1` and `0_1` execute | Yes, the same answer as the explained statement | | A statement carrying a run the reader cannot resolve (`SELECT '\';` and anything after it, an unterminated comment, literal, name or array) | Yes, and the dialog says why | | Any of those behind a **nested** comment (`/* a /* b */ c */ DROP TABLE users`), on a dialect that nests — PostgreSQL, SQL Server, ClickHouse | Yes: the comment is read whole, so the statement's own keyword is the one that answers | | The same text on a dialect that does **not** nest — MySQL, MariaDB, SQLite, Oracle | No, because there the comment closed at the first `*/` and what follows it is text the engine rejects outright | @@ -618,7 +618,10 @@ be read as an `EXPLAIN`, which is not a dangerous keyword, so `EXPLAIN ANALYZE D deleted the rows with no confirmation (#1863). The gate strips a leading run of `ANALYZE`, `ANALYSE` and `VERBOSE`, or a parenthesised option list, and when `ANALYZE` is enabled asks about the remaining statement the way it would on its own — including a data-modifying CTE written under the `EXPLAIN`. -`ANALYZE` with no value is on; `true`, `on` and `1` are on; `false`, `off` and `0` are off. A plain +`ANALYZE` with no value is on; `true`, `on` and `1` are on. A value is off only when the whole of it +is a boolean false (`false`, `off`, `no`, `0` and their accepted prefixes) or an integer literal of +value 0, so `0x0`, `0b0`, `0o0` and `0_0` plan while `0x1`, `0b1`, `0o1` and `0_1` execute. Anything +else is on. A plain `EXPLAIN`, `EXPLAIN VERBOSE`, or `ANALYZE` set off only plans, so it does not ask, and an `UPDATE … SET` inside that plan does not either. A word inside a string or a comment is not an option and is not the statement. CockroachDB connections are the `postgres` type-id and share the grammar, so they ask too. diff --git a/docs/providers/postgres.md b/docs/providers/postgres.md index ef003ca3a0..8ef56304cb 100644 --- a/docs/providers/postgres.md +++ b/docs/providers/postgres.md @@ -1820,7 +1820,7 @@ Before this, the same file stopped at its first row with `ERROR: malformed array The editor's confirmation gate asks before the shared destructive keywords, `MERGE` among them ([query-optimization.md](../editor/query-optimization.md#destructive-statement-confirmation)). On this dialect it also asks before a `DO` block, whatever the block does: the block's code is a literal, `$$ ... $$` or `'...'`, which the gate does not read. The grammar row's `confirmCodeForms` names `DO` followed by a body and `DO LANGUAGE` ([`postgres-sql-grammar.ts`](../../src/lib/db/providers/sql/postgres-sql-grammar.ts)), so the `DO` of `INSERT ... ON CONFLICT DO NOTHING` and of `CREATE RULE ... DO INSTEAD`, which a word follows, does not ask. -`explainExecutesWithAnalyze` is set, because `EXPLAIN ANALYZE` (and `EXPLAIN (ANALYZE, …)`) runs the explained statement: the gate strips the option and asks about what remains when `ANALYZE` is on, and a plain `EXPLAIN` or `ANALYZE` set to `false`, `off` or `0` only plans, so it does not ask. +`explainExecutesWithAnalyze` is set, because `EXPLAIN ANALYZE` (and `EXPLAIN (ANALYZE, …)`) runs the explained statement: the gate strips the option and asks about what remains when `ANALYZE` is on. A plain `EXPLAIN` only plans, and so does `ANALYZE` set to a boolean false (`false`, `off`, `no`, `0` and their accepted prefixes) or to an integer literal of value 0 (`0x0`, `0b0`, `0o0`, `0_0`). A nonzero literal (`0x1`, `0b1`, `0o1`, `0_1`) executes, so it asks. CockroachDB shares this type id and this grammar. --- diff --git a/src/components/QuerySafetyDialog.tsx b/src/components/QuerySafetyDialog.tsx index 286a3396db..0a4c7b3eb9 100644 --- a/src/components/QuerySafetyDialog.tsx +++ b/src/components/QuerySafetyDialog.tsx @@ -32,7 +32,7 @@ import { import { readsSqlText, resolveSqlGrammar, type SqlGrammar } from "@/lib/sql/grammar"; import { readLeadingKeyword } from "@/lib/sql/leading-keyword"; import { readOperativeKeyword } from "@/lib/sql/operative-keyword"; -import { hasUnterminatedSpan, readSqlSpan } from "@/lib/sql/spans"; +import { hasUnterminatedSpan, IDENTIFIER_PART, IDENTIFIER_START, readSqlSpan } from "@/lib/sql/spans"; import { splitStatements } from "@/lib/sql/statement-splitter"; import { findCodeWord, readSqlWord } from "@/lib/sql/words"; import type { DatabaseType } from "@/lib/types"; @@ -633,10 +633,172 @@ const EXPLAIN_KEYWORD_OPTIONS = new Set(["ANALYZE", "ANALYSE", "VERBOSE"]); const ANALYZE_OPTIONS = new Set(["ANALYZE", "ANALYSE"]); /** - * Parenthesised values that turn `ANALYZE` off. No value, and any other value (`true`, `on`, `1`), leaves it on: - * PostgreSQL's `defGetBoolean` reads an absent argument as true. + * Whether `value` is a boolean false, by PostgreSQL's `parse_bool` (`src/backend/utils/adt/bool.c`). + * + * The whole string must be a spelling of `false`, `off`, `no` or `0`. Unique prefixes count (`f`, `fa`, `of`, `n`), + * which is that function's rule, and the case does not. A longer word does not: `falsehood` and `note` are not + * booleans, and neither is a bare `o`, which is not a unique prefix of `on` or `off`. */ -const ANALYZE_OFF = new Set(["FALSE", "OFF", "0"]); +function postgresBooleanIsFalse(value: string): boolean { + const lower = value.toLowerCase(); + const head = lower[0]; + if (head === "f") return "false".startsWith(lower); + if (head === "n") return "no".startsWith(lower); + if (head === "o") return lower === "of" || lower === "off"; + if (head === "0") return lower.length === 1; + return false; +} + +/** A decimal digit run with a single underscore allowed between digits: PostgreSQL's `{decdigit}(_?{decdigit})*`. */ +function scanDecInteger(text: string, start: number, limit: number): number { + if (start >= limit || text[start] < "0" || text[start] > "9") return start; + let end = start + 1; + while (end < limit) { + const ch = text[end]; + if (ch === "_" && end + 1 < limit && text[end + 1] >= "0" && text[end + 1] <= "9") { + end += 2; + continue; + } + if (ch >= "0" && ch <= "9") { + end += 1; + continue; + } + break; + } + return end; +} + +/** Whether every digit in `text[from, to)` is `0`. A digit is required; underscores are separators and ignored. */ +function digitsAreZero(text: string, from: number, to: number): boolean { + let digit = false; + for (let i = from; i < to; i++) { + const ch = text[i]; + if (ch === "_") continue; + if (ch !== "0") return false; + digit = true; + } + return digit; +} + +function isHexDigit(ch: string): boolean { + return (ch >= "0" && ch <= "9") || (ch >= "a" && ch <= "f") || (ch >= "A" && ch <= "F"); +} + +/** + * A `0x` / `0o` / `0b` integer, PostgreSQL's `{prefix}(_?{digit})+`. Returns `start` when the prefix is absent or no + * digit follows it, so `0x` is not read as the integer 0. + */ +function scanRadixInteger( + text: string, + start: number, + limit: number, + mark: string, + digit: (ch: string) => boolean, +): number { + if (start + 1 >= limit || text[start] !== "0" || text[start + 1].toLowerCase() !== mark) return start; + let end = start + 2; + let any = false; + while (end < limit) { + if (text[end] === "_" && end + 1 < limit && digit(text[end + 1])) { + end += 2; + any = true; + continue; + } + if (digit(text[end])) { + end += 1; + any = true; + continue; + } + break; + } + return any ? end : start; +} + +/** A fractional literal, `{decinteger}.{decinteger}?`. `1..` is not one: the scanner gives the `..` back. */ +function scanNumericLiteral(text: string, start: number, limit: number): number { + const dec = scanDecInteger(text, start, limit); + if (dec === start || dec >= limit || text[dec] !== ".") return start; + if (dec + 1 < limit && text[dec + 1] === ".") return start; + return scanDecInteger(text, dec + 1, limit); +} + +/** The end of a real literal `(decinteger|numeric)[Ee][-+]?{decinteger}`, or `start` when the exponent has no digits. */ +function scanRealLiteral(text: string, start: number, limit: number): number { + let end = scanNumericLiteral(text, start, limit); + if (end === start) end = scanDecInteger(text, start, limit); + if (end === start || end >= limit || (text[end] !== "e" && text[end] !== "E")) return start; + let exp = end + 1; + if (exp < limit && (text[exp] === "+" || text[exp] === "-")) exp += 1; + const expEnd = scanDecInteger(text, exp, limit); + return expEnd === exp ? start : expEnd; +} + +/** + * The token PostgreSQL rejects as trailing junk after an exponent sign (`0e+`, `1e-`), consumed whole so the leading + * digits are not left behind as the integer 0. + */ +function scanRealFail(text: string, start: number, limit: number): number { + let end = scanNumericLiteral(text, start, limit); + if (end === start) end = scanDecInteger(text, start, limit); + if (end === start || end >= limit || (text[end] !== "e" && text[end] !== "E")) return start; + let exp = end + 1; + if (exp < limit && (text[exp] === "+" || text[exp] === "-")) exp += 1; + if (scanDecInteger(text, exp, limit) !== exp) return start; + return exp; +} + +/** Where an identifier glued to a literal ends, or `from` when none starts there. `$` continues one, as a word does. */ +function identifierEnd(text: string, from: number, limit: number): number { + if (from >= limit || !IDENTIFIER_START.test(text[from])) return from; + let end = from + 1; + while (end < limit && (IDENTIFIER_PART.test(text[end]) || text[end] === "$")) end += 1; + return end; +} + +/** + * The numeric literal at `start`, and whether it is an integer whose value is 0. + * + * PostgreSQL's scanner takes the longest of the integer, hex, octal, binary, numeric and real forms, and an + * identifier glued on is trailing junk rather than a second token (`scan.l`). Ties keep the earlier form, so `0x1` + * is the integer 1 and not the decimal 0 followed by a name. Only an integer of value 0 is off: `0`, `00`, `0x0`, + * `0b0`, `0o0` and `0_0`. `0x1`, `0b1`, `0o1` and `0_1` are 1. A float, an exponent and a rejected literal are not + * the integer 0, so they are not off. + */ +function readNumericLiteral(text: string, start: number, limit: number): { end: number; zero: boolean } { + let bestEnd = start; + let bestRank = Number.POSITIVE_INFINITY; + let bestZero = false; + const consider = (end: number, rank: number, zero: boolean): void => { + if (end <= start) return; + if (end > bestEnd || (end === bestEnd && rank < bestRank)) { + bestEnd = end; + bestRank = rank; + bestZero = zero; + } + }; + + const decEnd = scanDecInteger(text, start, limit); + consider(decEnd, 0, digitsAreZero(text, start, decEnd)); + + const hexEnd = scanRadixInteger(text, start, limit, "x", isHexDigit); + consider(hexEnd, 1, digitsAreZero(text, start + 2, hexEnd)); + const octEnd = scanRadixInteger(text, start, limit, "o", (ch) => ch >= "0" && ch <= "7"); + consider(octEnd, 2, digitsAreZero(text, start + 2, octEnd)); + const binEnd = scanRadixInteger(text, start, limit, "b", (ch) => ch === "0" || ch === "1"); + consider(binEnd, 3, digitsAreZero(text, start + 2, binEnd)); + + const numericEnd = scanNumericLiteral(text, start, limit); + consider(numericEnd, 4, false); + const realEnd = scanRealLiteral(text, start, limit); + consider(realEnd, 5, false); + consider(scanRealFail(text, start, limit), 6, false); + + if (decEnd > start) consider(identifierEnd(text, decEnd, limit), 7, false); + if (numericEnd > start) consider(identifierEnd(text, numericEnd, limit), 8, false); + if (realEnd > start) consider(identifierEnd(text, realEnd, limit), 9, false); + + return { end: bestEnd, zero: bestZero }; +} /** Whether a `confirmExemptForms` form led by `keyword` starts at the keyword that ends at `end`. */ function exemptFormAt(text: string, keyword: string, end: number, grammar: SqlGrammar): boolean { @@ -744,10 +906,12 @@ function configKeyAsks( /** One token of a parenthesised EXPLAIN option list. */ interface ExplainOptionToken { - /** A word is upper-cased; a string is its contents; a number is its digits. */ + /** A word is upper-cased; a string is its contents; a number is the whole literal. */ text: string; end: number; kind: "word" | "number" | "comma" | "string" | "other"; + /** For a number: the literal is an integer whose value is 0. A float, junk and a nonzero integer are not. */ + zero?: boolean; } /** @@ -768,9 +932,10 @@ function readExplainOptionToken( const word = readSqlWord(text, i); if (word !== null) return { text: word.text, end: word.end, kind: "word" }; if (text[i] >= "0" && text[i] <= "9") { - let end = i; - while (end < limit && text[end] >= "0" && text[end] <= "9") end++; - return { text: text.slice(i, end), end, kind: "number" }; + const literal = readNumericLiteral(text, i, limit); + // A digit always starts a decimal literal, so the scan advances. The max keeps a failed scan from looping. + const end = Math.max(literal.end, i + 1); + return { text: text.slice(i, end), end, kind: "number", zero: literal.end > i && literal.zero }; } const span = readSqlSpan(text, i, grammar); if (span?.kind === "string") return { text: text.slice(i + 1, span.end - 1), end: span.end, kind: "string" }; @@ -802,11 +967,27 @@ function endOfParenthesised(text: string, open: number, grammar: SqlGrammar): nu return -1; } +/** + * Whether one option value turns ANALYZE off. + * + * Off is only a boolean false (`false`, `off`, `no`, `0` and their accepted prefixes) or an integer literal of + * value 0, including the signed forms `+0` and `-0`. No value is on, and so is anything else: a nonzero literal + * (`0x1`, `0b1`, `0o1`, `0_1`), a float, a word that only starts like a boolean, and a literal the scanner rejects. + * Reading the leading `0` of `0x1` as the whole value is how that `DELETE` used to run with no dialog (#1863). + */ +function analyzeValueIsOff(text: string, value: ExplainOptionToken, limit: number, grammar: SqlGrammar): boolean { + if (value.kind === "word" || value.kind === "string") return postgresBooleanIsFalse(value.text); + if (value.kind === "number") return value.zero === true; + if (value.text !== "+" && value.text !== "-") return false; + const number = readExplainOptionToken(text, value.end, limit, grammar); + return number !== null && number.kind === "number" && number.zero === true; +} + /** * Whether a parenthesised option list enables ANALYZE. * - * `false`, `off` and `0` turn that option off, and an option with no value is on. Any enabled `ANALYZE` executes, - * even when another is off, which asks rather than missing a `DELETE`. + * An option with no value is on. Any enabled `ANALYZE` executes, even when another is off, which asks rather than + * missing a `DELETE`. */ function explainOptionsExecute(text: string, open: number, close: number, grammar: SqlGrammar): boolean { const limit = close - 1; @@ -820,8 +1001,7 @@ function explainOptionsExecute(text: string, open: number, close: number, gramma const value = readExplainOptionToken(text, i, limit, grammar); if (value !== null && value.kind !== "comma") i = value.end; if (!ANALYZE_OPTIONS.has(name.text)) continue; - const setting = value === null || value.kind === "comma" ? null : value.text.toUpperCase(); - if (setting === null || !ANALYZE_OFF.has(setting)) execute = true; + if (value === null || value.kind === "comma" || !analyzeValueIsOff(text, value, limit, grammar)) execute = true; } return execute; } @@ -871,8 +1051,8 @@ export function writesUnderGrammar(text: string, grammar: SqlGrammar): boolean { // PostgreSQL's EXPLAIN executes the explained statement only when ANALYZE is on // (`EXPLAIN ANALYZE DELETE`, `EXPLAIN (ANALYZE, BUFFERS) UPDATE`). The operative // keyword is EXPLAIN, which is not itself dangerous, so without this the DELETE - // runs unconfirmed (#1863). A plain EXPLAIN, or ANALYZE set to false, off or 0, - // only plans: returning here, instead of falling through, is what keeps an + // runs unconfirmed (#1863). A plain EXPLAIN, or ANALYZE set to a boolean false + // or the integer 0, only plans: returning here, instead of falling through, is what keeps an // `UPDATE … SET` inside a plan from asking. The remaining text is read as its own // statement, so a data-modifying CTE under the EXPLAIN asks too. CockroachDB // shares this grammar. diff --git a/src/lib/db/providers/sql/postgres-sql-grammar.ts b/src/lib/db/providers/sql/postgres-sql-grammar.ts index 0a34a7eade..2683e02fdd 100644 --- a/src/lib/db/providers/sql/postgres-sql-grammar.ts +++ b/src/lib/db/providers/sql/postgres-sql-grammar.ts @@ -62,8 +62,10 @@ const POSTGRES_GRAMMAR: SqlGrammar = { // `ON CONFLICT DO NOTHING | UPDATE` and of `CREATE RULE ... DO INSTEAD | ALSO` is followed by a word and does not ask. confirmCodeForms: [{ wordThenBody: "DO" }, ["DO", "LANGUAGE"]], // `EXPLAIN` only plans. `EXPLAIN ANALYZE` (and the British `ANALYSE`) executes the explained statement, and so does - // a parenthesised `ANALYZE` whose value is absent or anything other than false, off or 0 (PostgreSQL manual, - // EXPLAIN). CockroachDB shares this type-id, so the same statements ask there (#1863). + // a parenthesised `ANALYZE` whose value is absent or anything other than a boolean false (`false`, `off`, `no`, `0` + // and their accepted prefixes) or an integer literal of value 0. `0x1`, `0b1`, `0o1` and `0_1` are 1, so they + // execute (PostgreSQL manual, EXPLAIN; the integer scanner). CockroachDB shares this type-id, so the same + // statements ask there (#1863). explainExecutesWithAnalyze: true, }; diff --git a/tests/components/QuerySafetyDialog.test.tsx b/tests/components/QuerySafetyDialog.test.tsx index 54bc66029b..e8e715e0ac 100644 --- a/tests/components/QuerySafetyDialog.test.tsx +++ b/tests/components/QuerySafetyDialog.test.tsx @@ -1555,7 +1555,9 @@ describe("isDangerousQuery", () => { /** * PostgreSQL executes the statement under `EXPLAIN ANALYZE`, in the legacy spelling and in a parenthesised * option list. The operative keyword is `EXPLAIN`, so the gate reads the statement that remains once the - * options are stripped (#1863). `ANALYZE` set to false, off or 0 only plans, and so does a plain `EXPLAIN`. + * options are stripped (#1863). `ANALYZE` is off only for a boolean false (`false`, `off`, `no`, `0` and their + * accepted prefixes) or an integer literal of value 0, and so is a plain `EXPLAIN`. `0x1`, `0b1`, `0o1` and `0_1` + * are the integer 1, so they execute. * CockroachDB is the postgres type-id and shares the grammar. A dialect that does not set * `explainExecutesWithAnalyze` is unchanged: MySQL's `EXPLAIN ANALYZE` is outside this row. */ @@ -1573,6 +1575,42 @@ describe("isDangerousQuery", () => { ["ANALYZE true", "EXPLAIN (ANALYZE true) DELETE FROM users"], ["ANALYZE on", "EXPLAIN (ANALYZE on) DELETE FROM users"], ["ANALYZE 1", "EXPLAIN (ANALYZE 1) DELETE FROM users"], + ["ANALYZE 0x1, a hexadecimal 1", "EXPLAIN (ANALYZE 0x1) DELETE FROM users"], + ["ANALYZE 0b1, a binary 1", "EXPLAIN (ANALYZE 0b1) DELETE FROM users"], + ["ANALYZE 0o1, an octal 1", "EXPLAIN (ANALYZE 0o1) DELETE FROM users"], + ["ANALYZE 0_1, a decimal 1 with a separator", "EXPLAIN (ANALYZE 0_1) DELETE FROM users"], + ["ANALYZE 0X1", "EXPLAIN (ANALYZE 0X1) DELETE FROM users"], + ["ANALYZE 0B1", "EXPLAIN (ANALYZE 0B1) DELETE FROM users"], + ["ANALYZE 0O1", "EXPLAIN (ANALYZE 0O1) DELETE FROM users"], + ["ANALYZE 0x_1", "EXPLAIN (ANALYZE 0x_1) DELETE FROM users"], + ["ANALYZE 0o_1", "EXPLAIN (ANALYZE 0o_1) DELETE FROM users"], + ["ANALYZE 0b_1", "EXPLAIN (ANALYZE 0b_1) DELETE FROM users"], + ["ANALYZE 0x10", "EXPLAIN (ANALYZE 0x10) DELETE FROM users"], + ["ANALYZE 2", "EXPLAIN (ANALYZE 2) DELETE FROM users"], + ["ANALYZE +1", "EXPLAIN (ANALYZE +1) DELETE FROM users"], + ["ANALYZE -1", "EXPLAIN (ANALYZE -1) DELETE FROM users"], + ["ANALYZE +0x1", "EXPLAIN (ANALYZE +0x1) DELETE FROM users"], + ["ANALYZE 0.0, a float rather than the integer 0", "EXPLAIN (ANALYZE 0.0) DELETE FROM users"], + ["ANALYZE 0e0", "EXPLAIN (ANALYZE 0e0) DELETE FROM users"], + ["ANALYZE 0e+1", "EXPLAIN (ANALYZE 0e+1) DELETE FROM users"], + ["ANALYZE 0e+", "EXPLAIN (ANALYZE 0e+) DELETE FROM users"], + ["ANALYZE 0e-", "EXPLAIN (ANALYZE 0e-) DELETE FROM users"], + ["ANALYZE 0x, which is not an integer", "EXPLAIN (ANALYZE 0x) DELETE FROM users"], + ["ANALYZE 0x_", "EXPLAIN (ANALYZE 0x_) DELETE FROM users"], + ["ANALYZE 0o", "EXPLAIN (ANALYZE 0o) DELETE FROM users"], + ["ANALYZE 0b", "EXPLAIN (ANALYZE 0b) DELETE FROM users"], + ["ANALYZE 0b2, trailing junk on a binary literal", "EXPLAIN (ANALYZE 0b2) DELETE FROM users"], + ["ANALYZE 0x1g, trailing junk", "EXPLAIN (ANALYZE 0x1g) DELETE FROM users"], + ["ANALYZE 0.1a", "EXPLAIN (ANALYZE 0.1a) DELETE FROM users"], + ["ANALYZE 1e2a", "EXPLAIN (ANALYZE 1e2a) DELETE FROM users"], + ["ANALYZE falsehood, which is not the boolean", "EXPLAIN (ANALYZE falsehood) DELETE FROM users"], + ["ANALYZE note, which is not the boolean", "EXPLAIN (ANALYZE note) DELETE FROM users"], + ["ANALYZE offside", "EXPLAIN (ANALYZE offside) DELETE FROM users"], + ["a bare o, which is not a unique prefix", "EXPLAIN (ANALYZE o) DELETE FROM users"], + ["ANALYZE yes", "EXPLAIN (ANALYZE yes) DELETE FROM users"], + ["ANALYZE t, a prefix of true", "EXPLAIN (ANALYZE t) DELETE FROM users"], + ["a sign that is not followed by a number", "EXPLAIN (ANALYZE +false) DELETE FROM users"], + ["a sign with no number after it", "EXPLAIN (ANALYZE +) DELETE FROM users"], ["ANALYZE 'true'", "EXPLAIN (ANALYZE 'true') DELETE FROM users"], ["a parenthesised ANALYSE", "EXPLAIN (ANALYSE, VERBOSE) DROP TABLE users"], ["FORMAT before ANALYZE", "EXPLAIN (FORMAT JSON, ANALYZE) DELETE FROM users"], @@ -1596,8 +1634,25 @@ describe("isDangerousQuery", () => { test.each<[string, string]>([ ["ANALYZE false", "EXPLAIN (ANALYZE false) DELETE FROM users"], ["ANALYZE off", "EXPLAIN (ANALYZE off) DELETE FROM users"], + ["ANALYZE no", "EXPLAIN (ANALYZE no) DELETE FROM users"], ["ANALYZE 0", "EXPLAIN (ANALYZE 0) DELETE FROM users"], ["ANALYZE 'false'", "EXPLAIN (ANALYZE 'false') DELETE FROM users"], + ["ANALYZE 'no'", "EXPLAIN (ANALYZE 'no') DELETE FROM users"], + ["ANALYZE '0'", "EXPLAIN (ANALYZE '0') DELETE FROM users"], + ["ANALYZE f, a prefix of false", "EXPLAIN (ANALYZE f) DELETE FROM users"], + ["ANALYZE n, a prefix of no", "EXPLAIN (ANALYZE n) DELETE FROM users"], + ["ANALYZE of, a prefix of off", "EXPLAIN (ANALYZE of) DELETE FROM users"], + ["ANALYZE 00", "EXPLAIN (ANALYZE 00) DELETE FROM users"], + ["ANALYZE 0x0", "EXPLAIN (ANALYZE 0x0) DELETE FROM users"], + ["ANALYZE 0b0", "EXPLAIN (ANALYZE 0b0) DELETE FROM users"], + ["ANALYZE 0o0", "EXPLAIN (ANALYZE 0o0) DELETE FROM users"], + ["ANALYZE 0_0", "EXPLAIN (ANALYZE 0_0) DELETE FROM users"], + ["ANALYZE 0X0", "EXPLAIN (ANALYZE 0X0) DELETE FROM users"], + ["ANALYZE 0x_0", "EXPLAIN (ANALYZE 0x_0) DELETE FROM users"], + ["ANALYZE +0", "EXPLAIN (ANALYZE +0) DELETE FROM users"], + ["ANALYZE -0", "EXPLAIN (ANALYZE -0) DELETE FROM users"], + ["ANALYZE +0x0", "EXPLAIN (ANALYZE +0x0) DELETE FROM users"], + ["ANALYZE 0.., the integer 0 with the range operator given back", "EXPLAIN (ANALYZE 0..) DELETE FROM users"], ["a plain EXPLAIN of a DELETE", "EXPLAIN DELETE FROM users"], ["a plain EXPLAIN of an UPDATE … SET", "EXPLAIN UPDATE users SET id = 0"], ["ANALYZE false around an UPDATE … SET", "EXPLAIN (ANALYZE false) UPDATE users SET id = 0"],