diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 2fabf92530..5e9eb24ee5 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -c3b6f62ea579a3583d4b474e9885c77104cfc87e +2d6c66b32f20d9605c6a677727473ab66fcc8a87 diff --git a/backend/src/main.rs b/backend/src/main.rs index f8b150383a..69f28e11ee 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -380,6 +380,7 @@ async fn cache_hub_scripts(file_path: Option) -> anyhow::Result<()> { "", &mut None, &None, + None, ) .await { diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index 1fb566e57f..77221edf22 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -1256,6 +1256,7 @@ pub async fn prebundle_bun_script( token: &str, occupancy_metrics: &mut Option<&mut OccupancyMetrics>, temp_script_refs: &Option>, + modules: Option<&HashMap>, ) -> Result<()> { let (local_path, remote_path) = compute_bundle_local_and_remote_path( inner_content, @@ -1264,6 +1265,7 @@ pub async fn prebundle_bun_script( db, w_id, temp_script_refs, + modules, ) .await; if exists_in_cache(&local_path, &remote_path).await { @@ -1442,6 +1444,7 @@ pub async fn compute_bundle_local_and_remote_path( db: Option<&DB>, w_id: &str, temp_script_refs: &Option>, + modules: Option<&HashMap>, ) -> (String, String) { let mut input_src = format!("{inner_content}{lock}",); @@ -1470,7 +1473,10 @@ pub async fn compute_bundle_local_and_remote_path( let ws_suffix = crate::workspace_registry_cache_suffix(w_id).await; input_src.push_str(&ws_suffix); - let hash = windmill_common::utils::calculate_hash(&input_src); + + // The loader resolves relative imports against the module files in the job dir, so + // their content is inlined into the bundle this name covers. + let hash = crate::worker::artifact_cache_name(input_src, modules); let local_path = format!("{}/{hash}", *BUN_BUNDLE_CACHE_DIR); #[cfg(windows)] @@ -1569,6 +1575,7 @@ pub async fn handle_bun_job( Some(db), &job.workspace_id, &temp_script_refs, + modules.as_ref(), ) .await } @@ -2962,9 +2969,8 @@ pub async fn handle_wac_v2_output( version: flow_info.version, labels: flow_info.labels.clone(), }; - let on_behalf_of = flow_info - .on_behalf_of(&job.workspace_id, db) - .await?; + let on_behalf_of = + flow_info.on_behalf_of(&job.workspace_id, db).await?; let step_args: HashMap> = step .args .iter() @@ -4382,4 +4388,49 @@ export function main(x: number) { return x; }"#; assert!(wrapper.contains(r#"line.startsWith("exec_preprocess:")"#)); assert!(wrapper.contains(r#"line.startsWith("exec:")"#)); } + + /// The bundle cache is global and content-keyed, so a key that ignores the inline + /// modules hands one workspace's bundle — attacker helper code and all — to the next + /// job whose main content and lockfile happen to match. + #[tokio::test] + async fn bundle_cache_key_separates_inline_module_content() { + use windmill_common::scripts::ScriptModule; + + async fn key_for(modules: Option<&HashMap>) -> String { + compute_bundle_local_and_remote_path( + "import { h } from './helper.ts';\nexport async function main() { return h(); }", + "{}\n//bun.lock\n", + "u/alice/script", + None, + "w1", + &None, + modules, + ) + .await + .1 + } + fn modules(content: &str) -> HashMap { + HashMap::from([( + "helper.ts".to_string(), + ScriptModule { + content: content.to_string(), + language: ScriptLang::Bun, + lock: None, + }, + )]) + } + + let attacker = key_for(Some(&modules("export const h = () => 'attacker'"))).await; + let victim = key_for(Some(&modules("export const h = () => 'victim'"))).await; + assert_ne!(attacker, victim); + assert_eq!( + attacker, + key_for(Some(&modules("export const h = () => 'attacker'"))).await, + "same modules must still share a cache slot" + ); + + // An absent map and an empty one are the same script, so they share a slot. + assert_eq!(key_for(None).await, key_for(Some(&HashMap::new())).await); + assert_ne!(key_for(None).await, attacker); + } } diff --git a/backend/windmill-worker/src/csharp_executor.rs b/backend/windmill-worker/src/csharp_executor.rs index c95107d504..d393171f7f 100644 --- a/backend/windmill-worker/src/csharp_executor.rs +++ b/backend/windmill-worker/src/csharp_executor.rs @@ -13,7 +13,7 @@ use itertools::Itertools; #[cfg(feature = "csharp")] use tokio::{fs::File, io::AsyncReadExt, process::Command}; #[cfg(feature = "csharp")] -use windmill_common::{utils::calculate_hash, worker::write_file}; +use windmill_common::worker::write_file; #[cfg(feature = "csharp")] use crate::global_cache::save_cache; @@ -72,13 +72,21 @@ const CSHARP_OBJECT_STORE_PREFIX: &str = /// Cache key of a C# build. The run path and the deploy-time prebuild must derive it the /// same way or the prebuilt binary is never found and gets rebuilt on first run. #[cfg(feature = "csharp")] -async fn csharp_cache_key(code: &str, requirements_o: Option<&str>, w_id: &str) -> String { - let mut hash = calculate_hash(&format!( +async fn csharp_cache_key( + code: &str, + requirements_o: Option<&str>, + w_id: &str, + modules: Option<&HashMap>, +) -> String { + // The SDK project globs every `.cs` under the job dir, so companion modules are + // compiled into the binary this key names and have to be part of it. + let base = format!( "{}{}{}", code, requirements_o.unwrap_or(""), DOTNET_TARGET_FRAMEWORK.as_str() - )); + ); + let mut hash = crate::worker::artifact_cache_name(base, modules); hash.push_str(&crate::workspace_registry_cache_suffix(w_id).await); hash } @@ -487,10 +495,11 @@ pub async fn prebuild_csharp_binary( worker_name: &str, base_internal_url: &str, occupancy_metrics: &mut OccupancyMetrics, + modules: Option<&HashMap>, ) -> error::Result> { check_executor_binary_exists("dotnet", DOTNET_PATH.as_str(), "C#")?; - let hash = csharp_cache_key(code, Some(lock), &job.workspace_id).await; + let hash = csharp_cache_key(code, Some(lock), &job.workspace_id, modules).await; let remote_path = format!("{CSHARP_OBJECT_STORE_PREFIX}{hash}"); if crate::global_cache::exists_in_object_store(&remote_path).await { return Ok(None); @@ -549,6 +558,7 @@ pub async fn handle_csharp_job( _worker_name: &str, _envs: HashMap, _occupancy_metrics: &mut OccupancyMetrics, + _modules: Option<&HashMap>, ) -> Result, Error> { Err(anyhow!("C# is not available because the feature is not enabled").into()) } @@ -568,6 +578,7 @@ pub async fn handle_csharp_job( worker_name: &str, envs: HashMap, occupancy_metrics: &mut OccupancyMetrics, + modules: Option<&HashMap>, ) -> Result, Error> { check_executor_binary_exists("dotnet", DOTNET_PATH.as_str(), "C#")?; @@ -575,6 +586,7 @@ pub async fn handle_csharp_job( inner_content, requirements_o.map(|x| x.as_str()), &job.workspace_id, + modules, ) .await; let bin_path = format!("{}/{hash}", *CSHARP_CACHE_DIR); diff --git a/backend/windmill-worker/src/go_executor.rs b/backend/windmill-worker/src/go_executor.rs index 1d2009c5c0..9fe6d4a616 100644 --- a/backend/windmill-worker/src/go_executor.rs +++ b/backend/windmill-worker/src/go_executor.rs @@ -230,8 +230,15 @@ fn go_runtime_int32(v: &str) -> Option { /// Cache key of a Go build. The run path and the deploy-time prebuild must derive it the /// same way or the prebuilt binary is never found and gets rebuilt on first run. -fn go_cache_key(code: &str, maybe_lock: &MaybeLock) -> String { - calculate_hash(&format!("{}{:?}v2", code, maybe_lock)) +fn go_cache_key( + code: &str, + maybe_lock: &MaybeLock, + modules: Option<&HashMap>, +) -> String { + // A module whose path starts with `go/` lands inside the module dir this builds, so + // its content ends up in the binary the key names. + let base = format!("{}{:?}v2", code, maybe_lock); + crate::worker::artifact_cache_name(base, modules) } /// Install the deps, generate the entrypoint wrapper, `go build`, and push the binary to @@ -459,9 +466,10 @@ pub async fn prebuild_go_binary( worker_name: &str, base_internal_url: &str, occupancy_metrics: &mut OccupancyMetrics, + modules: Option<&HashMap>, ) -> Result, Error> { let maybe_lock = MaybeLock::Resolved { lock: lock.to_string() }; - let hash = go_cache_key(code, &maybe_lock); + let hash = go_cache_key(code, &maybe_lock, modules); let remote_path = format!("{GO_OBJECT_STORE_PREFIX}{hash}"); if crate::global_cache::exists_in_object_store(&remote_path).await { return Ok(None); @@ -512,6 +520,7 @@ pub async fn handle_go_job( envs: HashMap, occupation_metrics: &mut OccupancyMetrics, maybe_lock: MaybeLock, + modules: Option<&HashMap>, ) -> Result, Error> { //go does not like executing modules at temp root let job_dir = &format!("{job_dir}/go"); @@ -520,7 +529,7 @@ pub async fn handle_go_job( .create(&job_dir) .expect("could not create go job dir"); - let hash = go_cache_key(inner_content, &maybe_lock); + let hash = go_cache_key(inner_content, &maybe_lock, modules); let bin_path = format!("{}/{hash}", *GO_BIN_CACHE_DIR); let remote_path = format!("{GO_OBJECT_STORE_PREFIX}{hash}"); let (cache, cache_logs) = crate::global_cache::load_cache(&bin_path, &remote_path, false).await; diff --git a/backend/windmill-worker/src/java_executor.rs b/backend/windmill-worker/src/java_executor.rs index bb0518f22d..2e47fced18 100644 --- a/backend/windmill-worker/src/java_executor.rs +++ b/backend/windmill-worker/src/java_executor.rs @@ -66,6 +66,7 @@ pub(crate) struct JobHandlerInput<'a> { pub requirements_o: Option<&'a String>, pub shared_mount: &'a str, pub worker_name: &'a str, + pub modules: Option<&'a HashMap>, } pub async fn handle_java_job<'a>(mut args: JobHandlerInput<'a>) -> Result, Error> { @@ -612,25 +613,33 @@ async fn compile<'a>( inner_content, requirements_o, parent_runnable_path, + modules, .. }: &mut JobHandlerInput<'a>, classpath: &'a str, // plugins: Vec<&'a str>, ) -> Result<(), Error> { - fn compute_hash(code: &str, requirements_o: Option<&String>) -> String { - calculate_hash(&format!( + // The cached artifact is the whole `target/` dir, and companion modules are written + // into the job dir before this runs, so their content can land in it. + fn compute_hash( + code: &str, + requirements_o: Option<&String>, + modules: Option<&HashMap>, + ) -> String { + let base = format!( "{}{}", code, requirements_o .as_ref() .map(|x| x.to_string()) .unwrap_or_default() - )) + ); + crate::worker::artifact_cache_name(base, modules) } let reserved_variables = get_reserved_variables(job, &client.token, conn, parent_runnable_path.clone()).await?; let ws_suffix = crate::workspace_registry_cache_suffix(&job.workspace_id).await; - let mut hash = compute_hash(inner_content, *requirements_o); + let mut hash = compute_hash(inner_content, *requirements_o, *modules); hash.push_str(&ws_suffix); let bin_path = format!("{}/{hash}", *JAVA_CACHE_DIR); let remote_path = format!("java_jar/{hash}"); diff --git a/backend/windmill-worker/src/rust_executor.rs b/backend/windmill-worker/src/rust_executor.rs index b9d53a801b..dff7d58c2d 100644 --- a/backend/windmill-worker/src/rust_executor.rs +++ b/backend/windmill-worker/src/rust_executor.rs @@ -14,7 +14,6 @@ use tokio::{ }; use windmill_common::{ error::{self, Error}, - utils::calculate_hash, worker::{write_file, Connection}, }; use windmill_queue::MiniPulledJob; @@ -602,8 +601,13 @@ pub async fn build_rust_crate( /// Cache key of a Rust build. The run path and the deploy-time prebuild must derive it /// the same way or the prebuilt binary is never found and gets rebuilt on first run. -async fn rust_cache_key(code: &str, requirements_o: Option<&String>, w_id: &str) -> String { - let mut hash = compute_rust_hash(code, requirements_o); +async fn rust_cache_key( + code: &str, + requirements_o: Option<&String>, + w_id: &str, + modules: Option<&HashMap>, +) -> String { + let mut hash = compute_rust_hash(code, requirements_o, modules); hash.push_str(&crate::workspace_registry_cache_suffix(w_id).await); hash } @@ -621,11 +625,12 @@ pub async fn prebuild_rust_binary( worker_name: &str, base_internal_url: &str, occupancy_metrics: &mut OccupancyMetrics, + modules: Option<&HashMap>, ) -> error::Result> { ensure_rust_runtime_dirs(); check_executor_binary_exists("cargo", CARGO_PATH.as_str(), "rust")?; - let hash = rust_cache_key(code, Some(&lock.to_string()), &job.workspace_id).await; + let hash = rust_cache_key(code, Some(&lock.to_string()), &job.workspace_id, modules).await; let remote_path = format!("{RUST_OBJECT_STORE_PREFIX}{hash}"); if crate::global_cache::exists_in_object_store(&remote_path).await { return Ok(None); @@ -652,15 +657,22 @@ pub async fn prebuild_rust_binary( Ok(Some(logs)) } -pub fn compute_rust_hash(code: &str, requirements_o: Option<&String>) -> String { - calculate_hash(&format!( +pub fn compute_rust_hash( + code: &str, + requirements_o: Option<&String>, + // Companion modules are written into the crate dir and compiled into the binary this + // key names, so leaving them out shares one script's binary with another. + modules: Option<&HashMap>, +) -> String { + let base = format!( "{}{}", code, requirements_o .as_ref() .map(|x| x.to_string()) .unwrap_or_default() - )) + ); + crate::worker::artifact_cache_name(base, modules) } #[tracing::instrument(level = "trace", skip_all)] @@ -679,11 +691,12 @@ pub async fn handle_rust_job( worker_name: &str, envs: HashMap, occupancy_metrics: &mut OccupancyMetrics, + modules: Option<&HashMap>, ) -> Result, Error> { ensure_rust_runtime_dirs(); check_executor_binary_exists("cargo", CARGO_PATH.as_str(), "rust")?; - let hash = rust_cache_key(inner_content, requirements_o, &job.workspace_id).await; + let hash = rust_cache_key(inner_content, requirements_o, &job.workspace_id, modules).await; let bin_path = format!("{}/{hash}", *RUST_CACHE_DIR); let remote_path = format!("{RUST_OBJECT_STORE_PREFIX}{hash}"); diff --git a/backend/windmill-worker/src/worker.rs b/backend/windmill-worker/src/worker.rs index bc9cc42771..7e620439b0 100644 --- a/backend/windmill-worker/src/worker.rs +++ b/backend/windmill-worker/src/worker.rs @@ -950,6 +950,51 @@ pub async fn workspace_registry_cache_suffix(w_id: &str) -> String { } } +/// The name a build artifact is cached under, derived from `base` — the runnable's own +/// cache-key input — and its inline modules. +/// +/// `write_module_files` puts module content in the job dir where the build inlines it into +/// the artifact, so a name without it serves one runnable's modules to another whose main +/// content and lockfile match — across workspaces, the cache being global. +/// +/// Only the path and content may name the artifact, because they are all the build reads. +/// `ScriptModule::lock` especially must stay out: deploy regenerates it *after* the parent +/// has prebuilt, so naming it would strand every prebuilt artifact. +pub(crate) fn artifact_cache_name( + base: String, + modules: Option<&std::collections::HashMap>, +) -> String { + let Some(modules) = modules.filter(|m| !m.is_empty()) else { + // Byte-identical to the name a module-free runnable had before modules entered this, + // so its cached artifacts stay reachable. A pre-fix multi-file runnable also stored + // here, so one of those stays reachable too — accepted over invalidating every cache, + // and once this ships nothing can be stored here with module content in it again. + return windmill_common::utils::calculate_hash(&base); + }; + let mut entries: Vec<(&String, &ScriptModule)> = modules.iter().collect(); + entries.sort_by(|a, b| a.0.cmp(b.0)); + // `base` ends in caller-supplied bytes (a preview brings its own lockfile), so it is + // sealed to a fixed width before the module block is appended — raw, a crafted lockfile + // could spell out another runnable's block and reach its slot. + let mut keyed = format!( + "{}:modules:{}", + windmill_common::utils::calculate_hash(&base), + entries.len() + ); + for (path, module) in entries { + // Both length-prefixed, else `{"a": "bc"}` and `{"ab": "c"}` encode alike. + keyed.push_str(&format!( + ":{}:{path}:{}:{}", + path.len(), + module.content.len(), + module.content, + )); + } + // Its own namespace: `calculate_hash` emits hex, so however a module-free runnable + // crafts its content and lockfile it can never land on a module-bearing name. + format!("mod-{}", windmill_common::utils::calculate_hash(&keyed)) +} + pub fn is_sandboxing_enabled() -> bool { if !*DISABLE_NSJAIL { return true; @@ -5459,7 +5504,9 @@ async fn handle_code_execution_job( None => job, }; - // For preview jobs, extract modules from args._MODULES if not already set + // Any job kind, not just previews: whatever is here is what gets written to the job dir + // and built in, so the agent-worker server precomputing a cache name has to resolve + // modules the same way (`windmill-api-agent-workers`, `get_code_and_lock`). let modules = modules_from_data.clone().or_else(|| { job.args.as_ref().and_then(|args| { args.get("_MODULES").and_then(|raw| { @@ -5652,11 +5699,106 @@ mod write_module_files_tests { use super::*; use std::collections::HashMap; use windmill_common::scripts::ScriptLang; + use windmill_common::utils::calculate_hash; fn module(content: &str) -> ScriptModule { ScriptModule { content: content.to_string(), language: ScriptLang::Python3, lock: None } } + /// Every language's artifact cache name funnels module content through this, so an + /// ambiguous encoding puts two different runnables back on one name. + #[test] + fn artifact_name_cannot_be_re_cut_into_another_module_map() { + fn name(entries: &[(&str, &str)]) -> String { + let map: HashMap = entries + .iter() + .map(|(p, c)| (p.to_string(), module(c))) + .collect(); + artifact_cache_name("base".to_string(), Some(&map)) + } + + // Naive `path + content` concatenation renders both of these as "abc". + assert_ne!(name(&[("a", "bc")]), name(&[("ab", "c")])); + // Splitting one module into two must not read back as the joined one. + assert_ne!(name(&[("a", "b"), ("c", "d")]), name(&[("ac", "bd")])); + // Iteration order of the map must not move the name. + assert_eq!( + name(&[("a", "1"), ("b", "2")]), + name(&[("b", "2"), ("a", "1")]) + ); + } + + /// A module-free runnable must keep the exact name it had before modules entered the + /// derivation, or upgrading strands every artifact already in the cache. + #[test] + fn artifact_name_is_unchanged_without_modules() { + assert_eq!( + artifact_cache_name("code+lock".to_string(), None), + calculate_hash("code+lock") + ); + assert_eq!( + artifact_cache_name("code+lock".to_string(), Some(&HashMap::new())), + calculate_hash("code+lock") + ); + } + + /// A preview brings its own source and lockfile, so a module-free runnable picks its + /// whole `base`. Module-bearing names live in their own namespace precisely so that no + /// crafted `base` can be made to land on one. + #[test] + fn a_module_free_runnable_cannot_forge_a_module_bearing_name() { + let modules = HashMap::from([("h.ts".to_string(), module("evil"))]); + let victim = artifact_cache_name("code+lock".to_string(), Some(&modules)); + + // `calculate_hash` emits hex, so the namespace is unreachable however `base` is + // chosen — including by feeding it the victim's own name. + assert!(victim.starts_with("mod-")); + assert_ne!(artifact_cache_name(victim.clone(), None), victim); + assert!(!artifact_cache_name("anything".to_string(), None).starts_with("mod-")); + } + + /// The `mod-` namespace separates module-free from module-bearing, and nothing separates + /// two module-bearing runnables — only the seal does. Unsealed, `base` is variable-width, + /// so the split between it and the module block is ambiguous and a preview (which brings + /// its own source *and* lockfile) can absorb part of another runnable's block. + #[test] + fn a_module_bearing_runnable_cannot_absorb_another_ones_block() { + let victim = artifact_cache_name( + "V".to_string(), + Some(&HashMap::from([( + "h.ts".to_string(), + module(":modules:1:1:a:1:b"), + )])), + ); + // Byte-identical to the victim's without the seal: the forger's `base` spells out the + // victim's leading block, leaving its own single module to supply the tail. + let forged = artifact_cache_name( + "V:modules:1:4:h.ts:18:".to_string(), + Some(&HashMap::from([("a".to_string(), module("b"))])), + ); + + assert_ne!(victim, forged); + } + + /// Deploy fills a module's lock in after the parent has prebuilt, so a name that moved + /// with it would leave every prebuilt artifact unreachable by the runs it was built for. + #[test] + fn artifact_name_ignores_the_lock_deploy_fills_in_later() { + let prebuild = artifact_cache_name( + "base".to_string(), + Some(&HashMap::from([("h.ts".to_string(), module("x"))])), + ); + + let mut locked = module("x"); + locked.lock = Some("{}\n//bun.lock\n".to_string()); + let after_deploy = artifact_cache_name( + "base".to_string(), + Some(&HashMap::from([("h.ts".to_string(), locked)])), + ); + + assert_eq!(prebuild, after_deploy); + } + #[test] fn contained_relative_path_rejects_traversal_and_absolute() { assert!(is_contained_relative_path("u/admin/pkg")); @@ -6386,6 +6528,7 @@ mount {{ envs, occupancy_metrics, maybe_lock, + modules.as_ref(), )) .await } @@ -6515,6 +6658,7 @@ mount {{ worker_name, envs, occupancy_metrics, + modules.as_ref(), )) .await } @@ -6573,6 +6717,7 @@ mount {{ worker_name, envs, occupancy_metrics, + modules.as_ref(), )) .await } @@ -6637,6 +6782,7 @@ mount {{ worker_name, envs, occupancy_metrics, + modules: modules.as_ref(), })) .await } diff --git a/backend/windmill-worker/src/worker_lockfiles.rs b/backend/windmill-worker/src/worker_lockfiles.rs index 5cfb9f08ce..57b859075a 100644 --- a/backend/windmill-worker/src/worker_lockfiles.rs +++ b/backend/windmill-worker/src/worker_lockfiles.rs @@ -199,6 +199,7 @@ async fn handle_build_binary_job( worker_name, base_internal_url, occupancy_metrics, + script_data.modules.as_ref(), ) .await? } @@ -214,6 +215,7 @@ async fn handle_build_binary_job( worker_name, base_internal_url, occupancy_metrics, + script_data.modules.as_ref(), ) .await? } @@ -235,6 +237,7 @@ async fn handle_build_binary_job( worker_name, base_internal_url, occupancy_metrics, + script_data.modules.as_ref(), ) .await? } @@ -3155,7 +3158,11 @@ async fn capture_dependency_job( ) .await? { - if !wd_exist { + // Nothing here writes the module files, so a bundle built now resolves a + // multi-file script's relative imports remotely rather than from its + // modules; caching that under a key naming them would serve the wrong + // code to every run. Leave it to the first run instead. + if !wd_exist && modules.map_or(true, |m| m.is_empty()) { crate::bun_executor::prebundle_bun_script( job_raw_code, &lock, @@ -3169,6 +3176,7 @@ async fn capture_dependency_job( &token, &mut Some(occupancy_metrics), temp_script_refs, + modules, ) .await?; }