fix: validate the default origins on the declarative settings path

This commit is contained in:
hugocasa
2026-08-31 13:18:10 +02:00
parent 4ca68dbc93
commit cbbcd9729f
3 changed files with 71 additions and 7 deletions
+54
View File
@@ -1436,6 +1436,60 @@ async fn test_no_alert_in_config_table_after_migration(db: Pool<Postgres>) {
/// 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<Postgres>) {
// 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<Postgres>) {
clear_settings_and_configs(&db).await;
@@ -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
+10 -2
View File
@@ -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?;