diff --git a/lib/core/database/destructive_sql_detector.dart b/lib/core/database/destructive_sql_detector.dart index cbb94996..415cb956 100644 --- a/lib/core/database/destructive_sql_detector.dart +++ b/lib/core/database/destructive_sql_detector.dart @@ -1,3 +1,4 @@ +import 'package:querya_desktop/core/database/sql_lexical_scanner.dart'; import 'package:querya_desktop/core/database/sql_statement_splitter.dart'; /// Categorization of destructive SQL / Mongo / Redis operations that can @@ -267,7 +268,12 @@ abstract final class DestructiveSqlDetector { /// Offset after the single-quoted literal whose body starts at [start] /// (`''` and backslash escapes); [len] when it is never closed. - static int _skipStringLiteral(String sql, int start, int len) { + static int _skipStringLiteral( + String sql, + int start, + int len, { + required bool backslashEscapes, + }) { var i = start; while (i < len) { final c = sql.codeUnitAt(i); @@ -277,7 +283,7 @@ abstract final class DestructiveSqlDetector { } else { return i + 1; // closing quote } - } else if (c == _chBackslash && i + 1 < len) { + } else if (backslashEscapes && c == _chBackslash && i + 1 < len) { i += 2; // escaped char } else { i++; @@ -292,7 +298,14 @@ abstract final class DestructiveSqlDetector { /// Scans code units with the `_skip*` helpers (no per-character strings, no /// regex, no copy of the rest of the script at every `$`), and appends the /// text between tokens in runs (#1511). - static String stripCommentsAndStrings(String sql) { + /// + /// [backslashEscapes] is whether `\'` escapes a quote (MySQL; the default). + /// With `false` (PostgreSQL, SQLite) a backslash is an ordinary character, + /// except inside an `E'...'` string, as the server reads it. + static String stripCommentsAndStrings( + String sql, { + bool backslashEscapes = true, + }) { final buffer = StringBuffer(); final len = sql.length; var i = 0; @@ -322,7 +335,13 @@ abstract final class DestructiveSqlDetector { } replacement = "''"; } else if (c == _chQuote) { - after = _skipStringLiteral(sql, i + 1, len); + after = _skipStringLiteral( + sql, + i + 1, + len, + backslashEscapes: + backslashEscapes || SqlLexicalScanner.isEscapeStringPrefix(sql, i), + ); replacement = "''"; } else { i++; @@ -341,17 +360,33 @@ abstract final class DestructiveSqlDetector { /// Splits an SQL query into individual statements on `;`, taking into account /// comments and string literals. - static List splitStatements(String sql) => [ - for (final span in SqlStatementSplitter.spans(sql)) span.textIn(sql), + /// + /// [backslashEscapes]: see [stripCommentsAndStrings]. + static List splitStatements( + String sql, { + bool backslashEscapes = true, + }) => + [ + for (final span + in SqlStatementSplitter.spans(sql, backslashEscapes: backslashEscapes)) + span.textIn(sql), ]; /// Inspects [sql] and returns any detected destructive operations. - static DestructiveSqlInspectionResult inspect(String sql) { - final statements = splitStatements(sql); + /// + /// [backslashEscapes]: see [stripCommentsAndStrings]. + static DestructiveSqlInspectionResult inspect( + String sql, { + bool backslashEscapes = true, + }) { + final statements = splitStatements(sql, backslashEscapes: backslashEscapes); final operations = []; for (final rawStmt in statements) { - final sanitized = stripCommentsAndStrings(rawStmt).trim(); + final sanitized = stripCommentsAndStrings( + rawStmt, + backslashEscapes: backslashEscapes, + ).trim(); if (sanitized.isEmpty) continue; // 1. DROP DATABASE diff --git a/lib/core/database/sql_lexical_scanner.dart b/lib/core/database/sql_lexical_scanner.dart index f36296da..7125c0df 100644 --- a/lib/core/database/sql_lexical_scanner.dart +++ b/lib/core/database/sql_lexical_scanner.dart @@ -118,7 +118,7 @@ abstract final class SqlLexicalScanner { } if (code == _chSingleQuote) { - final escapes = backslashEscapes || _isEscapeStringPrefix(sql, i); + final escapes = backslashEscapes || isEscapeStringPrefix(sql, i); var j = i + 1; while (j < len) { final curr = sql.codeUnitAt(j); @@ -169,7 +169,7 @@ abstract final class SqlLexicalScanner { /// Whether the quote at [quote] opens a PostgreSQL escape string: `E'` or /// `e'`, the `E` not being the end of a longer word (`TYPE'...'`). - static bool _isEscapeStringPrefix(String sql, int quote) { + static bool isEscapeStringPrefix(String sql, int quote) { if (quote < 1) return false; final e = sql.codeUnitAt(quote - 1); if (e != 0x45 && e != 0x65) return false; diff --git a/lib/core/database/sql_mutation_classifier.dart b/lib/core/database/sql_mutation_classifier.dart index f051893b..b3366ddd 100644 --- a/lib/core/database/sql_mutation_classifier.dart +++ b/lib/core/database/sql_mutation_classifier.dart @@ -12,9 +12,14 @@ final _dataModifyingCte = RegExp(r'\b(INSERT|UPDATE|DELETE|MERGE)\b'); /// /// Comments and string literals are ignored. `WITH ...` statements count when /// a data-modifying statement appears inside them. -bool isMutatingSqlStatement(String statement) { - final sanitized = - DestructiveSqlDetector.stripCommentsAndStrings(statement).trim(); +/// +/// [backslashEscapes]: whether `\'` escapes a quote (MySQL; the default), see +/// [DestructiveSqlDetector.stripCommentsAndStrings]. +bool isMutatingSqlStatement(String statement, {bool backslashEscapes = true}) { + final sanitized = DestructiveSqlDetector.stripCommentsAndStrings( + statement, + backslashEscapes: backslashEscapes, + ).trim(); if (sanitized.isEmpty) return false; final upper = sanitized.toUpperCase(); final first = _firstWord.firstMatch(upper)?.group(1); @@ -25,6 +30,8 @@ bool isMutatingSqlStatement(String statement) { } /// Whether the SQL script [sql] contains at least one mutating statement. -bool containsMutatingSql(String sql) => DestructiveSqlDetector - .splitStatements(sql) - .any(isMutatingSqlStatement); +bool containsMutatingSql(String sql, {bool backslashEscapes = true}) => + DestructiveSqlDetector.splitStatements( + sql, + backslashEscapes: backslashEscapes, + ).any((s) => isMutatingSqlStatement(s, backslashEscapes: backslashEscapes)); diff --git a/lib/core/database/sql_statement_splitter.dart b/lib/core/database/sql_statement_splitter.dart index 5f36951d..b609c343 100644 --- a/lib/core/database/sql_statement_splitter.dart +++ b/lib/core/database/sql_statement_splitter.dart @@ -1,4 +1,5 @@ import 'package:flutter/foundation.dart'; +import 'package:querya_desktop/core/database/sql_lexical_scanner.dart'; /// One statement of a script, as offsets into the script. class SqlStatementSpan { @@ -24,21 +25,8 @@ class SqlStatementSpan { /// literals, quoted identifiers, comments and PostgreSQL dollar quotes do not /// end a statement. abstract final class SqlStatementSplitter { - /// `$tag$` or `$$`, matched at an offset (no `^`: it would only match at the - /// start of the script). One compiled instance for every dollar sign. - static final RegExp _dollarTag = RegExp(r'\$([a-zA-Z0-9_]*)\$'); - - static const int _chDash = 0x2D; - static const int _chSlash = 0x2F; - static const int _chAsterisk = 0x2A; - static const int _chDollar = 0x24; - static const int _chSingleQuote = 0x27; - static const int _chDoubleQuote = 0x22; - static const int _chBacktick = 0x60; static const int _chSemicolon = 0x3B; - static const int _chBackslash = 0x5C; static const int _chLf = 0x0A; - static const int _chCr = 0x0D; /// Default script size threshold (64 KB) above which [spansAsync] offloads /// parsing to a background isolate via [compute]. @@ -50,17 +38,28 @@ abstract final class SqlStatementSplitter { static Future> spansAsync( String sql, { int isolateThreshold = defaultIsolateThreshold, + bool backslashEscapes = true, }) async { if (sql.length <= isolateThreshold) { - return spans(sql); + return spans(sql, backslashEscapes: backslashEscapes); } - return compute(spans, sql); + return compute(_spansInIsolate, (sql, backslashEscapes)); } - static List spans(String sql) { + static List _spansInIsolate((String, bool) args) => + spans(args.$1, backslashEscapes: args.$2); + + /// [backslashEscapes] says whether `\'` escapes a quote inside `'...'`, as + /// it does in MySQL (the default, and what an unknown dialect gets). Pass + /// `dialect.backslashEscapesQuotes` when the dialect is known: reading + /// `'a\'` as an unfinished literal in PostgreSQL or SQLite hides the `;` + /// after it, and the statement the server then runs. + static List spans( + String sql, { + bool backslashEscapes = true, + }) { final out = []; final len = sql.length; - var i = 0; var segmentStart = 0; // Statement starts only move forward, so the line of each one is found by @@ -90,93 +89,19 @@ abstract final class SqlStatementSplitter { } } - while (i < len) { - final code = sql.codeUnitAt(i); - - // Line comment: runs to the end of the line. - if (code == _chDash && i + 1 < len && sql.codeUnitAt(i + 1) == _chDash) { - while (i < len && - sql.codeUnitAt(i) != _chLf && - sql.codeUnitAt(i) != _chCr) { - i++; - } - continue; - } - - // Block comment. - if (code == _chSlash && - i + 1 < len && - sql.codeUnitAt(i + 1) == _chAsterisk) { - i += 2; - while (i + 1 < len && - !(sql.codeUnitAt(i) == _chAsterisk && - sql.codeUnitAt(i + 1) == _chSlash)) { - i++; - } - i = i + 1 < len ? i + 2 : len; - continue; - } - - // Dollar quote: $tag$ ... $tag$. - if (code == _chDollar) { - final match = _dollarTag.matchAsPrefix(sql, i); - if (match != null) { - final tag = match.group(0)!; - final close = sql.indexOf(tag, i + tag.length); - i = close == -1 ? len : close + tag.length; - continue; + // A `;` ends a statement only in code: literals, quoted identifiers, + // comments and dollar quotes are the scanner's other segments. + for (final segment in SqlLexicalScanner.segments( + sql, + backslashEscapes: backslashEscapes, + )) { + if (segment.kind != SqlSegmentKind.code) continue; + for (var i = segment.start; i < segment.end; i++) { + if (sql.codeUnitAt(i) == _chSemicolon) { + close(i); + segmentStart = i + 1; } } - - // String literal, with doubled quotes and backslash escapes. - if (code == _chSingleQuote) { - i++; - while (i < len) { - final curr = sql.codeUnitAt(i); - if (curr == _chSingleQuote) { - if (i + 1 < len && sql.codeUnitAt(i + 1) == _chSingleQuote) { - i += 2; - } else { - i++; - break; - } - } else if (curr == _chBackslash && i + 1 < len) { - i += 2; - } else { - i++; - } - } - continue; - } - - // Quoted identifier: "name" (PostgreSQL) or `name` (MySQL). - if (code == _chDoubleQuote || code == _chBacktick) { - final quoteChar = code; - i++; - while (i < len) { - final curr = sql.codeUnitAt(i); - if (curr == quoteChar) { - if (i + 1 < len && sql.codeUnitAt(i + 1) == quoteChar) { - i += 2; - } else { - i++; - break; - } - } else { - i++; - } - } - continue; - } - - if (code == _chSemicolon) { - close(i); - i++; - segmentStart = i; - continue; - } - - i++; } close(len); return out; diff --git a/lib/core/database/table_mutation_engine.dart b/lib/core/database/table_mutation_engine.dart index b55778f3..bbeb4c0c 100644 --- a/lib/core/database/table_mutation_engine.dart +++ b/lib/core/database/table_mutation_engine.dart @@ -7,6 +7,13 @@ enum SqlDialect { sqlite, } +extension SqlDialectLexing on SqlDialect { + /// Whether `\'` inside `'...'` is an escaped quote. It is in MySQL; in + /// PostgreSQL and SQLite a backslash is an ordinary character and `'C:\'` + /// is a whole literal (PostgreSQL's `E'...'` strings are the exception). + bool get backslashEscapesQuotes => this == SqlDialect.mysql; +} + /// Type of DML mutation. enum MutationType { insert, diff --git a/lib/core/mcp/mcp_sql_guard.dart b/lib/core/mcp/mcp_sql_guard.dart index e8fbf830..caa0da5f 100644 --- a/lib/core/mcp/mcp_sql_guard.dart +++ b/lib/core/mcp/mcp_sql_guard.dart @@ -74,10 +74,17 @@ abstract final class McpSqlGuard { /// Returns `null` when [sql] may run, or why it may not: the rule that /// refused it (stable, for the audit log) and the message for the model. static McpSqlRefusal? refusal(String sql, SqlDialect dialect) { - final statements = DestructiveSqlDetector.splitStatements(sql) - .where((s) => DestructiveSqlDetector.stripCommentsAndStrings(s) - .trim() - .isNotEmpty) + // Read the text the way the server does: in PostgreSQL and SQLite `'a\'` + // is a whole literal, so a `;` after it ends the statement. + final escapes = dialect.backslashEscapesQuotes; + final statements = DestructiveSqlDetector.splitStatements( + sql, + backslashEscapes: escapes, + ) + .where((s) => DestructiveSqlDetector.stripCommentsAndStrings( + s, + backslashEscapes: escapes, + ).trim().isNotEmpty) .toList(); if (statements.isEmpty) return const McpSqlRefusal('empty_query', 'The query is empty.'); if (statements.length > 1) { @@ -90,7 +97,10 @@ abstract final class McpSqlGuard { RegExp(r'/\*M?!').hasMatch(statement)) { return const McpSqlRefusal('mysql_executable_comment', 'MySQL executable comments (/*! ... */) are not allowed.'); } - final upper = DestructiveSqlDetector.stripCommentsAndStrings(statement) + final upper = DestructiveSqlDetector.stripCommentsAndStrings( + statement, + backslashEscapes: escapes, + ) .trim() .replaceAll(RegExp(r';\s*$'), '') .toUpperCase(); @@ -113,7 +123,7 @@ abstract final class McpSqlGuard { } return null; } - if (isMutatingSqlStatement(statement)) { + if (isMutatingSqlStatement(statement, backslashEscapes: escapes)) { return const McpSqlRefusal('data_modifying', 'Data-modifying statements are not allowed over MCP.'); } if (_selectSideEffects.hasMatch(upper)) { diff --git a/lib/features/workspace/generic_sql_workspace.dart b/lib/features/workspace/generic_sql_workspace.dart index 7b00f606..6c65b58c 100644 --- a/lib/features/workspace/generic_sql_workspace.dart +++ b/lib/features/workspace/generic_sql_workspace.dart @@ -594,6 +594,7 @@ class GenericSqlWorkspaceState extends material.State { statementAtCursor: statementAtCursor, selected: selected, caret: selection.isValid ? selection.baseOffset : scope.length, + backslashEscapes: widget.dialect.backslashEscapesQuotes, ); if (spans.isEmpty) { // Say so: a silent Run on an empty editor reads as a broken button. @@ -623,7 +624,10 @@ class GenericSqlWorkspaceState extends material.State { final confirmDestructive = await AppSettings.instance.getConfirmDestructiveOperations(); if (confirmDestructive) { - final inspection = DestructiveSqlDetector.inspect(runSql); + final inspection = DestructiveSqlDetector.inspect( + runSql, + backslashEscapes: widget.dialect.backslashEscapesQuotes, + ); if (inspection.isDestructive) { if (!mounted) return; final confirmed = await showDestructiveQueryDialog( @@ -843,6 +847,7 @@ class GenericSqlWorkspaceState extends material.State { required bool statementAtCursor, required bool selected, required int caret, + required bool backslashEscapes, }) async { if (statementAtCursor && selected) { final start = scope.length - scope.trimLeft().length; @@ -851,7 +856,10 @@ class GenericSqlWorkspaceState extends material.State { ? [SqlStatementSpan(start: start, end: end, line: 1)] : const []; } - final all = await SqlStatementSplitter.spansAsync(scope); + final all = await SqlStatementSplitter.spansAsync( + scope, + backslashEscapes: backslashEscapes, + ); if (!statementAtCursor) return all; final at = SqlStatementSplitter.at(all, caret); return at == null ? const [] : [at]; diff --git a/test/core/database/destructive_sql_detector_test.dart b/test/core/database/destructive_sql_detector_test.dart index 6b76b264..aaf2cb49 100644 --- a/test/core/database/destructive_sql_detector_test.dart +++ b/test/core/database/destructive_sql_detector_test.dart @@ -219,4 +219,29 @@ void main() { expect(DestructiveSqlDetector.inspect('UPDATE users SET age = 20 WHERE id = 1;').isDestructive, isFalse); }); }); + + group('DestructiveSqlDetector backslash escapes (#1565 review)', () { + const sql = r"SELECT 'a\'; DROP TABLE users; --'"; + + test('MySQL (the default) reads \\\' as an escaped quote: nothing destructive', + () { + expect(DestructiveSqlDetector.inspect(sql).isDestructive, isFalse); + }); + + test('PostgreSQL and SQLite end the literal at the second quote', () { + final res = DestructiveSqlDetector.inspect(sql, backslashEscapes: false); + expect(res.isDestructive, isTrue); + expect(res.operations.single.type, DestructiveSqlType.dropTable); + }); + + test('an E string keeps its escape in every dialect', () { + expect( + DestructiveSqlDetector.inspect( + r"SELECT E'a\'; DROP TABLE users'", + backslashEscapes: false, + ).isDestructive, + isFalse, + ); + }); + }); } diff --git a/test/core/database/sql_mutation_classifier_test.dart b/test/core/database/sql_mutation_classifier_test.dart index ebb8b426..717be9b3 100644 --- a/test/core/database/sql_mutation_classifier_test.dart +++ b/test/core/database/sql_mutation_classifier_test.dart @@ -69,4 +69,14 @@ void main() { expect(containsMutatingSql("SELECT 'a;DELETE FROM t'"), isFalse); }); }); + + group('containsMutatingSql backslash escapes', () { + const sql = r"SELECT 'a\'; DELETE FROM t; --'"; + + test('the dialect decides whether the DELETE is code or part of a literal', + () { + expect(containsMutatingSql(sql), isFalse); // MySQL reading, the default + expect(containsMutatingSql(sql, backslashEscapes: false), isTrue); + }); + }); } diff --git a/test/core/database/sql_statement_splitter_test.dart b/test/core/database/sql_statement_splitter_test.dart index 60243d93..8f20444f 100644 --- a/test/core/database/sql_statement_splitter_test.dart +++ b/test/core/database/sql_statement_splitter_test.dart @@ -165,4 +165,54 @@ void main() { } }); }); + + // A backslash before the closing quote: MySQL reads it as an escaped quote, + // PostgreSQL and SQLite as an ordinary character. + group('SqlStatementSplitter backslash escapes', () { + const sql = r"SELECT 'a\'; DELETE FROM t; --'"; + + List run(String text, {required bool backslashEscapes}) => [ + for (final s in SqlStatementSplitter.spans( + text, + backslashEscapes: backslashEscapes, + )) + s.textIn(text), + ]; + + test('the default keeps the MySQL reading: one statement', () { + expect(texts(sql), [sql]); + expect(run(sql, backslashEscapes: true), [sql]); + }); + + test('without backslash escapes the ; after the literal ends the statement', + () { + expect(run(sql, backslashEscapes: false), [ + r"SELECT 'a\'", + 'DELETE FROM t', + "--'", + ]); + }); + + test('an E string escapes with a backslash whatever the dialect', () { + const e = r"SELECT E'a\'; DELETE FROM t'; SELECT 2"; + expect(run(e, backslashEscapes: false), [ + r"SELECT E'a\'; DELETE FROM t'", + 'SELECT 2', + ]); + }); + + test('a script past the isolate threshold splits the same way', () async { + final script = List.filled(4000, r"SELECT 'a\'; SELECT 2;").join('\n'); + expect(script.length, greaterThan(SqlStatementSplitter.defaultIsolateThreshold)); + for (final escapes in [true, false]) { + final inline = SqlStatementSplitter.spans(script, backslashEscapes: escapes); + final spread = await SqlStatementSplitter.spansAsync( + script, + backslashEscapes: escapes, + ); + expect(spread.length, inline.length, reason: 'backslashEscapes: $escapes'); + expect(spread.last.end, inline.last.end); + } + }); + }); } diff --git a/test/core/mcp/mcp_sql_guard_test.dart b/test/core/mcp/mcp_sql_guard_test.dart index f2e140fe..d9392012 100644 --- a/test/core/mcp/mcp_sql_guard_test.dart +++ b/test/core/mcp/mcp_sql_guard_test.dart @@ -151,4 +151,23 @@ void main() { 'The query is empty.'); }); }); + + // A backslash before the closing quote is an escape in MySQL only; reading it + // as one in PostgreSQL or SQLite hid the statements after the literal. + test('a trailing backslash in a literal does not hide statements (PostgreSQL, SQLite)', + () { + const smuggled = r"SELECT 'a\'; SELECT pg_terminate_backend(1); --'"; + refused(smuggled, SqlDialect.postgres); + refused(smuggled, SqlDialect.sqlite); + const write = r"SELECT 'a\'; DELETE FROM users; --'"; + refused(write, SqlDialect.postgres); + refused(write, SqlDialect.sqlite); + expect(McpSqlGuard.refusal(write, SqlDialect.postgres)!.rule, 'single_statement'); + }); + + test('the same text is one literal in MySQL, and an E string in PostgreSQL', + () { + allowed(r"SELECT 'a\'; SELECT pg_terminate_backend(1); --'", SqlDialect.mysql); + allowed(r"SELECT E'a\'; SELECT 1'", SqlDialect.postgres); + }); }