Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 8 additions & 3 deletions lib/core/mcp/mcp_query_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ import 'package:querya_desktop/features/workspace/sql_execution_delegate.dart';
/// A failure the MCP client should see as a tool error (the model reads the
/// message and can fix its call).
class McpToolException implements Exception {
const McpToolException(this.message);
const McpToolException(this.message, {this.rule});

final String message;

/// The guard rule that refused the call, when a rule did.
final String? rule;

@override
String toString() => message;
}
Expand Down Expand Up @@ -218,8 +221,10 @@ class McpQueryService {
}

void _guard(String sql, SqlDialect dialect) {
final reason = McpSqlGuard.check(sql, dialect);
if (reason != null) throw McpToolException(reason);
final refusal = McpSqlGuard.refusal(sql, dialect);
if (refusal != null) {
throw McpToolException(refusal.message, rule: refusal.rule);
}
}

Future<ErdSchema> _loadSchema(
Expand Down
1 change: 1 addition & 0 deletions lib/core/mcp/mcp_server_controller.dart
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ class McpServerController {
rowCount: r.rowCount,
durationMs: r.duration.inMilliseconds,
error: r.error,
refusalRule: r.rule,
));
}

Expand Down
37 changes: 26 additions & 11 deletions lib/core/mcp/mcp_sql_guard.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,15 @@ import 'package:querya_desktop/core/database/destructive_sql_detector.dart';
import 'package:querya_desktop/core/database/sql_mutation_classifier.dart';
import 'package:querya_desktop/core/database/table_mutation_engine.dart';

/// Why [McpSqlGuard] refused a query: a stable rule id for the audit log and
/// the message the model reads.
class McpSqlRefusal {
const McpSqlRefusal(this.rule, this.message);

final String rule;
final String message;
}

/// Decides whether SQL sent by an MCP client may run.
///
/// First of three read-only layers (the others are the read-only database
Expand Down Expand Up @@ -55,23 +64,29 @@ abstract final class McpSqlGuard {
static final _pragma =
RegExp(r'^PRAGMA\s+(?:[A-Z_]+\.)?([A-Z_]+)\s*(\(\s*[A-Z_0-9]*\s*\))?$');

/// Returns `null` when [sql] may run, or the reason it may not.
static String? check(String sql, SqlDialect dialect) {
/// Returns `null` when [sql] may run, or the reason it may not, as the
/// message the model sees.
static String? check(String sql, SqlDialect dialect) =>
refusal(sql, dialect)?.message;

/// 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)
.toList();
if (statements.isEmpty) return 'The query is empty.';
if (statements.isEmpty) return const McpSqlRefusal('empty_query', 'The query is empty.');
if (statements.length > 1) {
return 'Only one statement per call is allowed; send them separately.';
return const McpSqlRefusal('single_statement', 'Only one statement per call is allowed; send them separately.');
}
final statement = statements.single;
// MySQL / MariaDB execute the body of `/*! ... */` and `/*M! ... */`
// comments, which the comment stripper below would hide from the checks.
if (dialect == SqlDialect.mysql &&
RegExp(r'/\*M?!').hasMatch(statement)) {
return 'MySQL executable comments (/*! ... */) are not allowed.';
return const McpSqlRefusal('mysql_executable_comment', 'MySQL executable comments (/*! ... */) are not allowed.');
}
final upper = DestructiveSqlDetector.stripCommentsAndStrings(statement)
.trim()
Expand All @@ -82,25 +97,25 @@ abstract final class McpSqlGuard {
if (first == 'PRAGMA' && dialect == SqlDialect.sqlite) {
final m = _pragma.firstMatch(upper.replaceAll(RegExp(r'\s+'), ' '));
if (m != null && _readPragmas.contains(m.group(1))) return null;
return 'Only schema pragmas are allowed (${_readPragmas.map((p) => p.toLowerCase()).join(', ')}).';
return McpSqlRefusal('sqlite_pragma', 'Only schema pragmas are allowed (${_readPragmas.map((p) => p.toLowerCase()).join(', ')}).');
}
if (first == null || !_readStarts.contains(first)) {
return 'Only read-only queries are allowed (SELECT, WITH, EXPLAIN, SHOW, DESCRIBE).';
return const McpSqlRefusal('read_only_only', 'Only read-only queries are allowed (SELECT, WITH, EXPLAIN, SHOW, DESCRIBE).');
}
if (_forbiddenFunctions.hasMatch(upper)) {
return 'This query calls a server function that is not allowed over MCP.';
return const McpSqlRefusal('function_not_allowed', 'This query calls a server function that is not allowed over MCP.');
}
if (first == 'EXPLAIN') {
if (_explainForbidden.hasMatch(upper)) {
return 'EXPLAIN is allowed only for read-only statements and without ANALYZE.';
return const McpSqlRefusal('explain_analyze_or_write', 'EXPLAIN is allowed only for read-only statements and without ANALYZE.');
}
return null;
}
if (isMutatingSqlStatement(statement)) {
return 'Data-modifying statements are not allowed over MCP.';
return const McpSqlRefusal('data_modifying', 'Data-modifying statements are not allowed over MCP.');
}
if (_selectSideEffects.hasMatch(upper)) {
return 'SELECT ... INTO and row locks (FOR UPDATE / FOR SHARE) are not allowed.';
return const McpSqlRefusal('select_into_or_lock', 'SELECT ... INTO and row locks (FOR UPDATE / FOR SHARE) are not allowed.');
}
return null;
}
Expand Down
13 changes: 9 additions & 4 deletions lib/core/mcp/querya_mcp_server.dart
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class McpCallRecord {
this.sql,
this.rowCount,
this.error,
this.rule,
});

final DateTime at;
Expand All @@ -26,6 +27,9 @@ class McpCallRecord {
final String? sql;
final int? rowCount;
final String? error;

/// The guard rule that refused the call, when a rule did.
final String? rule;
}

/// MCP session for one client: read-only tools and a `schema://` resource on
Expand Down Expand Up @@ -210,24 +214,24 @@ base class QueryaMcpServer extends MCPServer with ToolsSupport, ResourcesSupport
}
try {
final (text, rows, sql) = await body(args);
_report(tool, started, sw.elapsed, connectionId, sql, rows, null);
_report(tool, started, sw.elapsed, connectionId, sql, rows, null, null);
return CallToolResult(content: [TextContent(text: text)]);
} on McpToolException catch (e) {
_report(tool, started, sw.elapsed, connectionId,
rawSql, null, e.message);
rawSql, null, e.message, e.rule);
return CallToolResult(
isError: true, content: [TextContent(text: e.message)]);
} catch (e) {
final message = McpRedaction.redact('Internal error: $e');
_report(tool, started, sw.elapsed, connectionId, rawSql, null, message);
_report(tool, started, sw.elapsed, connectionId, rawSql, null, message, null);
return CallToolResult(
isError: true, content: [TextContent(text: message)]);
}
};
}

void _report(String tool, DateTime at, Duration d, int? connectionId,
String? sql, int? rows, String? error) {
String? sql, int? rows, String? error, String? rule) {
onCall?.call(McpCallRecord(
at: at,
client: _clientName,
Expand All @@ -237,6 +241,7 @@ base class QueryaMcpServer extends MCPServer with ToolsSupport, ResourcesSupport
sql: sql,
rowCount: rows,
error: error,
rule: rule,
));
}

Expand Down
15 changes: 13 additions & 2 deletions lib/core/storage/local_db.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import 'package:querya_desktop/core/storage/connection_secrets_store.dart';
import 'package:sqflite_common_ffi/sqflite_ffi.dart';

const _dbName = 'querya.db';
const _dbVersion = 11;
const _dbVersion = 12;

/// `app_settings` key under which each profile database's random id is
/// stored (see [LocalDb._ensureProfileId] and issue #986).
Expand Down Expand Up @@ -221,7 +221,8 @@ class LocalDb {
sql_text TEXT,
row_count INTEGER,
duration_ms INTEGER NOT NULL,
error TEXT
error TEXT,
refusal_rule TEXT
)
''');
}
Expand Down Expand Up @@ -340,6 +341,10 @@ class LocalDb {
if (oldVersion < 11) {
await _createMcpActivityTable(db);
}
if (oldVersion >= 11 && oldVersion < 12) {
// The rule that refused a call, next to its error text.
await db.execute('ALTER TABLE mcp_activity ADD COLUMN refusal_rule TEXT');
}
}

Future<String?> getAppSetting(String key) async {
Expand Down Expand Up @@ -835,6 +840,7 @@ class McpActivityEntry {
this.rowCount,
required this.durationMs,
this.error,
this.refusalRule,
});

final int? id;
Expand All @@ -850,6 +856,9 @@ class McpActivityEntry {
final int durationMs;
final String? error;

/// The guard rule that refused the call (`read_only_only`, ...), when one did.
final String? refusalRule;

Map<String, Object?> toMap() => {
'id': id,
'recorded_at': recordedAt,
Expand All @@ -861,6 +870,7 @@ class McpActivityEntry {
'row_count': rowCount,
'duration_ms': durationMs,
'error': error,
'refusal_rule': refusalRule,
};

static McpActivityEntry fromMap(Map<String, Object?> m) => McpActivityEntry(
Expand All @@ -874,6 +884,7 @@ class McpActivityEntry {
rowCount: _sqliteInt(m['row_count']),
durationMs: _sqliteInt(m['duration_ms']) ?? 0,
error: m['error'] as String?,
refusalRule: m['refusal_rule'] as String?,
);
}

Expand Down
24 changes: 24 additions & 0 deletions test/core/mcp/mcp_client_config_and_activity_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,28 @@ void main() {
expect(await LocalDb.instance.listMcpActivity(), isEmpty);
});
});

test('an activity entry keeps the rule that refused its call (#1231)', () {
const entry = McpActivityEntry(
recordedAt: '2026-10-09T10:00:00Z',
client: 'test',
tool: 'run_query',
durationMs: 3,
error: 'Data-modifying statements are not allowed over MCP.',
refusalRule: 'data_modifying',
);
final back = McpActivityEntry.fromMap(entry.toMap());
expect(back.refusalRule, 'data_modifying');
expect(back.error, entry.error);
expect(
McpActivityEntry.fromMap({
'recorded_at': '2026-10-09T10:00:00Z',
'client': 'test',
'tool': 'run_query',
'duration_ms': 3,
}).refusalRule,
isNull,
reason: 'rows written before the column existed read as no rule',
);
});
}
35 changes: 35 additions & 0 deletions test/core/mcp/mcp_sql_guard_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,39 @@ void main() {
refused('PRAGMA journal_mode', SqlDialect.sqlite);
refused('PRAGMA table_info(users)', SqlDialect.postgres);
});

group('refusal rule ids (#1231)', () {
String? ruleOf(String sql, SqlDialect d) =>
McpSqlGuard.refusal(sql, d)?.rule;

test('every refusal names the rule that caught it', () {
expect(ruleOf('', SqlDialect.postgres), 'empty_query');
expect(ruleOf('SELECT 1; SELECT 2', SqlDialect.postgres),
'single_statement');
expect(ruleOf('INSERT INTO t VALUES (1)', SqlDialect.sqlite),
'read_only_only');
expect(ruleOf('PRAGMA writable_schema = 1', SqlDialect.sqlite),
'sqlite_pragma');
expect(ruleOf('/*! DROP TABLE t */ SELECT 1', SqlDialect.mysql),
'mysql_executable_comment');
expect(ruleOf('WITH x AS (SELECT 1) DELETE FROM t', SqlDialect.postgres),
'data_modifying');
expect(ruleOf('SELECT 1 FOR UPDATE', SqlDialect.postgres),
'select_into_or_lock');
expect(ruleOf('EXPLAIN ANALYZE SELECT 1', SqlDialect.postgres),
'explain_analyze_or_write');
});

test('a query that may run has no refusal and no rule', () {
expect(McpSqlGuard.refusal('SELECT 1', SqlDialect.postgres), isNull);
expect(McpSqlGuard.check('SELECT 1', SqlDialect.postgres), isNull);
});

test('the message the model reads is unchanged by the rule id', () {
expect(McpSqlGuard.check('', SqlDialect.postgres),
'The query is empty.');
expect(McpSqlGuard.refusal('', SqlDialect.postgres)!.message,
'The query is empty.');
});
});
}
Loading