mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix: make an empty allowlist deny rather than fall back to the default
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
ba544a7f6e
commit
25c8fda861
@@ -57,8 +57,19 @@ fn cors_lookup_method(req: &axum::extract::Request) -> Option<HttpMethod> {
|
||||
.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<HttpMethod> {
|
||||
if method == http::Method::HEAD {
|
||||
Some(HttpMethod::Get)
|
||||
} else {
|
||||
HttpMethod::try_from(method).ok()
|
||||
|
||||
@@ -224,12 +224,18 @@ pub struct RouteExists {
|
||||
pub fn effective_allowed_origins(
|
||||
route_allowed_origins: Option<&Vec<String>>,
|
||||
) -> Option<Vec<String>> {
|
||||
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]
|
||||
|
||||
@@ -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]
|
||||
|
||||
Reference in New Issue
Block a user