From 19ec5d3cd8a36600c8896a895d0e566735f729a9 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sun, 18 Oct 2026 12:27:06 +0200 Subject: [PATCH] fix: reject out-of-range integer args to narrow SQL types instead of wrapping (#11864) Co-authored-by: Claude Opus 5.5 --- backend/windmill-worker/src/common.rs | 10 ++ backend/windmill-worker/src/mssql_executor.rs | 8 +- backend/windmill-worker/src/pg_executor.rs | 132 ++++++++++++------ 3 files changed, 105 insertions(+), 45 deletions(-) diff --git a/backend/windmill-worker/src/common.rs b/backend/windmill-worker/src/common.rs index 32bd6dc7e9..462fb2a686 100644 --- a/backend/windmill-worker/src/common.rs +++ b/backend/windmill-worker/src/common.rs @@ -92,6 +92,16 @@ pub(crate) fn raw_to_string(x: &str) -> String { } } +/// Narrow a JSON integer to the Rust type bound for a SQL argument, erroring the way the +/// database would rather than letting an `as` cast wrap it into a different value. +pub(crate) fn narrow_sql_int(n: N, sql_type: &str) -> anyhow::Result +where + T: TryFrom, + N: std::fmt::Display + Copy, +{ + T::try_from(n).map_err(|_| anyhow!("value \"{n}\" is out of range for type {sql_type}")) +} + pub async fn build_args_map<'a>( job: &'a MiniPulledJob, client: &AuthedClient, diff --git a/backend/windmill-worker/src/mssql_executor.rs b/backend/windmill-worker/src/mssql_executor.rs index 02d0da1164..1d3a323e34 100644 --- a/backend/windmill-worker/src/mssql_executor.rs +++ b/backend/windmill-worker/src/mssql_executor.rs @@ -26,7 +26,7 @@ use windmill_queue::MiniPulledJob; use windmill_queue::{append_logs, CanceledBy}; use crate::common::{ - build_args_values, get_reserved_variables, s3_mode_args_to_worker_data, + build_args_values, get_reserved_variables, narrow_sql_int, s3_mode_args_to_worker_data, s3_stream_and_upload_with_logs, OccupancyMetrics, }; use crate::handle_child::run_future_with_polling_update_job_poller; @@ -398,13 +398,13 @@ fn json_value_to_sql<'a>( query.bind(b.to_owned()); } Value::Number(n) if n.is_u64() && arg_t == "tinyint" => { - query.bind(n.as_u64().unwrap().to_owned() as u8) + query.bind(narrow_sql_int::(n.as_u64().unwrap(), "tinyint")?) } Value::Number(n) if n.is_i64() && arg_t == "smallint" => { - query.bind(n.as_i64().unwrap().to_owned() as i16) + query.bind(narrow_sql_int::(n.as_i64().unwrap(), "smallint")?) } Value::Number(n) if n.is_i64() && arg_t == "int" => { - query.bind(n.as_i64().unwrap().to_owned() as i32) + query.bind(narrow_sql_int::(n.as_i64().unwrap(), "int")?) } Value::Number(n) if n.is_i64() && arg_t == "bigint" => { query.bind(n.as_i64().unwrap().to_owned() as i64) diff --git a/backend/windmill-worker/src/pg_executor.rs b/backend/windmill-worker/src/pg_executor.rs index 12a3665c27..f70e3dd0ca 100644 --- a/backend/windmill-worker/src/pg_executor.rs +++ b/backend/windmill-worker/src/pg_executor.rs @@ -38,7 +38,7 @@ use windmill_queue::{CanceledBy, MiniPulledJob}; use crate::agent_workers::get_datatable_resource_from_agent_http; use crate::common::{ - build_args_values, get_reserved_variables, s3_mode_args_to_worker_data, + build_args_values, get_reserved_variables, narrow_sql_int, s3_mode_args_to_worker_data, s3_stream_and_upload_with_logs, sizeof_val, OccupancyMetrics, S3ModeWorkerData, }; use crate::handle_child::run_future_with_polling_update_job_poller; @@ -1768,23 +1768,30 @@ fn parse_datetime_utc(s: &str) -> Result, chrono::ParseErr fn map_as_single_type( vec: Option<&Vec>, f: impl Fn(&Value) -> Option, +) -> anyhow::Result>>> { + try_map_as_single_type(vec, |v| Ok(f(v))) +} + +/// Like `map_as_single_type`, but `f` can reject an element of the right type +/// (e.g. an out-of-range integer) with its own error. +fn try_map_as_single_type( + vec: Option<&Vec>, + f: impl Fn(&Value) -> anyhow::Result>, ) -> anyhow::Result>>> { if let Some(vec) = vec { - Ok(Some( - vec.into_iter() - .map(|v| { - // first option is if the value is of the right type (if none, will stop the collection and throw error) - // second option is if the value is null - // allow nulls in arrays - if matches!(v, Value::Null) { - Some(None) - } else { - f(v).map(Some) - } - }) - .collect::>>>() - .ok_or_else(|| anyhow::anyhow!("Mixed types in array"))?, - )) + let mut out = Vec::with_capacity(vec.len()); + for v in vec { + // allow nulls in arrays + if matches!(v, Value::Null) { + out.push(None); + } else { + match f(v)? { + Some(x) => out.push(Some(x)), + None => return Err(anyhow::anyhow!("Mixed types in array")), + } + } + } + Ok(Some(out)) } else { Ok(None) } @@ -1819,26 +1826,23 @@ fn convert_vec_val( Type::BOOL_ARRAY, )), "char" | "character" => Ok(( - Box::new(map_as_single_type(vec, |v| { - v.as_i64() - .map(|x| x as i8) - .or_else(|| v.as_str().and_then(|s| s.parse::().ok())) + Box::new(try_map_as_single_type(vec, |v| match v.as_i64() { + Some(x) => narrow_sql_int::(x, "\"char\"").map(Some), + None => Ok(v.as_str().and_then(|s| s.parse::().ok())), })?), Type::CHAR_ARRAY, )), "smallint" | "smallserial" | "int2" | "serial2" => Ok(( - Box::new(map_as_single_type(vec, |v| { - v.as_i64() - .map(|x| x as i16) - .or_else(|| v.as_str().and_then(|s| s.parse::().ok())) + Box::new(try_map_as_single_type(vec, |v| match v.as_i64() { + Some(x) => narrow_sql_int::(x, "smallint").map(Some), + None => Ok(v.as_str().and_then(|s| s.parse::().ok())), })?), Type::INT2_ARRAY, )), "int" | "integer" | "int4" | "serial" => Ok(( - Box::new(map_as_single_type(vec, |v| { - v.as_i64() - .map(|x| x as i32) - .or_else(|| v.as_str().and_then(|s| s.parse::().ok())) + Box::new(try_map_as_single_type(vec, |v| match v.as_i64() { + Some(x) => narrow_sql_int::(x, "integer").map(Some), + None => Ok(v.as_str().and_then(|s| s.parse::().ok())), })?), Type::INT4_ARRAY, )), @@ -1859,18 +1863,18 @@ fn convert_vec_val( Type::NUMERIC_ARRAY, )), "oid" => Ok(( - Box::new(map_as_single_type(vec, |v| { - v.as_u64() - .map(|x| x as u32) - .or_else(|| v.as_str().and_then(|s| s.parse::().ok())) + Box::new(try_map_as_single_type(vec, |v| match v.as_u64() { + Some(x) => narrow_sql_int::(x, "oid").map(Some), + None => Ok(v.as_str().and_then(|s| s.parse::().ok())), })?), Type::OID_ARRAY, )), "bigint" | "bigserial" | "int8" | "serial8" => Ok(( - Box::new(map_as_single_type(vec, |v| { - v.as_i64() - .or_else(|| v.as_u64().map(|x| x as i64)) - .or_else(|| v.as_str().and_then(|s| s.parse::().ok())) + Box::new(try_map_as_single_type(vec, |v| match v.as_u64() { + Some(x) => narrow_sql_int::(x, "bigint").map(Some), + None => Ok(v + .as_i64() + .or_else(|| v.as_str().and_then(|s| s.parse::().ok()))), })?), Type::INT8_ARRAY, )), @@ -2074,7 +2078,8 @@ fn convert_val( Ok((Box::new(n.to_string()), t)) } Value::Number(n) if arg_t == "char" && n.is_i64() => { - Ok((Box::new(n.as_i64().unwrap() as i8), Type::CHAR)) + let n = narrow_sql_int::(n.as_i64().unwrap(), "\"char\"")?; + Ok((Box::new(n), Type::CHAR)) } Value::Number(n) if (arg_t == "smallint" @@ -2083,13 +2088,15 @@ fn convert_val( || arg_t == "serial2") && n.is_i64() => { - Ok((Box::new(n.as_i64().unwrap() as i16), Type::INT2)) + let n = narrow_sql_int::(n.as_i64().unwrap(), "smallint")?; + Ok((Box::new(n), Type::INT2)) } Value::Number(n) if (arg_t == "int" || arg_t == "integer" || arg_t == "int4" || arg_t == "serial") && n.is_i64() => { - Ok((Box::new(n.as_i64().unwrap() as i32), Type::INT4)) + let n = narrow_sql_int::(n.as_i64().unwrap(), "integer")?; + Ok((Box::new(n), Type::INT4)) } Value::Number(n) if (arg_t == "real" || arg_t == "float4") && n.as_f64().is_some() => { Ok((Box::new(n.as_f64().unwrap() as f32), Type::FLOAT4)) @@ -2109,7 +2116,8 @@ fn convert_val( Type::NUMERIC, )), Value::Number(n) if arg_t == "oid" && n.is_u64() => { - Ok((Box::new(n.as_u64().unwrap() as u32), Type::OID)) + let n = narrow_sql_int::(n.as_u64().unwrap(), "oid")?; + Ok((Box::new(n), Type::OID)) } Value::Number(n) if (arg_t == "bigint" @@ -2118,7 +2126,8 @@ fn convert_val( || arg_t == "serial8") && n.is_u64() => { - Ok((Box::new(n.as_u64().unwrap() as i64), Type::INT8)) + let n = narrow_sql_int::(n.as_u64().unwrap(), "bigint")?; + Ok((Box::new(n), Type::INT8)) } Value::Number(n) if n.is_i64() => Ok((Box::new(n.as_i64().unwrap()), Type::INT8)), Value::Number(n) => Ok((Box::new(n.as_f64().unwrap()), Type::FLOAT8)), @@ -3313,6 +3322,47 @@ mod tests { assert_convert_val_consistent("Number(0)/oid", json!(0), "oid", typ_for("oid"), Type::OID); } + #[test] + fn convert_val_rejects_out_of_range_integers() { + let convert = |value: Value, arg_t: &str| { + convert_val(&value, &arg_t.to_string(), &typ_for(arg_t), false) + .map(|_| ()) + .map_err(|e| e.to_string()) + }; + for (value, arg_t, sql_type) in [ + (json!(200), "char", "\"char\""), + (json!(70000), "smallint", "smallint"), + (json!(-40000), "int2", "smallint"), + (json!(4294967297u64), "int4", "integer"), + (json!(4294967296u64), "oid", "oid"), + (json!(u64::MAX), "bigint", "bigint"), + (json!([1, 70000]), "smallint[]", "smallint"), + (json!([u64::MAX]), "int8[]", "bigint"), + ] { + let err = convert(value.clone(), arg_t).expect_err(&format!("{value}/{arg_t}")); + let n = value.as_array().map_or(&value, |a| a.last().unwrap()); + assert!( + err.contains(&format!( + "value \"{n}\" is out of range for type {sql_type}" + )), + "{value}/{arg_t}: {err}" + ); + } + for (value, arg_t) in [ + (json!(127), "char"), + (json!(-128), "char"), + (json!(32767), "smallint"), + (json!(-32768), "smallint"), + (json!(2147483647), "int"), + (json!(-2147483648i64), "int"), + (json!(4294967295u64), "oid"), + (json!(i64::MAX), "bigint"), + (json!([null, -32768, 32767]), "smallint[]"), + ] { + convert(value.clone(), arg_t).unwrap_or_else(|e| panic!("{value}/{arg_t}: {e}")); + } + } + #[test] fn convert_val_float_number_against_every_arg_t() { let cases: &[(&str, Type)] = &[