fix(workspaces): validate fork id as a git branch name component (#9049)

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;
 

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/<original_branch>/<id>`,
+/// 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());
+    }
+}