From 862cb6a63ee8c9481d767e8c378d0546c8d12ec6 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 23 Sep 2026 16:58:00 +0200 Subject: [PATCH] fix(worker): escape " for MSSQL, hex-escape BigQuery quotes, substitute in one pass MSSQL reads "..." as a string under QUOTED_IDENTIFIER OFF, so double quotes are doubled too. BigQuery quotes become \x27/\x22, which leaves no quote character for parse_sql_blocks to misread or to close a raw string. Contextual variables are substituted in a single regex pass, so a value containing %%WM_*%% (valid in an email) is never expanded itself. Co-Authored-By: Claude Opus 5.5 (1M context) --- backend/windmill-worker/src/mssql_executor.rs | 2 +- .../src/sanitized_sql_params.rs | 86 +++++++++++-------- 2 files changed, 51 insertions(+), 37 deletions(-) diff --git a/backend/windmill-worker/src/mssql_executor.rs b/backend/windmill-worker/src/mssql_executor.rs index 36c785b8d6..f275191690 100644 --- a/backend/windmill-worker/src/mssql_executor.rs +++ b/backend/windmill-worker/src/mssql_executor.rs @@ -307,7 +307,7 @@ pub async fn do_mssql( &sig, &mssql_args, &reserved_variables, - SqlStringEscaping::Quote, + SqlStringEscaping::BothQuotes, )?; let mut prepared_query = Query::new(query.to_owned()); diff --git a/backend/windmill-worker/src/sanitized_sql_params.rs b/backend/windmill-worker/src/sanitized_sql_params.rs index 5f88f8d7e4..6d0f1dc638 100644 --- a/backend/windmill-worker/src/sanitized_sql_params.rs +++ b/backend/windmill-worker/src/sanitized_sql_params.rs @@ -1,6 +1,6 @@ use anyhow::anyhow; use regex::Regex; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use serde_json::Value; use windmill_common::error; @@ -38,27 +38,27 @@ fn sanitize_identifier(arg: &Arg, input: &str) -> Result<(), error::Error> { /// end user's email into a query that runs with the author's credentials: an email may /// contain `'` (and `\` or `"` in a quoted local part). /// -/// The escaping must hold whatever literal form and session mode the query uses, so where a -/// dialect can process backslash escapes in some literal (PostgreSQL/DuckDB `E'...'`, MySQL -/// outside `NO_BACKSLASH_ESCAPES`), `\` is doubled too: otherwise `\'` in the value becomes -/// `\''`, whose second quote closes the literal. The price is a doubled `\` (or, in MySQL, a -/// doubled quote of the other style than the literal) in the rare value that contains one. +/// Each mode must hold in every quote-delimited literal form and session mode the dialect +/// has, since a script can switch modes itself. Where some literal processes backslash +/// escapes, `\` is doubled too, or a value's `\'` would become `\''` and close the literal. +/// The price is a doubled `\` or quote in the rare value containing one, in a literal that +/// did not need it. Dollar-quoted, Oracle `q'[...]'` and raw literals cannot be escaped into. #[derive(Clone, Copy)] pub enum SqlStringEscaping { - /// `'` → `''`; `\` is never an escape (MSSQL, Oracle). - #[cfg_attr( - not(any(all(feature = "enterprise", feature = "mssql"), feature = "oracledb")), - allow(dead_code) - )] + /// `'` → `''` (Oracle). + #[cfg_attr(not(feature = "oracledb"), allow(dead_code))] Quote, - /// `\` → `\\`, `'` → `''`; `"` quotes identifiers (PostgreSQL, DuckDB, Snowflake). + /// `'` → `''`, `"` → `""`: `"..."` is a string under `QUOTED_IDENTIFIER OFF` (MSSQL). + #[cfg_attr(not(all(feature = "enterprise", feature = "mssql")), allow(dead_code))] + BothQuotes, + /// `\` → `\\`, `'` → `''` (PostgreSQL and DuckDB `E'...'`, Snowflake). QuoteAndBackslash, - /// `\` → `\\`, `'` → `''`, `"` → `""`: MySQL also accepts `"..."` literals, and only - /// doubling stays confined under `NO_BACKSLASH_ESCAPES`. + /// `\` → `\\`, `'` → `''`, `"` → `""`: `"..."` is a string, and only doubling stays + /// confined under `NO_BACKSLASH_ESCAPES` (MySQL). #[cfg_attr(not(feature = "mysql"), allow(dead_code))] MySql, - /// `\` → `\\`, `'` → `\'`, `"` → `\"`: backslash escapes are always on, both quote - /// styles delimit strings, and `''` is not accepted. + /// `\` → `\\`, quotes as `\x27`/`\x22`: `''` is not accepted, and a value with no quote + /// characters left cannot desync `parse_sql_blocks` or close a raw string (BigQuery). #[cfg_attr(not(feature = "bigquery"), allow(dead_code))] BigQuery, } @@ -67,6 +67,7 @@ impl SqlStringEscaping { fn escape(self, value: &str) -> String { match self { SqlStringEscaping::Quote => value.replace('\'', "''"), + SqlStringEscaping::BothQuotes => value.replace('\'', "''").replace('"', "\"\""), SqlStringEscaping::QuoteAndBackslash => value.replace('\\', "\\\\").replace('\'', "''"), SqlStringEscaping::MySql => value .replace('\\', "\\\\") @@ -74,33 +75,27 @@ impl SqlStringEscaping { .replace('"', "\"\""), SqlStringEscaping::BigQuery => value .replace('\\', "\\\\") - .replace('\'', "\\'") - .replace('"', "\\\""), + .replace('\'', "\\x27") + .replace('"', "\\x22"), } } } +/// A single pass, so a value containing `%%WM_*%%` is never itself expanded. fn replace_contextual_variables( code: &mut String, contextual_variables: &HashMap, escaping: SqlStringEscaping, ) -> () { - let vars = RE_SQL_CONTEXTUAL_VAR - .find_iter(&code) - .map(|m| m.as_str().to_string()) - .collect::>(); - - for var_pattern in vars { - let var_name = var_pattern - .strip_prefix("%%") - .unwrap() - .strip_suffix("%%") - .unwrap(); - let var_value = contextual_variables.get(var_name); - if let Some(var_value) = var_value { - *code = code.replace(&var_pattern, &escaping.escape(var_value)); - } - } + *code = RE_SQL_CONTEXTUAL_VAR + .replace_all(code, |caps: ®ex::Captures| { + let pattern = &caps[0]; + match contextual_variables.get(&pattern[2..pattern.len() - 2]) { + Some(value) => escaping.escape(value), + None => pattern.to_string(), + } + }) + .into_owned(); } pub fn sanitize_and_interpolate_unsafe_sql_args( @@ -209,11 +204,30 @@ mod tests { r#"SELECT 1 WHERE email = '""x\\''/**/OR/**/1=1#""@e.com'"# ); assert_eq!( - interpolate(code, backslash, SqlStringEscaping::BigQuery), - r#"SELECT 1 WHERE email = '\"x\\\'/**/OR/**/1=1#\"@e.com'"# + interpolate(code, backslash, SqlStringEscaping::BothQuotes), + r#"SELECT 1 WHERE email = '""x\''/**/OR/**/1=1#""@e.com'"# ); + assert_eq!( + interpolate(code, backslash, SqlStringEscaping::BigQuery), + r#"SELECT 1 WHERE email = '\x22x\\\x27/**/OR/**/1=1#\x22@e.com'"# + ); + // a value is never expanded itself: `%` is valid in an email's local part + let vars = HashMap::from([ + ("WM_EMAIL".to_string(), "%%WM_TOKEN%%@e.com".to_string()), + ("WM_TOKEN".to_string(), "secret".to_string()), + ]); + let (out, _) = sanitize_and_interpolate_unsafe_sql_args( + "SELECT '%%WM_EMAIL%%', '%%WM_TOKEN%%'", + &vec![], + &HashMap::new(), + &vars, + SqlStringEscaping::Quote, + ) + .unwrap(); + assert_eq!(out, "SELECT '%%WM_TOKEN%%@e.com', 'secret'"); for escaping in [ SqlStringEscaping::Quote, + SqlStringEscaping::BothQuotes, SqlStringEscaping::QuoteAndBackslash, SqlStringEscaping::MySql, SqlStringEscaping::BigQuery,