From 95303f63128b399775ca4b48df98313997e9b948 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 28 Aug 2026 17:58:20 +0200 Subject: [PATCH] refactor: make origin validation advisory except for null and non-ascii --- .../windmill-common/src/global_settings.rs | 94 ++++--------------- backend/windmill-trigger-http/src/lib.rs | 38 +++----- .../triggers/http/RouteCorsOption.svelte | 14 ++- .../src/lib/components/triggers/http/utils.ts | 74 ++++++--------- 4 files changed, 72 insertions(+), 148 deletions(-) diff --git a/backend/windmill-common/src/global_settings.rs b/backend/windmill-common/src/global_settings.rs index a28348aa5c..e28d1450ad 100644 --- a/backend/windmill-common/src/global_settings.rs +++ b/backend/windmill-common/src/global_settings.rs @@ -263,13 +263,15 @@ pub fn allows_any_origin(allowed_origins: &[String]) -> bool { allowed_origins.iter().any(|allowed| allowed == "*") } -/// Reject allowlist entries a browser would silently ignore. +/// Reject allowlist entries that cannot be compared, or that must not be allowed. /// -/// An origin is a bare `scheme://host[:port]`: anything with a path, query, -/// fragment, userinfo or whitespace never equals the `Origin` header a browser -/// sends, so it would look configured while matching nothing. `null` is rejected -/// outright — every sandboxed iframe sends `Origin: null`, so allowing it grants -/// access to any page that can open one. +/// The stored string is only ever an operand: `match_origin` echoes the +/// request's own `Origin` back, never this value, so a malformed entry matches +/// nothing and fails closed. That leaves the one entry where being permissive +/// has a consequence rather than just being dead config: `null` is what every +/// sandboxed iframe sends, so allowing it would grant access to any page that +/// can open one. Shapes that merely cannot match are the editor's business to +/// warn about, not this function's to refuse. pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Result<()> { for origin in allowed_origins { if origin == "*" { @@ -278,87 +280,23 @@ pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Res let invalid = |reason: &str| { crate::error::Error::BadRequest(format!( - "Invalid allowed origin '{}': {}. Expected an origin such as https://app.example.com, or * to allow any origin.", + "Invalid allowed origin '{}': {}.", origin, reason )) }; - // An Origin header is always visible ASCII — browsers punycode IDNs — - // so this is both the header-value check and a whitespace check. + if origin.eq_ignore_ascii_case("null") { + return Err(invalid( + "'null' is what a sandboxed iframe sends, so allowing it would allow any page that can open one", + )); + } + // An Origin header is always visible ASCII, so a value outside it can + // never be the string this is compared against. if !origin.chars().all(|c| c.is_ascii_graphic()) { return Err(invalid( "must contain only visible ASCII, with no whitespace", )); } - - let Some((scheme, rest)) = origin.split_once("://") else { - return Err(invalid("missing scheme")); - }; - if !scheme - .chars() - .next() - .is_some_and(|first| first.is_ascii_alphabetic()) - || !scheme - .chars() - .all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '+' || c == '-') - { - return Err(invalid("invalid scheme")); - } - if rest.is_empty() { - return Err(invalid("missing host")); - } - if rest.contains('/') { - return Err(invalid("must not contain a path or trailing slash")); - } - if rest.contains('?') || rest.contains('#') { - return Err(invalid("must not contain a query or fragment")); - } - if rest.contains('@') { - return Err(invalid("must not contain userinfo")); - } - - // An IPv6 literal is bracketed, so its own colons are not the port - // separator: splitting on the last colon would read `http://[::1]` as - // host `[:` and reject an origin a browser really does send. - let bracketed = rest.starts_with('['); - let (host, port) = match rest.strip_prefix('[') { - Some(after_bracket) => match after_bracket.split_once(']') { - Some((host, "")) => (host, None), - Some((host, tail)) => match tail.strip_prefix(':') { - Some(port) => (host, Some(port)), - None => return Err(invalid("invalid port")), - }, - None => return Err(invalid("invalid host")), - }, - None => match rest.split_once(':') { - Some((host, port)) => (host, Some(port)), - None => (rest, None), - }, - }; - if host.is_empty() { - return Err(invalid("missing host")); - } - let host_ok = if bracketed { - // Brackets promise an IPv6 literal, so parse one: a charset check - // would pass `[:::]`, which no browser can ever send. - host.parse::().is_ok() - } else { - host.chars() - .all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '-' || c == '_') - }; - if !host_ok { - return Err(invalid("invalid host")); - } - // `u16` gives the range. The digit and length checks are what keep `+80` - // and `000080` out, which it would otherwise accept as 80 and no browser - // sends; they also keep this identical to the editor's own check. - if port.is_some_and(|port| { - port.len() > 5 - || !port.chars().all(|c| c.is_ascii_digit()) - || port.parse::().is_err() - }) { - return Err(invalid("invalid port")); - } } Ok(()) diff --git a/backend/windmill-trigger-http/src/lib.rs b/backend/windmill-trigger-http/src/lib.rs index 5f5cb01c6d..0782fca6a5 100644 --- a/backend/windmill-trigger-http/src/lib.rs +++ b/backend/windmill-trigger-http/src/lib.rs @@ -712,12 +712,19 @@ mod tests { } #[test] - fn test_validate_allowed_origins_accepts_origins_and_wildcard() { + fn test_validate_allowed_origins_accepts_anything_comparable() { + // A shape that cannot match simply matches nothing, so it is the + // editor's job to warn and not this one's to refuse. Only `null` and + // values that are not header-comparable are rejected. let allowed = vec![ "https://app.example.com".to_string(), "http://localhost:3000".to_string(), - "http://[::1]".to_string(), "http://[::1]:8080".to_string(), + "chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai".to_string(), + // Never matches, but that is the caller's problem, not an error. + "https://app.example.com/".to_string(), + "https://app.example.com:99999".to_string(), + "not-an-origin".to_string(), "*".to_string(), ]; assert!(validate_allowed_origins(&allowed).is_ok()); @@ -725,32 +732,15 @@ mod tests { } #[test] - fn test_validate_allowed_origins_rejects_non_origins() { + fn test_validate_allowed_origins_rejects_null_and_uncomparable() { for invalid in [ - "https://app.example.com/", - "https://app.example.com/path", - "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", - "1://app.example.com", - "https://app.example.com:not-a-port", - "https://app.example.com:", - "https://:3000", - "https://app.example.com:99999", - "https://[notipv6]", - "https://exa[mple.com", - "https://[::1", - "https://[:::]", - "https://[1:2:3:4:5:6:7:8:9]", - "https://app.example.com:+80", - "https://app.example.com:000080", + "NULL", // Cannot be the string an Origin header is compared against. + "https://a b.com", + "https://app.example.com ", + "https://exämple.com", ] { assert!( validate_allowed_origins(&[invalid.to_string()]).is_err(), diff --git a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte index bbc394ca71..87f39bcf87 100644 --- a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte +++ b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte @@ -3,7 +3,7 @@ import Toggle from '$lib/components/Toggle.svelte' import Tooltip from '$lib/components/Tooltip.svelte' import TextInput from '$lib/components/text_input/TextInput.svelte' - import { parseAllowedOrigins } from './utils' + import { allowedOriginWarning, parseAllowedOrigins } from './utils' import type { Snippet } from 'svelte' interface Props { @@ -44,6 +44,11 @@ let inheritsInstanceDefault = $derived(!restricted && hasInstanceDefault) let origins = $derived(parseAllowedOrigins(raw)) + // Advisory: the value saves either way, so this only points out an entry + // that could never match rather than deciding what a browser may send. + let warning = $derived( + restricted ? origins.map(allowedOriginWarning).find((message) => message !== 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 @@ -102,10 +107,13 @@ error={error !== undefined} /> + slot. Red is what the API refuses, yellow is what saves but will + never match, and an empty list is neither: it allows no origin at + all, which is a state a route can legitimately be in. --> {#if error}
{error}
+ {:else if warning} +
{warning}
{:else if origins.length === 0}
Allows no origin. Add one, comma-separated, or * to allow any. diff --git a/frontend/src/lib/components/triggers/http/utils.ts b/frontend/src/lib/components/triggers/http/utils.ts index 075529abe9..d906dc20f0 100644 --- a/frontend/src/lib/components/triggers/http/utils.ts +++ b/frontend/src/lib/components/triggers/http/utils.ts @@ -20,60 +20,48 @@ export function parseAllowedOrigins(raw: string): string[] { } /** - * Mirrors `validate_allowed_origins` in windmill-common, so an entry that could - * never equal a browser `Origin` is caught in the editor rather than coming - * back as a 400. + * Entries the API refuses, mirroring `validate_allowed_origins`. + * + * Deliberately short: a stored origin is only ever compared against the + * request's `Origin`, so a shape that cannot match is dead config rather than a + * risk. `null` is the exception, since it is what every sandboxed iframe sends. */ -export function allowedOriginError(origin: string): string | undefined { +export function allowedOriginRejection(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 (origin.toLowerCase() === 'null') + return `'null' is what a sandboxed iframe sends, so it would allow any page that can open one` if (!/^[\x21-\x7e]+$/.test(origin)) return `'${origin}' must contain only visible ASCII, with no whitespace` - const separator = origin.indexOf('://') - if (separator < 0) return `'${origin}' is missing a scheme, such as https://` - const scheme = origin.slice(0, separator) - const rest = origin.slice(separator + 3) - if (!/^[A-Za-z][A-Za-z0-9.+-]*$/.test(scheme)) return `'${origin}' has an invalid scheme` - if (rest === '') return `'${origin}' is missing a host` - if (rest.includes('/')) return `'${origin}' must not contain a path or trailing slash` - if (rest.includes('?') || rest.includes('#')) - return `'${origin}' must not contain a query or fragment` - if (rest.includes('@')) return `'${origin}' must not contain userinfo` - // An IPv6 literal is bracketed, so its own colons are not the port separator - // and `http://[::1]` must not be read as host '[:'. - const bracketed = rest.startsWith('[') - const authority = bracketed - ? /^\[([^\]]*)\](?::(.*))?$/.exec(rest) - : /^([A-Za-z0-9._-]*)(?::(.*))?$/.exec(rest) - if (!authority) return `'${origin}' has an invalid host` - const [, host, port] = authority - if (host === '') return `'${origin}' is missing a host` - // Brackets promise an IPv6 literal. The URL parser is the same one that - // decides what a browser can put in an Origin, and it agrees with the - // backend's `Ipv6Addr` parse, so `[:::]` is rejected on both sides. - if (bracketed) { - try { - new URL(`http://[${host}]`) - } catch { - return `'${origin}' has an invalid host` - } - } - if (port !== undefined && !(/^[0-9]{1,5}$/.test(port) && Number(port) <= 65535)) - return `'${origin}' has an invalid port` return undefined } /** - * The first entry of a route's allowlist that could never match, if any. + * Shapes that save fine but can never equal an `Origin` header, so the route + * would read as configured while allowing nothing. * - * Derived from the stored list rather than from the field, so it stays correct - * while the editor is on another tab and the field is not even mounted. An - * empty list is not an error: it is the deny-every-origin state the backend - * accepts. + * Advisory only. What a browser sends is the caller's to know, so this points + * at the usual slips rather than deciding which origins are legitimate. + */ +export function allowedOriginWarning(origin: string): string | undefined { + if (origin === '*' || allowedOriginRejection(origin) !== undefined) return undefined + const separator = origin.indexOf('://') + if (separator < 0) return `'${origin}' has no scheme, such as https://` + const rest = origin.slice(separator + 3) + if (rest === '') return `'${origin}' has no host` + if (/[/?#]/.test(rest)) + return `'${origin}' should be scheme://host[:port], with no path, query or fragment` + if (rest.includes('@')) return `'${origin}' should not contain userinfo` + return undefined +} + +/** + * The first entry the API would refuse, if any. Derived from the stored list + * rather than the field, so it stays correct while the editor is on another tab + * and the field is not mounted. An empty list is not an error here: it is the + * deny-every-origin state the backend accepts. */ export function allowedOriginsError(allowed_origins: string[] | undefined): string | undefined { - return allowed_origins?.map(allowedOriginError).find((message) => message !== undefined) + return allowed_origins?.map(allowedOriginRejection).find((message) => message !== undefined) } /**