diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 95c7157d61..e00b728218 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -33,7 +33,7 @@ use windmill_common::db::UserDB; use windmill_common::global_settings::HTTP_ROUTE_WORKSPACED_ROUTE; use windmill_common::users::username_to_permissioned_as; use windmill_common::variables::{ - build_crypt, decrypt, encrypt, SECRET_SALT, WORKSPACE_CRYPT_CACHE, + build_crypt, decrypt, encrypt, is_valid_workspace_env_name, SECRET_SALT, WORKSPACE_CRYPT_CACHE, }; use windmill_common::worker::{to_raw_value, CLOUD_HOSTED}; use windmill_common::workspaces::GitRepositorySettings; @@ -3298,6 +3298,15 @@ async fn set_environment_variable( match value { Some(value) => { + // The name is interpolated verbatim into generated worker code + // (e.g. the NativeTS/bunnative `const = ...;` prologue), so + // it must be a strict identifier to prevent code injection. + if !is_valid_workspace_env_name(&name) { + return Err(Error::BadRequest(format!( + "Invalid environment variable name {name:?}: must match ^[A-Za-z_][A-Za-z0-9_]*$ \ + (letters, digits and underscores, not starting with a digit)" + ))); + } sqlx::query!( "INSERT INTO workspace_env (workspace_id, name, value) VALUES ($1, $2, $3) ON CONFLICT (workspace_id, name) DO UPDATE SET value = EXCLUDED.value", &w_id, diff --git a/backend/windmill-common/src/variables.rs b/backend/windmill-common/src/variables.rs index 57e41bb487..13ac22a64e 100644 --- a/backend/windmill-common/src/variables.rs +++ b/backend/windmill-common/src/variables.rs @@ -229,6 +229,18 @@ lazy_static::lazy_static! { pub static ref CUSTOM_ENVS_CACHE: Cache)> = Cache::new(100); pub static ref WORKSPACE_CRYPT_CACHE: Cache = Cache::new(1000); + static ref VALID_WORKSPACE_ENV_NAME: regex::Regex = + regex::Regex::new(r"^[A-Za-z_][A-Za-z0-9_]*$").unwrap(); +} + +/// Workspace custom environment variable names are interpolated verbatim into +/// generated worker code (e.g. the NativeTS/bunnative `const = ...;` +/// prologue). A name MUST therefore be restricted to a conventional identifier +/// so an attacker-controlled name cannot break out of the declaration into +/// arbitrary worker-process JavaScript. This subset is also a valid POSIX +/// environment variable name. +pub fn is_valid_workspace_env_name(name: &str) -> bool { + name.len() <= 255 && VALID_WORKSPACE_ENV_NAME.is_match(name) } pub async fn get_reserved_variables( @@ -465,6 +477,23 @@ async fn get_cached_workspace_envs(conn: &Connection, w_id: &str) -> Vec<(String .await .unwrap_or_default(), }; + // Defense-in-depth: never expose a custom env whose name is not a + // valid identifier. Names are interpolated verbatim into generated + // worker code; this neutralizes any malicious row that may already be + // persisted from before name validation was enforced at write time. + let custom_envs = custom_envs + .into_iter() + .filter(|(name, _)| { + let valid = is_valid_workspace_env_name(name); + if !valid { + tracing::warn!( + workspace_id = %w_id, + "Ignoring workspace env variable with invalid name: {name:?}" + ); + } + valid + }) + .collect::>(); CUSTOM_ENVS_CACHE.insert( w_id.to_string(), (chrono::Utc::now().timestamp(), custom_envs.clone()), @@ -556,3 +585,42 @@ pub async fn get_variable_or_self_as( ))) } } + +#[cfg(test)] +mod tests { + use super::is_valid_workspace_env_name; + + #[test] + fn valid_env_names_are_accepted() { + for name in ["FOO", "foo_bar", "_private", "API_KEY_2", "a", "X1"] { + assert!(is_valid_workspace_env_name(name), "expected {name:?} valid"); + } + } + + #[test] + fn malicious_env_names_are_rejected() { + // Regression for GHSA-5f5q-2vg2-r2x4: a workspace env variable name is + // interpolated verbatim into the NativeTS/bunnative `const = ...;` + // worker prologue. Any name that is not a strict identifier could break + // out of the declaration into attacker-controlled worker JavaScript. + for name in [ + "INJ = (globalThis.x = 1); let _y", // breaks out of `const ='...'` + "x'];globalThis.y=1;//", // breaks out of `process.env['']` + "1FOO", // starts with a digit + "FOO-BAR", // hyphen + "FOO BAR", // space + "FOO;BAR", + "FOO.BAR", + "$FOO", + "FOO\n BAR", + "", + ] { + assert!( + !is_valid_workspace_env_name(name), + "expected {name:?} to be rejected" + ); + } + // Over-long names are rejected (DB column is varchar(255)). + assert!(!is_valid_workspace_env_name(&"A".repeat(256))); + } +}