mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-13 00:03:00 +00:00
fix(worker): make contextual-variable escaping independent of literal form and sql_mode
Reading the MySQL sql_mode, and scanning the script for NO_BACKSLASH_ESCAPES, cannot see every way the mode changes, and quote doubling that leaves \ alone turns a value's \' into \'' under backslash escapes (PostgreSQL and DuckDB E'...' literals too). Double backslashes wherever a literal can process escapes, and double both quote styles in MySQL, so a value stays confined whatever literal form or mode the query runs under. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
e8d4ae8fd0
commit
59b8e92909
@@ -438,7 +438,7 @@ pub async fn do_bigquery(
|
||||
&sig,
|
||||
&bigquery_args,
|
||||
&reserved_variables,
|
||||
SqlStringEscaping::Backslash,
|
||||
SqlStringEscaping::BigQuery,
|
||||
)?;
|
||||
|
||||
let queries = parse_sql_blocks(query, false);
|
||||
|
||||
@@ -1396,7 +1396,7 @@ pub async fn do_duckdb(
|
||||
&sig,
|
||||
&job_args,
|
||||
&reserved_variables,
|
||||
SqlStringEscaping::Standard,
|
||||
SqlStringEscaping::QuoteAndBackslash,
|
||||
)?;
|
||||
let mut query = transform_s3_uris(query).await?;
|
||||
|
||||
@@ -1414,7 +1414,7 @@ pub async fn do_duckdb(
|
||||
&sig,
|
||||
&job_args,
|
||||
&reserved_variables,
|
||||
SqlStringEscaping::Standard,
|
||||
SqlStringEscaping::QuoteAndBackslash,
|
||||
)?;
|
||||
Some(transform_s3_uris(&interp).await?)
|
||||
}
|
||||
|
||||
@@ -307,7 +307,7 @@ pub async fn do_mssql(
|
||||
&sig,
|
||||
&mssql_args,
|
||||
&reserved_variables,
|
||||
SqlStringEscaping::Standard,
|
||||
SqlStringEscaping::Quote,
|
||||
)?;
|
||||
|
||||
let mut prepared_query = Query::new(query.to_owned());
|
||||
|
||||
@@ -31,9 +31,7 @@ use crate::{
|
||||
s3_stream_and_upload_with_logs, OccupancyMetrics, S3ModeWorkerData,
|
||||
},
|
||||
handle_child::run_future_with_polling_update_job_poller,
|
||||
sanitized_sql_params::{
|
||||
has_contextual_variables, sanitize_and_interpolate_unsafe_sql_args, SqlStringEscaping,
|
||||
},
|
||||
sanitized_sql_params::{sanitize_and_interpolate_unsafe_sql_args, SqlStringEscaping},
|
||||
};
|
||||
|
||||
#[derive(Deserialize)]
|
||||
@@ -163,26 +161,6 @@ fn do_mysql_inner<'a>(
|
||||
Ok(result_f.boxed())
|
||||
}
|
||||
|
||||
/// Under `NO_BACKSLASH_ESCAPES` a backslash-escaped quote closes the literal, so quotes are
|
||||
/// doubled instead when the session has it or when the script may turn it on itself.
|
||||
async fn mysql_string_escaping(
|
||||
conn: &mut mysql_async::Conn,
|
||||
query: &str,
|
||||
) -> windmill_common::error::Result<SqlStringEscaping> {
|
||||
let sql_mode: Option<String> = conn
|
||||
.query_first("SELECT @@SESSION.sql_mode")
|
||||
.await
|
||||
.map_err(to_anyhow)?;
|
||||
let no_backslash_escapes = |s: &str| s.to_ascii_uppercase().contains("NO_BACKSLASH_ESCAPES");
|
||||
Ok(
|
||||
if sql_mode.as_deref().is_some_and(no_backslash_escapes) || no_backslash_escapes(query) {
|
||||
SqlStringEscaping::DoubledQuotes
|
||||
} else {
|
||||
SqlStringEscaping::Backslash
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
pub async fn do_mysql(
|
||||
job: &MiniPulledJob,
|
||||
client: &AuthedClient,
|
||||
@@ -284,21 +262,12 @@ pub async fn do_mysql(
|
||||
let reserved_variables =
|
||||
get_reserved_variables(job, &client.token, conn, parent_runnable_path).await?;
|
||||
|
||||
let pool = mysql_async::Pool::new(opts);
|
||||
let mut mysql_conn = pool.get_conn().await.map_err(to_anyhow)?;
|
||||
|
||||
let escaping = if has_contextual_variables(query) {
|
||||
mysql_string_escaping(&mut mysql_conn, query).await?
|
||||
} else {
|
||||
SqlStringEscaping::Backslash
|
||||
};
|
||||
|
||||
let (query, args_to_skip) = &sanitize_and_interpolate_unsafe_sql_args(
|
||||
query,
|
||||
&sig,
|
||||
&job_args,
|
||||
&reserved_variables,
|
||||
escaping,
|
||||
SqlStringEscaping::MySql,
|
||||
)?;
|
||||
|
||||
let using_named_params = RE_ARG_MYSQL_NAMED.captures_iter(query).count() > 0;
|
||||
@@ -372,6 +341,8 @@ pub async fn do_mysql(
|
||||
}
|
||||
}
|
||||
|
||||
let pool = mysql_async::Pool::new(opts);
|
||||
let mysql_conn = pool.get_conn().await.map_err(to_anyhow)?;
|
||||
let conn_a = Arc::new(Mutex::new(mysql_conn));
|
||||
|
||||
let queries = parse_sql_blocks(query, false);
|
||||
|
||||
@@ -410,7 +410,7 @@ pub async fn do_oracledb(
|
||||
&sig,
|
||||
&job_args,
|
||||
&reserved_variables,
|
||||
SqlStringEscaping::Standard,
|
||||
SqlStringEscaping::Quote,
|
||||
)?;
|
||||
|
||||
let (_, errors) = get_statement_values(sig.clone(), &job_args, &args_to_skip);
|
||||
|
||||
@@ -903,7 +903,7 @@ pub async fn do_postgresql(
|
||||
&sig.args,
|
||||
&pg_args,
|
||||
&reserved_variables,
|
||||
SqlStringEscaping::Standard,
|
||||
SqlStringEscaping::QuoteAndBackslash,
|
||||
)?;
|
||||
|
||||
let queries = parse_sql_blocks(query, true);
|
||||
|
||||
@@ -33,47 +33,53 @@ fn sanitize_identifier(arg: &Arg, input: &str) -> Result<(), error::Error> {
|
||||
}
|
||||
}
|
||||
|
||||
/// How a dialect escapes characters inside a 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 `\` or `"`
|
||||
/// in a quoted local part), so every value is escaped for any string literal it lands in.
|
||||
/// How a dialect escapes a value substituted into a quote-delimited 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 `\` 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.
|
||||
#[derive(Clone, Copy)]
|
||||
pub enum SqlStringEscaping {
|
||||
/// `'` → `''`; `\` is ordinary and `"` quotes identifiers (PostgreSQL, MSSQL, Oracle,
|
||||
/// DuckDB).
|
||||
Standard,
|
||||
/// `\` → `\\`, `'` → `\'` and `"` → `\"`, exact in both `'...'` and `"..."` literals
|
||||
/// (MySQL, Snowflake, BigQuery; BigQuery does not accept `''`).
|
||||
/// `'` → `''`; `\` is never an escape (MSSQL, Oracle).
|
||||
#[cfg_attr(
|
||||
not(any(feature = "mysql", feature = "snowflake", feature = "bigquery")),
|
||||
not(any(all(feature = "enterprise", feature = "mssql"), feature = "oracledb")),
|
||||
allow(dead_code)
|
||||
)]
|
||||
Backslash,
|
||||
/// `'` → `''` and `"` → `""`, for MySQL under `NO_BACKSLASH_ESCAPES`, where `\` is
|
||||
/// ordinary and `\'` would close the literal. A quote of the other style than the
|
||||
/// surrounding literal comes out doubled.
|
||||
Quote,
|
||||
/// `\` → `\\`, `'` → `''`; `"` quotes identifiers (PostgreSQL, DuckDB, Snowflake).
|
||||
QuoteAndBackslash,
|
||||
/// `\` → `\\`, `'` → `''`, `"` → `""`: MySQL also accepts `"..."` literals, and only
|
||||
/// doubling stays confined under `NO_BACKSLASH_ESCAPES`.
|
||||
#[cfg_attr(not(feature = "mysql"), allow(dead_code))]
|
||||
DoubledQuotes,
|
||||
MySql,
|
||||
/// `\` → `\\`, `'` → `\'`, `"` → `\"`: backslash escapes are always on, both quote
|
||||
/// styles delimit strings, and `''` is not accepted.
|
||||
#[cfg_attr(not(feature = "bigquery"), allow(dead_code))]
|
||||
BigQuery,
|
||||
}
|
||||
|
||||
impl SqlStringEscaping {
|
||||
fn escape(self, value: &str) -> String {
|
||||
match self {
|
||||
SqlStringEscaping::Standard => value.replace('\'', "''"),
|
||||
SqlStringEscaping::Backslash => value
|
||||
SqlStringEscaping::Quote => value.replace('\'', "''"),
|
||||
SqlStringEscaping::QuoteAndBackslash => value.replace('\\', "\\\\").replace('\'', "''"),
|
||||
SqlStringEscaping::MySql => value
|
||||
.replace('\\', "\\\\")
|
||||
.replace('\'', "''")
|
||||
.replace('"', "\"\""),
|
||||
SqlStringEscaping::BigQuery => value
|
||||
.replace('\\', "\\\\")
|
||||
.replace('\'', "\\'")
|
||||
.replace('"', "\\\""),
|
||||
SqlStringEscaping::DoubledQuotes => value.replace('\'', "''").replace('"', "\"\""),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(feature = "mysql")]
|
||||
pub fn has_contextual_variables(code: &str) -> bool {
|
||||
RE_SQL_CONTEXTUAL_VAR.is_match(code)
|
||||
}
|
||||
|
||||
fn replace_contextual_variables(
|
||||
code: &mut String,
|
||||
contextual_variables: &HashMap<String, String>,
|
||||
@@ -190,21 +196,27 @@ mod tests {
|
||||
let backslash = r#""x\'/**/OR/**/1=1#"@e.com"#;
|
||||
|
||||
assert_eq!(
|
||||
interpolate(code, quote, SqlStringEscaping::Standard),
|
||||
interpolate(code, quote, SqlStringEscaping::Quote),
|
||||
r"SELECT 1 WHERE email = 'x''/**/OR/**/''1''=''1''--@e.com'"
|
||||
);
|
||||
// `\` doubled too, so `\'` cannot become `\''` in an `E'...'` literal
|
||||
assert_eq!(
|
||||
interpolate(code, backslash, SqlStringEscaping::Backslash),
|
||||
interpolate(code, backslash, SqlStringEscaping::QuoteAndBackslash),
|
||||
r#"SELECT 1 WHERE email = '"x\\''/**/OR/**/1=1#"@e.com'"#
|
||||
);
|
||||
assert_eq!(
|
||||
interpolate(code, backslash, SqlStringEscaping::MySql),
|
||||
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'"#
|
||||
);
|
||||
assert_eq!(
|
||||
interpolate(code, backslash, SqlStringEscaping::DoubledQuotes),
|
||||
r#"SELECT 1 WHERE email = '""x\''/**/OR/**/1=1#""@e.com'"#
|
||||
);
|
||||
for escaping in [
|
||||
SqlStringEscaping::Standard,
|
||||
SqlStringEscaping::Backslash,
|
||||
SqlStringEscaping::DoubledQuotes,
|
||||
SqlStringEscaping::Quote,
|
||||
SqlStringEscaping::QuoteAndBackslash,
|
||||
SqlStringEscaping::MySql,
|
||||
SqlStringEscaping::BigQuery,
|
||||
] {
|
||||
assert_eq!(
|
||||
interpolate(code, "a.b+c@e.com", escaping),
|
||||
|
||||
@@ -208,7 +208,7 @@ fn do_snowflake_inner<'a>(
|
||||
&sig,
|
||||
&job_args,
|
||||
reserved_variables,
|
||||
SqlStringEscaping::Backslash,
|
||||
SqlStringEscaping::QuoteAndBackslash,
|
||||
)?;
|
||||
|
||||
body.insert("statement".to_string(), json!(query));
|
||||
|
||||
Reference in New Issue
Block a user