fix(drafts): P1 hardening — save authz, secret scrubbing, hot-path index

1. save_draft had no authorization check (a regression from the old
   create_draft's require_writer_of_path): any workspace member could
   plant drafts in another user's u/ namespace or unwritable folders,
   and those drafts get surfaced to every reader of the path (home
   circles, others'-drafts modal, View JSON / Fork). New
   require_can_write_path: admins; own u/ namespace; g/ namespace when
   in the group; f/ folders with the write/owner bit (with the same
   folder-claim refresh deploy endpoints use). Operators are rejected
   outright — they're excluded from every other draft surface.

2. Secret variable values were persisted in the draft table in
   plaintext. save_draft now blanks variable.value for is_secret drafts
   at write time (the editor never round-trips secret values anyway —
   it fetches with decrypt_secret=false), and a migration scrubs rows
   persisted before the guard.

3. fetch_other_drafts_users runs on every get-by-path request with
   (workspace_id, path, typ) and no email predicate — neither partial
   unique index covers it, so it seq-scanned a table that accumulates
   per-user autosaves across all workspaces. Add a plain btree index;
   it also serves get_draft_for_user's IS NOT DISTINCT FROM lookup.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Diego Imbert
2026-06-10 12:02:50 +02:00
parent 02924d48a3
commit f91c7217be
5 changed files with 114 additions and 1 deletions
@@ -0,0 +1 @@
DROP INDEX draft_workspace_path_typ_idx;
@@ -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);
@@ -0,0 +1,2 @@
-- Irreversible: the plaintext secret values were deliberately destroyed.
SELECT 1;
@@ -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';
+90 -1
View File
@@ -75,8 +75,20 @@ async fn save_draft(
) -> Result<Json<SaveDraftResponse>> {
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::<serde_json::Value>(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.