diff --git a/backend/windmill-worker/src/bigquery_executor.rs b/backend/windmill-worker/src/bigquery_executor.rs index b383b22966..50a2d3811e 100644 --- a/backend/windmill-worker/src/bigquery_executor.rs +++ b/backend/windmill-worker/src/bigquery_executor.rs @@ -23,7 +23,7 @@ use crate::common::{ s3_stream_and_upload_with_logs, OccupancyMetrics, S3ModeWorkerData, }; use crate::handle_child::run_future_with_polling_update_job_poller; -use crate::sanitized_sql_params::sanitize_and_interpolate_unsafe_sql_args; +use crate::sanitized_sql_params::{sanitize_and_interpolate_unsafe_sql_args, SqlStringEscaping}; use gcp_auth::{AuthenticationManager, CustomServiceAccount}; @@ -438,6 +438,7 @@ pub async fn do_bigquery( &sig, &bigquery_args, &reserved_variables, + SqlStringEscaping::Backslash, )?; let queries = parse_sql_blocks(query, false); diff --git a/backend/windmill-worker/src/duckdb_executor.rs b/backend/windmill-worker/src/duckdb_executor.rs index aecdb738b3..56b7e4101f 100644 --- a/backend/windmill-worker/src/duckdb_executor.rs +++ b/backend/windmill-worker/src/duckdb_executor.rs @@ -27,7 +27,7 @@ use crate::common::{build_args_values, get_reserved_variables, OccupancyMetrics} use crate::handle_child::run_future_with_polling_update_job_poller; #[cfg(feature = "mysql")] use crate::mysql_executor::MysqlDatabase; -use crate::sanitized_sql_params::sanitize_and_interpolate_unsafe_sql_args; +use crate::sanitized_sql_params::{sanitize_and_interpolate_unsafe_sql_args, SqlStringEscaping}; use crate::sql_utils::remove_comments; use crate::MAX_SQL_RESULT_SIZE; use windmill_common::client::AuthedClient; @@ -1391,8 +1391,13 @@ pub async fn do_duckdb( let reserved_variables = get_reserved_variables(job, &client.token, conn, parent_runnable_path).await?; - let (query, _) = - &sanitize_and_interpolate_unsafe_sql_args(query, &sig, &job_args, &reserved_variables)?; + let (query, _) = &sanitize_and_interpolate_unsafe_sql_args( + query, + &sig, + &job_args, + &reserved_variables, + SqlStringEscaping::Standard, + )?; let mut query = transform_s3_uris(query).await?; // `on_schema_change=sync`: interpolate the pre-pass probe with the same @@ -1409,6 +1414,7 @@ pub async fn do_duckdb( &sig, &job_args, &reserved_variables, + SqlStringEscaping::Standard, )?; Some(transform_s3_uris(&interp).await?) } diff --git a/backend/windmill-worker/src/mssql_executor.rs b/backend/windmill-worker/src/mssql_executor.rs index 02d0da1164..b5acbc8c9b 100644 --- a/backend/windmill-worker/src/mssql_executor.rs +++ b/backend/windmill-worker/src/mssql_executor.rs @@ -30,7 +30,7 @@ use crate::common::{ s3_stream_and_upload_with_logs, OccupancyMetrics, }; use crate::handle_child::run_future_with_polling_update_job_poller; -use crate::sanitized_sql_params::sanitize_and_interpolate_unsafe_sql_args; +use crate::sanitized_sql_params::{sanitize_and_interpolate_unsafe_sql_args, SqlStringEscaping}; use crate::sql_s3_input::fetch_s3object_as_json_text; use windmill_common::client::AuthedClient; use windmill_types::s3::S3Object; @@ -302,8 +302,13 @@ pub async fn do_mssql( let reserved_variables = get_reserved_variables(job, &authed_client.token, conn, parent_runnable_path).await?; - let (query, args_to_skip) = - &sanitize_and_interpolate_unsafe_sql_args(query, &sig, &mssql_args, &reserved_variables)?; + let (query, args_to_skip) = &sanitize_and_interpolate_unsafe_sql_args( + query, + &sig, + &mssql_args, + &reserved_variables, + SqlStringEscaping::Standard, + )?; let mut prepared_query = Query::new(query.to_owned()); for arg in &sig { diff --git a/backend/windmill-worker/src/mysql_executor.rs b/backend/windmill-worker/src/mysql_executor.rs index dcec77e298..e9154bb8eb 100644 --- a/backend/windmill-worker/src/mysql_executor.rs +++ b/backend/windmill-worker/src/mysql_executor.rs @@ -31,7 +31,7 @@ use crate::{ s3_stream_and_upload_with_logs, OccupancyMetrics, S3ModeWorkerData, }, handle_child::run_future_with_polling_update_job_poller, - sanitized_sql_params::sanitize_and_interpolate_unsafe_sql_args, + sanitized_sql_params::{sanitize_and_interpolate_unsafe_sql_args, SqlStringEscaping}, }; #[derive(Deserialize)] @@ -262,8 +262,13 @@ pub async fn do_mysql( let reserved_variables = get_reserved_variables(job, &client.token, conn, parent_runnable_path).await?; - let (query, args_to_skip) = - &sanitize_and_interpolate_unsafe_sql_args(query, &sig, &job_args, &reserved_variables)?; + let (query, args_to_skip) = &sanitize_and_interpolate_unsafe_sql_args( + query, + &sig, + &job_args, + &reserved_variables, + SqlStringEscaping::BackslashAndStandard, + )?; let using_named_params = RE_ARG_MYSQL_NAMED.captures_iter(query).count() > 0; diff --git a/backend/windmill-worker/src/oracledb_executor.rs b/backend/windmill-worker/src/oracledb_executor.rs index d66f22e25d..787386d9e6 100644 --- a/backend/windmill-worker/src/oracledb_executor.rs +++ b/backend/windmill-worker/src/oracledb_executor.rs @@ -28,7 +28,7 @@ use crate::{ S3ModeWorkerData, }, handle_child::run_future_with_polling_update_job_poller, - sanitized_sql_params::sanitize_and_interpolate_unsafe_sql_args, + sanitized_sql_params::{sanitize_and_interpolate_unsafe_sql_args, SqlStringEscaping}, }; use windmill_common::client::AuthedClient; @@ -405,8 +405,13 @@ pub async fn do_oracledb( let reserved_variables = get_reserved_variables(job, &client.token, conn, parent_runnable_path).await?; - let (query, args_to_skip) = - sanitize_and_interpolate_unsafe_sql_args(query, &sig, &job_args, &reserved_variables)?; + let (query, args_to_skip) = sanitize_and_interpolate_unsafe_sql_args( + query, + &sig, + &job_args, + &reserved_variables, + SqlStringEscaping::Standard, + )?; let (_, errors) = get_statement_values(sig.clone(), &job_args, &args_to_skip); diff --git a/backend/windmill-worker/src/pg_executor.rs b/backend/windmill-worker/src/pg_executor.rs index 69aa3ee0fb..89e2290b18 100644 --- a/backend/windmill-worker/src/pg_executor.rs +++ b/backend/windmill-worker/src/pg_executor.rs @@ -45,7 +45,7 @@ use crate::common::{ s3_stream_and_upload_with_logs, sizeof_val, OccupancyMetrics, S3ModeWorkerData, }; use crate::handle_child::run_future_with_polling_update_job_poller; -use crate::sanitized_sql_params::sanitize_and_interpolate_unsafe_sql_args; +use crate::sanitized_sql_params::{sanitize_and_interpolate_unsafe_sql_args, SqlStringEscaping}; use crate::sql_s3_input::fetch_s3object_as_json_text; use crate::sql_utils::remove_comments; use crate::{max_sql_result_size, sql_result_too_large_error, to_raw_value_within}; @@ -898,8 +898,13 @@ pub async fn do_postgresql( let reserved_variables = get_reserved_variables(job, &client.token, conn, parent_runnable_path).await?; - let (query, _) = - &sanitize_and_interpolate_unsafe_sql_args(query, &sig.args, &pg_args, &reserved_variables)?; + let (query, _) = &sanitize_and_interpolate_unsafe_sql_args( + query, + &sig.args, + &pg_args, + &reserved_variables, + SqlStringEscaping::Standard, + )?; let queries = parse_sql_blocks(query, true); diff --git a/backend/windmill-worker/src/sanitized_sql_params.rs b/backend/windmill-worker/src/sanitized_sql_params.rs index 8560f14798..e0f7323ba3 100644 --- a/backend/windmill-worker/src/sanitized_sql_params.rs +++ b/backend/windmill-worker/src/sanitized_sql_params.rs @@ -33,9 +33,39 @@ fn sanitize_identifier(arg: &Arg, input: &str) -> Result<(), error::Error> { } } +/// How a dialect escapes characters inside a single-quoted string literal. Contextual +/// variables are substituted as raw text, and `WM_END_USER_EMAIL` carries an app end user's +/// email into a query that runs with the author's credentials: an email may contain `'` +/// (and `\` in a quoted local part), so every value is escaped for the literal it lands in. +#[derive(Clone, Copy)] +pub enum SqlStringEscaping { + /// `'` → `''`; `\` is an ordinary character (PostgreSQL, MSSQL, Oracle, DuckDB). + Standard, + /// `\` → `\\` and `'` → `''`. `''` rather than `\'` because MySQL under + /// `NO_BACKSLASH_ESCAPES` would close the literal on `\'` (MySQL, Snowflake). + #[cfg_attr(not(any(feature = "mysql", feature = "snowflake")), allow(dead_code))] + BackslashAndStandard, + /// `\` → `\\` and `'` → `\'`; the dialect does not accept `''` (BigQuery). + #[cfg_attr(not(feature = "bigquery"), allow(dead_code))] + Backslash, +} + +impl SqlStringEscaping { + fn escape(self, value: &str) -> String { + match self { + SqlStringEscaping::Standard => value.replace('\'', "''"), + SqlStringEscaping::BackslashAndStandard => { + value.replace('\\', "\\\\").replace('\'', "''") + } + SqlStringEscaping::Backslash => value.replace('\\', "\\\\").replace('\'', "\\'"), + } + } +} + fn replace_contextual_variables( code: &mut String, contextual_variables: &HashMap, + escaping: SqlStringEscaping, ) -> () { let vars = RE_SQL_CONTEXTUAL_VAR .find_iter(&code) @@ -50,7 +80,7 @@ fn replace_contextual_variables( .unwrap(); let var_value = contextual_variables.get(var_name); if let Some(var_value) = var_value { - *code = code.replace(&var_pattern, var_value); + *code = code.replace(&var_pattern, &escaping.escape(var_value)); } } } @@ -60,11 +90,12 @@ pub fn sanitize_and_interpolate_unsafe_sql_args( args: &Vec, args_map: &HashMap, contextual_variables: &HashMap, + escaping: SqlStringEscaping, ) -> Result<(String, Vec), error::Error> { let mut ret = code.to_string(); let mut args_to_skip = vec![]; - replace_contextual_variables(&mut ret, contextual_variables); + replace_contextual_variables(&mut ret, contextual_variables, escaping); for arg in args { if let Some(typ) = &arg.otyp { @@ -128,3 +159,45 @@ pub fn sanitize_and_interpolate_unsafe_sql_args( Ok((ret, args_to_skip)) } + +#[cfg(test)] +mod tests { + use super::*; + + fn interpolate(code: &str, email: &str, escaping: SqlStringEscaping) -> String { + let vars = HashMap::from([("WM_END_USER_EMAIL".to_string(), email.to_string())]); + sanitize_and_interpolate_unsafe_sql_args(code, &vec![], &HashMap::new(), &vars, escaping) + .unwrap() + .0 + } + + #[test] + fn contextual_variables_stay_inside_their_string_literal() { + let code = "SELECT 1 WHERE email = '%%WM_END_USER_EMAIL%%'"; + let quote = r"x'/**/OR/**/'1'='1'--@e.com"; + let backslash = r#""x\'/**/OR/**/1=1#"@e.com"#; + + assert_eq!( + interpolate(code, quote, SqlStringEscaping::Standard), + r"SELECT 1 WHERE email = 'x''/**/OR/**/''1''=''1''--@e.com'" + ); + assert_eq!( + interpolate(code, backslash, SqlStringEscaping::BackslashAndStandard), + r#"SELECT 1 WHERE email = '"x\\''/**/OR/**/1=1#"@e.com'"# + ); + assert_eq!( + interpolate(code, backslash, SqlStringEscaping::Backslash), + r#"SELECT 1 WHERE email = '"x\\\'/**/OR/**/1=1#"@e.com'"# + ); + for escaping in [ + SqlStringEscaping::Standard, + SqlStringEscaping::BackslashAndStandard, + SqlStringEscaping::Backslash, + ] { + assert_eq!( + interpolate(code, "a.b+c@e.com", escaping), + "SELECT 1 WHERE email = 'a.b+c@e.com'" + ); + } + } +} diff --git a/backend/windmill-worker/src/snowflake_executor.rs b/backend/windmill-worker/src/snowflake_executor.rs index b4bb448b99..4502070e3c 100644 --- a/backend/windmill-worker/src/snowflake_executor.rs +++ b/backend/windmill-worker/src/snowflake_executor.rs @@ -26,7 +26,7 @@ use crate::common::{ s3_stream_and_upload_with_logs, OccupancyMetrics, S3ModeWorkerData, }; use crate::handle_child::run_future_with_polling_update_job_poller; -use crate::sanitized_sql_params::sanitize_and_interpolate_unsafe_sql_args; +use crate::sanitized_sql_params::{sanitize_and_interpolate_unsafe_sql_args, SqlStringEscaping}; use windmill_common::client::AuthedClient; #[derive(Serialize)] @@ -203,8 +203,13 @@ fn do_snowflake_inner<'a>( .map_err(|x| Error::ExecutionErr(x.to_string()))? .args; - let (query, args_to_skip) = - &sanitize_and_interpolate_unsafe_sql_args(query, &sig, &job_args, reserved_variables)?; + let (query, args_to_skip) = &sanitize_and_interpolate_unsafe_sql_args( + query, + &sig, + &job_args, + reserved_variables, + SqlStringEscaping::BackslashAndStandard, + )?; body.insert("statement".to_string(), json!(query));