diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 9ca9a0dd84..a4173fc612 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -40,9 +40,9 @@ use windmill_common::workspaces::GitRepositorySettings; #[cfg(feature = "enterprise")] use windmill_common::workspaces::WorkspaceDeploymentUISettings; use windmill_common::workspaces::{ - check_user_against_rule, get_datatable_resource_from_db_unchecked, DataTable, - DataTableCatalogResourceType, DataTableForkBehavior, ProtectionRuleKind, ProtectionRules, - ProtectionRuleset, RuleCheckResult, WorkspaceGitSyncSettings, WM_FORK_PREFIX, + check_user_against_rule, get_datatable_resource_from_db_unchecked, validate_fork_workspace_id, + DataTable, DataTableCatalogResourceType, DataTableForkBehavior, ProtectionRuleKind, + ProtectionRules, ProtectionRuleset, RuleCheckResult, WorkspaceGitSyncSettings, }; use windmill_common::workspaces::{Ducklake, DucklakeCatalogResourceType}; use windmill_common::PgDatabase; @@ -4776,6 +4776,8 @@ async fn create_workspace_fork_branch( return Err(Error::PermissionDenied(msg)); } + validate_fork_workspace_id(&nw.id)?; + Ok(Json( handle_fork_branch_creation(&authed.email, &authed.username, &db, &w_id, &nw.id).await?, )) @@ -4935,13 +4937,7 @@ async fn create_workspace_fork( let mut tx: Transaction<'_, Postgres> = db.begin().await?; - // Generate unique forked workspace ID with wm-fork prefix - if !nw.id.starts_with(WM_FORK_PREFIX) { - return Err(Error::BadRequest(format!( - "The id `{}` is invalid for a forked workspace. It should be prefixed by {}", - nw.id, WM_FORK_PREFIX - ))); - } + validate_fork_workspace_id(&nw.id)?; let forked_id = nw.id; diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 77e69a3340..74ea9f9c9e 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -163,6 +163,65 @@ pub const LATEST_GIT_SYNC_SCRIPT_PATH: &str = "hub/28217/sync-script-to-git-repo /// fork of another workspace. pub const WM_FORK_PREFIX: &str = "wm-fork-"; +/// Validate that a fork workspace id is safe to interpolate into a git branch name. +/// +/// The id is appended verbatim to a branch like `wm-fork//`, +/// so it must satisfy `git check-ref-format` rules. We validate synchronously at the API +/// layer because the actual branch creation runs in a deferred git-sync worker job — without +/// this check, the API returns 200 and the failure only surfaces later in the worker. +pub fn validate_fork_workspace_id(id: &str) -> error::Result<()> { + if !id.starts_with(WM_FORK_PREFIX) { + return Err(Error::BadRequest(format!( + "The id `{}` is invalid for a forked workspace. It should be prefixed by {}", + id, WM_FORK_PREFIX + ))); + } + + let reject = |reason: &str| { + Err::<(), _>(Error::BadRequest(format!( + "Fork workspace id `{}` is invalid: {} (must be a valid git branch name component)", + id, reason + ))) + }; + + if id.ends_with('.') { + return reject("cannot end with '.'"); + } + if id.ends_with(".lock") { + return reject("cannot end with '.lock'"); + } + if id.contains("..") { + return reject("cannot contain '..'"); + } + if id.contains("@{") { + return reject("cannot contain '@{'"); + } + if id.contains("//") { + return reject("cannot contain '//'"); + } + for ch in id.chars() { + match ch { + ':' | '~' | '^' | '?' | '*' | '[' | '\\' | ' ' => { + return reject(&format!("contains forbidden character '{}'", ch)); + } + c if c.is_ascii_control() || c == '\u{7f}' => { + return reject("contains a control character"); + } + _ => {} + } + } + // Each slash-separated component cannot start with '.' or end with '.lock'. + for component in id.split('/') { + if component.starts_with('.') { + return reject("a path component cannot start with '.'"); + } + if component.ends_with(".lock") { + return reject("a path component cannot end with '.lock'"); + } + } + Ok(()) +} + #[derive(Serialize, Deserialize, Debug)] pub struct GitRepositorySettings { #[serde(skip_serializing_if = "Option::is_none")] @@ -666,3 +725,53 @@ async fn transform_json_unchecked( Ok(value) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_validate_fork_workspace_id_accepts_valid() { + validate_fork_workspace_id("wm-fork-test-allow").unwrap(); + validate_fork_workspace_id("wm-fork-my_workspace.42").unwrap(); + validate_fork_workspace_id("wm-fork-a").unwrap(); + } + + #[test] + fn test_validate_fork_workspace_id_rejects_missing_prefix() { + assert!(validate_fork_workspace_id("not-a-fork").is_err()); + assert!(validate_fork_workspace_id("wm-fork").is_err()); + } + + #[test] + fn test_validate_fork_workspace_id_rejects_git_unsafe_chars() { + for bad in [ + "wm-fork-test:allow", + "wm-fork-test allow", + "wm-fork-test~allow", + "wm-fork-test^allow", + "wm-fork-test?allow", + "wm-fork-test*allow", + "wm-fork-test[allow", + "wm-fork-test\\allow", + "wm-fork-test\nallow", + ] { + assert!( + validate_fork_workspace_id(bad).is_err(), + "expected `{}` to be rejected", + bad + ); + } + } + + #[test] + fn test_validate_fork_workspace_id_rejects_git_unsafe_sequences() { + assert!(validate_fork_workspace_id("wm-fork-foo..bar").is_err()); + assert!(validate_fork_workspace_id("wm-fork-foo@{bar").is_err()); + assert!(validate_fork_workspace_id("wm-fork-foo//bar").is_err()); + assert!(validate_fork_workspace_id("wm-fork-foo.").is_err()); + assert!(validate_fork_workspace_id("wm-fork-foo.lock").is_err()); + assert!(validate_fork_workspace_id("wm-fork-foo/.bar").is_err()); + assert!(validate_fork_workspace_id("wm-fork-foo/bar.lock").is_err()); + } +}