From 05d13db02cee6abb249ed753c51386004e87eec5 Mon Sep 17 00:00:00 2001 From: ZhuchkaTriplesix Date: Sun, 15 Nov 2026 16:34:05 +0300 Subject: [PATCH] feat(rust): dangerous DML confirmation gate for execute_query DROP/TRUNCATE/unconditional DELETE|UPDATE return typed NeedsConfirmation unless the UI passes confirmed=true. Closes #1885 --- apps/tauri/src-tauri/src/commands.rs | 8 +- apps/tauri/src-tauri/src/connection/error.rs | 19 ++ .../tauri/src-tauri/src/connection/manager.rs | 9 +- .../src-tauri/src/connection/sql_guard.rs | 203 +++++++++++++++--- apps/tauri/src/api/connection.ts | 32 ++- apps/tauri/src/api/query.ts | 5 +- apps/tauri/src/stores/queryResult.ts | 27 ++- 7 files changed, 271 insertions(+), 32 deletions(-) diff --git a/apps/tauri/src-tauri/src/commands.rs b/apps/tauri/src-tauri/src/commands.rs index 2a9b9ad0..6c56c2a0 100644 --- a/apps/tauri/src-tauri/src/commands.rs +++ b/apps/tauri/src-tauri/src/commands.rs @@ -484,16 +484,22 @@ pub fn db_erd_schema( /// Execute SQL and stream `QueryEvent` batches over `on_event`. /// Failures return as the invoke `Err` only (no dual channel `Error` event). +/// +/// Pass `confirmed: true` after the UI acknowledged a +/// [`ConnectionError::NeedsConfirmation`] for DROP / TRUNCATE / unconditional +/// DELETE|UPDATE. #[tauri::command] pub fn db_execute_query( id: ConnectionId, sql: String, limit: Option, + confirmed: Option, on_event: Channel, registry: State<'_, ConnectionRegistry>, ) -> Result<(), ConnectionError> { let limit = limit.map(|n| n as usize); - registry.execute_query(&id, &sql, limit, |ev| { + let confirmed = confirmed.unwrap_or(false); + registry.execute_query(&id, &sql, limit, confirmed, |ev| { on_event .send(ev) .map_err(|e| ConnectionError::io(format!("query channel closed: {e}"))) diff --git a/apps/tauri/src-tauri/src/connection/error.rs b/apps/tauri/src-tauri/src/connection/error.rs index a5da31d3..d080ffe1 100644 --- a/apps/tauri/src-tauri/src/connection/error.rs +++ b/apps/tauri/src-tauri/src/connection/error.rs @@ -18,6 +18,12 @@ pub enum ConnectionError { rule: super::sql_guard::SqlGuardRule, message: String, }, + /// Dangerous DML/DDL needs an explicit UI confirm flag before execute. + NeedsConfirmation { + #[serde(rename = "danger")] + kind: super::sql_guard::DangerousSqlKind, + message: String, + }, } impl ConnectionError { @@ -49,6 +55,16 @@ impl ConnectionError { message: message.into(), } } + + pub fn needs_confirmation( + kind: super::sql_guard::DangerousSqlKind, + message: impl Into, + ) -> Self { + Self::NeedsConfirmation { + kind, + message: message.into(), + } + } } impl fmt::Display for ConnectionError { @@ -68,6 +84,9 @@ impl fmt::Display for ConnectionError { Self::SqlGuard { rule, message } => { write!(f, "sql guard ({}): {message}", rule.as_str()) } + Self::NeedsConfirmation { kind, message } => { + write!(f, "needs confirmation ({}): {message}", kind.as_str()) + } } } } diff --git a/apps/tauri/src-tauri/src/connection/manager.rs b/apps/tauri/src-tauri/src/connection/manager.rs index 508c04b0..d80a7b64 100644 --- a/apps/tauri/src-tauri/src/connection/manager.rs +++ b/apps/tauri/src-tauri/src/connection/manager.rs @@ -226,17 +226,22 @@ impl ConnectionRegistry { } /// Driver-aware query stream (SQLite + Postgres + MySQL). + /// + /// `confirmed` must be true for DROP / TRUNCATE / unconditional DELETE|UPDATE + /// (see `sql_guard::assert_query_allowed`); otherwise returns + /// [`ConnectionError::NeedsConfirmation`]. pub fn execute_query( &self, id: &ConnectionId, sql: &str, limit: Option, + confirmed: bool, emit: impl FnMut(QueryEvent) -> ConnectionResult<()>, ) -> ConnectionResult<()> { let driver = self .driver_of(id) .ok_or_else(|| ConnectionError::NotFound { id: id.clone() })?; - super::sql_guard::assert_single_statement(sql, driver)?; + super::sql_guard::assert_query_allowed(sql, driver, confirmed)?; match driver { DriverKind::Sqlite => self.execute_sqlite_query(id, sql, limit, emit), DriverKind::Postgres => { @@ -500,7 +505,7 @@ mod tests { }) .unwrap(); let err = reg - .execute_query(&id, "SELECT 1; SELECT 2", None, |_| Ok(())) + .execute_query(&id, "SELECT 1; SELECT 2", None, false, |_| Ok(())) .unwrap_err(); match err { ConnectionError::SqlGuard { rule, .. } => { diff --git a/apps/tauri/src-tauri/src/connection/sql_guard.rs b/apps/tauri/src-tauri/src/connection/sql_guard.rs index f730e915..9ff3a3dc 100644 --- a/apps/tauri/src-tauri/src/connection/sql_guard.rs +++ b/apps/tauri/src-tauri/src/connection/sql_guard.rs @@ -1,12 +1,13 @@ -//! sqlparser-rs multi-statement guard (Flutter MCP / table-browser parity). +//! sqlparser-rs query guards (Flutter MCP / table-browser parity). //! -//! Product rule for `db_execute_query`: exactly one statement per call. -//! Trailing semicolons and comments are fine; scripts with two+ statements -//! are refused so the UI (or a later splitter) must send them separately. -//! `#1885` will layer dangerous-DML confirmation on top of this gate. +//! Product rules for `db_execute_query`: +//! 1. Exactly one statement per call (trailing `;` / comments OK). +//! 2. Dangerous DML/DDL (DROP / TRUNCATE / unconditional DELETE|UPDATE) +//! requires an explicit `confirmed` flag from the UI (#1885). use super::error::{ConnectionError, ConnectionResult}; use super::id::DriverKind; +use sqlparser::ast::{ObjectType, Statement}; use sqlparser::dialect::{GenericDialect, MySqlDialect, PostgreSqlDialect, SQLiteDialect}; use sqlparser::parser::Parser; @@ -37,6 +38,35 @@ impl SqlGuardRule { } } +/// Why a statement needs an explicit UI confirmation before running. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DangerousSqlKind { + Drop, + Truncate, + UnconditionalDelete, + UnconditionalUpdate, +} + +impl serde::Serialize for DangerousSqlKind { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.serialize_str(self.as_str()) + } +} + +impl DangerousSqlKind { + pub fn as_str(self) -> &'static str { + match self { + Self::Drop => "drop", + Self::Truncate => "truncate", + Self::UnconditionalDelete => "unconditional_delete", + Self::UnconditionalUpdate => "unconditional_update", + } + } +} + const EMPTY_QUERY_MSG: &str = "The query is empty."; fn empty_err() -> ConnectionError { @@ -47,17 +77,32 @@ fn parse_err(e: impl std::fmt::Display) -> ConnectionError { ConnectionError::sql_guard(SqlGuardRule::Parse, format!("SQL parse error: {e}")) } -/// Parse `sql` and ensure it is exactly one statement for `driver`. -pub fn assert_single_statement(sql: &str, driver: DriverKind) -> ConnectionResult<()> { +/// Full gate for `db_execute_query`: single-statement + destructive confirm. +pub fn assert_query_allowed( + sql: &str, + driver: DriverKind, + confirmed: bool, +) -> ConnectionResult<()> { + let stmt = parse_single_statement(sql, driver)?; + if confirmed { + return Ok(()); + } + if let Some((kind, message)) = classify_dangerous(&stmt) { + return Err(ConnectionError::needs_confirmation(kind, message)); + } + Ok(()) +} + +fn parse_single_statement(sql: &str, driver: DriverKind) -> ConnectionResult { let trimmed = sql.trim(); if trimmed.is_empty() { return Err(empty_err()); } - let count = statement_count(trimmed, driver)?; - match count { + let mut statements = parse_statements(trimmed, driver)?; + match statements.len() { 0 => Err(empty_err()), - 1 => Ok(()), + 1 => Ok(statements.pop().expect("len checked")), n => Err(ConnectionError::sql_guard( SqlGuardRule::MultiStatement, format!( @@ -67,22 +112,83 @@ pub fn assert_single_statement(sql: &str, driver: DriverKind) -> ConnectionResul } } -fn statement_count(sql: &str, driver: DriverKind) -> ConnectionResult { +fn parse_statements(sql: &str, driver: DriverKind) -> ConnectionResult> { let primary = match driver { DriverKind::Postgres => Parser::parse_sql(&PostgreSqlDialect {}, sql), DriverKind::Sqlite => Parser::parse_sql(&SQLiteDialect {}, sql), DriverKind::Mysql => Parser::parse_sql(&MySqlDialect {}, sql), }; - let statements = match primary { - Ok(stmts) => stmts, + match primary { + Ok(stmts) => Ok(stmts), Err(primary_err) if !matches!(driver, DriverKind::Mysql) => { - Parser::parse_sql(&GenericDialect {}, sql).map_err(|_| parse_err(primary_err))? + Parser::parse_sql(&GenericDialect {}, sql).map_err(|_| parse_err(primary_err)) } - Err(primary_err) => return Err(parse_err(primary_err)), - }; + Err(primary_err) => Err(parse_err(primary_err)), + } +} - Ok(statements.len()) +fn classify_dangerous(stmt: &Statement) -> Option<(DangerousSqlKind, String)> { + match stmt { + Statement::Drop { object_type, names, .. } => { + let target = names + .first() + .map(ToString::to_string) + .unwrap_or_else(|| "object".into()); + let label = object_type_label(*object_type); + Some(( + DangerousSqlKind::Drop, + format!("DROP {label} \"{target}\" requires confirmation."), + )) + } + Statement::Truncate(truncate) => { + let target = truncate + .table_names + .first() + .map(|t| t.name.to_string()) + .unwrap_or_else(|| "table".into()); + Some(( + DangerousSqlKind::Truncate, + format!("TRUNCATE \"{target}\" requires confirmation."), + )) + } + Statement::Delete(delete) if delete.selection.is_none() => { + let target = delete_target_name(delete); + Some(( + DangerousSqlKind::UnconditionalDelete, + format!( + "DELETE FROM \"{target}\" without WHERE requires confirmation." + ), + )) + } + Statement::Update(update) if update.selection.is_none() => { + let target = update.table.relation.to_string(); + Some(( + DangerousSqlKind::UnconditionalUpdate, + format!( + "UPDATE \"{target}\" without WHERE requires confirmation." + ), + )) + } + _ => None, + } +} + +fn object_type_label(object_type: ObjectType) -> String { + object_type.to_string() +} + +fn delete_target_name(delete: &sqlparser::ast::Delete) -> String { + if let Some(name) = delete.tables.first() { + return name.to_string(); + } + match &delete.from { + sqlparser::ast::FromTable::WithFromKeyword(tables) + | sqlparser::ast::FromTable::WithoutKeyword(tables) => tables + .first() + .map(|t| t.relation.to_string()) + .unwrap_or_else(|| "table".into()), + } } #[cfg(test)] @@ -90,19 +196,34 @@ mod tests { use super::*; fn allow(sql: &str, driver: DriverKind) { - assert_single_statement(sql, driver).unwrap_or_else(|e| { + assert_query_allowed(sql, driver, false).unwrap_or_else(|e| { panic!("expected allow for {sql:?} ({driver}): {e}"); }); } + fn allow_confirmed(sql: &str, driver: DriverKind) { + assert_query_allowed(sql, driver, true).unwrap_or_else(|e| { + panic!("expected allow with confirm for {sql:?} ({driver}): {e}"); + }); + } + fn block_rule(sql: &str, driver: DriverKind, rule: SqlGuardRule) { - let err = assert_single_statement(sql, driver).expect_err("expected block"); + let err = assert_query_allowed(sql, driver, false).expect_err("expected block"); match err { ConnectionError::SqlGuard { rule: r, .. } => assert_eq!(r, rule), other => panic!("expected SqlGuard, got {other:?}"), } } + fn needs_confirm(sql: &str, driver: DriverKind, kind: DangerousSqlKind) { + let err = assert_query_allowed(sql, driver, false).expect_err("expected confirm"); + match err { + ConnectionError::NeedsConfirmation { kind: k, .. } => assert_eq!(k, kind), + other => panic!("expected NeedsConfirmation, got {other:?}"), + } + allow_confirmed(sql, driver); + } + #[test] fn allows_single_select() { allow("SELECT 1", DriverKind::Sqlite); @@ -137,13 +258,36 @@ mod tests { } #[test] - fn allows_single_dml_for_now() { - // Dangerous-DML confirmation is #1885; this gate only counts statements. - allow("DELETE FROM t", DriverKind::Postgres); - allow("UPDATE t SET a = 1", DriverKind::Sqlite); + fn allows_conditional_dml_without_confirm() { + allow("DELETE FROM t WHERE id = 1", DriverKind::Postgres); + allow("UPDATE t SET a = 1 WHERE id = 2", DriverKind::Sqlite); allow("INSERT INTO t VALUES (1)", DriverKind::Mysql); } + #[test] + fn requires_confirm_for_drop_truncate_unconditional() { + needs_confirm( + "DROP TABLE t", + DriverKind::Postgres, + DangerousSqlKind::Drop, + ); + needs_confirm( + "TRUNCATE TABLE t", + DriverKind::Postgres, + DangerousSqlKind::Truncate, + ); + needs_confirm( + "DELETE FROM t", + DriverKind::Sqlite, + DangerousSqlKind::UnconditionalDelete, + ); + needs_confirm( + "UPDATE t SET a = 1", + DriverKind::Mysql, + DangerousSqlKind::UnconditionalUpdate, + ); + } + #[test] fn rejects_multi_statement_even_when_first_is_select() { block_rule( @@ -214,4 +358,15 @@ mod tests { assert_eq!(json["kind"], "sql_guard"); assert_eq!(json["rule"], "single_statement"); } -} \ No newline at end of file + + #[test] + fn needs_confirmation_serde() { + let err = ConnectionError::needs_confirmation( + DangerousSqlKind::Drop, + "DROP TABLE \"t\" requires confirmation.", + ); + let json = serde_json::to_value(&err).unwrap(); + assert_eq!(json["kind"], "needs_confirmation"); + assert_eq!(json["danger"], "drop"); + } +} diff --git a/apps/tauri/src/api/connection.ts b/apps/tauri/src/api/connection.ts index ac1f66f4..2233aa13 100644 --- a/apps/tauri/src/api/connection.ts +++ b/apps/tauri/src/api/connection.ts @@ -35,6 +35,12 @@ export type SqlObjectNode = { /** Mirrors Rust `ConnectionError` (serde tag = kind). */ export type SqlGuardRule = "empty_query" | "single_statement" | "parse_error"; +export type DangerousSqlKind = + | "drop" + | "truncate" + | "unconditional_delete" + | "unconditional_update"; + export type ConnectionError = | { kind: "not_found"; id: string } | { kind: "already_connected"; id: string } @@ -43,7 +49,12 @@ export type ConnectionError = | { kind: "unsupported_driver"; driver: string } | { kind: "io"; message: string } | { kind: "driver"; driver: string; message: string } - | { kind: "sql_guard"; rule: SqlGuardRule; message: string }; + | { kind: "sql_guard"; rule: SqlGuardRule; message: string } + | { + kind: "needs_confirmation"; + danger: DangerousSqlKind; + message: string; + }; export type ColumnMeta = { name: string; @@ -396,6 +407,8 @@ export function formatConnectionError(err: unknown): string { switch (err.kind) { case "sql_guard": return `sql_guard: ${err.rule}: ${err.message}`; + case "needs_confirmation": + return err.message; case "driver": return `${err.driver}: ${err.message}`; case "not_found": @@ -415,3 +428,20 @@ export function formatConnectionError(err: unknown): string { } return String(err); } + +/** Unwrap Tauri invoke payload when the error is nested under `error` / data. */ +export function asConnectionError(err: unknown): ConnectionError | null { + if (isConnectionError(err)) return err; + if (err && typeof err === "object") { + const e = err as Record; + if (isConnectionError(e.error)) return e.error; + if (isConnectionError(e.data)) return e.data; + } + return null; +} + +export function isNeedsConfirmation( + err: unknown, +): err is Extract { + return asConnectionError(err)?.kind === "needs_confirmation"; +} diff --git a/apps/tauri/src/api/query.ts b/apps/tauri/src/api/query.ts index 3ef225ad..3afa4a4b 100644 --- a/apps/tauri/src/api/query.ts +++ b/apps/tauri/src/api/query.ts @@ -19,14 +19,15 @@ export async function dbExecuteQuery( id: string, sql: string, onEvent: QueryEventHandler, - limit?: number, + opts?: { limit?: number; confirmed?: boolean }, ): Promise { const channel = new Channel(); channel.onmessage = onEvent; await invoke("db_execute_query", { id, sql, - limit: limit ?? null, + limit: opts?.limit ?? null, + confirmed: opts?.confirmed ?? false, onEvent: channel, }); } diff --git a/apps/tauri/src/stores/queryResult.ts b/apps/tauri/src/stores/queryResult.ts index aae63a02..bbfafb4e 100644 --- a/apps/tauri/src/stores/queryResult.ts +++ b/apps/tauri/src/stores/queryResult.ts @@ -5,8 +5,12 @@ import { applyQueryEvent, emptyAccumulatedQuery, } from "../api/queryAccumulate"; -import { formatConnectionError } from "../api/connection"; +import { + asConnectionError, + formatConnectionError, +} from "../api/connection"; import { useConnectionsStore } from "./connections"; +import { usePreferencesStore } from "./preferences"; import { useShellNavStore } from "./shellNav"; import { useShellStatusStore } from "./shellStatus"; @@ -118,7 +122,26 @@ export const useQueryResultStore = defineStore("queryResult", () => { running.value = true; status.setQueryMetrics({ message: "Running…", busy: true }); try { - await dbExecuteQuery(id, trimmed, feedEvent); + const prefs = usePreferencesStore(); + // Pref off → skip dialog and send confirmed on the first attempt. + let confirmed = !prefs.state.confirmDestructive; + try { + await dbExecuteQuery(id, trimmed, feedEvent, { confirmed }); + } catch (err) { + const needs = asConnectionError(err); + if (needs?.kind !== "needs_confirmation" || confirmed) throw err; + const ok = window.confirm( + `${needs.message}\n\nRun this statement anyway?`, + ); + if (!ok) { + error.value = "Cancelled"; + status.setQueryMetrics({ message: error.value, busy: false }); + return; + } + confirmed = true; + reset(); + await dbExecuteQuery(id, trimmed, feedEvent, { confirmed }); + } if (error.value) { status.setQueryMetrics({ message: error.value, busy: false }); } else if (affectedRows.value != null && columns.value.length === 0) {