mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-13 00:03:00 +00:00
fix(worker): escape contextual variables substituted into SQL string literals
%%WM_*%% contextual variables were substituted into SQL as raw text, so a value carrying a quote (an end-user email such as x'/**/OR/**/'1'='1'--@e.com in WM_END_USER_EMAIL) could break out of the surrounding string literal and run with the app author's database credentials. Escape each value for the dialect's single-quoted literal: '' for PostgreSQL, MSSQL, Oracle and DuckDB; \\ plus '' for MySQL and Snowflake (a quoted email local part can carry a backslash, which those dialects treat as an escape); \\ plus \' for BigQuery, which does not accept ''. 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
318f89960e
commit
3f46ff8118
@@ -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);
|
||||
|
||||
@@ -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?)
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
@@ -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<String, String>,
|
||||
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<Arg>,
|
||||
args_map: &HashMap<String, Value>,
|
||||
contextual_variables: &HashMap<String, String>,
|
||||
escaping: SqlStringEscaping,
|
||||
) -> Result<(String, Vec<String>), 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'"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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));
|
||||
|
||||
|
||||
Reference in New Issue
Block a user