fix: bound allowlists, reject commas, and decide cors after the handler

backend/windmill-api/src/triggers/http/handler.rs

@@ -25,6 +25,7 @@ use std::{collections::HashMap, sync::Arc};
 use windmill_common::{
     db::UserDB,
     error::{Error, Result},
+    global_settings::HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS,
     jobs::JobTriggerKind,
     triggers::{TriggerKind, TriggerMetadata},
     utils::{not_found_if_none, StripPath},
@@ -99,17 +100,33 @@ enum CorsRouteLookup {
     Unavailable,
 }
 
-/// Resolve the trigger a request targets, for CORS purposes only.
+/// What the middleware should stamp, decided while the routers guard is held.
+///
+/// Deliberately small and owned: the allowlist itself never leaves the guard,
+/// so a large one is scanned in place instead of being copied per request onto
+/// a path an unauthenticated preflight can reach.
+enum CorsDecision {
+    /// No allowlist applies, so the permissive default stands.
+    Unrestricted,
+    /// An allowlist applies. `allow_origin` is the value to echo, present only
+    /// when the request's own `Origin` is on the list.
+    Restricted { route_method: Option<HttpMethod>, allow_origin: Option<http::HeaderValue> },
+    /// The routers could not be read, so nothing is known about this path.
+    Unavailable,
+}
+
+/// Decide the CORS answer for a request, from the routers cache.
 ///
 /// Loads the routers when the cache is cold, the way `get_http_route_trigger`
 /// does. Without that, a failed startup load would leave the middleware
-/// resolving nothing while `route_job` refreshes and serves a restricted route
+/// deciding nothing while `route_job` refreshes and serves a restricted route
 /// behind it, and the response would carry the permissive default.
-async fn resolve_cors_route(
+async fn resolve_cors_decision(
     db: &DB,
     http_method: HttpMethod,
     requested_path: &str,
-) -> CorsRouteLookup {
+    origin: Option<&http::HeaderValue>,
+) -> CorsDecision {
     let routers_cache = HTTP_ROUTERS_CACHE.read().await;
 
     let routers_cache = if routers_cache.routers.is_empty() {
@@ -118,7 +135,7 @@ async fn resolve_cors_route(
             Ok((_, routers_cache)) => routers_cache,
             Err(err) => {
                 tracing::error!("Could not load HTTP routers to resolve CORS: {err:#}");
-                return CorsRouteLookup::Unavailable;
+                return CorsDecision::Unavailable;
             }
         }
     } else {
@@ -126,13 +143,22 @@ async fn resolve_cors_route(
     };
 
     let Some(router) = routers_cache.routers.get(&http_method) else {
-        return CorsRouteLookup::Unavailable;
+        return CorsDecision::Unavailable;
     };
 
-    CorsRouteLookup::Resolved(router.at(requested_path).ok().map(|trigger| CorsRoute {
-        allowed_origins: trigger.value.allowed_origins.clone(),
-        http_method,
-    }))
+    let route = router.at(requested_path).ok();
+    let route_allowed_origins = route
+        .as_ref()
+        .and_then(|trigger| trigger.value.allowed_origins.as_deref());
+
+    let instance_default = HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS.load();
+    match effective_allowed_origins(route_allowed_origins, instance_default.as_slice()) {
+        None => CorsDecision::Unrestricted,
+        Some(allowed_origins) => CorsDecision::Restricted {
+            route_method: route.map(|_| http_method),
+            allow_origin: match_origin(allowed_origins, origin),
+        },
+    }
 }
 
 async fn conditional_cors_middleware(
@@ -141,19 +167,23 @@ async fn conditional_cors_middleware(
     next: axum::middleware::Next,
 ) -> Response {
     let origin = req.headers().get(http::header::ORIGIN).cloned();
-    // Resolved before `next.run` consumes the request. `&Request` is not `Send`
-    // (`Body` is not `Sync`), so the lookup takes owned pieces rather than a
-    // borrow of the request itself.
+    // Owned before `next.run` consumes the request. `&Request` is not `Send`
+    // (`Body` is not `Sync`), so nothing borrowed from it can cross the await.
     let lookup = cors_lookup_method(&req).zip(cors_lookup_path(req.uri().path()));
-    let route = match lookup {
-        Some((method, path)) => resolve_cors_route(&db, method, &path).await,
-        // Nothing to look up: not a preflight, not a method any route can be
-        // registered under, or a path that does not decode.
-        None => CorsRouteLookup::Resolved(None),
-    };
 
     let mut response = next.run(req).await;
 
+    // Decided after the handler, so the policy stamped here is never older than
+    // the one the handler resolved: a route made stricter while the request was
+    // in flight is answered with the stricter list, not the snapshot it started
+    // with. Reading the cache once, here, is what keeps the two from diverging.
+    let decision = match lookup {
+        Some((method, path)) => resolve_cors_decision(&db, method, &path, origin.as_ref()).await,
+        // Nothing to look up: not a preflight, not a method any route can be
+        // registered under, or a path that does not decode.
+        None => CorsDecision::Unrestricted,
+    };
+
     let headers = response.headers_mut();
 
     // Check existing headers first to determine what not to insert
@@ -178,50 +208,41 @@ async fn conditional_cors_middleware(
         }
     }
 
-    let resolved = match &route {
-        CorsRouteLookup::Resolved(resolved) => resolved.as_ref(),
+    match &decision {
+        CorsDecision::Restricted { allow_origin, .. } => {
+            // A configured allowlist decides, overriding any `wm_headers` value
+            // the runnable set. The preflight is answered before any code runs,
+            // so config is the only thing it can consult; letting the response
+            // widen what the preflight advertised would make the two disagree
+            // and leave the allowlist bounding nothing. A route escapes a
+            // stricter instance default — `wm_headers` included — by setting
+            // its own list to `*`.
+            match allow_origin {
+                Some(value) => {
+                    headers.insert(http::header::ACCESS_CONTROL_ALLOW_ORIGIN, value.clone())
+                }
+                // No match: omit the header entirely so the browser blocks the
+                // read, and drop any value the runnable set.
+                None => headers.remove(http::header::ACCESS_CONTROL_ALLOW_ORIGIN),
+            };
+            // Appended, not inserted: the answer now depends on the request's
+            // Origin, and a shared cache that ignores it would hand one
+            // origin's response to another.
+            headers.append(http::header::VARY, http::HeaderValue::from_static("origin"));
+        }
         // Whether this path is restricted could not be determined, and
-        // `route_job` may still load the routers and serve a restricted route
-        // behind this middleware. Emitting the permissive default here would
-        // hand that response to any origin, so emit nothing at all.
-        CorsRouteLookup::Unavailable => None,
-    };
-
-    // The route's own list, or the instance-wide default when it has none.
-    // `None` means neither is configured, or one of them opted out with `*`.
-    // Nothing is applied when the routers were unreadable: the instance default
-    // is not necessarily what the route this request lands on would have used.
-    let allowed_origins = match &route {
-        CorsRouteLookup::Unavailable => None,
-        CorsRouteLookup::Resolved(resolved) => effective_allowed_origins(
-            resolved
-                .as_ref()
-                .and_then(|route| route.allowed_origins.as_ref()),
-        ),
-    };
-
-    if let Some(allowed_origins) = allowed_origins.as_ref() {
-        // A configured allowlist decides, overriding any `wm_headers` value the
-        // runnable set. The preflight is answered before any code runs, so
-        // config is the only thing it can consult; letting the response widen
-        // what the preflight advertised would make the two disagree and leave
-        // the allowlist bounding nothing. A route escapes a stricter instance
-        // default — `wm_headers` included — by setting its own list to `*`.
-        match match_origin(allowed_origins, origin.as_ref()) {
-            Some(value) => headers.insert(http::header::ACCESS_CONTROL_ALLOW_ORIGIN, value),
-            // No match: omit the header entirely so the browser blocks the
-            // read, and drop any value the runnable set.
-            None => headers.remove(http::header::ACCESS_CONTROL_ALLOW_ORIGIN),
-        };
-        // Appended, not inserted: the answer now depends on the request's
-        // Origin, and a shared cache that ignores it would hand one origin's
-        // response to another.
-        headers.append(http::header::VARY, http::HeaderValue::from_static("origin"));
-    } else if !not_insert_origin && !matches!(route, CorsRouteLookup::Unavailable) {
-        headers.insert(
-            http::header::ACCESS_CONTROL_ALLOW_ORIGIN,
-            http::HeaderValue::from_static("*"),
-        );
+        // `route_job` may still have served a restricted route behind this
+        // middleware. Emitting the permissive default would hand that response
+        // to any origin, so emit nothing at all.
+        CorsDecision::Unavailable => {}
+        CorsDecision::Unrestricted => {
+            if !not_insert_origin {
+                headers.insert(
+                    http::header::ACCESS_CONTROL_ALLOW_ORIGIN,
+                    http::HeaderValue::from_static("*"),
+                );
+            }
+        }
     }
 
     if !not_insert_methods {
@@ -229,10 +250,13 @@ async fn conditional_cors_middleware(
         // overstates it — but only routes under an allowlist get the narrower
         // answer. An unrestricted route must respond exactly as it did before
         // this existed.
-        let restricted_route = resolved.filter(|_| allowed_origins.is_some());
+        let restricted_method = match &decision {
+            CorsDecision::Restricted { route_method, .. } => *route_method,
+            _ => None,
+        };
         headers.insert(
             http::header::ACCESS_CONTROL_ALLOW_METHODS,
-            http::HeaderValue::from_static(match restricted_route.map(|route| route.http_method) {
+            http::HeaderValue::from_static(match restricted_method {
                 Some(HttpMethod::Get) => "GET, OPTIONS",
                 Some(HttpMethod::Post) => "POST, OPTIONS",
                 Some(HttpMethod::Put) => "PUT, OPTIONS",

backend/windmill-common/src/global_settings.rs

@@ -263,16 +263,32 @@ pub fn allows_any_origin(allowed_origins: &[String]) -> bool {
     allowed_origins.iter().any(|allowed| allowed == "*")
 }
 
-/// Reject allowlist entries that cannot be compared, or that must not be allowed.
+/// An allowlist is scanned on every request to a restricted route, including
+/// the unauthenticated preflight, so its size is a request cost anyone can
+/// trigger.
+pub const MAX_ALLOWED_ORIGINS: usize = 100;
+pub const MAX_ALLOWED_ORIGIN_LEN: usize = 256;
+
+/// Reject allowlist entries that cannot be compared, stored, or safely allowed.
 ///
 /// The stored string is only ever an operand: `match_origin` echoes the
 /// request's own `Origin` back, never this value, so a malformed entry matches
-/// nothing and fails closed. That leaves the one entry where being permissive
-/// has a consequence rather than just being dead config: `null` is what every
-/// sandboxed iframe sends, so allowing it would grant access to any page that
-/// can open one. Shapes that merely cannot match are the editor's business to
-/// warn about, not this function's to refuse.
+/// nothing and fails closed. Shapes that merely cannot match are the editor's
+/// business to warn about, not this function's to refuse. What is left are the
+/// three cases where permissiveness costs something: `null` is what every
+/// sandboxed iframe sends, so allowing it would admit any page that can open
+/// one; a comma cannot survive the editor's comma-separated field, which would
+/// silently split one entry into two and widen the list; and an unbounded list
+/// makes every preflight pay for it.
 pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Result<()> {
+    if allowed_origins.len() > MAX_ALLOWED_ORIGINS {
+        return Err(crate::error::Error::BadRequest(format!(
+            "At most {} allowed origins, got {}.",
+            MAX_ALLOWED_ORIGINS,
+            allowed_origins.len()
+        )));
+    }
+
     for origin in allowed_origins {
         if origin == "*" {
             continue;
@@ -285,6 +301,17 @@ pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Res
             ))
         };
 
+        if origin.is_empty() {
+            return Err(invalid("must not be empty"));
+        }
+        if origin.len() > MAX_ALLOWED_ORIGIN_LEN {
+            return Err(invalid("is longer than any origin a browser sends"));
+        }
+        // The editor edits the whole list as one comma-separated field, so an
+        // entry carrying a comma comes back as two and widens the list.
+        if origin.contains(',') {
+            return Err(invalid("must not contain a comma, which separates entries"));
+        }
         if origin.eq_ignore_ascii_case("null") {
             return Err(invalid(
                 "'null' is what a sandboxed iframe sends, so allowing it would allow any page that can open one",

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

@@ -8,9 +8,7 @@ use tokio::sync::{RwLock, RwLockReadGuard};
 use windmill_common::{
     error::{Error, Result},
     flows::Retry,
-    global_settings::{
-        allows_any_origin, HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS, HTTP_ROUTE_WORKSPACED_ROUTE,
-    },
+    global_settings::{allows_any_origin, HTTP_ROUTE_WORKSPACED_ROUTE},
     utils::ExpiringCacheEntry,
     worker::CLOUD_HOSTED,
     DB,
@@ -229,20 +227,19 @@ pub struct RouteExists {
 ///
 /// A list containing `*` is treated as no restriction, which is how a route opts
 /// out of a stricter instance default.
-pub fn effective_allowed_origins(
-    route_allowed_origins: Option<&Vec<String>>,
-) -> Option<Vec<String>> {
+pub fn effective_allowed_origins<'a>(
+    route_allowed_origins: Option<&'a [String]>,
+    instance_default: &'a [String],
+) -> Option<&'a [String]> {
     match route_allowed_origins {
         // `*` is the opt-out, including out of a stricter instance default.
         Some(list) if allows_any_origin(list) => None,
         // Any other stored list restricts, an empty one included: it allows no
         // origin at all. Falling back to the default here would make `[]` more
         // permissive than `NULL`, which is the wrong direction to fail in.
-        Some(list) => Some(list.clone()),
-        None => {
-            let default = HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS.load().as_ref().clone();
-            (!default.is_empty() && !allows_any_origin(&default)).then_some(default)
-        }
+        Some(list) => Some(list),
+        None => (!instance_default.is_empty() && !allows_any_origin(instance_default))
+            .then_some(instance_default),
     }
 }
 
@@ -438,7 +435,9 @@ pub struct HttpTrigger;
 mod tests {
     use super::*;
     // Not used by the lib itself, only exercised here.
-    use windmill_common::global_settings::validate_allowed_origins;
+    use windmill_common::global_settings::{
+        validate_allowed_origins, MAX_ALLOWED_ORIGINS, MAX_ALLOWED_ORIGIN_LEN,
+    };
 
     #[test]
     fn test_request_type_backward_compatibility() {
@@ -683,25 +682,39 @@ mod tests {
         ]));
         assert!(!allows_any_origin(&["https://a.com".to_string()]));
         assert_eq!(
-            effective_allowed_origins(Some(&vec!["*".to_string()])),
+            effective_allowed_origins(Some(&["*".to_string()]), &[]),
             None
         );
     }
 
     #[test]
     fn test_effective_allowed_origins_prefers_the_route() {
-        let route = vec!["https://a.com".to_string()];
+        let route = ["https://a.com".to_string()];
+        let default = ["https://default.com".to_string()];
         assert_eq!(
-            effective_allowed_origins(Some(&route)),
-            Some(vec!["https://a.com".to_string()])
+            effective_allowed_origins(Some(&route), &default),
+            Some(&route[..])
+        );
+        // No route list: the instance default applies.
+        assert_eq!(
+            effective_allowed_origins(None, &default),
+            Some(&default[..])
         );
         // No route list and no instance default: nothing is restricted, so the
         // historical permissive behaviour is kept.
-        assert_eq!(effective_allowed_origins(None), None);
+        assert_eq!(effective_allowed_origins(None, &[]), None);
+        // A route opting out with `*` escapes a stricter instance default.
+        assert_eq!(
+            effective_allowed_origins(Some(&["*".to_string()]), &default),
+            None
+        );
         // An empty route list is a restriction that matches nothing, distinct
         // from `NULL` which inherits the instance default. It must never come
         // back as `None`, which the middleware reads as "any origin".
-        assert_eq!(effective_allowed_origins(Some(&vec![])), Some(vec![]));
+        assert_eq!(
+            effective_allowed_origins(Some(&[]), &default),
+            Some(&[][..])
+        );
         assert_eq!(match_origin(&[], Some(&origin("https://a.com"))), None);
     }
 
@@ -731,6 +744,18 @@ mod tests {
         assert!(validate_allowed_origins(&[]).is_ok());
     }
 
+    #[test]
+    fn test_validate_allowed_origins_bounds_the_list() {
+        // An allowlist is scanned on every request to a restricted route, the
+        // unauthenticated preflight included, so its size is a cost anyone can
+        // trigger.
+        let too_many = vec!["https://a.com".to_string(); MAX_ALLOWED_ORIGINS + 1];
+        assert!(validate_allowed_origins(&too_many).is_err());
+        assert!(validate_allowed_origins(&too_many[..MAX_ALLOWED_ORIGINS]).is_ok());
+        let too_long = format!("https://{}.com", "a".repeat(MAX_ALLOWED_ORIGIN_LEN));
+        assert!(validate_allowed_origins(&[too_long]).is_err());
+    }
+
     #[test]
     fn test_validate_allowed_origins_rejects_null_and_uncomparable() {
         for invalid in [
@@ -741,6 +766,10 @@ mod tests {
             "https://a b.com",
             "https://app.example.com ",
             "https://exämple.com",
+            // The editor edits the list as one comma-separated field, so an
+            // entry carrying a comma would come back as two and widen the list.
+            "https://a.com,https://b.com",
+            "",
         ] {
             assert!(
                 validate_allowed_origins(&[invalid.to_string()]).is_err(),

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

@@ -32,9 +32,30 @@ export function allowedOriginRejection(origin: string): string | undefined {
 		return `'null' is what a sandboxed iframe sends, so it would allow any page that can open one`
 	if (!/^[\x21-\x7e]+$/.test(origin))
 		return `'${origin}' must contain only visible ASCII, with no whitespace`
+	if (origin.length > MAX_ALLOWED_ORIGIN_LEN)
+		return `'${origin.slice(0, 40)}…' is longer than any origin a browser sends`
 	return undefined
 }
 
+/** Kept in step with `MAX_ALLOWED_ORIGIN{,S}` in windmill-common. */
+export const MAX_ALLOWED_ORIGINS = 100
+export const MAX_ALLOWED_ORIGIN_LEN = 256
+
+/**
+ * The first entry the API would refuse, if any. Derived from the stored list
+ * rather than the field, so it stays correct while the editor is on another tab
+ * and the field is not mounted. An empty list is not an error here: it is the
+ * deny-every-origin state the backend accepts.
+ *
+ * A comma never reaches this: it is the field's separator, so an entry cannot
+ * carry one in the first place.
+ */
+export function allowedOriginsError(allowed_origins: string[] | undefined): string | undefined {
+	if (allowed_origins !== undefined && allowed_origins.length > MAX_ALLOWED_ORIGINS)
+		return `At most ${MAX_ALLOWED_ORIGINS} origins, got ${allowed_origins.length}`
+	return allowed_origins?.map(allowedOriginRejection).find((message) => message !== undefined)
+}
+
 /**
  * Shapes that save fine but can never equal an `Origin` header, so the route
  * would read as configured while allowing nothing.
@@ -54,16 +75,6 @@ export function allowedOriginWarning(origin: string): string | undefined {
 	return undefined
 }
 
-/**
- * The first entry the API would refuse, if any. Derived from the stored list
- * rather than the field, so it stays correct while the editor is on another tab
- * and the field is not mounted. An empty list is not an error here: it is the
- * deny-every-origin state the backend accepts.
- */
-export function allowedOriginsError(allowed_origins: string[] | undefined): string | undefined {
-	return allowed_origins?.map(allowedOriginRejection).find((message) => message !== undefined)
-}
-
 /**
  * Read the instance-default setting, mirroring `parse_allowed_origins_setting`
  * in windmill-common: the settings UI writes a comma-separated string, but the