starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-21 00:02:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor: make origin validation advisory except for null and non-ascii

Browse Source
This commit is contained in:
hugocasa
2026-08-28 17:58:20 +02:00
parent c854af5f60
commit 95303f6312
4 changed files with 72 additions and 148 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/windmill-common/src/global_settings.rs
+16 -78
View File
@@ -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::<std::net::Ipv6Addr>().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::<u16>().is_err()
}) {
return Err(invalid("invalid port"));
}
}
Ok(())
backend/windmill-trigger-http/src/lib.rs
+14 -24
View File
@@ -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(),
frontend/src/lib/components/triggers/http/RouteCorsOption.svelte
+11 -3
View File
@@ -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}
/>
<!-- One line, per the form guideline's single Input -> Validation/Hint
slot. An empty list is a real state rather than a mistake: it allows
no origin at all, so say that instead of demanding an entry. -->
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}
<div class="text-2xs text-red-600 dark:text-red-400">{error}</div>
{:else if warning}
<div class="text-2xs text-yellow-600 dark:text-yellow-400">{warning}</div>
{:else if origins.length === 0}
<div class="text-2xs text-secondary">
Allows no origin. Add one, comma-separated, or * to allow any.
frontend/src/lib/components/triggers/http/utils.ts
+31 -43
View File
@@ -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)
}
/**