From 5eb40592342a42e0bfa376bb272979b309b26afd Mon Sep 17 00:00:00 2001 From: hugocasa Date: Tue, 25 Aug 2026 14:21:04 +0200 Subject: [PATCH] fix: fail closed on cold router cache and invalid origin input Co-Authored-By: Claude Opus 5 --- .../windmill-api/src/triggers/http/handler.rs | 79 ++++++++++++++----- .../triggers/http/RouteCorsOption.svelte | 32 ++++++-- .../triggers/http/RouteEditorInner.svelte | 9 ++- 3 files changed, 94 insertions(+), 26 deletions(-) diff --git a/backend/windmill-api/src/triggers/http/handler.rs b/backend/windmill-api/src/triggers/http/handler.rs index 7039eec8ef..1bd29e128a 100644 --- a/backend/windmill-api/src/triggers/http/handler.rs +++ b/backend/windmill-api/src/triggers/http/handler.rs @@ -64,24 +64,57 @@ fn cors_lookup_method(req: &axum::extract::Request) -> Option { } } +enum CorsRouteLookup { + /// The routers were readable: `Some` when a trigger matched the request. + Resolved(Option), + /// The routers could not be loaded, so whether this path is restricted is + /// unknown. + Unavailable, +} + /// Resolve the trigger a request targets, for CORS purposes only. /// -/// Reads the cache without ever refreshing it from the DB: -/// `refresh_routers_loop` keeps it current, and a cold miss falls back to the -/// permissive default rather than putting a query on the CORS path. -async fn resolve_cors_route(http_method: HttpMethod, requested_path: &str) -> Option { +/// 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 +/// behind it, and the response would carry the permissive default. +async fn resolve_cors_route( + db: &DB, + http_method: HttpMethod, + requested_path: &str, +) -> CorsRouteLookup { let routers_cache = HTTP_ROUTERS_CACHE.read().await; - let trigger = routers_cache - .routers - .get(&http_method)? - .at(requested_path.trim_end_matches('/')) - .ok()? - .value; - Some(CorsRoute { allowed_origins: trigger.allowed_origins.clone(), http_method }) + let routers_cache = if routers_cache.routers.is_empty() { + drop(routers_cache); + match refresh_routers(db).await { + Ok((_, routers_cache)) => routers_cache, + Err(err) => { + tracing::error!("Could not load HTTP routers to resolve CORS: {err:#}"); + return CorsRouteLookup::Unavailable; + } + } + } else { + routers_cache + }; + + let Some(router) = routers_cache.routers.get(&http_method) else { + return CorsRouteLookup::Unavailable; + }; + + CorsRouteLookup::Resolved( + router + .at(requested_path.trim_end_matches('/')) + .ok() + .map(|trigger| CorsRoute { + allowed_origins: trigger.value.allowed_origins.clone(), + http_method, + }), + ) } async fn conditional_cors_middleware( + Extension(db): Extension, req: axum::extract::Request, next: axum::middleware::Next, ) -> Response { @@ -91,8 +124,10 @@ async fn conditional_cors_middleware( // borrow of the request itself. let lookup = cors_lookup_method(&req).map(|method| (method, req.uri().path().to_string())); let route = match lookup { - Some((method, path)) => resolve_cors_route(method, &path).await, - None => None, + Some((method, path)) => resolve_cors_route(&db, method, &path).await, + // Nothing to look up: not a preflight, and not a method any route can + // be registered under. + None => CorsRouteLookup::Resolved(None), }; let mut response = next.run(req).await; @@ -121,10 +156,16 @@ async fn conditional_cors_middleware( } } - if let Some(allowed_origins) = route - .as_ref() - .and_then(|route| route.allowed_origins.as_ref()) - { + let resolved = match &route { + CorsRouteLookup::Resolved(resolved) => resolved.as_ref(), + // 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, + }; + + if let Some(allowed_origins) = resolved.and_then(|route| route.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 @@ -142,7 +183,7 @@ async fn conditional_cors_middleware( if !allowed_origins.iter().any(|allowed| allowed == "*") { headers.append(http::header::VARY, http::HeaderValue::from_static("origin")); } - } else if !not_insert_origin { + } else if !not_insert_origin && !matches!(route, CorsRouteLookup::Unavailable) { headers.insert( http::header::ACCESS_CONTROL_ALLOW_ORIGIN, http::HeaderValue::from_static("*"), @@ -154,7 +195,7 @@ async fn conditional_cors_middleware( // overstates it. Unresolved requests keep the historical list. headers.insert( http::header::ACCESS_CONTROL_ALLOW_METHODS, - http::HeaderValue::from_static(match route.as_ref().map(|route| route.http_method) { + http::HeaderValue::from_static(match resolved.map(|route| route.http_method) { Some(HttpMethod::Get) => "GET, OPTIONS", Some(HttpMethod::Post) => "POST, OPTIONS", Some(HttpMethod::Put) => "PUT, OPTIONS", diff --git a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte index 355d66e241..5e77619fe7 100644 --- a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte +++ b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte @@ -7,19 +7,22 @@ interface Props { allowed_origins: string[] | undefined + /** Bound so the editor can block saving while the list is unusable. */ + error?: string | undefined disabled?: boolean testingBadge?: Snippet | undefined } let { allowed_origins = $bindable(), + error = $bindable(), disabled = false, testingBadge = undefined }: Props = $props() // The text field is the editing surface, `allowed_origins` the saved value. // Keeping them separate lets a half-typed entry stay on screen while the - // trigger config holds only what parses. + // trigger config holds the parsed list. let raw = $state(allowed_origins?.join(', ') ?? '') let restricted = $state(allowed_origins !== undefined) @@ -47,10 +50,25 @@ } let origins = $derived(parse(raw)) - let error = $derived(origins.map(originError).find((error) => error !== undefined)) + // Split from `error` so an entry that is merely still empty reads as a hint + // rather than a rejection: both block the save, only one is a mistake. + let malformed = $derived( + restricted ? origins.map(originError).find((message) => message !== undefined) : undefined + ) $effect(() => { - allowed_origins = restricted && !error && origins.length > 0 ? origins : undefined + error = restricted + ? (malformed ?? (origins.length === 0 ? 'Enter at least one origin' : undefined)) + : undefined + }) + + // While the toggle is on, whatever is typed is what gets saved — a rejected + // entry must never collapse to `undefined`, because `undefined` is stored as + // NULL and NULL means "any origin". A typo would otherwise lift the + // restriction while the toggle still reads as on. `error` blocks the save, + // and the backend rejects the same values if one ever gets past it. + $effect(() => { + allowed_origins = restricted ? origins : undefined }) @@ -80,13 +98,15 @@
Separate origins with commas. Use * to allow any origin.
- {#if error} -
{error}
+ {#if malformed} +
{malformed}
+ {:else if error} +
{error}
{/if} {/if} diff --git a/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte b/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte index d0a69c8ff0..63cecb2f93 100644 --- a/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte +++ b/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte @@ -112,6 +112,7 @@ let raw_string = $state(false) let wrap_body = $state(false) let allowed_origins = $state(undefined) + let allowedOriginsError = $state(undefined) let drawerLoading = $state(true) let showLoader = $state(false) let authentication_resource_path = $state('') @@ -162,6 +163,7 @@ !can_write || pathError != '' || !isValid || + allowedOriginsError != undefined || (!static_asset_config && emptyString(script_path)) || !hasChanged ) @@ -967,7 +969,12 @@ {testingBadge} /> - + {:else}