fix(linux-safe-mode): run MySQL executable comments and read them as each server does

linux/changelog.d/3790.fixed.md

@@ -0,0 +1 @@
+The SQL editor skipped MySQL `/*! */` statements, so a dump's SET NAMES and time zone lines never ran.

linux/changelog.d/3790.security.md

@@ -0,0 +1 @@
+Safe Mode reads MySQL and MariaDB executable comments and dollar quotes the way each server version runs them.

linux/crates/app/src/services/history_service.rs

@@ -4,7 +4,7 @@ use std::time::Duration;
 use tablepro_core::dialect::SqlDialect;
 use tablepro_core::sql_syntax::HistoryRecording;
 use tablepro_session::runtime::Tasks;
-use tablepro_storage::query_history::NewEntry;
+use tablepro_storage::query_history::{MAX_QUERY_BYTES, NewEntry};
 use tablepro_storage::{QueryHistory, StoragePaths};
 use tokio::sync::watch;
 
@@ -97,6 +97,12 @@ impl HistoryService {
                 tracing::debug!(connection = %entry.connection_id, "no dialect, so the run is not recorded");
                 return;
             };
+            // The store refuses it anyway, and reading it for credentials
+            // lexes it once per way the server may read it.
+            if entry.query.len() > MAX_QUERY_BYTES {
+                tracing::debug!(connection = %entry.connection_id, "a run too large for the history was not recorded");
+                return;
+            }
             if let HistoryRecording::Withhold(reason) = dialect.history_recording(&entry.query) {
                 tracing::debug!(connection = %entry.connection_id, ?reason, "a run was kept out of the history");
                 return;

linux/crates/app/src/ui/editor/catalog_change.rs

@@ -1,5 +1,5 @@
 use tablepro_core::sql_syntax::SqlGrammar;
-use tablepro_core::sql_syntax::script::{LexicalSettings, ScriptPlan, ScriptTokenKind};
+use tablepro_core::sql_syntax::script::{ScriptPlan, ScriptTokenKind};
 
 /// Words that add, rename or remove what the sidebar lists. They count
 /// anywhere in a statement: a SQL Server batch needs no `;` between its
@@ -10,11 +10,15 @@ const SCHEMA_VERBS: [&str; 7] = ["CREATE", "ALTER", "DROP", "RENAME", "ATTACH",
 /// once it commits.
 const TRANSACTION_ENDS: [&str; 2] = ["COMMIT", "END"];
 
-/// Whether running `text` can change the tables the catalog lists. A
-/// commit counts as the statement's first word, and `SELECT … INTO`
-/// counts too, since PostgreSQL and SQL Server create its target table.
+/// Whether running `text` can change the tables the catalog lists, read
+/// every way the server may read it. A commit counts as the statement's
+/// first word, and `SELECT … INTO` counts too, since PostgreSQL and SQL
+/// Server create its target table.
 pub(super) fn changes_catalog(text: &str, grammar: SqlGrammar) -> bool {
-    let plan = ScriptPlan::build(text, grammar, LexicalSettings::default_for(grammar));
+    ScriptPlan::server_readings(text, grammar).is_none_or(|mut plans| plans.any(|plan| plan_changes(text, &plan)))
+}
+
+fn plan_changes(text: &str, plan: &ScriptPlan) -> bool {
     let is = |word: &str, set: &[&str]| set.iter().any(|verb| word.eq_ignore_ascii_case(verb));
     plan.statements().iter().any(|statement| {
         let words: Vec<&str> = plan
@@ -102,6 +106,17 @@ mod tests {
         assert!(!changes_catalog("SELECT id FROM orders", SqlGrammar::PostgreSql));
     }
 
+    #[test]
+    fn ddl_only_some_mysql_server_versions_run_counts() {
+        for sql in [
+            "/*!50001 CREATE VIEW v AS SELECT 1 */",
+            "SELECT 1 /*!99999 /* */ ' */ ; DROP TABLE orders; -- '",
+            r"SELECT 'C:\'; DROP TABLE orders; -- '",
+        ] {
+            assert!(changes_catalog(sql, SqlGrammar::MySql), "{sql}");
+        }
+    }
+
     #[test]
     fn sqlite_attach_and_rename_count() {
         assert!(changes_catalog("ATTACH DATABASE 'x.db' AS x", SqlGrammar::Sqlite));

linux/crates/app/src/ui/editor/completion/completion_scope.rs

@@ -63,9 +63,10 @@ pub fn analyze(text: &str, cursor: usize, grammar: SqlGrammar, settings: Lexical
             tables_in_statement: Vec::new(),
         });
     };
+    // What a MySQL executable comment holds is code, its markers are not.
     let lexemes: Vec<Lexeme<'_>> = plan
         .tokens(statement)
-        .filter(|token| !token.kind.is_trivia())
+        .filter(|token| !token.kind.is_trivia() && token.kind != ScriptTokenKind::ExecutableMarker)
         .filter_map(|token| Lexeme::new(text, token))
         .collect();
     let tables = TableMap::collect(&lexemes, grammar);
@@ -359,7 +360,8 @@ fn encloses(token: &ScriptToken, text: &str, cursor: usize) -> bool {
         ScriptTokenKind::StringLiteral
         | ScriptTokenKind::QuotedIdentifier
         | ScriptTokenKind::DollarQuoted
-        | ScriptTokenKind::BlockComment => inside,
+        | ScriptTokenKind::BlockComment
+        | ScriptTokenKind::ExecutableMarker => inside,
         _ => false,
     }
 }
@@ -577,6 +579,15 @@ mod tests {
         );
     }
 
+    #[test]
+    fn a_mysql_executable_comment_is_code() {
+        assert_eq!(
+            scope_in(SqlGrammar::MySql, "SELECT * FROM orders /*! AS */ o WHERE o.|"),
+            columns(vec![table(None, "orders")])
+        );
+        assert_eq!(scope_in(SqlGrammar::MySql, "SELECT /*!40|1 x */ 1"), None);
+    }
+
     #[test]
     fn strings_and_comments_are_not_completed() {
         assert_eq!(scope("SELECT 'FROM |'"), None);

linux/crates/app/src/ui/editor/safe_mode_gate.rs

@@ -105,6 +105,41 @@ mod tests {
         );
     }
 
+    #[test]
+    fn a_mysql_executable_comment_is_sent_and_read_as_the_server_reads_it() {
+        // MySQL 8.0 skips the second comment past its nested `/* */` and
+        // runs the DROP after it.
+        let script = "/*!40103 SET TIME_ZONE='+00:00' */;\nSELECT 1 /*!99999 /* */ ' */ ; DROP TABLE orders; -- '";
+
+        let off = gated(script, SqlGrammar::MySql, SafeModeLevel::Off);
+        let read_only = gated(script, SqlGrammar::MySql, SafeModeLevel::ReadOnly);
+
+        let texts: Vec<&str> = off.batches.iter().map(|batch| batch.text.as_str()).collect();
+        assert_eq!(
+            texts,
+            [
+                "/*!40103 SET TIME_ZONE='+00:00' */",
+                "SELECT 1 /*!99999 /* */ ' */ ; DROP TABLE orders; -- '"
+            ]
+        );
+        assert_eq!(off.confirm, ["SELECT 1 /*!99999 /* */ ' */ ; DROP TABLE orders; -- '"]);
+        assert!(off.destructive);
+        assert!(read_only.batches.iter().all(|batch| batch.refused));
+    }
+
+    #[test]
+    fn a_batch_the_driver_splits_again_is_read_piece_by_piece() {
+        // The driver reads the batch's second line as a DELIMITER line and
+        // sends the last line alone, where MySQL 8.0 runs the DROP.
+        let script = "DELIMITER //\nSELECT 1;\nDELIMITER ; /*!99999 '*/ `' */\n\
+                      SELECT 1 /*!99999 ' */ ; DROP TABLE t; -- ` ' */\n//";
+
+        let run = gated(script, SqlGrammar::MySql, SafeModeLevel::Off);
+
+        assert_eq!(run.batches.len(), 1);
+        assert!(run.destructive);
+    }
+
     #[test]
     fn read_only_keeps_back_every_write_and_asks_nothing() {
         let run = gated(

linux/crates/app/src/ui/editor/significant_tokens.rs

@@ -10,8 +10,26 @@ pub fn significant_tokens(text: &str, grammar: SqlGrammar, settings: LexicalSett
     let plan = ScriptPlan::build(text, grammar, settings);
     let mut tokens: Vec<SignificantToken> = Vec::new();
     let mut spacing = Spacing::default();
+    // A MySQL executable comment is compared as written: the formatter
+    // takes it for a comment, and which of its text is a string depends
+    // on the server.
+    let mut executable: Option<usize> = None;
     for token in plan.tokens(0..text.len()) {
         let slice = text.get(token.range.clone()).unwrap_or_default();
+        if token.kind == ScriptTokenKind::ExecutableMarker || executable.is_some() {
+            let start = *executable.get_or_insert(token.range.start);
+            if token.kind == ScriptTokenKind::ExecutableMarker && slice == "*/" {
+                executable = None;
+                push(
+                    &mut tokens,
+                    &mut spacing,
+                    grammar,
+                    token.kind,
+                    text.get(start..token.range.end).unwrap_or_default(),
+                );
+            }
+            continue;
+        }
         if token.kind == ScriptTokenKind::Whitespace {
             spacing.spaced = true;
             spacing.line_break |= slice.contains('\n');
@@ -23,19 +41,39 @@ pub fn significant_tokens(text: &str, grammar: SqlGrammar, settings: LexicalSett
             ScriptTokenKind::LineComment | ScriptTokenKind::BlockComment => slice.trim_end(),
             _ => slice,
         };
-        let gap = tokens.last().map_or(Gap::Free, |previous| {
-            gap_between(grammar, previous, token.kind, body, spacing)
-        });
-        tokens.push(SignificantToken {
-            kind: token.kind,
-            text: body.to_owned(),
-            gap,
-        });
-        spacing = Spacing::default();
+        push(&mut tokens, &mut spacing, grammar, token.kind, body);
+    }
+    if let Some(start) = executable {
+        let rest = text.get(start..).unwrap_or_default();
+        push(
+            &mut tokens,
+            &mut spacing,
+            grammar,
+            ScriptTokenKind::ExecutableMarker,
+            rest,
+        );
     }
     tokens
 }
 
+fn push(
+    tokens: &mut Vec<SignificantToken>,
+    spacing: &mut Spacing,
+    grammar: SqlGrammar,
+    kind: ScriptTokenKind,
+    body: &str,
+) {
+    let gap = tokens.last().map_or(Gap::Free, |previous| {
+        gap_between(grammar, previous, kind, body, *spacing)
+    });
+    tokens.push(SignificantToken {
+        kind,
+        text: body.to_owned(),
+        gap,
+    });
+    *spacing = Spacing::default();
+}
+
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct SignificantToken {
     kind: ScriptTokenKind,
@@ -109,7 +147,8 @@ fn spacing_is_free(kind: ScriptTokenKind, text: &str) -> bool {
         | ScriptTokenKind::CloseParen
         | ScriptTokenKind::Semicolon
         | ScriptTokenKind::LineComment
-        | ScriptTokenKind::BlockComment => true,
+        | ScriptTokenKind::BlockComment
+        | ScriptTokenKind::ExecutableMarker => true,
         ScriptTokenKind::Punct => matches!(
             text,
             "," | "." | "=" | "<" | ">" | "+" | "-" | "*" | "/" | "%" | "^" | "|" | "!" | "~" | "[" | "]"
@@ -130,6 +169,22 @@ mod tests {
         tokens_for(text, SqlGrammar::PostgreSql)
     }
 
+    #[test]
+    fn a_mysql_executable_comment_must_stay_as_written() {
+        // Under NO_BACKSLASH_ESCAPES the indented line is inside a string.
+        let source = "SELECT 1 /*! , 'a\\' AS x, '\n    foo\n' */";
+        let reindented = "SELECT 1 /*! , 'a\\' AS x, '\n foo\n' */";
+
+        assert_ne!(
+            tokens_for(source, SqlGrammar::MySql),
+            tokens_for(reindented, SqlGrammar::MySql)
+        );
+        assert_eq!(
+            tokens_for(source, SqlGrammar::MySql),
+            tokens_for(&source.replace("1 /*!", "1\n/*!"), SqlGrammar::MySql)
+        );
+    }
+
     #[test]
     fn spacing_alone_does_not_change_a_statement() {
         assert_eq!(tokens("select  a ,b from t"), tokens("select a, b\nfrom t"));

linux/crates/core/src/sql_syntax/history_recording.rs

@@ -1,6 +1,8 @@
 use super::SqlGrammar;
 use super::credential_rules::CredentialRules;
-use super::statement_tokens::{StatementToken, lex_statements};
+use super::script::ScriptPlan;
+use super::statement_access::CLIENT_SPLITS;
+use super::statement_tokens::{StatementToken, lex_statements, statements_of};
 
 /// Whether what the user ran may go into the query history.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -23,17 +25,43 @@ pub enum WithholdReason {
 /// nest.
 const MAX_BODY_DEPTH: u8 = 4;
 
-/// Whether `sql` may be kept, split the way the editor splits it for
-/// `grammar`. The text is never rewritten: an entry is kept whole or not
+/// Whether `sql` may be kept, read every way a server of `grammar` may
+/// read it. The text is never rewritten: an entry is kept whole or not
 /// at all, so History can neither show nor run a statement with a secret
 /// cut out of it.
 pub fn classify_for_history(grammar: SqlGrammar, rules: &CredentialRules, sql: &str) -> HistoryRecording {
-    let script = lex_statements(sql, grammar);
-    if script.unterminated {
+    classify_as_sent(grammar, rules, sql, CLIENT_SPLITS)
+}
+
+/// The whole text, then each piece a `DELIMITER` line cuts it into, which
+/// a client sends apart.
+fn classify_as_sent(grammar: SqlGrammar, rules: &CredentialRules, sql: &str, splits: u8) -> HistoryRecording {
+    let whole = classify_every_way(grammar, rules, sql);
+    if whole != HistoryRecording::Record || splits == 0 {
+        return whole;
+    }
+    ScriptPlan::client_pieces(sql, grammar)
+        .into_iter()
+        .map(|piece| classify_as_sent(grammar, rules, piece, splits - 1))
+        .find(|recording| *recording != HistoryRecording::Record)
+        .unwrap_or(HistoryRecording::Record)
+}
+
+fn classify_every_way(grammar: SqlGrammar, rules: &CredentialRules, sql: &str) -> HistoryRecording {
+    let Some(plans) = ScriptPlan::server_readings(sql, grammar) else {
         return HistoryRecording::Withhold(WithholdReason::Untokenizable);
+    };
+    let mut unterminated = false;
+    for plan in plans {
+        let script = statements_of(sql, &plan);
+        if script.unterminated {
+            unterminated = true;
+        } else if carries_credentials(grammar, rules, &script.statements, 0) {
+            return HistoryRecording::Withhold(WithholdReason::CredentialStatement);
+        }
     }
-    match carries_credentials(grammar, rules, &script.statements, 0) {
-        true => HistoryRecording::Withhold(WithholdReason::CredentialStatement),
+    match unterminated {
+        true => HistoryRecording::Withhold(WithholdReason::Untokenizable),
         false => HistoryRecording::Record,
     }
 }
@@ -395,6 +423,23 @@ mod tests {
         );
     }
 
+    #[test]
+    fn a_credential_any_server_reading_runs_is_withheld() {
+        for sql in [
+            "SELECT 1 /*!, '*/'; CREATE USER u IDENTIFIED BY 'p'; SELECT ' */",
+            "SELECT 1 /*!50000 # */ '\n; CREATE USER u IDENTIFIED BY 'p'; -- '\n*/",
+            // A server older than 99999 skips the comment past its nested
+            // `/* */`, so the CREATE USER after it runs.
+            "SELECT 1 /*!99999 /* */ ' */ ; CREATE USER u IDENTIFIED BY 'p'; -- ' */",
+            // Split again at its DELIMITER line, the last line goes alone.
+            "SELECT 1;\nDELIMITER ; /*!99999 '*/ `' */\nSELECT 1 /*!99999 ' */ ; CREATE USER u IDENTIFIED BY 'p'; -- ` ' */",
+            // Under NO_BACKSLASH_ESCAPES the first string ends at `\'`.
+            r"INSERT INTO paths VALUES ('C:\temp\'); CREATE USER u IDENTIFIED BY 'p'; -- it's done",
+        ] {
+            assert_ne!(classify(SqlGrammar::MySql, sql), HistoryRecording::Record, "{sql}");
+        }
+    }
+
     proptest! {
         #[test]
         fn any_text_classifies_without_panicking(sql in "\\PC{0,120}", grammar in 0usize..5) {