diff --git a/backend/windmill-api/src/triggers/http/handler.rs b/backend/windmill-api/src/triggers/http/handler.rs index 3eae36758d..a33094c90e 100644 --- a/backend/windmill-api/src/triggers/http/handler.rs +++ b/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, allow_origin: Option }, + /// 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", diff --git a/backend/windmill-common/src/global_settings.rs b/backend/windmill-common/src/global_settings.rs index e28d1450ad..b1c7c913e4 100644 --- a/backend/windmill-common/src/global_settings.rs +++ b/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", diff --git a/backend/windmill-trigger-http/src/lib.rs b/backend/windmill-trigger-http/src/lib.rs index 0782fca6a5..2be2ce7e68 100644 --- a/backend/windmill-trigger-http/src/lib.rs +++ b/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>, -) -> Option> { +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(), diff --git a/frontend/src/lib/components/triggers/http/utils.ts b/frontend/src/lib/components/triggers/http/utils.ts index d906dc20f0..5fba39afb3 100644 --- a/frontend/src/lib/components/triggers/http/utils.ts +++ b/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