fix(ai): harden write SQL confirmation flow
This commit is contained in:
1 parent
1e72d28ed3
commit
cb9d0f7cbf
20 files changed
+1258
-51
No files matched your search
@@ -132,6 +132,7 @@ pub async fn run_agent_loop(
|
||||
agent_mode: is_agent_mode,
|
||||
allow_writes: agent_ctx.sql_permissions.allow_writes,
|
||||
allow_dangerous: agent_ctx.sql_permissions.allow_dangerous,
|
||||
confirmed_write_sql: agent_ctx.sql_permissions.confirmed_write_sql.clone(),
|
||||
mcp_server_command: agent_ctx.cli_mcp_server_command.clone(),
|
||||
};
|
||||
if matches!(config.provider, AiProvider::ClaudeCodeCli) {
|
||||
@@ -163,7 +164,7 @@ pub async fn run_agent_loop(
|
||||
.await;
|
||||
}
|
||||
let tools = if is_agent_mode {
|
||||
agent_tools::all_tools(agent_ctx.db_type, agent_ctx.sql_permissions)
|
||||
agent_tools::all_tools(agent_ctx.db_type, agent_ctx.sql_permissions.clone())
|
||||
} else {
|
||||
agent_tools::read_only_tools(agent_ctx.db_type)
|
||||
};
|
||||
@@ -368,7 +369,7 @@ pub async fn run_agent_loop(
|
||||
let conn2 = agent_ctx.connection_id.clone();
|
||||
let db2 = agent_ctx.database.clone();
|
||||
let db_type = agent_ctx.db_type;
|
||||
let sql_permissions = agent_ctx.sql_permissions;
|
||||
let sql_permissions = agent_ctx.sql_permissions.clone();
|
||||
|
||||
// Split by index into parallel and sequential groups using tool metadata
|
||||
let tool_parallel_map: std::collections::HashMap<&str, bool> =
|
||||
@@ -387,7 +388,8 @@ pub async fn run_agent_loop(
|
||||
let state = Arc::clone(&state2);
|
||||
let conn = conn2.clone();
|
||||
let db = db2.clone();
|
||||
async move { agent_tools::execute_tool(&tc, &state, &conn, &db, &db_type, sql_permissions).await }
|
||||
let perms = sql_permissions.clone();
|
||||
async move { agent_tools::execute_tool(&tc, &state, &conn, &db, &db_type, perms).await }
|
||||
})
|
||||
.collect();
|
||||
let parallel_results = join_all(parallel_futures).await;
|
||||
@@ -397,7 +399,7 @@ pub async fn run_agent_loop(
|
||||
for &i in &sequential_indices {
|
||||
let tc = make_tc(&collected_tool_calls[i]);
|
||||
sequential_results
|
||||
.push(agent_tools::execute_tool(&tc, &state2, &conn2, &db2, &db_type, sql_permissions).await);
|
||||
.push(agent_tools::execute_tool(&tc, &state2, &conn2, &db2, &db_type, sql_permissions.clone()).await);
|
||||
}
|
||||
|
||||
// Merge results back into original order
|
||||
|
||||
@@ -27,10 +27,68 @@ const BROWSE_COLLECTION_LIMIT: usize = 20;
|
||||
/// Absolute maximum rows any query tool may request.
|
||||
const MAX_ALLOWED_ROWS: usize = 100;
|
||||
|
||||
#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
|
||||
#[derive(Debug, Clone, Default, PartialEq, Eq)]
|
||||
pub struct AgentSqlPermissions {
|
||||
pub allow_writes: bool,
|
||||
pub allow_dangerous: bool,
|
||||
/// When present, write/DDL execute_query calls must match this SQL
|
||||
/// (after trimming only surrounding whitespace). Set by the frontend
|
||||
/// when the user confirms a specific write-SQL proposal.
|
||||
pub confirmed_write_sql: Option<String>,
|
||||
}
|
||||
|
||||
/// Build the write permissions for one AI-agent run from an explicit user
|
||||
/// confirmation. Both Desktop and Web use this fail-closed boundary so an
|
||||
/// empty confirmation or a production target cannot enable writes.
|
||||
pub fn confirmed_write_sql_permissions(
|
||||
production_database: bool,
|
||||
allow_write_sql: bool,
|
||||
confirmed_write_sql: Option<String>,
|
||||
) -> AgentSqlPermissions {
|
||||
let confirmed_write_sql = confirmed_write_sql.filter(|sql| !sql.trim().is_empty());
|
||||
let write_sql_confirmed = !production_database && allow_write_sql && confirmed_write_sql.is_some();
|
||||
|
||||
AgentSqlPermissions {
|
||||
allow_writes: write_sql_confirmed,
|
||||
allow_dangerous: write_sql_confirmed,
|
||||
confirmed_write_sql: write_sql_confirmed.then_some(confirmed_write_sql).flatten(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Verify that the confirmed connection/database snapshot matches the actual
|
||||
/// target. Returns `(allow_write_sql, confirmed_write_sql)` — when the target
|
||||
/// does not match, the grant is voided (allow=false, confirmed=None).
|
||||
///
|
||||
/// This is defense-in-depth: the frontend also verifies synchronously, but
|
||||
/// this backend check protects CLI-provider and API-driven paths.
|
||||
pub fn verify_confirmed_target(
|
||||
allow_write_sql: Option<bool>,
|
||||
confirmed_write_sql: Option<String>,
|
||||
confirmed_connection_id: Option<String>,
|
||||
confirmed_database: Option<String>,
|
||||
actual_connection_id: &str,
|
||||
actual_database: &str,
|
||||
) -> (Option<bool>, Option<String>) {
|
||||
let Some(ref confirmed_sql) = confirmed_write_sql else {
|
||||
return (allow_write_sql, confirmed_write_sql);
|
||||
};
|
||||
// Only verify when a write SQL was actually confirmed.
|
||||
let target_mismatch = confirmed_connection_id.as_deref() != Some(actual_connection_id)
|
||||
|| confirmed_database.as_deref() != Some(actual_database);
|
||||
if target_mismatch {
|
||||
log::warn!(
|
||||
"Write-SQL grant voided: confirmed target (conn={:?}, db={:?}) does not match actual (conn={}, db={}).",
|
||||
confirmed_connection_id,
|
||||
confirmed_database,
|
||||
actual_connection_id,
|
||||
actual_database,
|
||||
);
|
||||
// SQL can contain literals or credentials. Keep diagnostic visibility
|
||||
// behind the shared debug-only redaction boundary.
|
||||
crate::sql_diagnostics::debug_sql("write_sql_grant_voided", confirmed_sql);
|
||||
return (Some(false), None);
|
||||
}
|
||||
(allow_write_sql, confirmed_write_sql)
|
||||
}
|
||||
|
||||
fn sql_risk_allowed(risk: SqlRisk, permissions: AgentSqlPermissions) -> bool {
|
||||
@@ -433,7 +491,35 @@ async fn execute_get_columns(
|
||||
Ok(lines.join("\n"))
|
||||
}
|
||||
|
||||
/// Execute a read-only SQL query via the execute_query tool.
|
||||
/// Normalize a SQL string for confirmation comparison.
|
||||
///
|
||||
/// Confirmation is intentionally fail-closed: only surrounding whitespace is
|
||||
/// ignored. SQL case, internal whitespace, comments, literals, and quoted
|
||||
/// identifiers can all affect execution semantics across supported dialects.
|
||||
pub fn normalize_sql_for_confirmation(sql: &str) -> String {
|
||||
sql.trim().to_string()
|
||||
}
|
||||
|
||||
fn truncate_sql_for_error(sql: &str) -> String {
|
||||
let s = sql.trim();
|
||||
let char_count = s.chars().count();
|
||||
if char_count <= 120 {
|
||||
s.to_string()
|
||||
} else {
|
||||
format!("{}...", s.chars().take(117).collect::<String>())
|
||||
}
|
||||
}
|
||||
|
||||
/// Check whether `executed_sql` matches the user-confirmed write SQL. Returns
|
||||
/// `true` when no confirmation is required (confirmed is `None`) or when the
|
||||
/// trimmed forms match.
|
||||
fn sql_matches_confirmed_write(executed_sql: &str, confirmed: &Option<String>) -> bool {
|
||||
match confirmed {
|
||||
None => true,
|
||||
Some(confirmed) => normalize_sql_for_confirmation(executed_sql) == normalize_sql_for_confirmation(confirmed),
|
||||
}
|
||||
}
|
||||
|
||||
async fn execute_execute_query(
|
||||
tool_call: &ToolCall,
|
||||
state: &Arc<AppState>,
|
||||
@@ -463,7 +549,7 @@ async fn execute_execute_query(
|
||||
return Err("Blocked: AI agents cannot execute writes or DDL on a production database. Return the SQL for the user to review and execute manually in DBX.".to_string());
|
||||
}
|
||||
}
|
||||
if !sql_risk_allowed(risk, sql_permissions) {
|
||||
if !sql_risk_allowed(risk, sql_permissions.clone()) {
|
||||
if risk == SqlRisk::Transaction {
|
||||
return Err("Blocked: transaction control statements are not available to the AI agent.".to_string());
|
||||
}
|
||||
@@ -473,6 +559,19 @@ async fn execute_execute_query(
|
||||
));
|
||||
}
|
||||
|
||||
// When the user confirmed a specific write SQL, the agent must
|
||||
// execute only that SQL — not an arbitrary different statement.
|
||||
if risk != SqlRisk::ReadOnly && !sql_matches_confirmed_write(sql, &sql_permissions.confirmed_write_sql) {
|
||||
let confirmed = sql_permissions.confirmed_write_sql.as_deref().unwrap_or("");
|
||||
return Err(format!(
|
||||
"Blocked: the executed SQL does not match the user-confirmed SQL.\n\
|
||||
Confirmed: {}\n\
|
||||
Attempted: {}",
|
||||
truncate_sql_for_error(confirmed),
|
||||
truncate_sql_for_error(sql),
|
||||
));
|
||||
}
|
||||
|
||||
// Execute query using existing infrastructure
|
||||
let options = QueryExecutionOptions { max_rows: Some(limit), timeout_secs: Some(30), ..Default::default() };
|
||||
let result =
|
||||
@@ -838,7 +937,10 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn confirmed_sql_permissions_update_execute_query_contract() {
|
||||
let tools = all_tools(DatabaseType::Mysql, AgentSqlPermissions { allow_writes: true, allow_dangerous: true });
|
||||
let tools = all_tools(
|
||||
DatabaseType::Mysql,
|
||||
AgentSqlPermissions { allow_writes: true, allow_dangerous: true, confirmed_write_sql: None },
|
||||
);
|
||||
let execute_query = tools.iter().find(|tool| tool.name == "execute_query").unwrap();
|
||||
|
||||
assert!(execute_query.description.contains("explicitly confirmed"));
|
||||
@@ -849,10 +951,13 @@ mod tests {
|
||||
fn sql_permissions_keep_writes_blocked_until_confirmation() {
|
||||
assert!(!sql_risk_allowed(SqlRisk::Write, AgentSqlPermissions::default()));
|
||||
assert!(!sql_risk_allowed(SqlRisk::Ddl, AgentSqlPermissions::default()));
|
||||
assert!(sql_risk_allowed(SqlRisk::Ddl, AgentSqlPermissions { allow_writes: true, allow_dangerous: true }));
|
||||
assert!(sql_risk_allowed(
|
||||
SqlRisk::Ddl,
|
||||
AgentSqlPermissions { allow_writes: true, allow_dangerous: true, confirmed_write_sql: None }
|
||||
));
|
||||
assert!(!sql_risk_allowed(
|
||||
SqlRisk::Transaction,
|
||||
AgentSqlPermissions { allow_writes: true, allow_dangerous: true }
|
||||
AgentSqlPermissions { allow_writes: true, allow_dangerous: true, confirmed_write_sql: None }
|
||||
));
|
||||
}
|
||||
|
||||
@@ -946,4 +1051,128 @@ mod tests {
|
||||
let result = build_browse_query(&DatabaseType::Postgres, "articles", "", 10);
|
||||
assert!(result.is_err());
|
||||
}
|
||||
|
||||
// ── SQL confirmation binding tests ──────────────────────────────────────
|
||||
|
||||
#[test]
|
||||
fn normalize_sql_only_trims_outer_whitespace() {
|
||||
assert_eq!(normalize_sql_for_confirmation(" CREATE TABLE users (id INT);\n"), "CREATE TABLE users (id INT);");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_sql_binding_rejects_quoted_identifier_case_change() {
|
||||
let confirmed = Some("DROP TABLE \"Users\"".to_string());
|
||||
assert!(!sql_matches_confirmed_write("DROP TABLE \"users\"", &confirmed));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_sql_binding_rejects_keyword_case_or_reformatting() {
|
||||
let confirmed = Some("DELETE FROM AuditLog WHERE id = 1".to_string());
|
||||
assert!(!sql_matches_confirmed_write("delete from AuditLog where id = 1", &confirmed));
|
||||
assert!(!sql_matches_confirmed_write("DELETE FROM AuditLog\nWHERE id = 1", &confirmed));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_sql_binding_rejects_line_comment_newline_change() {
|
||||
let confirmed = Some("DELETE FROM users -- only one record\nWHERE id = 1".to_string());
|
||||
assert!(!sql_matches_confirmed_write("DELETE FROM users -- only one record WHERE id = 1", &confirmed));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sql_matches_when_no_confirmation_required() {
|
||||
assert!(sql_matches_confirmed_write("DELETE FROM users WHERE id = 1", &None));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sql_matches_when_only_outer_whitespace_differs() {
|
||||
// The confirmed statement itself must be unchanged; only surrounding
|
||||
// whitespace is ignored before comparison.
|
||||
let confirmed = Some("CREATE TABLE users (id INT)".to_string());
|
||||
assert!(sql_matches_confirmed_write(" CREATE TABLE users (id INT) ", &confirmed,));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sql_mismatch_rejected_when_executed_differs_from_confirmed() {
|
||||
let confirmed = Some("CREATE TABLE users (id INT)".to_string());
|
||||
assert!(!sql_matches_confirmed_write("DROP TABLE users", &confirmed,));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_sql_binding_rejects_same_table_different_statement() {
|
||||
let confirmed = Some("INSERT INTO users (id, name) VALUES (1, 'test')".to_string());
|
||||
assert!(!sql_matches_confirmed_write("DELETE FROM users WHERE id = 1", &confirmed,));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_sql_binding_rejects_different_case_in_data_values() {
|
||||
// Confirmed VALUES ('Alice') must NOT match executed VALUES ('alice').
|
||||
// String-literal data values are preserved verbatim.
|
||||
let confirmed = Some("INSERT INTO users (name) VALUES ('Alice')".to_string());
|
||||
assert!(!sql_matches_confirmed_write("INSERT INTO users (name) VALUES ('alice')", &confirmed,));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_sql_binding_rejects_different_whitespace_in_data_values() {
|
||||
// Confirmed VALUES ('a b') must NOT match executed VALUES ('a b').
|
||||
let confirmed = Some("INSERT INTO t (c) VALUES ('a b')".to_string());
|
||||
assert!(!sql_matches_confirmed_write("INSERT INTO t (c) VALUES ('a b')", &confirmed,));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_sql_default_is_none() {
|
||||
let perms = AgentSqlPermissions::default();
|
||||
assert_eq!(perms.confirmed_write_sql, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_write_sql_diagnostics_redact_sensitive_literals() {
|
||||
let confirmed_sql = "CREATE USER app_user WITH PASSWORD 'secret-123'";
|
||||
let diagnostic = crate::sql_diagnostics::redact_sql_for_diagnostics(confirmed_sql);
|
||||
|
||||
assert!(!diagnostic.contains("secret-123"));
|
||||
assert!(diagnostic.contains("'[REDACTED]'"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_write_permissions_bind_only_a_nonproduction_nonempty_confirmation() {
|
||||
let confirmed_sql = Some("DELETE FROM sessions WHERE id = 7".to_string());
|
||||
let permissions = confirmed_write_sql_permissions(false, true, confirmed_sql.clone());
|
||||
|
||||
assert!(permissions.allow_writes);
|
||||
assert!(permissions.allow_dangerous);
|
||||
assert_eq!(permissions.confirmed_write_sql, confirmed_sql);
|
||||
assert!(!sql_matches_confirmed_write("DROP TABLE sessions", &permissions.confirmed_write_sql));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_write_permissions_fail_closed_for_production_or_empty_confirmation() {
|
||||
for (production_database, confirmed_write_sql) in
|
||||
[(true, Some("DELETE FROM sessions".to_string())), (false, None), (false, Some(" \n".to_string()))]
|
||||
{
|
||||
let permissions = confirmed_write_sql_permissions(production_database, true, confirmed_write_sql);
|
||||
assert!(!permissions.allow_writes);
|
||||
assert!(!permissions.allow_dangerous);
|
||||
assert_eq!(permissions.confirmed_write_sql, None);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn confirmed_sql_preserved_through_permission_construction() {
|
||||
let perms = AgentSqlPermissions {
|
||||
allow_writes: true,
|
||||
allow_dangerous: true,
|
||||
confirmed_write_sql: Some("CREATE TABLE t (c INT)".to_string()),
|
||||
};
|
||||
assert_eq!(perms.confirmed_write_sql.as_deref(), Some("CREATE TABLE t (c INT)"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn write_allowed_when_confirmed_sql_is_none_and_writes_enabled() {
|
||||
// confirmed_write_sql=None + allow_writes=true: write SQL is allowed
|
||||
// (the check passes because sql_matches_confirmed_write returns true
|
||||
// when no confirmation is required). This documents the current
|
||||
// contract — the frontend is responsible for only sending
|
||||
// allow_write_sql=true when a specific SQL was confirmed.
|
||||
assert!(sql_matches_confirmed_write("INSERT INTO t VALUES (1)", &None));
|
||||
}
|
||||
}
|
||||
@@ -568,6 +568,7 @@ mod tests {
|
||||
agent_mode: true,
|
||||
allow_writes: false,
|
||||
allow_dangerous: false,
|
||||
confirmed_write_sql: None,
|
||||
mcp_server_command: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -17,6 +17,9 @@ pub struct CliAgentRunOptions {
|
||||
pub agent_mode: bool,
|
||||
pub allow_writes: bool,
|
||||
pub allow_dangerous: bool,
|
||||
/// When present, write/DDL execute_query calls in the MCP subprocess must
|
||||
/// match this SQL after trimming only surrounding whitespace.
|
||||
pub confirmed_write_sql: Option<String>,
|
||||
pub mcp_server_command: Option<CliAgentCommandSpec>,
|
||||
}
|
||||
|
||||
@@ -61,13 +64,41 @@ pub fn dbx_mcp_enabled_tools(agent_mode: bool) -> Vec<&'static str> {
|
||||
}
|
||||
|
||||
pub fn dbx_mcp_scope_env(options: &CliAgentRunOptions) -> Vec<(&'static str, String)> {
|
||||
vec![
|
||||
let mut env = vec![
|
||||
("DBX_MCP_ALLOW_WRITES", if options.allow_writes { "1" } else { "0" }.to_string()),
|
||||
("DBX_MCP_ALLOW_DANGEROUS_SQL", if options.allow_dangerous { "1" } else { "0" }.to_string()),
|
||||
("DBX_MCP_SCOPE_CONNECTION_ID", options.connection_id.clone()),
|
||||
("DBX_MCP_SCOPE_CONNECTION_NAME", options.connection_name.clone()),
|
||||
("DBX_MCP_SCOPE_DATABASE", options.database.clone()),
|
||||
]
|
||||
];
|
||||
if let Some(ref sql) = options.confirmed_write_sql {
|
||||
env.push(("DBX_MCP_CONFIRMED_WRITE_SQL", sql.clone()));
|
||||
}
|
||||
env
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod scope_env_tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn confirmed_write_sql_is_passed_to_the_scoped_mcp_subprocess() {
|
||||
let options = CliAgentRunOptions {
|
||||
connection_id: "connection-1".to_string(),
|
||||
connection_name: "staging".to_string(),
|
||||
database: "app".to_string(),
|
||||
agent_mode: true,
|
||||
allow_writes: true,
|
||||
allow_dangerous: true,
|
||||
confirmed_write_sql: Some("DELETE FROM sessions WHERE id = 7".to_string()),
|
||||
mcp_server_command: None,
|
||||
};
|
||||
|
||||
let env = dbx_mcp_scope_env(&options);
|
||||
assert!(env.contains(&("DBX_MCP_CONFIRMED_WRITE_SQL", "DELETE FROM sessions WHERE id = 7".to_string())));
|
||||
assert!(env.contains(&("DBX_MCP_ALLOW_WRITES", "1".to_string())));
|
||||
assert!(env.contains(&("DBX_MCP_ALLOW_DANGEROUS_SQL", "1".to_string())));
|
||||
}
|
||||
}
|
||||
|
||||
pub fn append_config_overrides(args: &mut Vec<String>, overrides: impl IntoIterator<Item = String>) {
|
||||
|
||||
@@ -628,6 +628,7 @@ mod tests {
|
||||
agent_mode: true,
|
||||
allow_writes: false,
|
||||
allow_dangerous: false,
|
||||
confirmed_write_sql: None,
|
||||
mcp_server_command: None,
|
||||
}
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user