From fcbb27babf7783b4714d247bc3df6e1235921f23 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 7 Aug 2026 12:48:51 +0200 Subject: [PATCH] revert: drop the cloned-app identity resolvability check Preserving an unresolvable principal is not a privilege issue: the clone keeps the parent's identity only for creators who could have written it through the app API, and the fork rejects the job outright. Keeping the change here mixed hygiene into a security fix. Co-Authored-By: Claude Opus 5 (1M context) --- ...c68dba11f97fc03adb7532d860bce16faeb24.json | 23 --------- .../tests/fork_clone_on_behalf_of.rs | 31 ++---------- .../windmill-api-workspaces/src/workspaces.rs | 49 +------------------ 3 files changed, 6 insertions(+), 97 deletions(-) delete mode 100644 backend/.sqlx/query-1b1de4e27def09a49065a1deb2cc68dba11f97fc03adb7532d860bce16faeb24.json diff --git a/backend/.sqlx/query-1b1de4e27def09a49065a1deb2cc68dba11f97fc03adb7532d860bce16faeb24.json b/backend/.sqlx/query-1b1de4e27def09a49065a1deb2cc68dba11f97fc03adb7532d860bce16faeb24.json deleted file mode 100644 index f1ffff86be..0000000000 --- a/backend/.sqlx/query-1b1de4e27def09a49065a1deb2cc68dba11f97fc03adb7532d860bce16faeb24.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT CASE\n WHEN $2 LIKE 'u/%' THEN EXISTS (\n SELECT 1 FROM usr WHERE workspace_id = $1 AND username = substring($2 from 3)\n UNION ALL\n SELECT 1 FROM password WHERE super_admin\n AND (username = substring($2 from 3) OR email = substring($2 from 3)))\n WHEN $2 LIKE 'g/%' THEN EXISTS (\n SELECT 1 FROM group_ WHERE workspace_id = $1 AND name = substring($2 from 3))\n ELSE EXISTS (\n SELECT 1 FROM usr WHERE workspace_id = $1 AND username = $2\n UNION ALL\n SELECT 1 FROM password WHERE email = $2 AND super_admin)\n END AS \"resolves!\"", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "resolves!", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - null - ] - }, - "hash": "1b1de4e27def09a49065a1deb2cc68dba11f97fc03adb7532d860bce16faeb24" -} diff --git a/backend/windmill-api-integration-tests/tests/fork_clone_on_behalf_of.rs b/backend/windmill-api-integration-tests/tests/fork_clone_on_behalf_of.rs index dcbc6600a6..2b9f63d392 100644 --- a/backend/windmill-api-integration-tests/tests/fork_clone_on_behalf_of.rs +++ b/backend/windmill-api-integration-tests/tests/fork_clone_on_behalf_of.rs @@ -3,12 +3,11 @@ use sqlx::{Pool, Postgres}; use windmill_test_utils::*; -/// Seed an anonymous public app running as `on_behalf_of`, then fork as `token`. Returns the +/// Seed an anonymous public app owned by the parent's admin, then fork as `token`. Returns the /// cloned app's policy and custom path. async fn fork_with_public_app( db: &Pool, token: &str, - on_behalf_of: (&str, &str), ) -> anyhow::Result<(serde_json::Value, Option)> { initialize_tracing().await; @@ -20,8 +19,8 @@ async fn fork_with_public_app( VALUES ('test-workspace', 'u/test-user/pub', '', $1, '{}', 'pub-path') RETURNING id", json!({ - "on_behalf_of": on_behalf_of.0, - "on_behalf_of_email": on_behalf_of.1, + "on_behalf_of": "u/test-user", + "on_behalf_of_email": "test@windmill.dev", "execution_mode": "anonymous", }) ) @@ -69,8 +68,7 @@ async fn fork_with_public_app( async fn test_fork_downgrades_app_policy_for_unprivileged_creator( db: Pool, ) -> anyhow::Result<()> { - let (policy, custom_path) = - fork_with_public_app(&db, "SECRET_TOKEN_2", ("u/test-user", "test@windmill.dev")).await?; + let (policy, custom_path) = fork_with_public_app(&db, "SECRET_TOKEN_2").await?; assert_eq!(policy["on_behalf_of"], json!("u/test-user-2")); assert_eq!(policy["on_behalf_of_email"], json!("test2@windmill.dev")); @@ -86,9 +84,7 @@ async fn test_fork_downgrades_app_policy_for_unprivileged_creator( /// claiming it make it resolve to either one. #[sqlx::test(migrations = "../migrations", fixtures("base"))] async fn test_fork_keeps_app_policy_for_admin_creator(db: Pool) -> anyhow::Result<()> { - // The fork's creator is the one member it always gets, so their principal resolves there. - let (policy, custom_path) = - fork_with_public_app(&db, "SECRET_TOKEN", ("u/test-user", "test@windmill.dev")).await?; + let (policy, custom_path) = fork_with_public_app(&db, "SECRET_TOKEN").await?; assert_eq!(policy["on_behalf_of"], json!("u/test-user")); assert_eq!(policy["on_behalf_of_email"], json!("test@windmill.dev")); @@ -98,23 +94,6 @@ async fn test_fork_keeps_app_policy_for_admin_creator(db: Pool) -> any Ok(()) } -/// Preserving is still bounded by what the fork can authenticate: `test-user-2` is a parent member -/// the fork never receives, and a policy naming them would only reach `fetch_authed_from_ -/// permissioned_as`'s "user not found in workspace" — with an address left behind for a later -/// re-derivation to trust. -#[sqlx::test(migrations = "../migrations", fixtures("base"))] -async fn test_fork_drops_app_identity_that_names_no_fork_member( - db: Pool, -) -> anyhow::Result<()> { - let (policy, _) = - fork_with_public_app(&db, "SECRET_TOKEN", ("u/test-user-2", "test2@windmill.dev")).await?; - - assert_eq!(policy.get("on_behalf_of"), None); - assert_eq!(policy.get("on_behalf_of_email"), None); - - Ok(()) -} - /// A principal only means something in the workspace whose `usr`/`group_` rows define it, and a /// fork copies the creator and the groups but not the rest of the membership. Carrying one over /// blindly would leave a runnable naming somebody who cannot authenticate there; dropping them diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 98fec3c4af..80db26fe47 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -6424,51 +6424,6 @@ fn downgrade_cloned_app_policy(policy: &mut serde_json::Value, authed: &ApiAuthe } } -/// The rule [`clone_scripts`] applies to its column, for a caller holding one principal -/// rather than a whole table: a policy naming somebody who cannot authenticate in the fork -/// keeps no identity at all, address included — that address is all a later re-derivation -/// would have to go on. -async fn drop_unresolvable_policy_identity( - tx: &mut Transaction<'_, Postgres>, - target_workspace_id: &str, - policy: &mut serde_json::Value, -) -> Result<()> { - let Some(obj) = policy.as_object_mut() else { - return Ok(()); - }; - let Some(permissioned_as) = obj - .get("on_behalf_of") - .and_then(|v| v.as_str()) - .map(str::to_string) - else { - return Ok(()); - }; - let resolves = sqlx::query_scalar!( - r#"SELECT CASE - WHEN $2 LIKE 'u/%' THEN EXISTS ( - SELECT 1 FROM usr WHERE workspace_id = $1 AND username = substring($2 from 3) - UNION ALL - SELECT 1 FROM password WHERE super_admin - AND (username = substring($2 from 3) OR email = substring($2 from 3))) - WHEN $2 LIKE 'g/%' THEN EXISTS ( - SELECT 1 FROM group_ WHERE workspace_id = $1 AND name = substring($2 from 3)) - ELSE EXISTS ( - SELECT 1 FROM usr WHERE workspace_id = $1 AND username = $2 - UNION ALL - SELECT 1 FROM password WHERE email = $2 AND super_admin) - END AS "resolves!""#, - target_workspace_id, - permissioned_as, - ) - .fetch_one(&mut **tx) - .await?; - if !resolves { - obj.remove("on_behalf_of"); - obj.remove("on_behalf_of_email"); - } - Ok(()) -} - async fn clone_apps( tx: &mut Transaction<'_, Postgres>, source_workspace_id: &str, @@ -6499,9 +6454,7 @@ async fn clone_apps( if let Some(¤t_version) = app.versions.last() { latest_version_ids.insert(current_version); } - if preserve_identity { - drop_unresolvable_policy_identity(tx, target_workspace_id, &mut app.policy).await?; - } else { + if !preserve_identity { downgrade_cloned_app_policy(&mut app.policy, authed); } // An instance-wide custom path addresses one app (`create_app` rejects one already