fix: address review nits on origin validation and the CORS editor

backend/src/monitor.rs

@@ -355,9 +355,7 @@ pub async fn initial_load(
                     )
                 }
             });
-            pass.action(windmill_common::min_version::store_min_keep_alive_version(
-                db,
-            ));
+            pass.action(windmill_common::min_version::store_min_keep_alive_version(db));
             pass.setting(
                 windmill_common::global_settings::INSTANCE_EVENTS_WEBHOOK_SETTING,
                 false,
@@ -702,6 +700,7 @@ pub async fn initial_load(
     pass.run(conn).await;
 }
 
+
 pub fn apply_metrics_enabled(value: Option<serde_json::Value>) {
     if let Some(serde_json::Value::Bool(t)) = value {
         METRICS_ENABLED.store(t, Ordering::Relaxed)
@@ -1058,8 +1057,8 @@ pub fn apply_fork_workspace_tag_append_fork_suffix(value: Option<serde_json::Val
 }
 
 pub async fn reload_critical_alert_mute_ui_setting(conn: &Connection) -> error::Result<()> {
-    let v = load_value_from_global_settings_with_conn(conn, CRITICAL_ALERT_MUTE_UI_SETTING, true)
-        .await?;
+    let v =
+        load_value_from_global_settings_with_conn(conn, CRITICAL_ALERT_MUTE_UI_SETTING, true).await?;
     apply_critical_alert_mute_ui_setting(v);
     Ok(())
 }
@@ -2543,6 +2542,7 @@ pub async fn reload_timeout_wait_result_setting(conn: &Connection) {
     .await;
 }
 
+
 pub async fn reload_extra_pip_index_url_setting(conn: &Connection) {
     reload_option_setting_with_tracing(
         conn,
@@ -2633,6 +2633,7 @@ pub async fn reload_bunfig_install_scopes_setting(conn: &Connection) {
     .await;
 }
 
+
 pub async fn reload_nuget_config_setting(conn: &Connection) {
     reload_option_setting_with_tracing(
         conn,
@@ -2740,6 +2741,7 @@ pub async fn reload_ruby_repos_setting(conn: &Connection) {
     .await;
 }
 
+
 pub async fn reload_workspace_registries_setting(conn: &Connection) {
     match load_value_from_global_settings_with_conn(
         conn,
@@ -2963,6 +2965,7 @@ pub async fn apply_job_isolation_setting(value: Option<serde_json::Value>) {
     }
 }
 
+
 async fn resolve_license_key_value(conn: &Connection, quiet: bool) -> anyhow::Result<String> {
     let q = load_value_from_global_settings_with_conn(conn, LICENSE_KEY_SETTING, true)
         .await
@@ -3257,10 +3260,7 @@ impl<'a> SettingsPass<'a> {
         // on compile-time defaults until the next full reload. Only the single-query transport
         // can fail this way; over HTTP the batch already is the per-setting read.
         if matches!(conn, Connection::Sql(_)) && values.is_empty() && !names.is_empty() {
-            tracing::warn!(
-                "Falling back to per-setting reads for {} settings",
-                names.len()
-            );
+            tracing::warn!("Falling back to per-setting reads for {} settings", names.len());
             values = fetch_settings_individually(conn, &names).await;
         }
         for (name, http) in &declared {
@@ -3652,6 +3652,7 @@ pub fn parse_setting_value<T: FromStr + DeserializeOwned + Display>(
     value
 }
 
+
 #[cfg(feature = "prometheus")]
 pub async fn monitor_pool(db: &DB) {
     if METRICS_ENABLED.load(Ordering::Relaxed) {
@@ -6311,8 +6312,10 @@ pub async fn reload_http_route_default_allowed_origins_setting(conn: &DB) -> err
 pub fn apply_http_route_default_allowed_origins_setting(
     value: Option<serde_json::Value>,
 ) -> error::Result<()> {
-    // A bad value leaves the previous list in place rather than falling back to
-    // no restriction: silently widening CORS instance-wide is the worse failure.
+    // A bad value leaves whatever is already loaded in place rather than
+    // reverting to no restriction. On the boot path that is still the empty
+    // default, so what keeps a stored typo from widening CORS instance-wide is
+    // write-time validation, not this.
     let origins = match parse_allowed_origins_setting(value.as_ref()) {
         Ok(origins) => origins,
         Err(err) => {
@@ -6393,6 +6396,7 @@ pub async fn reload_critical_alerts_on_db_oversize(conn: &DB) -> error::Result<(
     Ok(())
 }
 
+
 pub async fn reload_jwt_secret_setting(db: &DB) -> error::Result<()> {
     let v = load_value_from_global_settings(db, JWT_SECRET_SETTING).await?;
     apply_jwt_secret_setting(db, v).await

backend/windmill-common/src/global_settings.rs

@@ -294,7 +294,10 @@ pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Res
         let Some((scheme, rest)) = origin.split_once("://") else {
             return Err(invalid("missing scheme"));
         };
-        if scheme.is_empty()
+        if !scheme
+            .chars()
+            .next()
+            .is_some_and(|first| first.is_ascii_alphabetic())
             || !scheme
                 .chars()
                 .all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '+' || c == '-')
@@ -313,6 +316,30 @@ pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Res
         if rest.contains('@') {
             return Err(invalid("must not contain userinfo"));
         }
+
+        // An IPv6 literal is bracketed, so its own colons are not the port
+        // separator: splitting on the last colon would read `http://[::1]` as
+        // host `[:` and reject an origin a browser really does send.
+        let (host, port) = match rest.strip_prefix('[') {
+            Some(after_bracket) => match after_bracket.split_once(']') {
+                Some((host, "")) => (host, None),
+                Some((host, tail)) => match tail.strip_prefix(':') {
+                    Some(port) => (host, Some(port)),
+                    None => return Err(invalid("invalid port")),
+                },
+                None => return Err(invalid("missing host")),
+            },
+            None => match rest.split_once(':') {
+                Some((host, port)) => (host, Some(port)),
+                None => (rest, None),
+            },
+        };
+        if host.is_empty() {
+            return Err(invalid("missing host"));
+        }
+        if port.is_some_and(|port| port.is_empty() || !port.chars().all(|c| c.is_ascii_digit())) {
+            return Err(invalid("invalid port"));
+        }
     }
 
     Ok(())

backend/windmill-trigger-http/src/lib.rs

@@ -688,6 +688,8 @@ mod tests {
         let allowed = vec![
             "https://app.example.com".to_string(),
             "http://localhost:3000".to_string(),
+            "http://[::1]".to_string(),
+            "http://[::1]:8080".to_string(),
             "*".to_string(),
         ];
         assert!(validate_allowed_origins(&allowed).is_ok());
@@ -709,6 +711,10 @@ mod tests {
             // Every sandboxed iframe sends `Origin: null`, so allowing it would
             // grant access to any page that can open one.
             "null",
+            "1://app.example.com",
+            "https://app.example.com:not-a-port",
+            "https://app.example.com:",
+            "https://:3000",
         ] {
             assert!(
                 validate_allowed_origins(&[invalid.to_string()]).is_err(),

frontend/src/lib/components/triggers/http/RouteCorsOption.svelte

@@ -40,15 +40,25 @@
 		// whitespace and a non-punycoded IDN, which the backend rejects too.
 		if (!/^[\x21-\x7e]+$/.test(origin))
 			return `'${origin}' must contain only visible ASCII, with no whitespace`
-		const [scheme, ...rest] = origin.split('://')
-		if (rest.length !== 1) return `'${origin}' is missing a scheme, such as https://`
-		const host = rest[0]
-		if (host === '') return `'${origin}' is missing a host`
-		if (host.includes('/')) return `'${origin}' must not contain a path or trailing slash`
-		if (host.includes('?') || host.includes('#'))
+		const separator = origin.indexOf('://')
+		if (separator < 0) return `'${origin}' is missing a scheme, such as https://`
+		const scheme = origin.slice(0, separator)
+		const rest = origin.slice(separator + 3)
+		if (!/^[A-Za-z][A-Za-z0-9.+-]*$/.test(scheme)) return `'${origin}' has an invalid scheme`
+		if (rest === '') return `'${origin}' is missing a host`
+		if (rest.includes('/')) return `'${origin}' must not contain a path or trailing slash`
+		if (rest.includes('?') || rest.includes('#'))
 			return `'${origin}' must not contain a query or fragment`
-		if (host.includes('@')) return `'${origin}' must not contain userinfo`
-		if (!/^[A-Za-z0-9.+-]+$/.test(scheme)) return `'${origin}' has an invalid scheme`
+		if (rest.includes('@')) return `'${origin}' must not contain userinfo`
+		// An IPv6 literal is bracketed, so its own colons are not the port
+		// separator, and `http://[::1]` must not be read as host '[:'.
+		const authority = rest.startsWith('[')
+			? /^\[([^\]]*)\](?::(.*))?$/.exec(rest)
+			: /^([^:]*)(?::(.*))?$/.exec(rest)
+		if (!authority) return `'${origin}' is missing a host`
+		const [, host, port] = authority
+		if (host === '') return `'${origin}' is missing a host`
+		if (port !== undefined && !/^[0-9]+$/.test(port)) return `'${origin}' has an invalid port`
 		return undefined
 	}
 
@@ -86,6 +96,16 @@
 		allowed_origins = restricted ? origins : undefined
 	})
 
+	// This option renders on one tab only, so an unusable list must not outlive
+	// it: `error` alone would keep Save disabled from a screen that cannot show
+	// why, and clearing it alone would let a half-typed list save as a real
+	// restriction. Leaving restores what was there on arrival, never widening.
+	const mountedWith = allowed_origins
+	$effect(() => () => {
+		if (error !== undefined) allowed_origins = mountedWith
+		error = undefined
+	})
+
 	// Re-seed the text field when the value is replaced from outside — applying
 	// a draft, or resetting to deployed, both write the prop while this
 	// component stays mounted. Comparing against what this component would

frontend/src/lib/components/triggers/http/RouteEditorInner.svelte

@@ -51,7 +51,7 @@
 		HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING,
 		HUB_SCRIPT_ID,
 		isOriginRestricted,
-		parseAllowedOrigins,
+		parseAllowedOriginsSetting,
 		saveHttpRouteFromCfg,
 		SECRET_KEY_PATH
 	} from './utils'
@@ -129,7 +129,7 @@
 			const setting = await SettingService.getGlobal({
 				key: HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING
 			})
-			instanceDefaultOrigins = typeof setting === 'string' ? parseAllowedOrigins(setting) : []
+			instanceDefaultOrigins = parseAllowedOriginsSetting(setting)
 		} catch {
 			instanceDefaultOrigins = []
 		}

frontend/src/lib/components/triggers/http/utils.ts

@@ -19,18 +19,34 @@ export function parseAllowedOrigins(raw: string): string[] {
 		.filter((origin) => origin !== '')
 }
 
+/**
+ * Read the instance-default setting, mirroring `parse_allowed_origins_setting`
+ * in windmill-common: the settings UI writes a comma-separated string, but the
+ * API accepts an array too.
+ */
+export function parseAllowedOriginsSetting(setting: unknown): string[] {
+	if (typeof setting === 'string') return parseAllowedOrigins(setting)
+	if (Array.isArray(setting))
+		return setting
+			.filter((origin): origin is string => typeof origin === 'string')
+			.map((origin) => origin.trim())
+			.filter((origin) => origin !== '')
+	return []
+}
+
 /**
  * Whether a route is restricted to specific origins, mirroring
- * `effective_allowed_origins` in windmill-trigger-http: the route's own list
- * when it has one, otherwise the instance default, and `*` in either means no
- * restriction at all.
+ * `effective_allowed_origins` in windmill-trigger-http: a route with its own
+ * list restricts, an empty one included since it then matches no origin at all;
+ * only a route without one falls back to the instance default, and `*` in
+ * either is the opt-out.
  */
 export function isOriginRestricted(
 	allowed_origins: string[] | undefined,
 	instanceDefaultOrigins: string[]
 ): boolean {
-	const effective = allowed_origins ?? instanceDefaultOrigins
-	return effective.length > 0 && !effective.includes('*')
+	if (allowed_origins !== undefined) return !allowed_origins.includes('*')
+	return instanceDefaultOrigins.length > 0 && !instanceDefaultOrigins.includes('*')
 }
 
 export const SECRET_KEY_PATH = 'secret_key_path'