From ff38e7d429cd268f18792271d4176624eaad7f70 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Tue, 25 Aug 2026 14:29:35 +0200 Subject: [PATCH] fix: resolve CORS route from the decoded path like the request handler Co-Authored-By: Claude Opus 5 --- .../windmill-api/src/triggers/http/handler.rs | 43 ++++++++++++------- backend/windmill-trigger-http/src/lib.rs | 9 ++++ .../triggers/http/RouteCorsOption.svelte | 14 ++++++ .../triggers/http/RouteEditorInner.svelte | 5 +++ 4 files changed, 56 insertions(+), 15 deletions(-) diff --git a/backend/windmill-api/src/triggers/http/handler.rs b/backend/windmill-api/src/triggers/http/handler.rs index 1bd29e128a..3847d26f1d 100644 --- a/backend/windmill-api/src/triggers/http/handler.rs +++ b/backend/windmill-api/src/triggers/http/handler.rs @@ -64,6 +64,21 @@ fn cors_lookup_method(req: &axum::extract::Request) -> Option { } } +/// The key to look a request up by, matching what `route_job` resolves it to. +/// +/// `Path` percent-decodes before `get_http_route_trigger` builds its +/// lookup key, so decoding here is what keeps the two agreeing: on the raw path, +/// `/us%65rs` misses the trigger registered at `/users` that goes on to serve the +/// request, and the response would carry the permissive default instead of that +/// trigger's allowlist. +fn cors_lookup_path(raw_path: &str) -> Option { + let decoded = urlencoding::decode(raw_path).ok()?; + // `StripPath::to_path` strips one leading slash and the handler trims + // trailing ones, before a single `/` is prefixed back on. + let stripped = decoded.strip_prefix('/').unwrap_or(&decoded); + Some(format!("/{}", stripped.trim_end_matches('/'))) +} + enum CorsRouteLookup { /// The routers were readable: `Some` when a trigger matched the request. Resolved(Option), @@ -102,15 +117,10 @@ async fn resolve_cors_route( 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, - }), - ) + CorsRouteLookup::Resolved(router.at(requested_path).ok().map(|trigger| CorsRoute { + allowed_origins: trigger.value.allowed_origins.clone(), + http_method, + })) } async fn conditional_cors_middleware( @@ -122,11 +132,11 @@ async fn conditional_cors_middleware( // 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. - let lookup = cors_lookup_method(&req).map(|method| (method, req.uri().path().to_string())); + 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, and not a method any route can - // be registered under. + // 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), }; @@ -191,11 +201,14 @@ async fn conditional_cors_middleware( } if !not_insert_methods { - // A resolved route accepts exactly one method, so advertising all seven - // overstates it. Unresolved requests keep the historical list. + // A route accepts exactly one method, so advertising all seven + // overstates it — but only routes that opted into an allowlist get the + // narrower answer. A route with no allowlist must respond exactly as it + // did before this existed. + let restricted_route = resolved.filter(|route| route.allowed_origins.is_some()); headers.insert( http::header::ACCESS_CONTROL_ALLOW_METHODS, - http::HeaderValue::from_static(match resolved.map(|route| route.http_method) { + http::HeaderValue::from_static(match restricted_route.map(|route| route.http_method) { Some(HttpMethod::Get) => "GET, OPTIONS", Some(HttpMethod::Post) => "POST, OPTIONS", Some(HttpMethod::Put) => "PUT, OPTIONS", diff --git a/backend/windmill-trigger-http/src/lib.rs b/backend/windmill-trigger-http/src/lib.rs index d052863727..58fa02a062 100644 --- a/backend/windmill-trigger-http/src/lib.rs +++ b/backend/windmill-trigger-http/src/lib.rs @@ -297,6 +297,11 @@ pub fn validate_allowed_origins(allowed_origins: Option<&Vec>) -> Result if rest.contains('@') { return Err(invalid("must not contain userinfo")); } + // A space is a legal header-value byte, so this survives + // `HeaderValue::from_str` and would sit in the list matching nothing. + if rest.contains(|c: char| c.is_whitespace()) { + return Err(invalid("must not contain whitespace")); + } } Ok(()) @@ -720,6 +725,10 @@ mod tests { "https://user@app.example.com", "https://app.example.com?a=b", "app.example.com", + // Legal header-value bytes, so nothing downstream rejects them, but + // no browser ever sends an Origin with a space in it. + "https://app.example.com ", + "https://a b.com", // Every sandboxed iframe sends `Origin: null`, so allowing it would // grant access to any page that can open one. "null", diff --git a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte index 5e77619fe7..de63fc19ba 100644 --- a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte +++ b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte @@ -70,6 +70,20 @@ $effect(() => { allowed_origins = restricted ? origins : 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 + // itself produce is what tells an external write apart from its own, so + // typing is never clobbered mid-edit. + $effect(() => { + const incoming = allowed_origins + const own = restricted ? origins : undefined + if (JSON.stringify(incoming) !== JSON.stringify(own)) { + raw = incoming?.join(', ') ?? '' + restricted = incoming !== undefined + } + })