From cbbcd9729fad57a019b7ca73495ca117a0ef4a1a Mon Sep 17 00:00:00 2001 From: hugocasa Date: Mon, 31 Aug 2026 13:18:10 +0200 Subject: [PATCH] fix: validate the default origins on the declarative settings path --- backend/tests/instance_config.rs | 54 +++++++++++++++++++ .../windmill-api/src/triggers/http/handler.rs | 12 +++-- .../windmill-common/src/instance_config.rs | 12 ++++- 3 files changed, 71 insertions(+), 7 deletions(-) diff --git a/backend/tests/instance_config.rs b/backend/tests/instance_config.rs index ddd94ea079..36f0c9b1c6 100644 --- a/backend/tests/instance_config.rs +++ b/backend/tests/instance_config.rs @@ -1436,6 +1436,60 @@ async fn test_no_alert_in_config_table_after_migration(db: Pool) { /// validating `github_app_webhook_base_url` itself. A rejected value must not be /// half-applied: nothing at all may be written, or an unreachable receiver would be /// persisted and only surface much later as a repository falling back to polling. +#[sqlx::test(fixtures("base"))] +async fn declarative_sync_rejects_an_unusable_default_allowed_origins(db: Pool) { + // The declarative writers (the sync-config CLI, the operator's ConfigMap + // sync) do not run the HTTP layer's pre-write hook, so an origin list that + // cannot be parsed would persist here, be dropped at boot, and leave the + // instance with no restriction at all. + clear_settings_and_configs(&db).await; + let before = count_global_settings(&db).await; + + for bad in [ + serde_json::json!([""]), + serde_json::json!(["https://a.example,https://b.example"]), + serde_json::json!("null"), + ] { + let mut desired = BTreeMap::new(); + desired.insert( + "http_route_default_allowed_origins".to_string(), + bad.clone(), + ); + let err = windmill_common::instance_config::sync_global_settings_declarative( + &db, + &BTreeMap::new(), + &desired, + ) + .await + .expect_err(&format!("{bad} must fail the sync")); + assert!( + err.to_string() + .contains("http_route_default_allowed_origins"), + "the error should name the offending setting, got: {err}" + ); + } + + assert_eq!( + count_global_settings(&db).await, + before, + "a rejected sync must not have persisted anything" + ); + + // A usable list still syncs. + let mut desired = BTreeMap::new(); + desired.insert( + "http_route_default_allowed_origins".to_string(), + serde_json::json!(["https://app.example.com"]), + ); + windmill_common::instance_config::sync_global_settings_declarative( + &db, + &BTreeMap::new(), + &desired, + ) + .await + .expect("a valid origin list must sync"); +} + #[sqlx::test(fixtures("base"))] async fn declarative_sync_rejects_an_unusable_webhook_base_url(db: Pool) { clear_settings_and_configs(&db).await; diff --git a/backend/windmill-api/src/triggers/http/handler.rs b/backend/windmill-api/src/triggers/http/handler.rs index d19c1e6762..cc85e473cf 100644 --- a/backend/windmill-api/src/triggers/http/handler.rs +++ b/backend/windmill-api/src/triggers/http/handler.rs @@ -137,8 +137,9 @@ impl ResolvedCorsPolicy { } /// Decide the CORS answer from the routers cache, for a request no handler -/// published a verdict for: a preflight, an unknown path, or a failure before -/// the trigger lookup. +/// published a verdict for: a preflight, an unknown path, or any failure ahead +/// of the publish — authentication included, which runs after the route itself +/// resolves. /// /// Loads the routers when the cache is cold, the way `get_http_route_trigger` /// does, so a preflight is answered from the same view of the routes as the @@ -203,9 +204,10 @@ async fn conditional_cors_middleware( // the policy this response was produced with, so nothing else can be // more authoritative. Some(decision) => decision, - // No trigger was resolved: a preflight, an unknown path, or a request - // that failed before the lookup. No runnable produced this body, so - // reading the cache now cannot contradict anything. + // No verdict was published: a preflight, an unknown path, or a request + // that failed before reaching the publish, authentication included. No + // runnable produced this body, so reading the cache cannot contradict + // anything. None => match lookup { Some((method, path)) => { resolve_cors_decision(&db, method, &path, origin.as_ref()).await diff --git a/backend/windmill-common/src/instance_config.rs b/backend/windmill-common/src/instance_config.rs index 7ea6dbe5ae..5c5c4b081c 100644 --- a/backend/windmill-common/src/instance_config.rs +++ b/backend/windmill-common/src/instance_config.rs @@ -1280,8 +1280,9 @@ pub fn diff_worker_configs( ConfigsDiff { upserts, deletes } } -/// Declaratively replace the global settings, rejecting a `github_app_webhook_base_url` -/// the API would reject. +/// Declaratively replace the global settings, rejecting a +/// `github_app_webhook_base_url` or `http_route_default_allowed_origins` the +/// API would reject. /// /// Every declarative writer (the `sync-config` CLI, the Kubernetes operator's /// ConfigMap sync) MUST go through this rather than calling @@ -1330,6 +1331,13 @@ pub async fn sync_global_settings_declarative( } } + // An origin list that cannot be parsed is dropped at boot, leaving the + // empty default — which is no restriction at all. Rejecting it here is what + // keeps a typo in a ConfigMap from silently widening CORS instance-wide. + let origins_key = crate::global_settings::HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING; + crate::global_settings::parse_allowed_origins_setting(desired.get(origins_key)) + .map_err(|e| anyhow::anyhow!("{origins_key}: {e}"))?; + let diff = diff_global_settings(current, desired, ApplyMode::Replace); apply_settings_diff(db, &diff).await?;