From 475f50c17f8cf4d9ba1c54d404561c4db24fa0f3 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Mon, 31 Aug 2026 14:08:00 +0200 Subject: [PATCH] perf: decode the cors path only when the fallback needs it --- .../windmill-api/src/triggers/http/handler.rs | 7 +++- .../src/lib/components/instanceSettings.ts | 5 +-- .../src/lib/components/triggers/http/utils.ts | 40 ++++++++++++++++--- 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/backend/windmill-api/src/triggers/http/handler.rs b/backend/windmill-api/src/triggers/http/handler.rs index cc85e473cf..7ef9f076ca 100644 --- a/backend/windmill-api/src/triggers/http/handler.rs +++ b/backend/windmill-api/src/triggers/http/handler.rs @@ -192,7 +192,10 @@ async fn conditional_cors_middleware( let origin = req.headers().get(http::header::ORIGIN).cloned(); // Owned before `next.run` consumes the request. `&Request` is not `Send` // (`Body` is not `Sync`), so nothing borrowed from it can cross the await. - let lookup = cors_lookup_method(&req).zip(cors_lookup_path(req.uri().path())); + // The URI is carried rather than the decoded path: cloning it is a refcount + // bump, while decoding allocates, and only the fallback below ever needs it. + let lookup_method = cors_lookup_method(&req); + let uri = req.uri().clone(); let resolved = ResolvedCorsPolicy::default(); req.extensions_mut().insert(resolved.clone()); @@ -208,7 +211,7 @@ async fn conditional_cors_middleware( // that failed before reaching the publish, authentication included. No // runnable produced this body, so reading the cache cannot contradict // anything. - None => match lookup { + None => match lookup_method.zip(cors_lookup_path(uri.path())) { Some((method, path)) => { resolve_cors_decision(&db, method, &path, origin.as_ref()).await } diff --git a/frontend/src/lib/components/instanceSettings.ts b/frontend/src/lib/components/instanceSettings.ts index 1ab5ad7201..857c7ae144 100644 --- a/frontend/src/lib/components/instanceSettings.ts +++ b/frontend/src/lib/components/instanceSettings.ts @@ -1,5 +1,5 @@ import type { ButtonType } from './common/button/model' -import { allowedOriginsError, parseAllowedOriginsSetting } from './triggers/http/utils' +import { allowedOriginsSettingError } from './triggers/http/utils' import { z } from 'zod' import { writable } from 'svelte/store' @@ -275,8 +275,7 @@ export const settings: Record = { 'Each origin must be visible ASCII with no comma, and there can be at most 100 of them. null is not allowed, since every sandboxed iframe sends it.', // The same check the API applies, so a value it would refuse cannot be // saved here and then silently drop to no restriction at the next boot. - isValid: (value: string | undefined) => - allowedOriginsError(parseAllowedOriginsSetting(value)) === undefined, + isValid: (value: unknown) => allowedOriginsSettingError(value) === undefined, ee_only: '', hideInQuickSetup: true }, diff --git a/frontend/src/lib/components/triggers/http/utils.ts b/frontend/src/lib/components/triggers/http/utils.ts index 492e3a94e9..89c6165d04 100644 --- a/frontend/src/lib/components/triggers/http/utils.ts +++ b/frontend/src/lib/components/triggers/http/utils.ts @@ -27,13 +27,18 @@ export function parseAllowedOrigins(raw: string): string[] { * risk. `null` is the exception, since it is what every sandboxed iframe sends. */ export function allowedOriginRejection(origin: string): string | undefined { + // Same order as `validate_allowed_origins`, so the same entry draws the same + // message on both sides rather than only the same verdict. if (origin === '*') return undefined + if (origin === '') return 'An origin must not be empty' + if (origin.length > MAX_ALLOWED_ORIGIN_LEN) + return `'${origin.slice(0, 40)}…' is longer than any origin a browser sends` + if (origin.includes(',')) + return `'${origin}' must not contain a comma, which separates entries` 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` - if (origin.length > MAX_ALLOWED_ORIGIN_LEN) - return `'${origin.slice(0, 40)}…' is longer than any origin a browser sends` return undefined } @@ -76,15 +81,38 @@ export function allowedOriginWarning(origin: string): string | undefined { // purpose: browsers send origins this cannot anticipate, `chrome-extension` // and IPv6 literals among them, and a warning that cries wolf on a working // origin is worse than one that stays quiet. - const port = rest.startsWith('[') - ? rest.slice(rest.indexOf(']') + 1).replace(/^:/, '') - : rest.split(':')[1] if (rest.startsWith(':')) return `'${origin}' has no host` - if (port !== undefined && port !== '' && !(/^[0-9]{1,5}$/.test(port) && Number(port) <= 65535)) + const portStart = rest.startsWith('[') ? rest.indexOf(']') + 1 : rest.indexOf(':') + // A trailing colon is a port, an empty one — distinct from having none. + const port = portStart > 0 && rest[portStart] === ':' ? rest.slice(portStart + 1) : undefined + if (port !== undefined && !(/^[0-9]{1,5}$/.test(port) && Number(port) <= 65535)) return `'${origin}' has a port no browser can send` return undefined } +/** + * What the settings API would refuse, mirroring `parse_allowed_origins_setting` + * in windmill-common. + * + * Distinct from reading the setting for display: that drops entries it cannot + * use, while this has to report them, or a shape only the YAML editor can + * produce would pass here and come back as a 400 on save. + */ +export function allowedOriginsSettingError(setting: unknown): string | undefined { + let origins: string[] + if (setting == null || typeof setting === 'string') { + origins = parseAllowedOrigins(typeof setting === 'string' ? setting : '') + } else if (Array.isArray(setting)) { + if (setting.some((entry) => typeof entry !== 'string')) return 'Entries must be strings' + // Not filtered for empties, unlike the comma-separated form, where a + // trailing separator is a typing artifact rather than an entry. + origins = setting.map((entry) => (entry as string).trim()) + } else { + return 'Expected a comma-separated string or a list of strings' + } + return allowedOriginsError(origins) +} + /** * Read the instance-default setting, mirroring `parse_allowed_origins_setting` * in windmill-common: the settings UI writes a comma-separated string, but the