diff --git a/backend/windmill-api/src/triggers/http/handler.rs b/backend/windmill-api/src/triggers/http/handler.rs index 20f2f96add..6e05651dba 100644 --- a/backend/windmill-api/src/triggers/http/handler.rs +++ b/backend/windmill-api/src/triggers/http/handler.rs @@ -57,8 +57,19 @@ fn cors_lookup_method(req: &axum::extract::Request) -> Option { .and_then(|method| method.to_str().ok()) .and_then(|method| http::Method::try_from(method).ok()) .as_ref() - .and_then(|method| HttpMethod::try_from(method).ok()) - } else if method == http::Method::HEAD { + .and_then(routable_method) + } else { + routable_method(method) + } +} + +/// The router key a request method maps to. `HEAD` resolves the `GET` route it +/// mirrors, and does so for a preflight naming it too: browsers send +/// `Access-Control-Request-Method: HEAD` when the HEAD carries a non-safelisted +/// header, and answering that preflight from a different route than the request +/// itself resolves is how the two come to disagree. +fn routable_method(method: &http::Method) -> Option { + if method == http::Method::HEAD { Some(HttpMethod::Get) } else { HttpMethod::try_from(method).ok() diff --git a/backend/windmill-trigger-http/src/lib.rs b/backend/windmill-trigger-http/src/lib.rs index e9ad7238ef..6a43db2e92 100644 --- a/backend/windmill-trigger-http/src/lib.rs +++ b/backend/windmill-trigger-http/src/lib.rs @@ -224,12 +224,18 @@ pub struct RouteExists { pub fn effective_allowed_origins( route_allowed_origins: Option<&Vec>, ) -> Option> { - let effective = match route_allowed_origins { - Some(route_allowed_origins) => route_allowed_origins.clone(), - None => HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS.load().as_ref().clone(), - }; - - (!effective.is_empty() && !allows_any_origin(&effective)).then_some(effective) + 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) + } + } } /// Resolve the `Access-Control-Allow-Origin` value for a request, or `None` to @@ -665,8 +671,10 @@ mod tests { // historical permissive behaviour is kept. assert_eq!(effective_allowed_origins(None), None); // An empty route list is a restriction that matches nothing, distinct - // from `NULL` which inherits the instance default. - assert_eq!(effective_allowed_origins(Some(&vec![])), None); + // 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!(match_origin(&[], Some(&origin("https://a.com"))), None); } #[test] diff --git a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte index eb3eec2174..1a79eb8f63 100644 --- a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte +++ b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte @@ -36,6 +36,10 @@ // shows before saving rather than as a 400 from the API. function originError(origin: string): string | undefined { if (origin === '*') return undefined + // An Origin header is always visible ASCII, so this covers both embedded + // 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]