Skip to content

fix(security): findings of the review of the guard, the detector and the zip extractor #1797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ZhuchkaTriplesix merged 2 commits into from
Oct 25, 2026
Merged

fix(security): findings of the review of the guard, the detector and the zip extractor #1797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b984bf4
fix(security): review of the SQL guard, the destructive detector and …
ZhuchkaTriplesix Oct 25, 2026
54883ee
fix(mcp): the ambiguous-syntax checks ignore string literal bodies
ZhuchkaTriplesix Oct 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 62 additions & 50 deletions lib/core/database/destructive_sql_detector.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ abstract final class DestructiveSqlDetector {
);

static final _dropTableRegex = RegExp(
_withId(r'^\s*DROP\s+TABLE\s+(?:IF\s+EXISTS\s+)?(?:(?:{id}\.)?{id})'),
_withId(r'^\s*DROP\s+(?:TEMPORARY\s+|FOREIGN\s+)?TABLE\s+(?:IF\s+EXISTS\s+)?(?:(?:{id}\.)?{id})'),
caseSensitive: false,
unicode: true,
);
Expand All @@ -204,7 +204,7 @@ abstract final class DestructiveSqlDetector {
);

static final _deleteRegex = RegExp(
_withId(r'^\s*DELETE\s+(?:(?:LOW_PRIORITY|QUICK|IGNORE)\s+)*(?:[^;]*?\s)?FROM\s+(?:(?:{id}\.)?{id})'),
_withId(r'^\s*DELETE\s+(?:[^;]*?\s)??FROM\s+(?:(?:{id}\.)?{id})'),
caseSensitive: false,
unicode: true,
);
Expand All @@ -217,15 +217,15 @@ abstract final class DestructiveSqlDetector {

static final _dropOtherRegex = RegExp(
_withId(
r'^\s*DROP\s+(?:UNIQUE\s+)?(?:INDEX|TYPE|FUNCTION|PROCEDURE|ROLE|USER|TRIGGER|SEQUENCE|DOMAIN|EXTENSION|TABLESPACE|PUBLICATION|SUBSCRIPTION|AGGREGATE|POLICY|RULE)\s+(?:CONCURRENTLY\s+)?(?:IF\s+EXISTS\s+)?(?:(?:{id}\.)?{id})',
r'^\s*DROP\s+(?:UNIQUE\s+)?(?:INDEX|TYPE|FUNCTION|PROCEDURE|ROLE|USER|TRIGGER|EVENT\s+TRIGGER|SEQUENCE|DOMAIN|EXTENSION|TABLESPACE|PUBLICATION|SUBSCRIPTION|AGGREGATE|POLICY|RULE|SERVER|OWNED\s+BY)\s+(?:CONCURRENTLY\s+)?(?:IF\s+EXISTS\s+)?(?:(?:{id}\.)?{id})',
),
caseSensitive: false,
unicode: true,
);

static final _dropColumnRegex = RegExp(
_withId(
r'^\s*ALTER\s+TABLE\s+(?:IF\s+EXISTS\s+)?(?:ONLY\s+)?(?:(?:{id}\.)?{id})[^;]*?\bDROP\s+COLUMN\b',
r'^\s*ALTER\s+TABLE\s+(?:IF\s+EXISTS\s+)?(?:ONLY\s+)?(?:(?:{id}\.)?{id})[^;]*?\bDROP\s+(?:COLUMN\b|(?!(?:CONSTRAINT|INDEX|KEY|PRIMARY|FOREIGN|CHECK|DEFAULT|NOT|IDENTITY|EXPRESSION|PARTITION|IF|TRIGGER|RULE|PERIOD)\b)[\p{L}"`_])',
),
caseSensitive: false,
unicode: true,
Expand All @@ -240,31 +240,72 @@ abstract final class DestructiveSqlDetector {
static final _withRegex =
RegExp(r'^\s*WITH\s+(?:RECURSIVE\s+)?', caseSensitive: false);

static final _dmlStartRegex =
RegExp(r'^(?:DELETE|UPDATE)\b', caseSensitive: false);
/// The keyword that starts the main statement after a `WITH` list.
static final _mainStartRegex = RegExp(
r'^(?:SELECT|INSERT|UPDATE|DELETE|MERGE|VALUES|TABLE)\b',
caseSensitive: false,
);

/// The statement that actually runs: past an `EXPLAIN ANALYZE` prefix and
/// the `WITH name AS (...)` list in front of a `DELETE` / `UPDATE`.
static String _effectiveStatement(String sanitized) {
/// Splits [sanitized] (after an `EXPLAIN ANALYZE` prefix) into the bodies of
/// its `WITH name AS (...)` list and the main statement that follows it. A
/// statement without `WITH` is its own main statement.
static ({List<String> bodies, String main}) _splitWith(String sanitized) {
var sql = sanitized;
final explain = _explainAnalyzeRegex.firstMatch(sql);
if (explain != null) sql = sql.substring(explain.end);
final withMatch = _withRegex.firstMatch(sql);
if (withMatch == null) return sql;
if (withMatch == null) return (bodies: const [], main: sql);
final bodies = <String>[];
var depth = 0;
var bodyStart = 0;
for (var i = withMatch.end; i < sql.length; i++) {
final ch = sql[i];
if (ch == '(') {
if (depth == 0) bodyStart = i + 1;
depth++;
} else if (ch == ')') {
if (depth > 0) depth--;
if (depth == 0) bodies.add(sql.substring(bodyStart, i));
} else if (depth == 0 &&
(i == 0 || !RegExp(r'[\w$]').hasMatch(sql[i - 1])) &&
_dmlStartRegex.hasMatch(sql.substring(i))) {
return sql.substring(i);
!RegExp(r'[\w$]').hasMatch(sql[i - 1]) &&
_mainStartRegex.hasMatch(sql.substring(i))) {
return (bodies: bodies, main: sql.substring(i));
}
}
return sql;
return (bodies: bodies, main: sql);
}

/// An unconditional `DELETE` / `UPDATE` in [sql], or null.
static DestructiveSqlOperation? _unconditionalDml(
String sql,
String rawStatement,
) {
String targetOf(RegExpMatch m) {
final schema = m.group(1);
final table = m.group(2) ?? 'table';
return (schema != null && schema.isNotEmpty) ? '$schema.$table' : table;
}

final deleteMatch = _deleteRegex.firstMatch(sql);
if (deleteMatch != null) {
if (_hasTopLevelWhere(sql)) return null;
return DestructiveSqlOperation(
type: DestructiveSqlType.unconditionalDelete,
targetName: targetOf(deleteMatch),
rawStatement: rawStatement,
);
}
final updateMatch = _updateRegex.firstMatch(sql);
if (updateMatch != null &&
_setRegex.hasMatch(sql) &&
!_hasTopLevelWhere(sql)) {
return DestructiveSqlOperation(
type: DestructiveSqlType.unconditionalUpdate,
targetName: targetOf(updateMatch),
rawStatement: rawStatement,
);
}
return null;
}

static final _whereRegex = RegExp(r'\bWHERE\b', caseSensitive: false);
Expand Down Expand Up @@ -464,7 +505,8 @@ abstract final class DestructiveSqlDetector {
backslashEscapes: backslashEscapes,
).trim();
if (sanitized.isEmpty) continue;
final effective = _effectiveStatement(sanitized);
final withParts = _splitWith(sanitized);
final effective = withParts.main;

// 1. DROP DATABASE
final dropDbMatch = _dropDatabaseRegex.firstMatch(sanitized);
Expand Down Expand Up @@ -594,41 +636,11 @@ abstract final class DestructiveSqlDetector {
continue;
}

// 7. DELETE FROM table without WHERE
final deleteMatch = _deleteRegex.firstMatch(effective);
if (deleteMatch != null) {
if (!_hasTopLevelWhere(effective)) {
final schema = deleteMatch.group(1);
final table = deleteMatch.group(2) ?? 'table';
final target =
(schema != null && schema.isNotEmpty) ? '$schema.$table' : table;
operations.add(
DestructiveSqlOperation(
type: DestructiveSqlType.unconditionalDelete,
targetName: target,
rawStatement: rawStmt.trim(),
),
);
}
continue;
}

// 8. UPDATE table SET ... without WHERE
final updateMatch = _updateRegex.firstMatch(effective);
if (updateMatch != null &&
_setRegex.hasMatch(effective) &&
!_hasTopLevelWhere(effective)) {
final schema = updateMatch.group(1);
final table = updateMatch.group(2) ?? 'table';
final target =
(schema != null && schema.isNotEmpty) ? '$schema.$table' : table;
operations.add(
DestructiveSqlOperation(
type: DestructiveSqlType.unconditionalUpdate,
targetName: target,
rawStatement: rawStmt.trim(),
),
);
// 7. DELETE / UPDATE without WHERE: the main statement, and a
// data-modifying CTE (`WITH d AS (DELETE FROM t RETURNING *) SELECT ...`).
for (final body in [effective, ...withParts.bodies]) {
final op = _unconditionalDml(body.trim(), rawStmt.trim());
if (op != null) operations.add(op);
}
}

Expand Down
87 changes: 85 additions & 2 deletions lib/core/mcp/mcp_sql_guard.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,76 @@ abstract final class McpSqlGuard {
'VALUES',
};

/// Syntax the comment/string stripper and the server read differently, so
/// text after it could hide a call from the denylist: a quoted identifier
/// that holds a comment or quote character, a PostgreSQL `U&"..."`
/// identifier (escapes spell any name), MySQL `#` comments and `--` without
/// a space after it (which MySQL reads as two minus signs).
/// Reads [sql] left to right, skipping string literals, and returns whether
/// a `"..."` or `` `...` `` identifier holds `--`, `'`, `#` or `/*`, and the
/// text with every string literal body removed. Reading in order means a
/// closing quote is never taken for an opening one.
static ({bool ambiguousIdentifier, String outsideStrings}) _scanQuoting(
String sql, {
required bool backslashEscapes,
}) {
final outside = StringBuffer();
var ambiguous = false;
var i = 0;
while (i < sql.length) {
final c = sql[i];
if (c == "'") {
outside.write("''");
i++;
while (i < sql.length) {
if (backslashEscapes && sql[i] == r'\') {
i += 2;
} else if (sql[i] == "'") {
if (i + 1 < sql.length && sql[i + 1] == "'") {
i += 2;
} else {
break;
}
} else {
i++;
}
}
i++;
} else if (c == '"' || c == '`') {
final start = i + 1;
i = start;
while (i < sql.length) {
if (sql[i] == c) {
if (i + 1 < sql.length && sql[i + 1] == c) {
i += 2;
continue;
}
break;
}
i++;
}
final end = i < sql.length ? i : sql.length;
final body = sql.substring(start, end);
if (body.contains('--') ||
body.contains("'") ||
body.contains('#') ||
body.contains('/*')) {
ambiguous = true;
}
outside.write(sql.substring(start - 1, i < sql.length ? i + 1 : end));
i++;
} else {
outside.write(c);
i++;
}
}
return (ambiguousIdentifier: ambiguous, outsideStrings: outside.toString());
}

static final _unicodeEscapeIdentifier =
RegExp(r'\bU&\s*"', caseSensitive: false);
static final _mysqlDashDashNoSpace = RegExp(r'--(?!\s)');

static final _firstWord = RegExp(r'^\(*\s*([A-Z_]+)');

/// `SELECT ... INTO` creates a table (Postgres) or writes a file (MySQL
Expand All @@ -48,8 +118,9 @@ abstract final class McpSqlGuard {
r'QUERY_TO_XML\w*|CURSOR_TO_XML|TABLE_TO_XML\w*|PG_(?:TRY_)?ADVISORY_\w*|PG_SLEEP\w*|'
r'NEXTVAL|SETVAL|DBLINK\w*|SET_CONFIG|LOAD_FILE|LOAD_EXTENSION|WRITEFILE|'
r'READFILE|FSDIR|SLEEP|BENCHMARK|GET_LOCK|RELEASE_LOCK|RELEASE_ALL_LOCKS|IS_FREE_LOCK|'
r'PG_STAT_RESET\w*|PG_SWITCH_WAL|PG_CREATE_RESTORE_POINT|PG_LOGICAL_EMIT_MESSAGE|'
r'PG_PROMOTE|PG_DROP_REPLICATION_SLOT)\s*\(',
r'PG_STAT_\w*RESET\w*|PG_SWITCH_WAL|PG_CREATE_RESTORE_POINT|PG_LOGICAL_\w*|PG_REPLICATION_\w*|'
r'PG_NOTIFY|PG_LS_\w*|PG_PROMOTE|PG_DROP_REPLICATION_SLOT|PG_CREATE_\w*_SLOT|PG_COPY_\w*_SLOT|'
r'PG_BACKUP_\w*|PG_START_BACKUP|PG_STOP_BACKUP)\s*\(',
);

/// SQLite pragmas that only read the schema.
Expand Down Expand Up @@ -99,6 +170,14 @@ abstract final class McpSqlGuard {
RegExp(r'/\*M?!').hasMatch(statement)) {
return const McpSqlRefusal('mysql_executable_comment', 'MySQL executable comments (/*! ... */) are not allowed.');
}
final quoting = _scanQuoting(statement, backslashEscapes: escapes);
if (quoting.ambiguousIdentifier ||
_unicodeEscapeIdentifier.hasMatch(quoting.outsideStrings) ||
(dialect == SqlDialect.mysql &&
_mysqlDashDashNoSpace.hasMatch(quoting.outsideStrings))) {
return const McpSqlRefusal('ambiguous_syntax',
'Quoted identifiers with comment or quote characters, U&"..." identifiers and "--" without a following space are not allowed over MCP.');
}
final upper = DestructiveSqlDetector.stripCommentsAndStrings(
statement,
backslashEscapes: escapes,
Expand All @@ -113,6 +192,10 @@ abstract final class McpSqlGuard {
if (m != null && _readPragmas.contains(m.group(1))) return null;
return McpSqlRefusal('sqlite_pragma', 'Only schema pragmas are allowed (${_readPragmas.map((p) => p.toLowerCase()).join(', ')}).');
}
if (dialect == SqlDialect.mysql && upper.contains('#')) {
return const McpSqlRefusal('ambiguous_syntax',
'MySQL "#" comments are not allowed over MCP.');
}
if (first == null || !_readStarts.contains(first)) {
return const McpSqlRefusal('read_only_only', 'Only read-only queries are allowed (SELECT, WITH, EXPLAIN, SHOW, DESCRIBE).');
}
Expand Down
22 changes: 22 additions & 0 deletions lib/core/updater/installers/update_install_utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ Future<void> extractZipSecurely({
);
}

// A link already on disk would carry this entry out of the root, e.g.
// `a -> .`, `a/b -> ..`, then a file under `b/`; the checks above only read
// the names.
if (await _hasLinkBelowRoot(root, p.dirname(targetPath))) {
throw AppUpdaterException(
'Security violation: archive entry "$name" is below a symlink',
);
}

if (entry.isSymbolicLink) {
// The macOS bundle keeps its framework symlinks (`Versions/Current`);
// flattened to text files they fail `codesign --verify`.
Expand Down Expand Up @@ -71,6 +80,19 @@ Future<void> extractZipSecurely({
}
}

/// Whether any directory from [root] (exclusive) down to [directory] is a
/// symlink.
Future<bool> _hasLinkBelowRoot(String root, String directory) async {
final parts = p.split(p.relative(directory, from: root));
var current = root;
for (final part in parts) {
if (part == '.' || part.isEmpty) continue;
current = p.join(current, part);
if (await FileSystemEntity.isLink(current)) return true;
}
return false;
}

/// Restores the permission bits of an extracted file (POSIX only), so helper
/// binaries in an update stay executable.
Future<void> _applyUnixPermissions(File file, int permissions) async {
Expand Down
Loading
Loading