From e2e2e7d7c4924fe29a912c603554f8545184d704 Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:14:31 +0100 Subject: [PATCH] fix: build hub script urls and cache keys from the parsed id only (#12028) --- backend/windmill-common/src/flows.rs | 51 ++++------------- backend/windmill-common/src/scripts.rs | 79 +++++++++++--------------- backend/windmill-common/src/utils.rs | 15 +++++ 3 files changed, 58 insertions(+), 87 deletions(-) diff --git a/backend/windmill-common/src/flows.rs b/backend/windmill-common/src/flows.rs index 500549c549..1ea323386a 100644 --- a/backend/windmill-common/src/flows.rs +++ b/backend/windmill-common/src/flows.rs @@ -18,7 +18,7 @@ use crate::{ db::DB, error::{to_anyhow, Error}, scripts::ScriptHash, - utils::{http_get_from_hub, StripPath}, + utils::{http_get_from_hub, parse_hub_id, StripPath}, worker::{to_raw_value, Connection}, DEFAULT_HUB_BASE_URL, HUB_BASE_URL, PRIVATE_HUB_MIN_VERSION, }; @@ -33,45 +33,13 @@ struct HubFlowResponse { flow: HubFlow, } -fn extract_hub_flow_id_from_path(path: &str) -> Result { - let hub_flow_path = path.strip_prefix("hub/flows/").ok_or_else(|| { - Error::BadRequest(format!( - "expected hub flow path to start with hub/flows/ (got {path})" - )) - })?; - - let flow_id = hub_flow_path - .split('/') - .next() - .filter(|segment| !segment.is_empty()) - .ok_or_else(|| { - Error::BadRequest(format!( - "expected hub flow path to include a numeric id after hub/flows/ (got {path})" - )) - })?; - - let flow_id = flow_id.parse::().map_err(|_| { - Error::BadRequest(format!( - "expected hub flow path to include a numeric id after hub/flows/ (got {path})" - )) - })?; - - if flow_id <= 0 { - return Err(Error::BadRequest(format!( - "expected hub flow path to include a positive numeric id after hub/flows/ (got {path})" - ))); - } - - Ok(flow_id) -} - pub async fn get_full_hub_flow_by_path( path: StripPath, http_client: &reqwest::Client, db: Option<&DB>, ) -> crate::error::Result { let path = path.to_path(); - let flow_id = extract_hub_flow_id_from_path(&path)?; + let flow_id = parse_hub_id(&path, "hub/flows/")?; let hub_base_url = (**HUB_BASE_URL.load()).clone(); let hub_url = format!("{hub_base_url}/flows/{flow_id}/json"); @@ -472,33 +440,36 @@ mod tests { #[test] fn extract_hub_flow_id_accepts_id_only_paths() { - assert_eq!(extract_hub_flow_id_from_path("hub/flows/76").unwrap(), 76); + assert_eq!(parse_hub_id("hub/flows/76", "hub/flows/").unwrap(), 76); } #[test] fn extract_hub_flow_id_accepts_id_and_slug_paths() { assert_eq!( - extract_hub_flow_id_from_path("hub/flows/76/send-message-to-company-ai-assistant") - .unwrap(), + parse_hub_id( + "hub/flows/76/send-message-to-company-ai-assistant", + "hub/flows/" + ) + .unwrap(), 76 ); } #[test] fn extract_hub_flow_id_rejects_non_numeric_ids() { - let err = extract_hub_flow_id_from_path("hub/flows/send_message").unwrap_err(); + let err = parse_hub_id("hub/flows/send_message", "hub/flows/").unwrap_err(); assert!(matches!(err, Error::BadRequest(_))); } #[test] fn extract_hub_flow_id_rejects_missing_ids() { - let err = extract_hub_flow_id_from_path("hub/flows/").unwrap_err(); + let err = parse_hub_id("hub/flows/", "hub/flows/").unwrap_err(); assert!(matches!(err, Error::BadRequest(_))); } #[test] fn extract_hub_flow_id_rejects_zero_ids() { - let err = extract_hub_flow_id_from_path("hub/flows/0").unwrap_err(); + let err = parse_hub_id("hub/flows/0", "hub/flows/").unwrap_err(); assert!(matches!(err, Error::BadRequest(_))); } diff --git a/backend/windmill-common/src/scripts.rs b/backend/windmill-common/src/scripts.rs index 87148f3f4c..1e97eae944 100644 --- a/backend/windmill-common/src/scripts.rs +++ b/backend/windmill-common/src/scripts.rs @@ -9,7 +9,7 @@ pub use windmill_types::scripts::*; use crate::{ - error::{to_anyhow, Error}, + error::to_anyhow, runnable_settings::{self}, utils::http_get_from_hub, workspace_dependencies::WorkspaceDependenciesAnnotatedRefs, @@ -22,7 +22,7 @@ use backon::ConstantBuilder; use backon::{BackoffBuilder, Retryable}; use regex::Regex; -use crate::utils::StripPath; +use crate::utils::{parse_hub_id, StripPath}; /// The tag a script's dependency job runs on, given the script's own tag; `None` leaves it to /// `push`'s default (the language tag). Every relock of a script must use this, or a relock @@ -195,17 +195,12 @@ pub async fn get_hub_script_by_path( http_client: &reqwest::Client, db: &DB, ) -> crate::error::Result { - let path = path - .to_path() - .strip_prefix("hub/") - .ok_or_else(|| Error::BadRequest("Impossible to remove prefix hex".to_string()))?; - + let id = parse_hub_id(path.to_path(), "hub/")?; let hub_base_url = (**HUB_BASE_URL.load()).clone(); - // let result = http_get_from_hub( http_client, - &format!("{}/raw/{}.ts", hub_base_url, path), + &format!("{hub_base_url}/raw/{id}/script.ts"), true, None, Some(db), @@ -220,19 +215,11 @@ pub async fn get_hub_script_by_path( match result { Ok(result) => Ok(result), Err(e) => { - if hub_base_url != DEFAULT_HUB_BASE_URL - && path - .split("/") - .next() - .is_some_and(|x| x.parse::().is_ok_and(|x| x < PRIVATE_HUB_MIN_VERSION)) - { - tracing::info!( - "Not found on private hub, fallback to default hub for {}", - path - ); + if hub_base_url != DEFAULT_HUB_BASE_URL && id < PRIVATE_HUB_MIN_VERSION { + tracing::info!("Not found on private hub, fallback to default hub for {id}"); let content = http_get_from_hub( http_client, - &format!("{}/raw/{}.ts", DEFAULT_HUB_BASE_URL, path), + &format!("{DEFAULT_HUB_BASE_URL}/raw/{id}/script.ts"), true, None, Some(db), @@ -257,15 +244,8 @@ pub async fn get_full_hub_script_by_path( http_client: &reqwest::Client, db: Option<&DB>, ) -> crate::error::Result { - let path = path - .to_path() - .strip_prefix("hub/") - .ok_or_else(|| Error::BadRequest("Impossible to remove prefix hex".to_string()))?; - - let mut path_iterator = path.split("/"); - let version = path_iterator - .next() - .ok_or_else(|| Error::internal_err(format!("expected hub path to have version number")))?; + let path = path.to_path(); + let version = parse_hub_id(path, "hub/")?; // A cache entry that cannot be read or parsed counts as a miss rather than an error: // a truncated write leaves a file that exists but deserializes to nothing, and refetching // it is always preferable to failing the job push it was read for. @@ -288,10 +268,10 @@ pub async fn get_full_hub_script_by_path( return Ok(script); } - let script = get_full_hub_script_by_path_inner(path, http_client, db).await?; + let script = get_full_hub_script_by_path_inner(version, http_client, db).await?; if let Err(e) = crate::worker::write_file( &HUB_CACHE_DIR, - &version, + &version.to_string(), &serde_json::to_string(&script).map_err(to_anyhow)?, ) { tracing::error!("failed to write hub script {path} to cache: {e}"); @@ -302,7 +282,7 @@ pub async fn get_full_hub_script_by_path( } async fn get_full_hub_script_by_path_inner( - path: &str, + id: i32, http_client: &reqwest::Client, db: Option<&DB>, ) -> crate::error::Result { @@ -311,7 +291,7 @@ async fn get_full_hub_script_by_path_inner( let response = (|| async { let response = http_get_from_hub( http_client, - &format!("{}/raw2/{}", hub_base_url, path), + &format!("{hub_base_url}/raw2/{id}/script"), true, None, db, @@ -322,19 +302,12 @@ async fn get_full_hub_script_by_path_inner( match response { Ok(response) => Ok(response), Err(e) => { - if hub_base_url != DEFAULT_HUB_BASE_URL - && path.split("/").next().is_some_and(|x| { - x.parse::().is_ok_and(|x| x < PRIVATE_HUB_MIN_VERSION) - }) - { + if hub_base_url != DEFAULT_HUB_BASE_URL && id < PRIVATE_HUB_MIN_VERSION { // TODO: should only fallback to default hub if status is 404 (hub returns 500 currently) - tracing::info!( - "Not found on private hub, fallback to default hub for {}", - path - ); + tracing::info!("Not found on private hub, fallback to default hub for {id}"); http_get_from_hub( http_client, - &format!("{}/raw2/{}", DEFAULT_HUB_BASE_URL, path), + &format!("{DEFAULT_HUB_BASE_URL}/raw2/{id}/script"), true, None, db, @@ -355,9 +328,7 @@ async fn get_full_hub_script_by_path_inner( .build(), ) .notify(|err, dur| { - tracing::warn!( - "Could not get hub script at path {path}, retrying in {dur:#?}, err: {err:#?}" - ); + tracing::warn!("Could not get hub script {id}, retrying in {dur:#?}, err: {err:#?}"); }) .sleep(tokio::time::sleep) .await?; @@ -365,7 +336,7 @@ async fn get_full_hub_script_by_path_inner( let script = response .json::() .await - .context(format!("Decoding hub response for script at path {path}"))?; + .context(format!("Decoding hub response for script {id}"))?; Ok(script) } @@ -509,3 +480,17 @@ pub async fn deploy_relocked_version( Ok(new_hash) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn hub_script_id_is_the_first_segment_only() { + let id = |p: &str| parse_hub_id(p, "hub/").ok(); + assert_eq!(id("hub/28102/windmill/git_sync"), Some(28102)); + assert_eq!(id("hub/28102/../19862/x"), Some(28102)); + assert_eq!(id("hub/../19862/x"), None); + assert_eq!(id("hub/flows/76"), None); + } +} diff --git a/backend/windmill-common/src/utils.rs b/backend/windmill-common/src/utils.rs index 074469893d..6ae44ccf7f 100644 --- a/backend/windmill-common/src/utils.rs +++ b/backend/windmill-common/src/utils.rs @@ -216,6 +216,21 @@ impl StripPath { } } +/// The Hub serves scripts and flows by their numeric id alone and ignores the rest of the +/// path, so URLs and cache keys must be built from the parsed id: passing the path through +/// lets dot segments make the URL name one item while the cache key names another. +pub(crate) fn parse_hub_id(path: &str, prefix: &str) -> Result { + path.strip_prefix(prefix) + .and_then(|p| p.split('/').next()) + .and_then(|id| id.parse::().ok()) + .filter(|id| *id > 0) + .ok_or_else(|| { + Error::BadRequest(format!( + "expected a positive numeric id after {prefix} (got {path})" + )) + }) +} + /// Escape ILIKE special characters (`%`, `_`, `\`) so user input is matched /// literally. Use this when building `ILIKE '%…%'` patterns from user-supplied /// strings to prevent wildcard injection.