From 8276ae09fa0c19b3bb4498b55e58f6c2cad614b5 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 8 Sep 2026 13:46:54 +0200 Subject: [PATCH] fix(datatables): fail loudly where a role or a pointer can be left half-recorded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways the feature could end up in a state nobody could see or undo. Creating a role writes the cluster first and the catalog second, but the catalog write was an `UPDATE` that matched nothing when the instance Postgres settings row was absent — leaving a live login with a password nobody recorded: invisible to the catalog, un-recreatable because the name is taken, and un-deletable because there is no entry to delete. It now errors, so the operation is retryable once the row is restored. Deleting a workspace only nulls the fork lineage; the data table entries pointing at it are left resolving to nothing. Sweeping them is not an option — turning a pointer back into a copy would hand each fork the database outright — so the delete now names the data tables it stranded, and resolving one says which workspace is missing rather than reporting a data table this workspace never had. `InstanceDatatableRole` derived `Debug` while holding a Postgres password; it is now hand-written so `{:?}` on the catalog cannot put a live credential in a log line. Adds the two branches the reviews found unpinned: a caller who is not a member of the governing workspace at all, and `NoIdentity` — the compatibility path for an agent worker that predates this and sends no job id. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR --- ...19ebd65b80e7ed9f29b9fb2e03767c2aa94ba.json | 28 ++++++++ .../tests/datatable_roles.rs | 69 +++++++++++++++++++ backend/windmill-api-settings/src/lib.rs | 17 ++++- .../src/workspaces_extra.rs | 30 +++++++- .../windmill-common/src/datatable_roles.rs | 14 +++- backend/windmill-common/src/workspaces.rs | 18 ++++- 6 files changed, 171 insertions(+), 5 deletions(-) create mode 100644 backend/.sqlx/query-c5451ea9d9fa5146af242d1ee8c19ebd65b80e7ed9f29b9fb2e03767c2aa94ba.json diff --git a/backend/.sqlx/query-c5451ea9d9fa5146af242d1ee8c19ebd65b80e7ed9f29b9fb2e03767c2aa94ba.json b/backend/.sqlx/query-c5451ea9d9fa5146af242d1ee8c19ebd65b80e7ed9f29b9fb2e03767c2aa94ba.json new file mode 100644 index 0000000000..5d8ca8dc00 --- /dev/null +++ b/backend/.sqlx/query-c5451ea9d9fa5146af242d1ee8c19ebd65b80e7ed9f29b9fb2e03767c2aa94ba.json @@ -0,0 +1,28 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"datatable!\"\n FROM workspace_settings ws\n CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt\n WHERE dt.value->'reference'->>'workspace_id' = $1\n ORDER BY ws.workspace_id, dt.key", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id!", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "datatable!", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false, + null + ] + }, + "hash": "c5451ea9d9fa5146af242d1ee8c19ebd65b80e7ed9f29b9fb2e03767c2aa94ba" +} diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 0b0ca1f71c..7ec93dd159 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -260,3 +260,72 @@ async fn a_fork_renaming_its_own_entry_leaves_the_governing_bookkeeping_alone( assert_eq!(left, 1, "the fork's delete reached the parent's migrations"); Ok(()) } + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_caller_who_is_not_a_member_of_the_governing_workspace_reaches_nothing( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + // A fork member who was never added to the parent. Their fork membership says nothing there, + // and the email lookup that would evaluate them as a member of it finds no row. + sqlx::query( + "INSERT INTO usr (workspace_id, email, username, is_admin, role) + VALUES ('wm-fork-dt', 'test3@windmill.dev', 'test-user-3', false, 'User')", + ) + .execute(&db) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let resp = authed( + client().get(format!( + "http://localhost:{port}/api/w/wm-fork-dt/workspaces/datatable_usable_roles/main" + )), + "SECRET_TOKEN_3", + ) + .send() + .await?; + assert_eq!(resp.status(), 200); + let body: Value = resp.json().await?; + assert_eq!(body["roles"], json!([]), "{body}"); + Ok(()) +} + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_caller_with_no_identity_reaches_a_permissioned_data_table_not_at_all( + db: Pool, +) -> anyhow::Result<()> { + use windmill_common::workspaces::{get_datatable_resource_from_db, DatatableAccess}; + + initialize_tracing().await; + // The compatibility story for an agent worker that predates data table roles and sends no job + // id: it keeps resolving an unpermissioned data table, and is refused on a permissioned one + // rather than handed an unattributed admin connection. + let refused = get_datatable_resource_from_db( + &db, + "test-workspace", + "main", + None, + DatatableAccess::NoIdentity, + ) + .await; + assert!(refused.is_err(), "an unidentified caller was let in"); + + sqlx::query( + "UPDATE workspace_settings + SET datatable = datatable #- '{datatables,main,permissions}' + WHERE workspace_id = 'test-workspace'", + ) + .execute(&db) + .await?; + let resolved = get_datatable_resource_from_db( + &db, + "test-workspace", + "main", + None, + DatatableAccess::NoIdentity, + ) + .await?; + assert_eq!(resolved["dbname"], "dt_main", "{resolved}"); + Ok(()) +} diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index a4a0ec11c3..6a87e2f9e1 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -2635,18 +2635,31 @@ fn datatable_role_infos( /// Persist the catalog next to the instance Postgres password, in the same `global_settings` row /// the instance database registry lives in. +/// +/// Errors when it matches nothing. The cluster is written first, so a silent no-op here would +/// leave a live Postgres login with a password nobody recorded: invisible to the catalog, +/// un-recreatable (the name is taken) and un-deletable (there is no entry to delete). The row is +/// normally planted by the boot converge, but that swallows its own failures, so this is a check +/// rather than an assumption. async fn write_role_catalog( db: &DB, catalog: &windmill_common::datatable_roles::DatatableRoleCatalog, ) -> error::Result<()> { let value = serde_json::to_value(catalog).map_err(to_anyhow)?; - sqlx::query!( + let written = sqlx::query!( "UPDATE global_settings SET value = jsonb_set(COALESCE(value, '{}'::jsonb), '{roles}', $1) WHERE name = 'custom_instance_pg_databases'", value ) .execute(db) - .await?; + .await? + .rows_affected(); + if written == 0 { + return Err(error::Error::internal_err( + "The instance Postgres settings row is missing, so the data table role catalog could not be recorded. Refresh the custom instance user password in instance settings to recreate it, then try again." + .to_string(), + )); + } Ok(()) } diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index 4da6d24624..61b4cc39be 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -995,6 +995,22 @@ pub(crate) async fn delete_workspace( // but the destructive cleanup itself runs only after the commit below: a delete that // fails mid-way must never leave a live workspace with its fork data destroyed and no // registry row to retry from. Read-only: nothing is dropped here. + // Read before the delete: another workspace's data table entry can point at one of this + // workspace's, and deleting the workspace it names leaves that pointer resolving to nothing. + // Nothing sweeps them — turning them back into copies would hand each fork the database + // outright — so the deleter is told which data tables they just stranded. + let stranded_pointers = sqlx::query!( + r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!" + FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt + WHERE dt.value->'reference'->>'workspace_id' = $1 + ORDER BY ws.workspace_id, dt.key"#, + &w_id, + ) + .fetch_all(&db) + .await + .unwrap_or_default(); + let fork_ducklake_cleanups = prepare_fork_ducklake_cleanups(&db, &w_id, None) .await .unwrap_or_else(|e| { @@ -1313,7 +1329,19 @@ pub(crate) async fn delete_workspace( tracing::warn!("failed to broadcast fork lineage change: {e:#}"); } - Ok(format!("Deleted workspace {}", &w_id)) + if stranded_pointers.is_empty() { + Ok(format!("Deleted workspace {}", &w_id)) + } else { + let stranded = stranded_pointers + .iter() + .map(|r| format!("{}/{}", r.workspace_id, r.datatable)) + .collect::>() + .join(", "); + Ok(format!( + "Deleted workspace {}. These data tables were governed by it and no longer resolve: {}. Their databases still exist; a superadmin can point them at another workspace's data table.", + &w_id, stranded + )) + } } #[derive(Deserialize)] diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 9efa05eca9..1a2f0df96b 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -36,7 +36,7 @@ pub const CUSTOM_INSTANCE_USER: &str = "custom_instance_user"; /// One catalog entry. The password is per role and instance-wide; it lives here rather than in any /// workspace's settings, next to the `custom_instance_user` password in the same /// `custom_instance_pg_databases` row. -#[derive(Deserialize, Serialize, Clone, Debug)] +#[derive(Deserialize, Serialize, Clone)] #[cfg_attr(feature = "instance_config_schema", derive(schemars::JsonSchema))] pub struct InstanceDatatableRole { /// The Postgres role name, verbatim. @@ -49,6 +49,18 @@ pub struct InstanceDatatableRole { pub pwd: Option, } +/// Hand-written so `{:?}` on a catalog cannot put a live Postgres password in a log line or an +/// audit record. Everything else about the entry is safe to print. +impl std::fmt::Debug for InstanceDatatableRole { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("InstanceDatatableRole") + .field("name", &self.name) + .field("enabled", &self.enabled) + .field("pwd", &self.pwd.as_ref().map(|_| "")) + .finish() + } +} + pub type DatatableRoleCatalog = BTreeMap; /// Names Postgres or Windmill already owns. `admin` is excluded because it never reaches the diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 0dcddf5600..2d5c6a4901 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1477,8 +1477,24 @@ pub async fn resolve_governing_datatable( ) -> Result { let mut workspace_id = w_id.to_string(); let mut name = name.to_string(); + let mut hops = 0; for _ in 0..DATATABLE_REFERENCE_MAX_DEPTH { - let datatable = read_datatable_entry(db, &workspace_id, &name).await?; + let datatable = read_datatable_entry(db, &workspace_id, &name) + .await + .map_err(|e| { + if hops == 0 { + e + } else { + // A pointer outlives the workspace it names: deleting one only nulls the fork + // lineage, it does not sweep the entries that pointed at it. Say which one is + // gone rather than reporting a data table this workspace never had. + Error::NotFound(format!( + "Data table '{name}' of workspace '{workspace_id}' governs this one and no \ + longer exists. A superadmin can point this data table somewhere else." + )) + } + })?; + hops += 1; validate_datatable_shape(&name, &datatable)?; match &datatable.reference { None => return Ok(GoverningDatatable { workspace_id, name, datatable }),