diff --git a/backend/.sqlx/query-104a1aac6325f763f4ae8c42ad008f824419f2d2c161b1fd44844f0c6ff07d8c.json b/backend/.sqlx/query-104a1aac6325f763f4ae8c42ad008f824419f2d2c161b1fd44844f0c6ff07d8c.json new file mode 100644 index 0000000000..c03fc60c3f --- /dev/null +++ b/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" +} diff --git a/backend/.sqlx/query-2e4e783de828f8e33a98a1b082e24603e7390ef36aebaf53afffa7e3c2a7be04.json b/backend/.sqlx/query-2e4e783de828f8e33a98a1b082e24603e7390ef36aebaf53afffa7e3c2a7be04.json new file mode 100644 index 0000000000..e608f4e7e0 --- /dev/null +++ b/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" +} diff --git a/backend/windmill-api-integration-tests/tests/drafts.rs b/backend/windmill-api-integration-tests/tests/drafts.rs index 9c82887d20..9c08c14a6d 100644 --- a/backend/windmill-api-integration-tests/tests/drafts.rs +++ b/backend/windmill-api-integration-tests/tests/drafts.rs @@ -345,6 +345,23 @@ async fn test_transfer_drafts_to_fork(db: Pool) -> 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) diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index 375c6102be..55c78970ce 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/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, Extension(user_db): Extension, Path(w_id): Path, @@ -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!( diff --git a/backend/windmill-common/src/user_drafts.rs b/backend/windmill-common/src/user_drafts.rs index cb17fb9257..fa869bd253 100644 --- a/backend/windmill-common/src/user_drafts.rs +++ b/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 diff --git a/frontend/src/lib/components/sessions/SessionWrapper.svelte b/frontend/src/lib/components/sessions/SessionWrapper.svelte index 005ddbb708..ed7a82802a 100644 --- a/frontend/src/lib/components/sessions/SessionWrapper.svelte +++ b/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?.() diff --git a/frontend/src/lib/components/sessions/moveSessionToFork.ts b/frontend/src/lib/components/sessions/moveSessionToFork.ts index 03b6f2458c..4e50959142 100644 --- a/frontend/src/lib/components/sessions/moveSessionToFork.ts +++ b/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 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 } }