diff --git a/backend/.sqlx/query-61caaa5c4ae62f618ba18f36330bbf58ff58e8c448874ffc9ca466165e545878.json b/backend/.sqlx/query-61caaa5c4ae62f618ba18f36330bbf58ff58e8c448874ffc9ca466165e545878.json new file mode 100644 index 0000000000..f50b4b743f --- /dev/null +++ b/backend/.sqlx/query-61caaa5c4ae62f618ba18f36330bbf58ff58e8c448874ffc9ca466165e545878.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM ws_specific\n WHERE workspace_id = $1 AND item_kind = 'variable' AND path = ANY($2)", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "TextArray" + ] + }, + "nullable": [] + }, + "hash": "61caaa5c4ae62f618ba18f36330bbf58ff58e8c448874ffc9ca466165e545878" +} diff --git a/backend/.sqlx/query-a7b6731427d51eb34744ca6a39e30d02949f149901ec95323b5725911944df4d.json b/backend/.sqlx/query-a7b6731427d51eb34744ca6a39e30d02949f149901ec95323b5725911944df4d.json new file mode 100644 index 0000000000..fa43d27ed8 --- /dev/null +++ b/backend/.sqlx/query-a7b6731427d51eb34744ca6a39e30d02949f149901ec95323b5725911944df4d.json @@ -0,0 +1,24 @@ +{ + "db_name": "PostgreSQL", + "query": "WITH survivors AS (\n SELECT value::text AS rendered FROM resource\n WHERE workspace_id = $1 AND NOT (path = ANY($2::text[]))\n )\n SELECT v.path FROM unnest($3::text[]) AS v(path)\n WHERE EXISTS (\n SELECT 1 FROM survivors s\n WHERE strpos(s.rendered, '\"$var:' || v.path || '\"') > 0\n OR strpos(s.rendered, '\"$jsonvar:' || v.path || '\"') > 0\n )", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text", + "TextArray", + "TextArray" + ] + }, + "nullable": [ + null + ] + }, + "hash": "a7b6731427d51eb34744ca6a39e30d02949f149901ec95323b5725911944df4d" +} diff --git a/backend/.sqlx/query-d88b1c445acc5375e9c543dcec81059d0e7018a6cea79ba743693098d223edf5.json b/backend/.sqlx/query-d88b1c445acc5375e9c543dcec81059d0e7018a6cea79ba743693098d223edf5.json new file mode 100644 index 0000000000..b0c2d95175 --- /dev/null +++ b/backend/.sqlx/query-d88b1c445acc5375e9c543dcec81059d0e7018a6cea79ba743693098d223edf5.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM variable WHERE workspace_id = $1 AND path = ANY($2) RETURNING path", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text", + "TextArray" + ] + }, + "nullable": [ + false + ] + }, + "hash": "d88b1c445acc5375e9c543dcec81059d0e7018a6cea79ba743693098d223edf5" +} diff --git a/backend/tests/ws_specific.rs b/backend/tests/ws_specific.rs index cc15161381..322de598b7 100644 --- a/backend/tests/ws_specific.rs +++ b/backend/tests/ws_specific.rs @@ -391,7 +391,8 @@ async fn test_create_resource_upsert_clears_ws_specific(db: Pool) -> a /// Regression for GHSA-xmr2-98m6-cjf7: a token scoped only to `resources:write:` /// must NOT use the resource-delete cascade to delete a linked secret variable it has -/// no `variables:write` scope for. +/// no `variables:write` scope for. The victim sits at a path the resource owns, which is +/// the only kind the cascade reaches at all. #[sqlx::test(fixtures("ws_specific"))] async fn test_scoped_token_cannot_cascade_delete_linked_variable( db: Pool, @@ -406,7 +407,7 @@ async fn test_scoped_token_cannot_cascade_delete_linked_variable( "SECRET_TOKEN", ) .json(&json!({ - "path": "u/test-user/victim_secret", + "path": "u/test-user/db_victim_secret", "value": "hunter2", "is_secret": true, "description": "" @@ -421,7 +422,7 @@ async fn test_scoped_token_cannot_cascade_delete_linked_variable( ) .json(&json!({ "path": "u/test-user/db", - "value": { "password": "$var:u/test-user/victim_secret" }, + "value": { "password": "$var:u/test-user/db_victim_secret" }, "resource_type": "object" })) .send() @@ -443,9 +444,206 @@ async fn test_scoped_token_cannot_cascade_delete_linked_variable( resp.text().await? ); assert!( - variable_exists(&db, "test-workspace", "u/test-user/victim_secret").await?, + variable_exists(&db, "test-workspace", "u/test-user/db_victim_secret").await?, "victim variable must survive the denied cascade" ); + // The scope check runs after the resource DELETE, so only the rollback keeps the resource + // alive — moving the check out of the transaction would silently delete it on a 403. + let resource_left: Option = + sqlx::query_scalar("SELECT COUNT(*) FROM resource WHERE workspace_id = $1 AND path = $2") + .bind("test-workspace") + .bind("u/test-user/db") + .fetch_one(&db) + .await?; + assert_eq!( + resource_left.unwrap_or(0), + 1, + "the denied delete must roll the resource back too" + ); + + Ok(()) +} + +/// Deleting a resource must not take a variable other things still need. Two gates, each +/// with a way past the other: a variable outside the resource's own path is never its to +/// delete, and even one it owns stays if another resource points at it. +#[sqlx::test(fixtures("ws_specific"))] +async fn test_resource_delete_spares_variables_it_does_not_own( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace"); + + let create_var = |path: &'static str| { + authed( + client().post(format!("{base}/variables/create")), + "SECRET_TOKEN", + ) + .json(&json!({ "path": path, "value": "hunter2", "is_secret": true, "description": "" })) + .send() + }; + let create_res = |path: &'static str, var: &'static str| { + authed( + client().post(format!("{base}/resources/create")), + "SECRET_TOKEN", + ) + .json(&json!({ + "path": path, + "value": { "password": format!("$var:{var}") }, + "resource_type": "object" + })) + .send() + }; + + // A shared secret at a path of its own, and two resources reading it. + assert_eq!(create_var("u/test-user/shared_canary").await?.status(), 201); + assert_eq!( + create_res("u/test-user/probe_a", "u/test-user/shared_canary") + .await? + .status(), + 201 + ); + assert_eq!( + create_res("u/test-user/probe_b", "u/test-user/shared_canary") + .await? + .status(), + 201 + ); + + // A secret the resource at the same path owns, which a second resource also reads. + assert_eq!(create_var("u/test-user/owned").await?.status(), 201); + assert_eq!( + create_res("u/test-user/owned", "u/test-user/owned") + .await? + .status(), + 201 + ); + assert_eq!( + create_res("u/test-user/borrower", "u/test-user/owned") + .await? + .status(), + 201 + ); + + for resource in ["u/test-user/probe_a", "u/test-user/owned"] { + let resp = authed( + client().delete(format!("{base}/resources/delete/{resource}")), + "SECRET_TOKEN", + ) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "delete {resource}: {}", + resp.text().await? + ); + } + + assert!( + variable_exists(&db, "test-workspace", "u/test-user/shared_canary").await?, + "a variable the deleted resource only referenced must survive" + ); + assert!( + variable_exists(&db, "test-workspace", "u/test-user/owned").await?, + "an owned variable another resource still references must survive" + ); + + Ok(()) +} + +/// The bulk cascade follows what RLS actually deleted, not what the caller asked for: a +/// resource the request names but leaves standing neither cascades nor stops counting as a +/// referrer. Both halves matter, and neither covers the other. +#[sqlx::test(fixtures("ws_specific"))] +async fn test_bulk_delete_follows_what_rls_deleted(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace"); + + let create_var = |path: &'static str| { + authed( + client().post(format!("{base}/variables/create")), + "SECRET_TOKEN", + ) + .json(&json!({ "path": path, "value": "hunter2", "is_secret": true, "description": "" })) + .send() + }; + // ws_specific so the flag assertion at the end has something to check. + let create_res = |path: &'static str, var: &'static str| { + authed( + client().post(format!("{base}/resources/create")), + "SECRET_TOKEN", + ) + .json(&json!({ + "path": path, + "value": { "password": format!("$var:{var}") }, + "resource_type": "object", + "ws_specific": true + })) + .send() + }; + + // Private to test-user: a resource and the secret it owns. + assert_eq!(create_var("u/test-user/hidden_pwd").await?.status(), 201); + assert_eq!( + create_res("u/test-user/hidden", "u/test-user/hidden_pwd") + .await? + .status(), + 201 + ); + // test-user-2's own resource and secret, which the private resource above also reads. + assert_eq!(create_var("u/test-user-2/own_pwd").await?.status(), 201); + assert_eq!( + create_res("u/test-user-2/own", "u/test-user-2/own_pwd") + .await? + .status(), + 201 + ); + assert_eq!( + create_res("u/test-user/reader", "u/test-user-2/own_pwd") + .await? + .status(), + 201 + ); + + // test-user-2 may write the private secret but has no access to its resource at all. + sqlx::query( + "UPDATE variable SET extra_perms = '{\"u/test-user-2\": true}'::jsonb + WHERE workspace_id = 'test-workspace' AND path = 'u/test-user/hidden_pwd'", + ) + .execute(&db) + .await?; + + let resp = authed( + client().delete(format!("{base}/resources/delete_bulk")), + "SECRET_TOKEN_2", + ) + .json(&json!({ + "paths": ["u/test-user/hidden", "u/test-user-2/own", "u/test-user/reader"] + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "bulk delete: {}", resp.text().await?); + + assert!( + variable_exists(&db, "test-workspace", "u/test-user/hidden_pwd").await?, + "the variable of a resource RLS refused to delete must survive" + ); + assert!( + variable_exists(&db, "test-workspace", "u/test-user-2/own_pwd").await?, + "a requested resource RLS left standing still counts as a referrer" + ); + // ws_specific has no RLS policy of its own, so clearing it by requested path rather than + // by deleted path would quietly turn a surviving resource workspace-generic. + assert_eq!( + ws_specific_row_count(&db, "test-workspace", "resource", "u/test-user/hidden").await?, + 1, + "a resource RLS refused to delete must keep its ws_specific flag" + ); Ok(()) } diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 2df5dbab5f..72513d2d63 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -7,7 +7,7 @@ */ use dashmap::DashMap; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::net::IpAddr; use std::sync::LazyLock; @@ -1339,53 +1339,21 @@ async fn delete_resource( return Err(Error::PermissionDenied(msg)); } + let cascade = plan_linked_var_cascade(&db, &w_id, &[path.to_string()]).await?; + let mut tx = user_db.begin(&authed).await?; - // Capture resource data for trashbin before deleting - let trash_resource: Option = sqlx::query_scalar( - "SELECT to_jsonb(t) FROM resource t WHERE path = $1 AND workspace_id = $2", + // The whole row comes back out of the delete, so the trashbin entry below is built from + // what RLS actually removed, and the cascade runs only once RLS has allowed the delete. + let deleted: Option<(String, serde_json::Value)> = sqlx::query_as( + "DELETE FROM resource AS t WHERE t.path = $1 AND t.workspace_id = $2 + RETURNING t.path, to_jsonb(t)", ) .bind(path) .bind(&w_id) .fetch_optional(&mut *tx) .await?; - - // Fetch the resource value before deleting, so we can find linked $var: references - let resource_value: Option> = - sqlx::query_scalar("SELECT value FROM resource WHERE path = $1 AND workspace_id = $2") - .bind(path) - .bind(&w_id) - .fetch_optional(&mut *tx) - .await?; - - // Collect all $var: paths referenced in the resource value - let mut linked_var_paths: Vec = Vec::new(); - if let Some(Some(ref value)) = resource_value { - collect_var_refs(value, &mut linked_var_paths); - } - - // A scoped token must not delete linked variables it lacks variables:write for. - check_linked_var_delete_scopes(&authed, &linked_var_paths)?; - - // Capture linked variables for trashbin before deleting them - let trash_linked_vars: Vec = if linked_var_paths.is_empty() { - Vec::new() - } else { - let placeholders: Vec = linked_var_paths - .iter() - .enumerate() - .map(|(i, _)| format!("${}", i + 2)) - .collect(); - let query = format!( - "SELECT to_jsonb(t) FROM variable t WHERE workspace_id = $1 AND path IN ({})", - placeholders.join(", ") - ); - let mut q = sqlx::query_scalar::<_, serde_json::Value>(&query).bind(&w_id); - for var_path in &linked_var_paths { - q = q.bind(var_path); - } - q.fetch_all(&mut *tx).await? - }; + let (deleted_path, res_data) = not_found_if_none(deleted, "Resource", &path)?; sqlx::query!( "DELETE FROM ws_specific WHERE workspace_id = $1 AND item_kind = 'resource' AND path = $2", @@ -1395,64 +1363,63 @@ async fn delete_resource( .execute(&mut *tx) .await?; - let deleted_path = sqlx::query_scalar!( - "DELETE FROM resource WHERE path = $1 AND workspace_id = $2 RETURNING path", - path, - w_id + let linked_var_paths = cascade.resolve(std::slice::from_ref(&deleted_path)); + + // A scoped token must not delete linked variables it lacks variables:write for. Erroring + // here rolls the resource delete back with it, so nothing is deleted either way. + check_linked_var_delete_scopes(&authed, &linked_var_paths)?; + + // Capture linked variables for trashbin before deleting them + let trash_linked_vars: Vec<(String, serde_json::Value)> = sqlx::query_as( + "SELECT path, to_jsonb(t) FROM variable t WHERE workspace_id = $1 AND path = ANY($2)", ) - .fetch_optional(&mut *tx) + .bind(&w_id) + .bind(&linked_var_paths) + .fetch_all(&mut *tx) .await?; - not_found_if_none(deleted_path, "Resource", &path)?; - // Delete linked variables that are actually referenced in the resource value - let deleted_linked_variables: Vec = if linked_var_paths.is_empty() { - Vec::new() - } else { - // Clean up any ws_specific rows for these variables first - // (mark_linked_variables_ws_specific may have auto-inserted them) so - // they don't survive the variable deletion as orphans — a variable - // later recreated at the same path would otherwise inherit the stale - // ws_specific flag. - sqlx::query!( - "DELETE FROM ws_specific - WHERE workspace_id = $1 AND item_kind = 'variable' AND path = ANY($2)", - w_id, - &linked_var_paths - ) - .execute(&mut *tx) - .await?; + let deleted_linked_variables = sqlx::query_scalar!( + "DELETE FROM variable WHERE workspace_id = $1 AND path = ANY($2) RETURNING path", + w_id, + &linked_var_paths + ) + .fetch_all(&mut *tx) + .await?; - let placeholders: Vec = linked_var_paths - .iter() - .enumerate() - .map(|(i, _)| format!("${}", i + 2)) - .collect(); - let query = format!( - "DELETE FROM variable WHERE workspace_id = $1 AND path IN ({}) RETURNING path", - placeholders.join(", ") - ); - let mut q = sqlx::query_scalar::<_, String>(&query).bind(&w_id); - for var_path in &linked_var_paths { - q = q.bind(var_path); - } - q.fetch_all(&mut *tx).await? - }; + // ws_specific has no FK to variable, so a row mark_linked_variables_ws_specific inserted + // would survive as an orphan and a variable later recreated at that path would inherit + // the stale flag. + sqlx::query!( + "DELETE FROM ws_specific + WHERE workspace_id = $1 AND item_kind = 'variable' AND path = ANY($2)", + w_id, + &deleted_linked_variables + ) + .execute(&mut *tx) + .await?; - if let Some(res_data) = trash_resource { - let mut trash_data = serde_json::json!({"row": res_data}); - if !trash_linked_vars.is_empty() { - trash_data["linked_variables"] = serde_json::Value::Array(trash_linked_vars); - } - windmill_common::trashbin::move_to_trash( - &mut *tx, - &w_id, - "resource", - path, - trash_data, - &authed.username, - ) - .await?; + // Only the rows that actually went: the snapshot above is what the caller could read, + // which is not necessarily what RLS let it delete, and the trashbin must not hold a copy + // of a secret that is still live. + let trash_linked_vars: Vec = trash_linked_vars + .into_iter() + .filter(|(var_path, _)| deleted_linked_variables.contains(var_path)) + .map(|(_, row)| row) + .collect(); + + let mut trash_data = serde_json::json!({"row": res_data}); + if !trash_linked_vars.is_empty() { + trash_data["linked_variables"] = serde_json::Value::Array(trash_linked_vars); } + windmill_common::trashbin::move_to_trash( + &mut *tx, + &w_id, + "resource", + path, + trash_data, + &authed.username, + ) + .await?; audit_log( &mut *tx, @@ -1464,6 +1431,24 @@ async fn delete_resource( None, ) .await?; + + // The cascade is the one way a variable dies without a variables/delete request of its + // own, so give each one the audit row it would have had, stamped with what took it. + for var_path in &deleted_linked_variables { + let mut params = HashMap::new(); + params.insert("via_resource", path); + audit_log( + &mut *tx, + &authed, + "variables.delete", + ActionKind::Delete, + &w_id, + Some(var_path), + Some(params), + ) + .await?; + } + tx.commit().await?; // Resource gone for everyone: wipe ALL users' drafts at this path (and any linked @@ -1515,35 +1500,205 @@ async fn delete_resource( ); } - Ok(format!("resource {} deleted", path)) + // Name what else went: the cascade is silent from the caller's side otherwise, and a + // secret it took is not something to discover later from a failing job. + if deleted_linked_variables.is_empty() { + Ok(format!("resource {} deleted", path)) + } else { + Ok(format!( + "resource {} deleted, along with its linked variables: {}", + path, + deleted_linked_variables.join(", ") + )) + } } -/// Recursively collect all `$var:path` references from a JSON value. -fn collect_var_refs(value: &serde_json::Value, out: &mut Vec) { +/// The forms that resolve a variable path against `variable`, so a value carrying any of them +/// breaks when that variable goes. Only `$var:` is minted by the resource editor, which is why +/// `collect_var_refs` stays narrower than this. +const REFERRER_PREFIXES: [&str; 2] = ["$var:", "$jsonvar:"]; + +/// Recursively collect the variable paths a JSON value references through any of `prefixes`. +fn collect_refs_with_prefixes(value: &serde_json::Value, prefixes: &[&str], out: &mut Vec) { match value { serde_json::Value::String(s) => { - if let Some(var_path) = s.strip_prefix("$var:") { + if let Some(var_path) = prefixes.iter().find_map(|p| s.strip_prefix(p)) { out.push(var_path.to_string()); } } serde_json::Value::Object(m) => { for v in m.values() { - collect_var_refs(v, out); + collect_refs_with_prefixes(v, prefixes, out); } } serde_json::Value::Array(arr) => { for v in arr { - collect_var_refs(v, out); + collect_refs_with_prefixes(v, prefixes, out); } } _ => {} } } -/// Deleting a resource cascades into the `$var:` variables its value references. A -/// scoped token must not use that cascade to delete variables it could not delete -/// directly via `delete_variable` (which gates on `variables:write:`), so require -/// `variables:write` for EVERY linked variable and fail the whole delete otherwise. +/// Recursively collect all `$var:path` references from a JSON value. +fn collect_var_refs(value: &serde_json::Value, out: &mut Vec) { + collect_refs_with_prefixes(value, &["$var:"], out) +} + +/// Whether the variable at `var_path` is the resource's own secret rather than one its value +/// merely points at: the same-path twin `delete_variable` and the `update_resource` rename +/// already act on, or `_`, which the connect form mints for a resource +/// type with several secret fields. Anything else is a standalone workspace variable, and +/// deleting one destroys a secret its other referrers still need. +/// +/// A rename moves only the twin, so `_` secrets stop matching and are left +/// behind instead. An orphaned secret can be deleted by hand; a destroyed one cannot. +fn is_owned_linked_var(resource_path: &str, var_path: &str) -> bool { + var_path == resource_path + || var_path + .strip_prefix(resource_path) + .is_some_and(|suffix| suffix.starts_with('_')) +} + +/// Which of `var_paths` a resource outside `excluded_resource_paths` still references. +/// +/// Must run off the non-RLS pool: a referrer in a folder the caller cannot read is precisely +/// the one whose variable has to survive. Nothing from those rows reaches the response. +async fn linked_vars_referenced_elsewhere( + db: &DB, + w_id: &str, + var_paths: &[String], + excluded_resource_paths: &[String], +) -> Result> { + if var_paths.is_empty() { + return Ok(HashSet::new()); + } + // The quotes around the pattern are what make it a whole-JSON-string match rather than a + // prefix one, so `f/db` does not match `"$var:f/db_replica"`. Paths are `proper_id` + // segments, so no JSON escaping or LIKE wildcard can reach this. + let referenced = sqlx::query_scalar!( + "WITH survivors AS ( + SELECT value::text AS rendered FROM resource + WHERE workspace_id = $1 AND NOT (path = ANY($2::text[])) + ) + SELECT v.path FROM unnest($3::text[]) AS v(path) + WHERE EXISTS ( + SELECT 1 FROM survivors s + WHERE strpos(s.rendered, '\"$var:' || v.path || '\"') > 0 + OR strpos(s.rendered, '\"$jsonvar:' || v.path || '\"') > 0 + )", + w_id, + excluded_resource_paths, + var_paths, + ) + .fetch_all(db) + .await?; + Ok(referenced.into_iter().flatten().collect()) +} + +/// A resource delete's candidate cascade, gathered before the caller's transaction opens: the +/// referrer scan runs on `db` because it has to see resources RLS hides, and a second acquire +/// from that same pool under an open `user_db` transaction stalls to the acquire timeout when +/// `DATABASE_CONNECTIONS` is small. `resolve` then decides without touching the database. +/// +/// So a resource that starts referencing a candidate between the scan and the delete keeps a +/// `$var:` pointing at nothing. Narrowing that window means running the scan on the +/// transaction's own connection under a tightly scoped `SET LOCAL ROLE NONE` (the elevation +/// `windmill-queue/src/schedule.rs` uses); closing it needs a lock on every resource write. +struct LinkedVarCascade { + /// Each owned `$var:` path with the requested resource whose value carries it. + candidates: Vec<(String, String)>, + /// Requested resource paths, each with every variable path its value references. + requested: Vec<(String, Vec)>, + /// Candidate paths a resource outside the requested set still references. + referenced_outside: HashSet, +} + +impl LinkedVarCascade { + /// The variables to delete, now that RLS has settled which resources went. + /// + /// A requested resource left standing is the one referrer the scan could not account for, + /// having had to exclude every requested path before RLS had ruled. + fn resolve(&self, deleted_paths: &[String]) -> Vec { + let referenced_by_survivors: HashSet<&str> = self + .requested + .iter() + .filter(|(path, _)| !deleted_paths.contains(path)) + .flat_map(|(_, refs)| refs.iter().map(String::as_str)) + .collect(); + + let mut resolved: Vec = self + .candidates + .iter() + .filter(|(var_path, owner)| { + deleted_paths.contains(owner) + && !self.referenced_outside.contains(var_path.as_str()) + && !referenced_by_survivors.contains(var_path.as_str()) + }) + .map(|(var_path, _)| var_path.clone()) + .collect(); + resolved.sort(); + resolved.dedup(); + resolved + } + + /// Which deleted resource the cascade took `var_path` for, to stamp on its audit row. + fn owner_of<'a>(&'a self, var_path: &str, deleted_paths: &[String]) -> Option<&'a str> { + self.candidates + .iter() + .find(|(candidate, owner)| candidate == var_path && deleted_paths.contains(owner)) + .map(|(_, owner)| owner.as_str()) + } +} + +/// Gather what `LinkedVarCascade::resolve` needs for a delete of `paths`. +async fn plan_linked_var_cascade( + db: &DB, + w_id: &str, + paths: &[String], +) -> Result { + let rows: Vec<(String, Option)> = sqlx::query_as( + "SELECT path, value FROM resource WHERE workspace_id = $1 AND path = ANY($2)", + ) + .bind(w_id) + .bind(paths) + .fetch_all(db) + .await?; + + let mut candidates: Vec<(String, String)> = Vec::new(); + let mut requested: Vec<(String, Vec)> = Vec::new(); + for (path, value) in rows { + let mut owned: Vec = Vec::new(); + let mut refs: Vec = Vec::new(); + if let Some(value) = &value { + collect_var_refs(value, &mut owned); + collect_refs_with_prefixes(value, &REFERRER_PREFIXES, &mut refs); + } + candidates.extend( + owned + .into_iter() + .filter(|var_path| is_owned_linked_var(&path, var_path)) + .map(|var_path| (var_path, path.clone())), + ); + requested.push((path, refs)); + } + candidates.sort(); + candidates.dedup(); + + let mut candidate_paths: Vec = candidates + .iter() + .map(|(var_path, _)| var_path.clone()) + .collect(); + candidate_paths.dedup(); + let referenced_outside = + linked_vars_referenced_elsewhere(db, w_id, &candidate_paths, paths).await?; + Ok(LinkedVarCascade { candidates, requested, referenced_outside }) +} + +/// Deleting a resource cascades into the `$var:` variables it owns. A scoped token must not +/// use that cascade to delete variables it could not delete directly via `delete_variable` +/// (which gates on `variables:write:`), so require `variables:write` for EVERY cascaded +/// variable and fail the whole delete otherwise. /// /// No co-located-path exemption: a resource and a variable may share a path, and a /// resource-write token can create a resource over an existing standalone variable and @@ -1646,111 +1801,91 @@ async fn delete_resources_bulk( return Err(Error::PermissionDenied(msg)); } + let cascade = plan_linked_var_cascade(&db, &w_id, &request.paths).await?; + let mut tx = user_db.begin(&authed).await?; - // Capture resources for trashbin per path before bulk delete, and - // collect $var: references so we can cascade-delete the linked variables - // (matching single-resource delete semantics). - let mut linked_var_paths: Vec = Vec::new(); - for path in &request.paths { - let trash_resource: Option = sqlx::query_scalar( - "SELECT to_jsonb(t) FROM resource t WHERE path = $1 AND workspace_id = $2", - ) - .bind(path) - .bind(&w_id) - .fetch_optional(&mut *tx) - .await?; - - if let Some(res_data) = trash_resource { - // Per-resource linked vars so each resource's trash entry carries - // exactly the variables that vanished with it (matching the - // single-delete shape: trash_data["linked_variables"]). - let mut this_linked: Vec = Vec::new(); - if let Some(value) = res_data.get("value") { - collect_var_refs(value, &mut this_linked); - } - this_linked.sort(); - this_linked.dedup(); - - let trash_linked_vars: Vec = if this_linked.is_empty() { - Vec::new() - } else { - let placeholders: Vec = this_linked - .iter() - .enumerate() - .map(|(i, _)| format!("${}", i + 2)) - .collect(); - let query = format!( - "SELECT to_jsonb(t) FROM variable t WHERE workspace_id = $1 AND path IN ({})", - placeholders.join(", ") - ); - let mut q = sqlx::query_scalar::<_, serde_json::Value>(&query).bind(&w_id); - for var_path in &this_linked { - q = q.bind(var_path); - } - q.fetch_all(&mut *tx).await? - }; - - let mut trash_data = serde_json::json!({"row": res_data}); - if !trash_linked_vars.is_empty() { - trash_data["linked_variables"] = serde_json::Value::Array(trash_linked_vars); - } - windmill_common::trashbin::move_to_trash( - &mut *tx, - &w_id, - "resource", - path, - trash_data, - &authed.username, - ) - .await?; - - linked_var_paths.extend(this_linked); - } - } - linked_var_paths.sort(); - linked_var_paths.dedup(); - - // A scoped token must not delete linked variables it lacks variables:write for. - check_linked_var_delete_scopes(&authed, &linked_var_paths)?; + // Whole rows out of the delete; see delete_resource. RLS can leave a requested resource + // standing, so everything below is driven by this list rather than by `request.paths`. + let deleted: Vec<(String, serde_json::Value)> = sqlx::query_as( + "DELETE FROM resource AS t WHERE t.path = ANY($1) AND t.workspace_id = $2 + RETURNING t.path, to_jsonb(t)", + ) + .bind(&request.paths) + .bind(&w_id) + .fetch_all(&mut *tx) + .await?; + let deleted_paths: Vec = deleted.iter().map(|(path, _)| path.clone()).collect(); sqlx::query!( "DELETE FROM ws_specific WHERE workspace_id = $1 AND item_kind = 'resource' AND path = ANY($2)", w_id, - &request.paths + &deleted_paths ) .execute(&mut *tx) .await?; - let deleted_paths = sqlx::query_scalar!( - "DELETE FROM resource WHERE path = ANY($1) AND workspace_id = $2 RETURNING path", - &request.paths, - w_id + let linked_var_paths = cascade.resolve(&deleted_paths); + + // A scoped token must not delete linked variables it lacks variables:write for. Erroring + // here rolls the resource deletes back with it. + check_linked_var_delete_scopes(&authed, &linked_var_paths)?; + + // Snapshot before the delete below: the trashbin entries need the rows. + let trash_linked_vars: Vec<(String, serde_json::Value)> = sqlx::query_as( + "SELECT path, to_jsonb(t) FROM variable t WHERE workspace_id = $1 AND path = ANY($2)", + ) + .bind(&w_id) + .bind(&linked_var_paths) + .fetch_all(&mut *tx) + .await?; + + let deleted_linked_variables = sqlx::query_scalar!( + "DELETE FROM variable WHERE workspace_id = $1 AND path = ANY($2) RETURNING path", + w_id, + &linked_var_paths ) .fetch_all(&mut *tx) .await?; - // Cascade-clean linked variables: delete any ws_specific 'variable' rows - // (typically auto-inserted by mark_linked_variables_ws_specific when the - // resource was ws_specific) BEFORE deleting the variable rows themselves - // — otherwise those ws_specific rows survive as orphans and a later - // variable created at the same path would inherit a stale flag. - if !linked_var_paths.is_empty() { - sqlx::query!( - "DELETE FROM ws_specific - WHERE workspace_id = $1 AND item_kind = 'variable' AND path = ANY($2)", - w_id, - &linked_var_paths - ) - .execute(&mut *tx) - .await?; + // See delete_resource: ws_specific has no FK, so the rows would orphan. + sqlx::query!( + "DELETE FROM ws_specific + WHERE workspace_id = $1 AND item_kind = 'variable' AND path = ANY($2)", + w_id, + &deleted_linked_variables + ) + .execute(&mut *tx) + .await?; - sqlx::query!( - "DELETE FROM variable WHERE workspace_id = $1 AND path = ANY($2)", - w_id, - &linked_var_paths + for (path, res_data) in &deleted { + // Every cascaded variable this resource's value points at, ownership aside: restoring + // it on its own must bring back each secret it needs, and the one it borrowed from a + // sibling in the same batch is gone too. + let mut refs: Vec = Vec::new(); + if let Some(value) = res_data.get("value") { + collect_var_refs(value, &mut refs); + } + let this_linked: Vec = trash_linked_vars + .iter() + .filter(|(var_path, _)| { + refs.contains(var_path) && deleted_linked_variables.contains(var_path) + }) + .map(|(_, row)| row.clone()) + .collect(); + + let mut trash_data = serde_json::json!({"row": res_data}); + if !this_linked.is_empty() { + trash_data["linked_variables"] = serde_json::Value::Array(this_linked); + } + windmill_common::trashbin::move_to_trash( + &mut *tx, + &w_id, + "resource", + path, + trash_data, + &authed.username, ) - .execute(&mut *tx) .await?; } @@ -1765,13 +1900,30 @@ async fn delete_resources_bulk( ) .await?; + // See delete_resource: a cascaded variable gets the audit row it would have had. + for var_path in &deleted_linked_variables { + let params = cascade + .owner_of(var_path, &deleted_paths) + .map(|resource_path| HashMap::from([("via_resource", resource_path)])); + audit_log( + &mut *tx, + &authed, + "variables.delete", + ActionKind::Delete, + &w_id, + Some(var_path), + params, + ) + .await?; + } + tx.commit().await?; // Wipe ALL users' drafts at these paths (and linked variables); see delete_resource. for path in &deleted_paths { delete_all_drafts_for_path(&db, &w_id, UserDraftItemKind::Resource, path).await?; } - for var_path in &linked_var_paths { + for var_path in &deleted_linked_variables { delete_all_drafts_for_path(&db, &w_id, UserDraftItemKind::Variable, var_path).await?; } diff --git a/frontend/src/routes/(root)/(logged)/resources/+page.svelte b/frontend/src/routes/(root)/(logged)/resources/+page.svelte index a5372b91bb..f21598fd0b 100644 --- a/frontend/src/routes/(root)/(logged)/resources/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/resources/+page.svelte @@ -343,7 +343,9 @@ if (account) { OauthService.disconnectAccount({ workspace: $workspaceStore!, id: account }) } - await ResourceService.deleteResource({ workspace: $workspaceStore!, path }) + // The response names the linked variables that went with it, which nothing else on the + // page would show. + sendUserToast(await ResourceService.deleteResource({ workspace: $workspaceStore!, path })) reload() } @@ -762,8 +764,8 @@ > {#if deleteIsLinked} - This resource is linked with a variable of the same path. The linked variable will also be - deleted. + This resource is linked with a variable of the same path. That variable is deleted with it, + unless another resource still references it. {/if}