From 027302d93be7504e6804e33154efc37107f46c26 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 28 Aug 2026 16:56:00 +0200 Subject: [PATCH] fix: derive the origins error from the stored list and tighten host validation --- .../windmill-common/src/global_settings.rs | 18 ++++- backend/windmill-trigger-http/src/lib.rs | 4 + .../triggers/http/RouteCorsOption.svelte | 81 ++++--------------- .../triggers/http/RouteEditorInner.svelte | 14 ++-- .../src/lib/components/triggers/http/utils.ts | 46 +++++++++++ 5 files changed, 86 insertions(+), 77 deletions(-) diff --git a/backend/windmill-common/src/global_settings.rs b/backend/windmill-common/src/global_settings.rs index 76e997302a..c53d0107a8 100644 --- a/backend/windmill-common/src/global_settings.rs +++ b/backend/windmill-common/src/global_settings.rs @@ -320,6 +320,7 @@ pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Res // 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), @@ -327,7 +328,7 @@ pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Res Some(port) => (host, Some(port)), None => return Err(invalid("invalid port")), }, - None => return Err(invalid("missing host")), + None => return Err(invalid("invalid host")), }, None => match rest.split_once(':') { Some((host, port)) => (host, Some(port)), @@ -337,7 +338,20 @@ pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Res if host.is_empty() { return Err(invalid("missing host")); } - if port.is_some_and(|port| port.is_empty() || !port.chars().all(|c| c.is_ascii_digit())) { + let host_ok = if bracketed { + host.contains(':') + && host + .chars() + .all(|c| c.is_ascii_hexdigit() || c == ':' || c == '.') + } else { + host.chars() + .all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '-' || c == '_') + }; + if !host_ok { + return Err(invalid("invalid host")); + } + // `u16` is exactly the rule: digits only, and no port above 65535. + if port.is_some_and(|port| port.parse::().is_err()) { return Err(invalid("invalid port")); } } diff --git a/backend/windmill-trigger-http/src/lib.rs b/backend/windmill-trigger-http/src/lib.rs index eda7e9fa90..1499e6caa6 100644 --- a/backend/windmill-trigger-http/src/lib.rs +++ b/backend/windmill-trigger-http/src/lib.rs @@ -715,6 +715,10 @@ mod tests { "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", ] { 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 cd59f94728..0ac8b50a65 100644 --- a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte +++ b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte @@ -3,13 +3,11 @@ 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 { allowedOriginError, parseAllowedOrigins } from './utils' import type { Snippet } from 'svelte' interface Props { allowed_origins: string[] | undefined - /** Bound so the editor can block saving while the list is unusable. */ - error?: string | undefined /** Fetched once by the editor, so the badge and this field agree. */ instanceDefaultOrigins?: string[] disabled?: boolean @@ -18,7 +16,6 @@ let { allowed_origins = $bindable(), - error = $bindable(), instanceDefaultOrigins = [], disabled = false, testingBadge = undefined @@ -30,38 +27,6 @@ let raw = $state(allowed_origins?.join(', ') ?? '') let restricted = $state(allowed_origins !== undefined) - const parse = parseAllowedOrigins - - // Mirrors `validate_allowed_origins` in windmill-trigger-http so the error - // 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 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 authority = rest.startsWith('[') - ? /^\[([^\]]*)\](?::(.*))?$/.exec(rest) - : /^([^:]*)(?::(.*))?$/.exec(rest) - if (!authority) return `'${origin}' is missing a host` - const [, host, port] = authority - if (host === '') return `'${origin}' is missing a host` - if (port !== undefined && !/^[0-9]+$/.test(port)) return `'${origin}' has an invalid port` - return undefined - } - // Independent of the toggle: it decides what the toggle is called, which must // not change as it is flipped. let hasInstanceDefault = $derived( @@ -71,41 +36,21 @@ // anywhere" on an instance that has narrowed the default. let inheritsInstanceDefault = $derived(!restricted && hasInstanceDefault) - let origins = $derived(parse(raw)) - // Only a typed entry that cannot work is shown as an error. A list that is - // merely still empty is where the reader has just arrived, not a mistake to - // report back at them. + let origins = $derived(parseAllowedOrigins(raw)) + // Shown here; the editor gates the save on the same check applied to the + // stored list, so leaving this tab cannot strand a save. let malformed = $derived( - restricted ? origins.map(originError).find((message) => message !== undefined) : undefined + restricted ? origins.map(allowedOriginError).find((message) => message !== undefined) : undefined ) - // Gates the save; only `malformed` is rendered. An empty list still blocks - // saving, because storing it as NULL would mean "any origin". - $effect(() => { - error = restricted - ? (malformed ?? (origins.length === 0 ? 'Enter at least one origin' : undefined)) - : undefined - }) - - // While the toggle is on, whatever is typed is what gets saved — a rejected + // 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 - // NULL and NULL means "any origin". A typo would otherwise lift the - // restriction while the toggle still reads as on. `error` blocks the save, - // and the backend rejects the same values if one ever gets past it. + // NULL and NULL means "any origin", so a typo would lift the restriction + // while the toggle still reads as on. $effect(() => { allowed_origins = restricted ? origins : undefined }) - // This option renders on one tab only, so an unusable list must not outlive - // it: `error` alone would keep Save disabled from a screen that cannot show - // why, and clearing it alone would let a half-typed list save as a real - // restriction. Leaving restores what was there on arrival, never widening. - const mountedWith = allowed_origins - $effect(() => () => { - if (error !== undefined) allowed_origins = mountedWith - error = 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 @@ -155,14 +100,16 @@ error={malformed !== undefined} /> + 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. --> {#if malformed}
{malformed}
- {:else} + {:else if origins.length === 0}
- At least one origin, comma-separated. Use * to allow any. + Allows no origin. Add one, comma-separated, or * to allow any.
+ {:else} +
Comma-separated. Use * to allow any.
{/if} {:else if inheritsInstanceDefault}
diff --git a/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte b/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte index b38a8a8f0a..2edd1f4024 100644 --- a/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte +++ b/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte @@ -50,6 +50,7 @@ import { HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING, HUB_SCRIPT_ID, + allowedOriginsError, isOriginRestricted, parseAllowedOriginsSetting, saveHttpRouteFromCfg, @@ -120,7 +121,10 @@ let raw_string = $state(false) let wrap_body = $state(false) let allowed_origins = $state(undefined) - let allowedOriginsError = $state(undefined) + // Derived from the stored list, not reported by the field: the field only + // exists on the request-options tab, so an error owned by it would keep Save + // disabled from a screen that cannot show why. + const originsError = $derived(allowedOriginsError(allowed_origins)) // Fetched once here rather than in RouteCorsOption so the Advanced badge can // show an inherited restriction without the section being expanded. let instanceDefaultOrigins = $state([]) @@ -185,7 +189,7 @@ !can_write || pathError != '' || !isValid || - allowedOriginsError != undefined || + originsError != undefined || (!static_asset_config && emptyString(script_path)) || !hasChanged ) @@ -322,7 +326,6 @@ 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 @@ -352,10 +355,6 @@ 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 ?? '' @@ -1002,7 +1001,6 @@ origin !== '') } +/** + * 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. + */ +export function allowedOriginError(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 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 authority = rest.startsWith('[') + ? /^\[([0-9A-Fa-f.]*:[0-9A-Fa-f:.]*)\](?::(.*))?$/.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` + 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. + * + * 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. + */ +export function allowedOriginsError(allowed_origins: string[] | undefined): string | undefined { + return allowed_origins?.map(allowedOriginError).find((message) => message !== undefined) +} + /** * Read the instance-default setting, mirroring `parse_allowed_origins_setting` * in windmill-common: the settings UI writes a comma-separated string, but the