fix: harden the draft transfer against bound tokens, key drift and concurrent saves

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

backend/.sqlx/query-104a1aac6325f763f4ae8c42ad008f824419f2d2c161b1fd44844f0c6ff07d8c.json

@@ -0,0 +1,23 @@
+{
+  "db_name": "PostgreSQL",
+  "query": "SELECT EXISTS(SELECT 1 FROM token WHERE token_hash = $1\n                 AND (expiration > NOW() OR expiration IS NULL)\n                 AND (workspace_id IS NULL OR workspace_id = $2))",
+  "describe": {
+    "columns": [
+      {
+        "ordinal": 0,
+        "name": "exists",
+        "type_info": "Bool"
+      }
+    ],
+    "parameters": {
+      "Left": [
+        "Text",
+        "Text"
+      ]
+    },
+    "nullable": [
+      null
+    ]
+  },
+  "hash": "104a1aac6325f763f4ae8c42ad008f824419f2d2c161b1fd44844f0c6ff07d8c"
+}

backend/.sqlx/query-2e4e783de828f8e33a98a1b082e24603e7390ef36aebaf53afffa7e3c2a7be04.json

@@ -0,0 +1,25 @@
+{
+  "db_name": "PostgreSQL",
+  "query": "SELECT 1 AS \"one\" FROM draft\n           WHERE workspace_id = $1 AND email = $2\n             AND (typ::text, path) IN (SELECT * FROM unnest($3::text[], $4::text[]))\n           FOR UPDATE",
+  "describe": {
+    "columns": [
+      {
+        "ordinal": 0,
+        "name": "one",
+        "type_info": "Int4"
+      }
+    ],
+    "parameters": {
+      "Left": [
+        "Text",
+        "Text",
+        "TextArray",
+        "TextArray"
+      ]
+    },
+    "nullable": [
+      null
+    ]
+  },
+  "hash": "2e4e783de828f8e33a98a1b082e24603e7390ef36aebaf53afffa7e3c2a7be04"
+}

backend/windmill-api-integration-tests/tests/drafts.rs

@@ -345,6 +345,23 @@ async fn test_transfer_drafts_to_fork(db: Pool<Postgres>) -> anyhow::Result<()>
         { "kind": "script", "path": "u/test-user/b" }
     ]);
 
+    // A token bound to the source authenticates on its routes but must not write the fork.
+    sqlx::query(
+        "INSERT INTO token(token_hash, token_prefix, token, email, label, super_admin, workspace_id)
+         VALUES (encode(sha256('BOUND_TOKEN'::bytea), 'hex'), 'BOUND_TOK', 'BOUND_TOKEN', $1, 'bound', true, $2)",
+    )
+    .bind(me)
+    .bind(WS)
+    .execute(&db)
+    .await?;
+    let r = client_for("BOUND_TOKEN")
+        .post(&transfer_url)
+        .json(&json!({ "target_workspace": fork, "items": items }))
+        .send()
+        .await?;
+    assert_eq!(r.status(), 403);
+    assert_eq!(fork_count("u/test-user/b").await?, 0);
+
     // `a` was cloned into the fork: the whole transfer is refused and nothing moves.
     let body: serde_json::Value = c
         .post(&transfer_url)

backend/windmill-api/src/drafts.rs

@@ -961,6 +961,7 @@ pub struct TransferDraftsResponse {
 /// in the target.
 async fn transfer_drafts(
     authed: ApiAuthed,
+    tokened: crate::auth::Tokened,
     Extension(db): Extension<DB>,
     Extension(user_db): Extension<UserDB>,
     Path(w_id): Path<String>,
@@ -990,6 +991,32 @@ async fn transfer_drafts(
                 "{target} is not a fork of {w_id}"
             )));
         }
+        // Authentication checked the token against `w_id` only. A token bound to that
+        // workspace, or a job token (not in `token`), must not reach into the fork.
+        let token_valid_for_target = sqlx::query_scalar!(
+            "SELECT EXISTS(SELECT 1 FROM token WHERE token_hash = $1
+                 AND (expiration > NOW() OR expiration IS NULL)
+                 AND (workspace_id IS NULL OR workspace_id = $2))",
+            windmill_common::auth::hash_token(&tokened.token),
+            target,
+        )
+        .fetch_one(&db)
+        .await?
+        .unwrap_or(false);
+        if !token_valid_for_target {
+            return Err(Error::PermissionDenied(format!(
+                "this token cannot act on {target}"
+            )));
+        }
+        // Secret variable drafts hold values encrypted with the source workspace's key.
+        if items.iter().any(|(k, _)| *k == UserDraftItemKind::Variable)
+            && windmill_common::variables::get_workspace_key(&w_id, &db).await?
+                != windmill_common::variables::get_workspace_key(target, &db).await?
+        {
+            return Err(Error::BadRequest(format!(
+                "{target} has a different encryption key than {w_id}, so variable drafts cannot be copied to it"
+            )));
+        }
         // `authed` speaks for this workspace only; the copy writes into the fork, so its
         // rights there are loaded separately. A non-member gets an empty authed, not an error.
         let target_authed =
@@ -1012,6 +1039,19 @@ async fn transfer_drafts(
     }
 
     let mut tx = db.begin().await?;
+    // Holds off a concurrent save of these rows, so the version removed below is the one copied.
+    sqlx::query!(
+        r#"SELECT 1 AS "one" FROM draft
+           WHERE workspace_id = $1 AND email = $2
+             AND (typ::text, path) IN (SELECT * FROM unnest($3::text[], $4::text[]))
+           FOR UPDATE"#,
+        &w_id,
+        &authed.email,
+        &kinds as &[&str],
+        &paths as &[&str],
+    )
+    .fetch_all(&mut *tx)
+    .await?;
     let mut copied = vec![];
     if let Some(target) = req.target_workspace.as_deref() {
         let conflicts = sqlx::query!(

backend/windmill-common/src/user_drafts.rs

@@ -649,6 +649,10 @@ pub async fn delete_own_draft_for_path(
 /// copied. With `items = None` this is the fork-time clone: every row of `email` plus the legacy
 /// NULL-email row. With `items`, only `email`'s own rows at those `(kind, path)` pairs.
 ///
+/// No authorization here. The caller MUST have checked that `target` is a fork of `source`, that
+/// the credential behind `email` may act on `target`, and that it may write each copied path
+/// there. Secret values are copied as encrypted, so the two workspaces must share a key.
+///
 /// A plain INSERT: the caller guarantees the target holds none of the copied rows (an empty
 /// fork, or a conflict check under the same transaction), else `draft_pkey_with_user` aborts it.
 /// `created_at` is preserved so the per-tab `last_sync` baseline the editor reads lines up with

frontend/src/lib/components/sessions/SessionWrapper.svelte

@@ -449,6 +449,7 @@
 				.catch((e) => console.error('Failed to persist the move note', e))
 			runtime?.manager.rebuildGlobalSystemMessage()
 			sendUserToast(`Session moved to ${req.targetName}`)
+			if (res.warning) sendUserToast(res.warning, true)
 		} finally {
 			moving = false
 			releaseAutoResume?.()

frontend/src/lib/components/sessions/moveSessionToFork.ts

@@ -12,7 +12,7 @@ import { materializeFork, moveSessionToWorkspace, type PendingFork } from './ses
 export type MoveTarget = { kind: 'existing'; id: string } | { kind: 'new'; fork: PendingFork }
 
 export type MoveSessionResult =
-	| { ok: true; forkId: string; copied: number; removed: number }
+	| { ok: true; forkId: string; copied: number; removed: number; warning?: string }
 	| { ok: false; error: string; conflicts?: DraftItemRef[] }
 
 /**
@@ -41,8 +41,9 @@ export async function flushPendingSaves(workspace: string): Promise<string | und
  * leaves it acting on the parent with its drafts intact.
  *
  * A new fork already receives every draft of the user through the fork clone, so only an
- * existing fork needs the copy. Retrying a failed new-fork move with the same `target` is
- * safe: `materializeFork` adopts the fork it created the first time.
+ * existing fork needs the copy. Once a new fork exists the move always completes: a retry
+ * would adopt that fork without cloning again, and then remove parent drafts saved after the
+ * clone. Removing the parent's copies is therefore best-effort there, reported as `warning`.
  */
 export async function moveSessionToFork(
 	sessionId: string,
@@ -64,19 +65,23 @@ export async function moveSessionToFork(
 	let forkId: string
 	let copied = 0
 	let removed = 0
+	let warning: string | undefined
 	try {
 		if (target.kind === 'new') {
 			const created = await materializeFork(target.fork)
 			if (!created) return { ok: false, error: 'Could not create the fork' }
-			if (opts.isBusy?.()) return busy
 			forkId = created
 			copied = opts.items.length
 			if (opts.removeFromParent && opts.items.length > 0) {
-				const res = await DraftService.transferDrafts({
-					workspace: parent,
-					requestBody: { items: opts.items, remove_from_source: true }
-				})
-				removed = res.removed.length
+				try {
+					const res = await DraftService.transferDrafts({
+						workspace: parent,
+						requestBody: { items: opts.items, remove_from_source: true }
+					})
+					removed = res.removed.length
+				} catch (e: any) {
+					warning = `The drafts are in the fork but could not be removed from ${parent}: ${e?.body ?? e?.message ?? e}`
+				}
 			}
 		} else {
 			forkId = target.id
@@ -104,7 +109,7 @@ export async function moveSessionToFork(
 		return { ok: false, error: String(e?.body ?? e?.message ?? e) }
 	}
 
-	if (opts.removeFromParent) {
+	if (removed > 0) {
 		for (const it of opts.items) UserDraft.forgetLocal(it.kind, it.path, { workspace: parent })
 	}
 	await moveSessionToWorkspace(sessionId, forkId)
@@ -117,5 +122,5 @@ export async function moveSessionToFork(
 		entityId: sessionId,
 		workspace: forkId
 	})
-	return { ok: true, forkId, copied, removed }
+	return { ok: true, forkId, copied, removed, warning }
 }