diff --git a/backend/windmill-common/src/global_settings.rs b/backend/windmill-common/src/global_settings.rs index c53d0107a8..a28348aa5c 100644 --- a/backend/windmill-common/src/global_settings.rs +++ b/backend/windmill-common/src/global_settings.rs @@ -339,10 +339,9 @@ pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Res return Err(invalid("missing host")); } let host_ok = if bracketed { - host.contains(':') - && host - .chars() - .all(|c| c.is_ascii_hexdigit() || c == ':' || c == '.') + // 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 == '_') @@ -350,8 +349,14 @@ pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Res 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()) { + // `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")); } } diff --git a/backend/windmill-trigger-http/src/lib.rs b/backend/windmill-trigger-http/src/lib.rs index 1499e6caa6..a08769d193 100644 --- a/backend/windmill-trigger-http/src/lib.rs +++ b/backend/windmill-trigger-http/src/lib.rs @@ -719,6 +719,10 @@ mod tests { "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", ] { 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 0ac8b50a65..bbc394ca71 100644 --- a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte +++ b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte @@ -3,11 +3,17 @@ import Toggle from '$lib/components/Toggle.svelte' import Tooltip from '$lib/components/Tooltip.svelte' import TextInput from '$lib/components/text_input/TextInput.svelte' - import { allowedOriginError, parseAllowedOrigins } from './utils' + import { parseAllowedOrigins } from './utils' import type { Snippet } from 'svelte' interface Props { allowed_origins: string[] | undefined + /** + * Why the list cannot be saved, owned and derived by the editor. Passed + * one way: this component displays it and never writes it back, so it + * cannot outlive the tab or go stale against the stored value. + */ + error?: string | undefined /** Fetched once by the editor, so the badge and this field agree. */ instanceDefaultOrigins?: string[] disabled?: boolean @@ -16,6 +22,7 @@ let { allowed_origins = $bindable(), + error = undefined, instanceDefaultOrigins = [], disabled = false, testingBadge = undefined @@ -37,11 +44,6 @@ let inheritsInstanceDefault = $derived(!restricted && hasInstanceDefault) 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(allowedOriginError).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 @@ -97,13 +99,13 @@ - {#if malformed} -
{malformed}
+ {#if error} +
{error}
{: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/RouteEditorInner.svelte b/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte index 2edd1f4024..7647945b9b 100644 --- a/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte +++ b/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte @@ -121,10 +121,6 @@ let raw_string = $state(false) let wrap_body = $state(false) let allowed_origins = $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([]) @@ -167,6 +163,21 @@ let suspendedJobsModal = $state(null) let originalConfig = $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. + // + // An empty list denies every origin, which a route can legitimately be in + // and must stay editable in. Only turning the toggle on and saving without + // naming one is refused, since that reads as "I restricted this", not "I + // locked every browser out". Both sides come from editor state, so nothing + // here can go stale the way a component-held snapshot did. + const originsError = $derived( + allowedOriginsError(allowed_origins) ?? + (allowed_origins?.length === 0 && originalConfig?.allowed_origins?.length !== 0 + ? 'Enter at least one origin, or turn this off' + : undefined) + ) let userSettings = $state(undefined) let hasChanged = $derived(!deepEqual(getRouteConfig(), originalConfig ?? {})) @@ -1001,6 +1012,7 @@