mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix: resolve CORS route from the decoded path like the request handler
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
5eb4059234
commit
ff38e7d429
@@ -64,6 +64,21 @@ fn cors_lookup_method(req: &axum::extract::Request) -> Option<HttpMethod> {
|
||||
}
|
||||
}
|
||||
|
||||
/// The key to look a request up by, matching what `route_job` resolves it to.
|
||||
///
|
||||
/// `Path<StripPath>` 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<String> {
|
||||
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<CorsRoute>),
|
||||
@@ -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",
|
||||
|
||||
@@ -297,6 +297,11 @@ pub fn validate_allowed_origins(allowed_origins: Option<&Vec<String>>) -> 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",
|
||||
|
||||
@@ -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
|
||||
}
|
||||
})
|
||||
</script>
|
||||
|
||||
<Label label="Restrict origins" for="allowed-origins-toggle" class="w-full">
|
||||
|
||||
@@ -300,6 +300,7 @@
|
||||
raw_string = defaultValues?.raw_string ?? false
|
||||
wrap_body = defaultValues?.wrap_body ?? false
|
||||
allowed_origins = defaultValues?.allowed_origins ?? undefined
|
||||
allowedOriginsError = undefined
|
||||
summary = defaultValues?.summary ?? ''
|
||||
routeDescription = defaultValues?.description ?? ''
|
||||
error_handler_path = defaultValues?.error_handler_path ?? undefined
|
||||
@@ -329,6 +330,10 @@
|
||||
wrap_body = cfg?.wrap_body ?? false
|
||||
raw_string = cfg?.raw_string ?? false
|
||||
allowed_origins = cfg?.allowed_origins ?? undefined
|
||||
// RouteCorsOption only lives on the request-options tab, so its error
|
||||
// would otherwise outlive the trigger it came from and block saving one
|
||||
// that has nothing wrong with it.
|
||||
allowedOriginsError = undefined
|
||||
summary = cfg?.summary ?? ''
|
||||
mode = cfg?.mode ?? 'enabled'
|
||||
routeDescription = cfg?.description ?? ''
|
||||
|
||||
Reference in New Issue
Block a user