From 4fff89f98ce72997a055cc313c8fe217d2f1fe78 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 10 Apr 2026 02:00:03 -0400 Subject: [PATCH] fix: hide legacy global_settings.worker_configs ghost row (#8790) Co-authored-by: Claude Opus 4.6 (1M context) --- backend/tests/instance_config.rs | 73 +++++++++++++++++++ .../windmill-common/src/instance_config.rs | 22 +++++- .../lib/components/InstanceSettings.svelte | 8 +- 3 files changed, 100 insertions(+), 3 deletions(-) diff --git a/backend/tests/instance_config.rs b/backend/tests/instance_config.rs index 055b633d28..49ab424b72 100644 --- a/backend/tests/instance_config.rs +++ b/backend/tests/instance_config.rs @@ -172,6 +172,79 @@ async fn test_from_db_unknown_settings_go_to_extra(db: Pool) { ); } +/// Regression: a legacy `global_settings.worker_configs` row must not leak +/// into `GlobalSettings::extra`. Older Windmill versions stored worker configs +/// as a single blob in `global_settings`; the row could silently drift from +/// the real `config WHERE name LIKE 'worker__%'` rows and get resurrected on +/// every bulk InstanceSettings save via the flatten+extra round-trip. +#[sqlx::test(fixtures("base"))] +async fn test_from_db_hides_legacy_worker_configs_ghost_row(db: Pool) { + clear_settings_and_configs(&db).await; + + insert_global_setting( + &db, + "worker_configs", + serde_json::json!({ + "default": {"worker_tags": ["deno", "python3"]}, + "sldc-standard": {"worker_tags": ["sldc-standard"]} + }), + ) + .await; + insert_config(&db, "worker__sldc-standard", serde_json::json!({})).await; + + let config = InstanceConfig::from_db(&db).await.unwrap(); + assert!( + !config.global_settings.extra.contains_key("worker_configs"), + "legacy worker_configs row must be filtered out of GlobalSettings::extra" + ); + assert!( + !config + .global_settings + .to_settings_map() + .contains_key("worker_configs"), + "legacy worker_configs row must not re-appear in to_settings_map()" + ); + // The real config-table row is still read normally. + assert!(config.worker_configs.contains_key("sldc-standard")); +} + +/// Regression: a bulk `diff_global_settings` upsert must reject a +/// `worker_configs` key, even if a client PUT carries one in the flattened +/// extra map. This is the write-side guard that prevents the ghost row from +/// being resurrected on every InstanceSettings YAML save. +#[sqlx::test(fixtures("base"))] +async fn test_diff_global_settings_rejects_worker_configs_upsert(db: Pool) { + clear_settings_and_configs(&db).await; + + let current: BTreeMap = BTreeMap::new(); + let mut desired: BTreeMap = BTreeMap::new(); + desired.insert( + "base_url".to_string(), + serde_json::json!("https://windmill.test"), + ); + desired.insert( + "worker_configs".to_string(), + serde_json::json!({"default": {"worker_tags": ["deno"]}}), + ); + + let diff = diff_global_settings(¤t, &desired, ApplyMode::Merge); + assert!(diff.upserts.contains_key("base_url")); + assert!( + !diff.upserts.contains_key("worker_configs"), + "worker_configs must never be written as a global setting" + ); + + apply_settings_diff(&db, &diff).await.unwrap(); + assert!( + get_global_setting(&db, "worker_configs").await.is_none(), + "worker_configs row must not exist in global_settings after apply" + ); + assert_eq!( + get_global_setting(&db, "base_url").await, + Some(serde_json::json!("https://windmill.test")) + ); +} + #[sqlx::test(fixtures("base"))] async fn test_from_db_with_worker_configs(db: Pool) { clear_settings_and_configs(&db).await; diff --git a/backend/windmill-common/src/instance_config.rs b/backend/windmill-common/src/instance_config.rs index 46224c29ec..c183536347 100644 --- a/backend/windmill-common/src/instance_config.rs +++ b/backend/windmill-common/src/instance_config.rs @@ -874,6 +874,12 @@ pub const HIDDEN_SETTINGS: &[&str] = &[ "min_keep_alive_version", "automate_username_creation", "_restart_coordination", + // Legacy ghost: worker configs live in the `config` table with a + // `worker__` prefix, not as a single blob in `global_settings`. Older + // Windmill versions stored them here and the row would be resurrected on + // every bulk InstanceSettings save via `GlobalSettings::extra`. Hiding it + // on read + rejecting it in `diff_global_settings` breaks that loop. + "worker_configs", ]; /// Top-level settings whose entire value is sensitive and must be fully redacted in logs. @@ -899,7 +905,10 @@ const SENSITIVE_SETTINGS: &[&str] = &[ /// Maps a top-level key to the sub-field names that must be redacted. const NESTED_SENSITIVE_FIELDS: &[(&str, &[&str])] = &[ ("smtp_settings", &["smtp_password"]), - ("secret_backend", &["token", "client_secret", "secret_access_key"]), + ( + "secret_backend", + &["token", "client_secret", "secret_access_key"], + ), ( "object_store_cache_config", &["secret_key", "serviceAccountKey"], @@ -1045,6 +1054,17 @@ pub fn diff_global_settings( let mut previous_values = BTreeMap::new(); let mut unchanged_count: usize = 0; for (key, desired_value) in desired { + // `worker_configs` is a legacy ghost: worker configs belong in the + // `config` table with a `worker__` prefix. If a client PUT carries a + // top-level `worker_configs` key (it flattens into + // `GlobalSettings::extra` on deserialize), drop it here instead of + // letting it resurrect a stale `global_settings` row. + if key == "worker_configs" { + tracing::warn!( + "Ignoring 'worker_configs' in global_settings diff: worker configs must be written to the config table (worker__ prefix), not global_settings" + ); + continue; + } if PROTECTED_SETTINGS.contains(&key.as_str()) && is_empty_or_null(desired_value) && current.contains_key(key) diff --git a/frontend/src/lib/components/InstanceSettings.svelte b/frontend/src/lib/components/InstanceSettings.svelte index 909fcbf13f..731ff161ae 100644 --- a/frontend/src/lib/components/InstanceSettings.svelte +++ b/frontend/src/lib/components/InstanceSettings.svelte @@ -657,8 +657,12 @@ 'workspace_registries' ]) - // Settings that should never appear in YAML export/import - const excludedKeys: Set = new Set([]) + // Settings that should never appear in YAML export/import. + // `worker_configs` is a legacy ghost key: worker configs live in the `config` + // table (managed from /workers), not in `global_settings`. Older DBs may + // still carry a stale `global_settings.worker_configs` row; filter it here + // so it never round-trips through this editor. + const excludedKeys: Set = new Set(['worker_configs']) // Nested fields inside object-valued settings that contain secrets. // Each entry maps a top-level key to its sensitive sub-field names.