starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-10-19 00:03:19 +00:00
Code Issues Packages Projects Releases Wiki Activity

revert: drop the cloned-app identity resolvability check

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
hugocasa
2026-08-07 12:48:51 +02:00
co-authored by Claude Opus 5
parent 210833cac4
commit fcbb27babf
3 changed files with 6 additions and 97 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/.sqlx/query-1b1de4e27def09a49065a1deb2cc68dba11f97fc03adb7532d860bce16faeb24.json
Generated
-23
View File
@@ -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"
}
backend/windmill-api-integration-tests/tests/fork_clone_on_behalf_of.rs
+5 -26
View File
@@ -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<Postgres>,
token: &str,
on_behalf_of: (&str, &str),
) -> anyhow::Result<(serde_json::Value, Option<String>)> {
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<Postgres>,
) -> 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<Postgres>) -> 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<Postgres>) -> 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<Postgres>,
) -> 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
backend/windmill-api-workspaces/src/workspaces.rs
+1 -48
View File
@@ -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(&current_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