diff --git a/backend/windmill-api/src/triggers/http/handler.rs b/backend/windmill-api/src/triggers/http/handler.rs index 10ac68c588..0ebde4c60c 100644 --- a/backend/windmill-api/src/triggers/http/handler.rs +++ b/backend/windmill-api/src/triggers/http/handler.rs @@ -91,6 +91,7 @@ fn cors_lookup_path(raw_path: &str) -> Option { /// 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. +#[derive(Clone)] enum CorsDecision { /// No allowlist applies, so the permissive default stands. Unrestricted, @@ -101,41 +102,37 @@ enum CorsDecision { Unavailable, } -impl CorsDecision { - /// Combine the decision taken before the handler ran with the one taken - /// after it, keeping whichever is stricter. - /// - /// The routers are rebuilt while requests are in flight — a route renamed, - /// deleted, or given a wider list — and the response was produced under - /// whatever the handler resolved. Reading only before, or only after, - /// leaves one ordering in which a policy is stamped that is more permissive - /// than the one the response was served under. A deleted route is the sharp - /// case: the later read matches nothing, which on its own reads as "no - /// allowlist applies" and would answer `*`. - fn stricter(self, other: Self) -> Self { - use CorsDecision::*; - match (self, other) { - // A read that could not see the routers knows nothing, so it defers - // to one that could: a transient load failure on either side must - // not discard a real verdict. Only when neither read saw them is - // the answer genuinely unknown — and then the handler could not - // resolve the trigger either, so the response is an error rather - // than a runnable's output. - (Unavailable, Unavailable) => Unavailable, - (Unavailable, known) | (known, Unavailable) => known, - ( - Restricted { route_method, allow_origin }, - Restricted { allow_origin: also_allowed, .. }, - ) => Restricted { - route_method, - // Echo only where both reads agreed the origin is allowed, so a - // list narrowed mid-request is honoured too. - allow_origin: allow_origin.filter(|_| also_allowed.is_some()), +/// The CORS verdict for a request, published by whoever resolved its trigger. +/// +/// The middleware stamps headers after the handler returns, but only the +/// handler knows which trigger it actually served. Re-deriving that from the +/// routers cache is a second lookup which can disagree with the first when a +/// route is edited, deleted or widened mid-request, and every ordering of the +/// two is wrong in some case. So the verdict travels with the request instead +/// of being worked out twice. +#[derive(Clone, Default)] +struct ResolvedCorsPolicy(std::sync::Arc>); + +impl ResolvedCorsPolicy { + /// Record what the trigger being served allows. Called once, where the + /// route is resolved, so the answer cannot drift from the response. + fn publish(&self, trigger: &TriggerRoute, method: Option, headers: &HeaderMap) { + let instance_default = HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS.load(); + let decision = match effective_allowed_origins( + trigger.allowed_origins.as_deref(), + instance_default.as_slice(), + ) { + None => CorsDecision::Unrestricted, + Some(allowed_origins) => CorsDecision::Restricted { + route_method: method, + allow_origin: match_origin(allowed_origins, headers.get(http::header::ORIGIN)), }, - (restricted @ Restricted { .. }, Unrestricted) - | (Unrestricted, restricted @ Restricted { .. }) => restricted, - (Unrestricted, Unrestricted) => Unrestricted, - } + }; + let _ = self.0.set(decision); + } + + fn published(&self) -> Option { + self.0.get().cloned() } } @@ -187,7 +184,7 @@ async fn resolve_cors_decision( async fn conditional_cors_middleware( Extension(db): Extension, - req: axum::extract::Request, + mut req: axum::extract::Request, next: axum::middleware::Next, ) -> Response { let origin = req.headers().get(http::header::ORIGIN).cloned(); @@ -195,23 +192,27 @@ async fn conditional_cors_middleware( // (`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())); - // Decided on both sides of the handler and combined strictly. Only the - // verdict crosses the await, never the allowlist, so the second read costs - // a cache lookup rather than a copy of the list. - let before = match lookup.as_ref() { - 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 resolved = ResolvedCorsPolicy::default(); + req.extensions_mut().insert(resolved.clone()); let mut response = next.run(req).await; - let decision = match lookup.as_ref() { - Some((method, path)) => { - before.stricter(resolve_cors_decision(&db, *method, path, origin.as_ref()).await) - } - None => before, + let decision = match resolved.published() { + // The handler resolved a trigger and said what it served under. That is + // the policy this response was produced with, so nothing else can be + // more authoritative. + Some(decision) => decision, + // No trigger was resolved: a preflight, an unknown path, or a request + // that failed before the lookup. No runnable produced this body, so + // reading the cache now cannot contradict anything. + None => match lookup { + Some((method, path)) => { + resolve_cors_decision(&db, method, &path, origin.as_ref()).await + } + // Not a preflight, not a routable method, or a path that does not + // decode. + None => CorsDecision::Unrestricted, + }, }; let headers = response.headers_mut(); @@ -455,6 +456,7 @@ async fn route_job( Extension(db): Extension, Extension(user_db): Extension, Extension(auth_cache): Extension>, + Extension(cors_policy): Extension, OptTokened { token }: OptTokened, Path(route_path): Path, headers: HeaderMap, @@ -473,6 +475,10 @@ async fn route_job( .await .map_err(|e| e.into_response())?; + // Publish before anything else can fail: the CORS middleware stamps this + // response either way, and it must reflect the trigger actually served. + cors_policy.publish(&trigger, routable_method(&args.0.metadata.method), &headers); + if trigger.script_path.is_empty() && trigger.static_asset_config.is_none() { return Err(Error::NotFound(format!( "Runnable path of HTTP route at path: {}",