diff --git a/backend/migrations/20260610095349_draft_workspace_path_typ_index.down.sql b/backend/migrations/20260610095349_draft_workspace_path_typ_index.down.sql new file mode 100644 index 0000000000..d38d96b61d --- /dev/null +++ b/backend/migrations/20260610095349_draft_workspace_path_typ_index.down.sql @@ -0,0 +1 @@ +DROP INDEX draft_workspace_path_typ_idx; diff --git a/backend/migrations/20260610095349_draft_workspace_path_typ_index.up.sql b/backend/migrations/20260610095349_draft_workspace_path_typ_index.up.sql new file mode 100644 index 0000000000..e32b966bc6 --- /dev/null +++ b/backend/migrations/20260610095349_draft_workspace_path_typ_index.up.sql @@ -0,0 +1,11 @@ +-- Hot path: `fetch_other_drafts_users` runs on EVERY get-by-path request +-- (scripts, flows, apps, variables, resources, schedules, triggers) with +-- `WHERE workspace_id = ? AND path = ? AND typ = ?` and no email +-- predicate. Neither partial unique index (`draft_pkey_with_user` / +-- `draft_pkey_legacy`) can serve it — their `email IS [NOT] NULL` +-- predicates aren't implied by the query — and `draft_user_sync_idx` is +-- partial too, so the planner fell back to a sequential scan over a +-- table that accumulates per-user autosaves across all workspaces. +-- A plain btree over the three columns also covers `get_draft_for_user` +-- (same three + `email IS NOT DISTINCT FROM ?` as a filter). +CREATE INDEX draft_workspace_path_typ_idx ON draft (workspace_id, path, typ); diff --git a/backend/migrations/20260610100018_scrub_secret_variable_drafts.down.sql b/backend/migrations/20260610100018_scrub_secret_variable_drafts.down.sql new file mode 100644 index 0000000000..a77b36d2b6 --- /dev/null +++ b/backend/migrations/20260610100018_scrub_secret_variable_drafts.down.sql @@ -0,0 +1,2 @@ +-- Irreversible: the plaintext secret values were deliberately destroyed. +SELECT 1; diff --git a/backend/migrations/20260610100018_scrub_secret_variable_drafts.up.sql b/backend/migrations/20260610100018_scrub_secret_variable_drafts.up.sql new file mode 100644 index 0000000000..3a2ce23840 --- /dev/null +++ b/backend/migrations/20260610100018_scrub_secret_variable_drafts.up.sql @@ -0,0 +1,10 @@ +-- Secret variable values used to be autosaved into `draft.value` in +-- plaintext (deployed secrets are encrypted with the workspace crypt key +-- precisely so DB dumps don't leak them). `save_draft` now blanks +-- `variable.value` for `is_secret: true` drafts at write time; this +-- scrubs the rows persisted before that guard existed. +UPDATE draft +SET value = jsonb_set(value::jsonb, '{variable,value}', '""'::jsonb)::json +WHERE typ = 'variable' + AND (value::jsonb -> 'variable' ->> 'is_secret')::boolean IS TRUE + AND value::jsonb -> 'variable' ? 'value'; diff --git a/backend/windmill-api/src/drafts.rs b/backend/windmill-api/src/drafts.rs index f539563722..651728d413 100644 --- a/backend/windmill-api/src/drafts.rs +++ b/backend/windmill-api/src/drafts.rs @@ -75,8 +75,20 @@ async fn save_draft( ) -> Result> { let email = &authed.email; let path = path.to_path(); + require_can_write_path(&authed, &db, &w_id, path).await?; let applied_at = if let Some(value) = &req.value { + // Secret variable values must never sit in the draft table in + // plaintext — deployed secrets are encrypted with the workspace + // crypt key precisely so DB dumps don't leak them, and the + // variable editor never round-trips secret values back anyway + // (it fetches with decrypt_secret=false; the field starts empty + // on every edit). Strip the typed value before persisting. + let serialized = if kind == UserDraftItemKind::Variable { + scrub_secret_variable_value(value.0.get()) + } else { + serde_json::to_string(value).unwrap() + }; // Upsert branch. Conflict check rides on a WHERE clause attached // to DO UPDATE — when the existing row is newer than `last_sync`, // the statement is a no-op and RETURNING yields nothing. @@ -93,7 +105,7 @@ async fn save_draft( email, path, kind as UserDraftItemKind, - serde_json::to_string(value).unwrap(), + serialized, req.last_sync, req.force, ) @@ -163,6 +175,28 @@ async fn save_draft( } } +/// For variable-kind drafts: when the JSON says `variable.is_secret == +/// true`, blank `variable.value` so the typed secret never persists in +/// plaintext at rest. Unexpected shapes pass through unchanged — the +/// draft store is schema-less by design and a malformed draft is the +/// editor's problem, not a save error. +fn scrub_secret_variable_value(raw: &str) -> String { + let Ok(mut v) = serde_json::from_str::(raw) else { + return raw.to_string(); + }; + let is_secret = v + .get("variable") + .and_then(|x| x.get("is_secret")) + .and_then(|x| x.as_bool()) + .unwrap_or(false); + if is_secret { + if let Some(val) = v.get_mut("variable").and_then(|x| x.get_mut("value")) { + *val = serde_json::Value::String(String::new()); + } + } + v.to_string() +} + #[derive(Deserialize, Debug)] pub struct GetDraftQuery { /// Workspace username of the draft owner to fetch. Omit to fetch the @@ -271,6 +305,61 @@ fn table_for_kind(kind: UserDraftItemKind) -> Option<&'static str> { } } +/// Resolves to `Ok(())` if `authed` may SAVE a draft at `path`: +/// - admins always +/// - the user's own namespace (`u/{username}`) +/// - group namespace (`g/{group}`) when the user is in the group +/// - folders (`f/{folder}`) when the user has WRITE (or owns) the folder +/// +/// Drafts can exist at paths with no deployed item (draft-only), so there +/// is no row to lean on for item-level extra_perms — the namespace rules +/// are the whole check. Without this, any workspace member could plant +/// drafts in another user's `u/` namespace or in folders they can't +/// write, and those drafts get surfaced to every reader of the path +/// (home-page circles, others'-drafts modal, View JSON / Fork). +/// +/// JWT folder claims can lag behind fresh grants — refresh them the same +/// way the deploy endpoints do before concluding "no write". +async fn require_can_write_path(authed: &ApiAuthed, db: &DB, w_id: &str, path: &str) -> Result<()> { + if authed.is_admin { + return Ok(()); + } + // Operators are read-only users — they're excluded from every draft + // surface (list synthesis, badges) and must not write drafts either. + if authed.is_operator { + return Err(Error::NotAuthorized( + "operators cannot save drafts".to_string(), + )); + } + let parts: Vec<&str> = path.splitn(3, '/').collect(); + if parts.len() >= 3 { + match parts[0] { + "u" if parts[1] == authed.username => return Ok(()), + "g" if authed.groups.iter().any(|g| g == parts[1]) => return Ok(()), + "f" => { + let folder = parts[1]; + let has_write = |a: &ApiAuthed| { + a.folders + .iter() + .any(|(name, write, owner)| name == folder && (*write || *owner)) + }; + if has_write(authed) { + return Ok(()); + } + let refreshed = + windmill_api_auth::maybe_refresh_folders(path, w_id, authed.clone(), db).await; + if has_write(&refreshed) { + return Ok(()); + } + } + _ => {} + } + } + Err(Error::NotAuthorized(format!( + "you don't have write permission on {path}" + ))) +} + /// Resolves to `Ok(())` if `authed` can read at `path`. Three layers, in /// order of cheapness: /// 1. admin → always.