Skip to content

feat: add per-route CORS origin allowlist for HTTP triggers - #10833

Open
hugocasa wants to merge 13 commits into
mainfrom
http-trigger-cors-config
Open

feat: add per-route CORS origin allowlist for HTTP triggers#10833
hugocasa wants to merge 13 commits into
mainfrom
http-trigger-cors-config

Conversation

@hugocasa

@hugocasa hugocasa commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #10826.

Every response from /api/r/* is stamped Access-Control-Allow-Origin: *, and a route owner has no way to narrow it. The documented escape hatch — returning wm_headers from the runnable — is narrower than it looks: it is applied by result_to_response, which is only reached from the sync path, so async, sync_sse and static-asset routes never had it either.

The structural gap is the preflight. OPTIONS was answered by the router with an empty body before any trigger lookup happened, and the middleware then stamped the permissive defaults onto it because its skip-if-present logic only skips headers the response already carries. No amount of wm_headers can reach that — by the time the runnable returns, a route whose purpose is a side effect has already run.

This adds an origin allowlist at two levels, enforced in the middleware for both the preflight and the response.

Resolution order

route allowed_originshttp_route_default_allowed_origins instance setting → *

State Behaviour
Neither configured (every instance today) Unchanged: ACAO: *, and wm_headers still wins on sync routes
Instance default set, route NULL The default governs the route
Route list set It governs, overriding the instance default
Route list is ["*"] Explicit opt-out: no restriction at all, back to the pre-feature behaviour including the wm_headers escape hatch
Route list is [] A restriction matching nothing — distinct from NULL, which inherits

Because the preflight is answered before any code runs, config is the only thing it can consult. So whenever an allowlist is in effect it also bounds wm_headers: letting the response widen what the preflight advertised would make the two disagree and leave the allowlist bounding nothing. A route that genuinely computes its own origin opts out with ["*"].

The instance setting is a default, not a ceiling — a workspace user can still set ["*"] on their own route. It is a house-style convenience for self-hosted admins, not a tenant boundary; against an untrusted tenant the control is authentication_method, since CORS only stops a browser reading a response, not the request itself. It is an instance setting rather than an env var so a malformed origin is rejected at write time — a typo matches no request, so via env it would silently block the very app it names.

Access-Control-Allow-Credentials is still never set, so this is not a cookie-CSRF change. The exposure it closes is unauthenticated routes, and routes whose credential is a header the calling page already holds.

Workspace-level defaults are deliberately left out — a tenant can already do this per route, and inserting a workspace step into the resolution order later is non-breaking.

Changes

  • Migration adding nullable allowed_origins TEXT[] to http_trigger
  • allowed_origins on TriggerRoute, HttpConfig and HttpConfigRequest, threaded through create, update, the refresh_routers query, and the workspace-fork clone
  • HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS instance setting, validated on write and hot-reloaded (no restart, and no router-cache bump — it is read per request)
  • match_origin (exact, case-insensitive, echoes the request origin only on a match — never reflects unchecked) and validate_allowed_origins (rejects paths, queries, userinfo, whitespace, non-ASCII and null, since a sandboxed iframe sends Origin: null), shared by the route field and the setting
  • conditional_cors_middleware resolves the trigger from HTTP_ROUTERS_CACHE and is now the single place CORS headers are decided, covering the preflight without a second code path that could drift
  • RouteCorsOption.svelte: a "Restrict origins" toggle and comma-separated field in Advanced → Request Options, with inline validation mirroring the backend, and the inherited instance default shown when the route sets none

Screenshots

A route's own allowlist, with inline validation mirroring the backend's rules:

allowed origins with validation

With an instance default configured, the toggle relabels to "Override allowed origins" and names what is currently in effect, so an off toggle never reads as "callable from anywhere". The CORS badge on the Advanced header shows the restriction without the section being expanded:

inherited instance default

Test plan

Exercised against a running instance built with --features quickjs,http_trigger (/api/r/* is not mounted in the default dev build).

  • Backwards compatibility: with nothing configured at either level, a route answers ACAO: * with the full method list on both preflight and response, and wm_headers still overrides it
  • Allowed origin → echoed back with Vary: origin; disallowed → no ACAO on both preflight and response
  • Instance default governs a route with NULL, applies without a restart, and bounds wm_headers
  • A route's own list overrides the instance default (the default's origins are then denied)
  • ["*"] on a route opts out of the instance default entirely — wm_headers wins again, full method list, no Vary
  • Clearing the setting and the route restores pre-feature behaviour exactly
  • Real browser: a page on localhost:3140 fetching the route on localhost:8140 reads the body from an allowed origin; from a disallowed one Chromium reports "Response to preflight request doesn't pass access control check" and the job count is unchanged — the runnable never executes
  • Percent-encoded paths (/api/r/corspr%6fbe) resolve to the same trigger the handler serves, so the allowlist cannot be stepped around by re-encoding a character
  • An unresolved route answers identically to an unknown path, so this is not a route-existence oracle
  • Invalid origins rejected at write time by both the trigger API and the settings API; the UI blocks Save rather than persisting NULL
  • UI round-trip: set, save, reopen, clear; the field follows a reset-to-deployed, and a validation error does not leak across triggers
  • The route editor shows the inherited default, and the CORS badge on the Advanced header tracks the same rule the backend applies: on for a route list or an inherited default, off when nothing is configured or the route opted out with ["*"]
  • A non-superadmin can read http_route_default_allowed_origins (so the inherited hint renders for the people who did not set it), while license_key stays superadmin-only
  • Workspace-fork clone carries the column
  • cargo check --features http_trigger and --features http_trigger,enterprise,private; 112 unit tests in windmill-trigger-http; npm run check 0 errors; sqlx cache regenerated (+5 entries, 0 lost)

Not exercised at runtime: the branch where the routers are genuinely unloadable because the database is unreachable. Reaching it needs a failed load and a cold cache at once, which is only reproducible by disrupting the Postgres shared with other worktrees. It fails closed by omitting the header.

🤖 Generated with Claude Code

hugocasa and others added 3 commits August 25, 2026 14:10
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 25, 2026

Copy link
Copy Markdown

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8a9a028
Status: ✅  Deploy successful!
Preview URL: https://be817055.windmill.pages.dev
Branch Preview URL: https://http-trigger-cors-config.windmill.pages.dev

View logs

hugocasa and others added 7 commits August 25, 2026 15:30
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…xists

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@hugocasa

Copy link
Copy Markdown
Collaborator Author

/review

@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @hugocasa's task in 5m 45s —— View job


cc @hugocasa

Should address issues before merging: empty allowed_origins list fails open and silently escapes the instance default (effective_allowed_origins, contradicting the PR description, the OpenAPI text and the test's own comment).

The design is sound and the hard part — moving the CORS decision into the middleware so the preflight is covered by the same code that stamps the response — is done well: the origin is never reflected unchecked, Vary: origin is appended (not inserted) whenever the answer depends on the request, the routers-unavailable branch omits the header rather than falling back to *, and cors_lookup_path percent-decodes so the middleware and get_http_route_trigger agree on the lookup key. Backwards compatibility is preserved for the unconfigured case, which is the thing that mattered most here.

Findings

P1 — allowed_origins: [] is treated as no restriction, and bypasses http_route_default_allowed_originsbackend/windmill-trigger-http/src/lib.rs:227-232 (inline)

Some([]) hits !effective.is_empty() and returns None, which the middleware reads as unrestricted → Access-Control-Allow-Origin: *. A route stored with '{}' therefore ends up more permissive than one left NULL under a configured instance default. The PR table ("A restriction matching nothing — distinct from NULL, which inherits"), the test comment at lib.rs:665 (whose assertion pins the opposite), and the OpenAPI description all describe the other behaviour. The UI blocks the empty list, but validate_allowed_origins(&[]) is Ok and HttpConfigRequest accepts it, so the CLI / git-synced trigger yaml / the copilot tool schema can all reach it — and the failure direction is open. Either make Some(list) always a restriction (falling back to the default only on None), or normalize []NULL at write time and correct the three descriptions.

P2 — frontend originError does not mirror the backend's ASCII/whitespace rulefrontend/src/lib/components/triggers/http/RouteCorsOption.svelte:37 (inline)

https://a b.com and non-ASCII IDNs pass the inline check and 400 on save, which is the case the comment above the function says it exists to prevent.

P2 — HEAD maps to Get for the request but not for a HEAD preflightbackend/windmill-api/src/triggers/http/handler.rs:54-62 (inline)

Access-Control-Request-Method: HEAD resolves to nothing, so the preflight advertises * and the full method list even for a restricted route, while the real HEAD gets the restricted answer. No leak (the HEAD handler returns an empty body), just the two halves disagreeing.

P2 — unrelated formatting churn in backend/src/monitor.rs

Seven blank-line deletions and three rewraps (store_min_keep_alive_version, reload_critical_alert_mute_ui_setting, the tracing::warn! in SettingsPass) in functions this feature does not touch. AGENTS.md: "Keep changes focused — don't refactor beyond what's asked". They make the diff harder to read against a file this large; worth reverting.

P2 — allowedOriginsError outlives the tab it is set onfrontend/src/lib/components/triggers/http/RouteEditorInner.svelte:999

RouteCorsOption only renders on the request-options tab, so turning the toggle on with an empty field and then switching to the retries tab leaves Save disabled with nothing on screen explaining why. The reset on cfg load handles the cross-trigger case; this is the cross-tab one.

AGENTS.md compliance

Migration created with the right tooling and reversible; sqlx cache regenerated (5 new entries) rather than hand-edited; system_prompts/auto-generated/ and cli/src/guidance/skills.gen.ts regenerated as the CLI-docs rule requires; the new field is threaded through create, update, refresh_routers, ADDITIONAL_SELECT_FIELDS and the workspace-fork clone, so no read path silently drops it; no crypto.randomUUID; the new Svelte component uses runes and $bindable props consistently with its siblings. One thing to weigh against "Comments record constraints, not narration": the same rationale (preflight answered before any code runs → config bounds wm_headers) is restated in effective_allowed_origins' doc, in the middleware body, in the OpenAPI description and in the component comments. Stating it once at the decision site would read better, and several of the new blocks run well past the ≤4-line guideline.

Test coverage

Backend — 12 new unit tests in windmill-trigger-http cover match_origin (exact, case-insensitive, prefix non-match, missing header), allows_any_origin, effective_allowed_origins and validate_allowed_origins' rejection set, which is the right granularity for the pure logic. What has no automated coverage is the part this PR actually restructures: conditional_cors_middleware — preflight vs response, an allowlist overriding a wm_headers ACAO, and the CorsRouteLookup::Unavailable fail-closed branch. backend/windmill-api-integration-tests/tests/triggers.rs already stands up HTTP triggers, so a test that fires an OPTIONS with Access-Control-Request-Method against a route with an allowlist and asserts the header on both allowed and disallowed origins would pin the exact behaviour a future refactor of that middleware could quietly reopen. Note the empty-list case is currently pinned by a test asserting the opposite of what the PR documents — whichever way that is resolved, that assertion should move with it.

FrontendparseAllowedOrigins / isOriginRestricted in utils.ts are pure, but http/utils.ts has no sibling *.test.ts and the repo does not test these components, so nothing is expected here.

Manual verification still worth doing before merge — the PR's test plan is unusually thorough (real-browser preflight rejection, percent-encoded path equivalence, route-existence oracle, hot reload of the setting) and covers the paths I would otherwise have asked for. What it does not cover is the case above: set a route's allowed_origins to [] through the API (not the UI) on an instance that has http_route_default_allowed_origins configured, then call the route from an origin not in the instance default and confirm what Access-Control-Allow-Origin comes back — today that answers *. Worth re-running after whichever fix is chosen. The other one is a static-website route under an allowlist: fetch a subpath asset and confirm the /*wm_subpath match yields the same allowlist decision as the base path.

--- · branch http-trigger-cors-config

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment on lines +227 to +232
let effective = match route_allowed_origins {
Some(route_allowed_origins) => route_allowed_origins.clone(),
None => HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS.load().as_ref().clone(),
};

(!effective.is_empty() && !allows_any_origin(&effective)).then_some(effective)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — an explicit empty allowlist fails open, and escapes the instance default.

Some([]) short-circuits on !effective.is_empty() and returns None, which the middleware reads as "no restriction" and answers with Access-Control-Allow-Origin: *. So a route stored with allowed_origins = '{}' is more permissive than one left NULL on an instance that has http_route_default_allowed_origins set — the empty list bypasses the default entirely.

That contradicts three places that document the opposite:

  • the PR description table: "Route list is [] → A restriction matching nothing — distinct from NULL, which inherits"
  • the test comment at lib.rs:665: "An empty route list is a restriction that matches nothing, distinct from NULL which inherits the instance default" — while the assertion right below it pins None
  • by implication, the OpenAPI description ("When set, the list governs both the preflight and the response")

The UI blocks it (Enter at least one origin), but validate_allowed_origins(&[]) is Ok, HttpConfigRequest accepts "allowed_origins": [], and the insert/update store it verbatim — so anything driving the API directly (CLI, git-sync'd trigger yaml, the copilot tool whose Zod schema now accepts z.array(z.string())) can reach it, and the failure direction is open.

Either make Some(list) always a restriction and only fall back to the default on None:

match route_allowed_origins {
    Some(list) if allows_any_origin(list) => None,
    Some(list) => Some(list.clone()),
    None => {
        let default = HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS.load().as_ref().clone();
        (!default.is_empty() && !allows_any_origin(&default)).then_some(default)
    }
}

…or normalize [] to NULL at write time and fix the doc/test comments to say "an empty list is stored as NULL and inherits". Right now the code and every description of it disagree.

Fix this →

Comment on lines +35 to +49
// 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
const [scheme, ...rest] = origin.split('://')
if (rest.length !== 1) return `'${origin}' is missing a scheme, such as https://`
const host = rest[0]
if (host === '') return `'${origin}' is missing a host`
if (host.includes('/')) return `'${origin}' must not contain a path or trailing slash`
if (host.includes('?') || host.includes('#'))
return `'${origin}' must not contain a query or fragment`
if (host.includes('@')) return `'${origin}' must not contain userinfo`
if (!/^[A-Za-z0-9.+-]+$/.test(scheme)) return `'${origin}' has an invalid scheme`
return undefined
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — this does not fully mirror validate_allowed_origins, so some entries still 400 from the API.

The backend's first check is origin.chars().all(|c| c.is_ascii_graphic()), which rejects embedded whitespace and any non-ASCII. originError has no equivalent, so https://a b.com (space inside the host, not trimmed by parseAllowedOrigins) and https://münchen.de both pass the inline check, enable Save, and then come back as a 400 — the exact thing the comment above says this function exists to avoid. One extra guard covers both:

if (!/^[\x21-\x7e]+$/.test(origin)) return `'${origin}' must contain only visible ASCII, with no whitespace`

Fix this →

Comment on lines +54 to +62
if method == http::Method::OPTIONS {
req.headers()
.get(http::header::ACCESS_CONTROL_REQUEST_METHOD)
.and_then(|method| method.to_str().ok())
.and_then(|method| http::Method::try_from(method).ok())
.as_ref()
.and_then(|method| HttpMethod::try_from(method).ok())
} else if method == http::Method::HEAD {
Some(HttpMethod::Get)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — HEAD is folded into Get for the real request but not for its preflight.

HttpMethod::try_from(&Method::HEAD) returns Err (lib.rs:84-96), so a preflight carrying Access-Control-Request-Method: HEAD — which browsers do send when the HEAD request carries a non-safelisted header, e.g. Content-Type: application/json — resolves to None, and the preflight is answered with the permissive * / full method list even for a route under an allowlist. The subsequent HEAD then takes the else if branch, resolves the GET router, and gets the restricted answer, so the two disagree.

No leak follows (the .head(|| async { "" }) handler returns an empty body, and the real response is still restricted), but the same mapping in both branches would be one line:

.and_then(|method| if method == http::Method::HEAD { Some(HttpMethod::Get) } else { HttpMethod::try_from(&method).ok() })

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@hugocasa
hugocasa marked this pull request as ready for review August 26, 2026 12:27
@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @hugocasa's task in 4m 12s —— View job


cc @hugocasa

Mergeable, but should ideally address nits: isOriginRestricted no longer mirrors the backend for [], unrelated rustfmt churn in monitor.rs, allowedOriginsError still outlives the tab it is set on, and the "keeps the previous value" comment in apply_http_route_default_allowed_origins_setting doesn't hold at boot.

Re-reviewed at 25c8fda. All three findings from the previous round are fixed, and fixed correctly:

  • Some([]) now denies rather than falling through to the instance default (effective_allowed_origins, lib.rs:224-238). The test comment and its assertion now agree, and the added match_origin(&[], …) == None pins the middleware-visible half of it.
  • routable_method folds HEAD → Get for the preflight and the request through one function, so Access-Control-Request-Method: HEAD resolves the same route the HEAD itself does. Narrowing Access-Control-Allow-Methods to "GET, OPTIONS" is still correct for it — HEAD is a CORS-safelisted method, so the Fetch spec's preflight check passes without it being listed.
  • originError now carries the visible-ASCII guard, matching is_ascii_graphic() on the backend (\x21-\x7e is exactly that range).

I re-walked the parts the previous round only asserted rather than checked, and they hold: nest wraps each inner route in StripPrefix, so req.uri().path() in the middleware really is the path StripPath::to_path() sees, and cors_lookup_path's decode/strip/trim reproduces format!("/{}", route_path.to_path().trim_end_matches("/")) exactly; the HTTP_ROUTERS_CACHE read guard is dropped before next.run, so no lock is held across the handler; Extension<DB> is layered on the app router above the /r nest, so the extractor resolves.

Findings

P2 — isOriginRestricted diverges from effective_allowed_origins on the empty listfrontend/src/lib/components/triggers/http/utils.ts:32 (inline)

allowed_origins = [] gives effective.length === 0 → not restricted, so the CORS badge reads "unrestricted" for a route that now denies every origin. Cosmetic and only reachable via the API, but the doc comment claims the two mirror each other and this is the one input where they no longer do.

P2 — unrelated rustfmt churn in backend/src/monitor.rs (unchanged since the last round)

Seven blank-line deletions and three rewraps (store_min_keep_alive_version, reload_critical_alert_mute_ui_setting, the tracing::warn! in SettingsPass::run) in functions this feature doesn't touch. AGENTS.md: "Keep changes focused — don't refactor beyond what's asked". Against a 6k-line file it's noise in the diff; worth reverting.

P2 — allowedOriginsError still outlives the tab it is set onfrontend/src/lib/components/triggers/http/RouteEditorInner.svelte:1003

The reset added on cfg load fixes the cross-trigger case. The cross-tab one remains: RouteCorsOption only renders on the request-options tab, so toggling "Allowed origins" on with an empty field and switching to the retries tab leaves Save disabled with nothing on screen saying why (the component's $effect is torn down on unmount, so the stale 'Enter at least one origin' just sits there).

P2 — the "keeps the previous value" rationale doesn't hold on the boot pathbackend/src/monitor.rs:6314

A bad value leaves the previous list in place rather than falling back to no restriction: silently widening CORS instance-wide is the worse failure.

True for a hot reload, but initial_load runs this against HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS's initial vec![] — so at boot "the previous list" is no restriction, and a stored value that fails to parse silently widens the whole instance with only a log line. Write-time validation makes it hard to reach, so the code is fine; it's the comment that overstates what it guarantees. Either say "on reload" or make the boot case refuse to serve routes unrestricted.

Nit — comment budget. AGENTS.md: "State each invariant once, at the place where someone would break it, in ≤4 lines." The preflight-bounds-wm_headers rationale is now stated in effective_allowed_origins' doc, in the middleware body (6 lines), in the OpenAPI description and in RouteCorsOption's comments; several of the new blocks in handler.rs and RouteCorsOption.svelte run 5-6 lines. This was raised last round and is a judgement call — flagging once, not blocking.

AGENTS.md compliance

Migration created with cargo sqlx migrate add -r and reversible; sqlx cache regenerated (5 new entries) rather than hand-edited; system_prompts/auto-generated/ and cli/src/guidance/skills.gen.ts regenerated per the CLI-docs rule; allowed_origins threaded through insert, both update branches, refresh_routers, ADDITIONAL_SELECT_FIELDS and the workspace-fork clone, so no read path drops it; no crypto.randomUUID; the new component uses runes and $bindable consistently with its siblings; no customer attribution anywhere. The two-way $effect sync in RouteCorsOption (write allowed_origins from raw, re-seed raw on an external write) converges because the re-seed compares against what the component would itself produce — worth keeping the comment that says so if this is ever refactored.

Test coverage

Backend — 12 unit tests in windmill-trigger-http cover match_origin, allows_any_origin, effective_allowed_origins (including the now-corrected empty-list case) and validate_allowed_origins' rejection set. Right granularity for the pure logic.

Still uncovered by anything automated is the piece this PR actually restructures: conditional_cors_middleware. Preflight vs response, an allowlist overriding a wm_headers ACAO, and the CorsRouteLookup::Unavailable fail-closed branch all live in one function that a future refactor could quietly reopen, and backend/windmill-api-integration-tests/tests/triggers.rs already stands up HTTP triggers. An OPTIONS with Access-Control-Request-Method against a route with an allowlist, asserting Access-Control-Allow-Origin on an allowed and a disallowed origin, would pin it in a handful of lines. Not a merge blocker given how thorough the manual plan is, but it's the one thing that would keep this correct a year from now.

FrontendparseAllowedOrigins / isOriginRestricted are pure and would be testable, but http/utils.ts has no sibling *.test.ts and the repo doesn't test these components, so nothing is expected. If the isOriginRestricted fix above lands, it's worth eyeballing the badge on a route whose list is [].

Manual verification still worth doing — the previously-requested case is now the regression check for the fix: set a route's allowed_origins to [] through the API (not the UI) on an instance with http_route_default_allowed_origins configured, call it from an origin in the instance default, and confirm no Access-Control-Allow-Origin comes back at all (it answered * before 25c8fda). Separately, a HEAD against a restricted route with a non-safelisted header, to confirm the preflight and the HEAD now resolve the same route — that's the path routable_method was added for and the only new behaviour not covered by the existing plan. The static-website subpath case (/*wm_subpath under an allowlist) is also still worth one fetch.

--- · · branch http-trigger-cors-config

instanceDefaultOrigins: string[]
): boolean {
const effective = allowed_origins ?? instanceDefaultOrigins
return effective.length > 0 && !effective.includes('*')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — this no longer mirrors effective_allowed_origins for the empty list.

Since 25c8fda the backend treats Some([]) as a restriction that matches nothing, but here allowed_origins = [] gives effective.length === 0false, so the CORS badge on the Advanced header reads "unrestricted" for a route that in fact denies every origin. The doc comment above says it mirrors the Rust function, which it now doesn't in exactly that case.

Only reachable for a route whose list was set to [] through the API (the UI blocks it), so it's cosmetic — but it's the one input where the two implementations disagree, and the comment claims they don't.

Suggested change
return effective.length > 0 && !effective.includes('*')
if (allowed_origins !== undefined) return !allowed_origins.includes('*')
return instanceDefaultOrigins.length > 0 && !instanceDefaultOrigins.includes('*')
}

@github-actions

Copy link
Copy Markdown
Contributor

Codex Review

cc @hugocasa

Mergeable, but should ideally address nits: incomplete origin validation, editor misreports valid allowlist states, static-site CORS controls are hidden, raw HTML in the new Svelte component

Findings

P2 — Origin validation accepts values no browser can sendbackend/windmill-common/src/global_settings.rs:297

The scheme check allows a digit or punctuation as the first character, while the authority is never parsed as a host and optional numeric port. Consequently, values such as 1://host and https://example.com:not-a-port pass both APIs but can never match a browser Origin, contradicting the write-time validation contract. Parse the origin structurally or validate the full scheme/host/port grammar; mirror the fix in RouteCorsOption.svelte.

P2 — The editor misreports two valid backend allowlist statesfrontend/src/lib/components/triggers/http/RouteEditorInner.svelte:132, frontend/src/lib/components/triggers/http/utils.ts:33

The backend explicitly accepts an instance default as a JSON array, but the editor converts every non-string setting to []. Separately, the backend treats a route-level [] as “deny every origin,” while isOriginRestricted([]) returns false. Defaults set through the API and empty route lists can therefore produce the wrong badge, toggle label, and inherited-policy hint.

P2 — Static website triggers cannot view or configure CORS in the editorfrontend/src/lib/components/triggers/http/RouteEditorInner.svelte:786

RouteCorsOption and its badge are inside the existing !is_static_website block. Although the middleware enforces allowlists for static website responses, their owners cannot inspect, add, or remove a route-level policy through the UI.

P2 — The new component uses raw HTML feedback elementsfrontend/src/lib/components/triggers/http/RouteCorsOption.svelte:141

The validation and hint blocks introduce raw <div> elements. frontend/AGENTS.md requires: “UI components: use Windmill's design-system components — never raw HTML elements. Start from the barrel src/lib/components/common/index.ts and grep src/lib/components/; the component you need almost certainly exists.”

Test coverage

Backend unit tests cover matching, wildcard/empty semantics, and several validation cases, but not settings parsing or middleware integration. Frontend component tests are not generally expected; focused coverage of the new pure helper states would guard the array/empty-list mismatch.

Manually verify malformed schemes and ports are rejected through both trigger and settings APIs.

Configure the instance default as a JSON array, open an API-created allowed_origins: [] route, and edit a static website trigger; the badge, hint, toggle, and saved policy should all match backend behavior.

@github-actions

Copy link
Copy Markdown
Contributor

Pi Review

cc @hugocasa

Mergeable, but should ideally address nits: isOriginRestricted badge disagrees with the backend on a deny-all ([]) route; allowedOriginsError still survives a tab switch.

The P1 from the prior pass (empty allowlist failing open and silently escaping the instance default) is fixed: effective_allowed_origins now returns Some(vec![]) for [] and match_origin turns that into "no ACAO" — deny, not fall-through. The HEAD-preflight mapping (routable_method) and the frontend ASCII/whitespace validation mirror are also fixed. The design (single CORS decision point in the middleware, echo-on-match only, Vary: origin appended, fail-closed on unreadable routers) holds up.

Findings

P2 — isOriginRestricted reports a deny-all route as unrestricted, contradicting effective_allowed_originsfrontend/src/lib/components/triggers/http/utils.ts:28-33 (used at RouteEditorInner.svelte:798)

The doc comment claims it mirrors effective_allowed_origins, but it does not for the empty-list case. A route stored with allowed_origins: [] now means "allow no origin at all" on the backend (effective_allowed_origins(Some(&[]))Some([]), so the middleware omits ACAO and even overrides wm_headers), yet isOriginRestricted([], …) evaluates effective.length > 0false, so the Advanced-header CORS badge renders as off for a route that is in fact maximally restricted. The two surfaces disagree on a config the API/CLI/git-sync can reach (validate_allowed_origins(&[]) is Ok); only the UI blocks saving it. Either treat [] as restricted in isOriginRestricted, or normalize [] to NULL at write time so the two can never diverge.

P2 — allowedOriginsError still outlives the tab it is set onfrontend/src/lib/components/triggers/http/RouteEditorInner.svelte:355-358 / RouteCorsOption.svelte

The reset added on cfg load closes the cross-trigger case, but RouteCorsOption only renders on the request options tab. Turning the toggle on with an empty field sets allowedOriginsError = 'Enter at least one origin', and switching to the retries tab unmounts the component without clearing the bound error — canSave stays false with nothing on screen explaining why. A $effect cleanup (or resetting the error on tab change) would close the last hole.

Considered and dismissed

  • Empty list failing open / bypassing the instance default — fixed by 25c8fda; effective_allowed_origins(Some(&[])) now returns Some(vec![]), and the middleware's match_origin(&[], …) yields no ACAO.
  • HEAD preflight resolving nothing — fixed: routable_method maps HEADGet for both the request method and Access-Control-Request-Method.
  • Frontend originError missing the ASCII/whitespace rule — fixed: the ^[\x21-\x7e]+$ guard now matches the backend's is_ascii_graphic check (verified entry-by-entry against validate_allowed_origins).
  • /api/r prefix mismatch in cors_lookup_path — dismissed: the middleware is layered on the inner router nested at /api/r, and axum's nest strips the prefix before the inner service runs, so req.uri().path() is /corsprobe, matching get_http_route_trigger's key. Consistent with the documented percent-encoding and real-browser tests.
  • refresh_routers(db) single-arg vs two-arg — dismissed: at the PR head refresh_routers takes only (db); the two-arg form I saw is from a local merge of main into the checkout, not part of this diff.
  • wm_headers override, Vary re-append, per-request clone of the default, OPTIONS-without-Access-Control-Request-Method, unknown-path-plus-default — all reviewed and benign: fail-closed or informational, no data exposure.
  • monitor.rs blank-line/rewrap churn — dismissed as formatter output; not reported per the "no style nits" policy.

Test coverage

  • Backend — good: 112 unit tests including new cases for match_origin (exact, case-insensitive, no-prefix-match, missing header), effective_allowed_origins (route preference, [] deny, * opt-out), and validate_allowed_origins (accept/reject). Noted gap: the middleware helpers cors_lookup_path, cors_lookup_method, and routable_method are pure functions in windmill-api with no unit tests; the percent-decode and HEAD behaviors are only covered by the documented manual browser pass. Worth a couple of table tests if this path is ever refactored, not a blocker.
  • Frontendhttp/utils.ts has no sibling *.test.ts, so no component/util tests expected by convention; the isOriginRestricted mismatch above is the kind of pure-logic drift a small test would have caught, but that's the finding, not a coverage demand.
  • Migration / generated files — config-only; no automated tests expected.

Manual verification already documented by the author (real-browser allow/deny, preflight, wm_headers bounding, hot reload, UI round-trip) is comprehensive; the one unexercised path (cold cache + unreachable DB) fails closed by omitting the header, which is the correct behavior and acceptable to leave unverified.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 issues found across 24 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="frontend/src/lib/components/instanceSettings.ts">

<violation number="1" location="frontend/src/lib/components/instanceSettings.ts:270">
P2: The new http_route_default_allowed_origins setting has no UI validation, but the backend rejects invalid origins on write (parse_allowed_origins_setting/validate_allowed_origins). Save is only blocked for settings that define isValid, and saveSettings() awaits setInstanceConfig without try/catch, so typing an invalid origin makes the whole bulk save of all instance settings fail with no visible error. Add isValid/error mirroring the backend rules (reject paths, queries, userinfo, whitespace, non-ASCII, "Origin: null") like email_domain/webhook_base_url do, so bad input is caught before submit.</violation>
</file>

<file name="backend/windmill-api-settings/src/lib.rs">

<violation number="1" location="backend/windmill-api-settings/src/lib.rs:1021">
P1: When `http_route_default_allowed_origins` is supplied through `sync-config` or operator reconciliation, this hook is bypassed because those paths apply settings directly. Validate this key in the shared declarative write path too; otherwise malformed values persist and runtime keeps the empty allowlist, leaving CORS unrestricted.</violation>
</file>

<file name="backend/src/monitor.rs">

<violation number="1" location="backend/src/monitor.rs:427">
P1: When the global-settings read fails while route lookup still succeeds, `SettingsPass` skips this applier, leaving the new `ArcSwap` empty; middleware then treats the default as unset and emits `*`. Preserve a last-known value or mark the default unresolved and omit CORS until it is successfully read, otherwise a transient settings-query failure bypasses a configured allowlist.</violation>
</file>

<file name="backend/windmill-api/src/triggers/http/handler.rs">

<violation number="1" location="backend/windmill-api/src/triggers/http/handler.rs:220">
P2: When router resolution fails but the route later succeeds, `wm_headers` can leave an `Access-Control-Allow-Origin` header in the response. Remove that header for `CorsRouteLookup::Unavailable` instead of only skipping the fallback insertion.</violation>

<violation number="2" location="backend/windmill-api/src/triggers/http/handler.rs:235">
P2: When a runnable sets `Access-Control-Allow-Methods` through `wm_headers`, the restricted route keeps that value instead of the route-specific allowlist. Override the runnable method header whenever a restricted route is resolved.</violation>
</file>

<file name="frontend/src/lib/components/triggers/http/RouteEditorInner.svelte">

<violation number="1" location="frontend/src/lib/components/triggers/http/RouteEditorInner.svelte:1005">
P2: When the request-options tab unmounts after an invalid allowlist, `allowedOriginsError` remains bound in the parent and keeps Save disabled on other tabs without a visible error. Clear `allowedOriginsError` when leaving that tab.</violation>
</file>

Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic

// Rejected at write time rather than at boot: a mistyped origin
// matches no request, so it would silently block the very app it
// names with nothing but a log line to go on.
windmill_common::global_settings::parse_allowed_origins_setting(Some(value))?;

@cubic-dev-ai cubic-dev-ai Bot Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: When http_route_default_allowed_origins is supplied through sync-config or operator reconciliation, this hook is bypassed because those paths apply settings directly. Validate this key in the shared declarative write path too; otherwise malformed values persist and runtime keeps the empty allowlist, leaving CORS unrestricted.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/windmill-api-settings/src/lib.rs, line 1021:

<comment>When `http_route_default_allowed_origins` is supplied through `sync-config` or operator reconciliation, this hook is bypassed because those paths apply settings directly. Validate this key in the shared declarative write path too; otherwise malformed values persist and runtime keeps the empty allowlist, leaving CORS unrestricted.</comment>

<file context>
@@ -1014,6 +1014,12 @@ async fn run_setting_pre_write_hook(
+            // Rejected at write time rather than at boot: a mistyped origin
+            // matches no request, so it would silently block the very app it
+            // names with nothing but a log line to go on.
+            windmill_common::global_settings::parse_allowed_origins_setting(Some(value))?;
+        }
         HTTP_ROUTE_WORKSPACED_ROUTE_SETTING => {
</file context>
Fix with cubic

Comment thread backend/src/monitor.rs
apply_app_workspaced_route_setting(v)
});
pass.setting(
HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING,

@cubic-dev-ai cubic-dev-ai Bot Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: When the global-settings read fails while route lookup still succeeds, SettingsPass skips this applier, leaving the new ArcSwap empty; middleware then treats the default as unset and emits *. Preserve a last-known value or mark the default unresolved and omit CORS until it is successfully read, otherwise a transient settings-query failure bypasses a configured allowlist.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/src/monitor.rs, line 427:

<comment>When the global-settings read fails while route lookup still succeeds, `SettingsPass` skips this applier, leaving the new `ArcSwap` empty; middleware then treats the default as unset and emits `*`. Preserve a last-known value or mark the default unresolved and omit CORS until it is successfully read, otherwise a transient settings-query failure bypasses a configured allowlist.</comment>

<file context>
@@ -420,6 +423,18 @@ pub async fn initial_load(
             apply_app_workspaced_route_setting(v)
         });
+        pass.setting(
+            HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING,
+            false,
+            |v| async move {
</file context>
Fix with cubic

Comment thread frontend/src/lib/components/triggers/http/utils.ts Outdated
// Origin, and a shared cache that ignores it would hand one origin's
// response to another.
headers.append(http::header::VARY, http::HeaderValue::from_static("origin"));
} else if !not_insert_origin && !matches!(route, CorsRouteLookup::Unavailable) {

@cubic-dev-ai cubic-dev-ai Bot Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When router resolution fails but the route later succeeds, wm_headers can leave an Access-Control-Allow-Origin header in the response. Remove that header for CorsRouteLookup::Unavailable instead of only skipping the fallback insertion.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/windmill-api/src/triggers/http/handler.rs, line 220:

<comment>When router resolution fails but the route later succeeds, `wm_headers` can leave an `Access-Control-Allow-Origin` header in the response. Remove that header for `CorsRouteLookup::Unavailable` instead of only skipping the fallback insertion.</comment>

<file context>
@@ -67,18 +178,68 @@ async fn conditional_cors_middleware(
+        // Origin, and a shared cache that ignores it would hand one origin's
+        // response to another.
+        headers.append(http::header::VARY, http::HeaderValue::from_static("origin"));
+    } else if !not_insert_origin && !matches!(route, CorsRouteLookup::Unavailable) {
         headers.insert(
             http::header::ACCESS_CONTROL_ALLOW_ORIGIN,
</file context>
Suggested change
} else if !not_insert_origin && !matches!(route, CorsRouteLookup::Unavailable) {
} else if matches!(route, CorsRouteLookup::Unavailable) {
headers.remove(http::header::ACCESS_CONTROL_ALLOW_ORIGIN);
} else if !not_insert_origin {
Fix with cubic

description:
'Origins that HTTP routes allow to call them from a browser when the route sets none of its own. A route overrides this with its own list, and opts out entirely by setting its allowed origins to *. Leave empty to let every route be called from any origin.',
key: 'http_route_default_allowed_origins',
fieldType: 'text',

@cubic-dev-ai cubic-dev-ai Bot Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The new http_route_default_allowed_origins setting has no UI validation, but the backend rejects invalid origins on write (parse_allowed_origins_setting/validate_allowed_origins). Save is only blocked for settings that define isValid, and saveSettings() awaits setInstanceConfig without try/catch, so typing an invalid origin makes the whole bulk save of all instance settings fail with no visible error. Add isValid/error mirroring the backend rules (reject paths, queries, userinfo, whitespace, non-ASCII, "Origin: null") like email_domain/webhook_base_url do, so bad input is caught before submit.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/lib/components/instanceSettings.ts, line 270:

<comment>The new http_route_default_allowed_origins setting has no UI validation, but the backend rejects invalid origins on write (parse_allowed_origins_setting/validate_allowed_origins). Save is only blocked for settings that define isValid, and saveSettings() awaits setInstanceConfig without try/catch, so typing an invalid origin makes the whole bulk save of all instance settings fail with no visible error. Add isValid/error mirroring the backend rules (reject paths, queries, userinfo, whitespace, non-ASCII, "Origin: null") like email_domain/webhook_base_url do, so bad input is caught before submit.</comment>

<file context>
@@ -262,6 +262,17 @@ export const settings: Record<string, Setting[]> = {
+			description:
+				'Origins that HTTP routes allow to call them from a browser when the route sets none of its own. A route overrides this with its own list, and opts out entirely by setting its allowed origins to *. Leave empty to let every route be called from any origin.',
+			key: 'http_route_default_allowed_origins',
+			fieldType: 'text',
+			placeholder: 'https://app.example.com, https://admin.example.com',
+			storage: 'setting',
</file context>
Fix with cubic

Comment thread frontend/src/lib/components/triggers/http/RouteEditorInner.svelte Outdated
Comment thread backend/windmill-common/src/global_settings.rs
headers.insert(
http::header::ACCESS_CONTROL_ALLOW_METHODS,
http::HeaderValue::from_static("GET, POST, PUT, DELETE, PATCH, HEAD, OPTIONS"),
http::HeaderValue::from_static(match restricted_route.map(|route| route.http_method) {

@cubic-dev-ai cubic-dev-ai Bot Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When a runnable sets Access-Control-Allow-Methods through wm_headers, the restricted route keeps that value instead of the route-specific allowlist. Override the runnable method header whenever a restricted route is resolved.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/windmill-api/src/triggers/http/handler.rs, line 235:

<comment>When a runnable sets `Access-Control-Allow-Methods` through `wm_headers`, the restricted route keeps that value instead of the route-specific allowlist. Override the runnable method header whenever a restricted route is resolved.</comment>

<file context>
@@ -67,18 +178,68 @@ async fn conditional_cors_middleware(
         headers.insert(
             http::header::ACCESS_CONTROL_ALLOW_METHODS,
-            http::HeaderValue::from_static("GET, POST, PUT, DELETE, PATCH, HEAD, OPTIONS"),
+            http::HeaderValue::from_static(match restricted_route.map(|route| route.http_method) {
+                Some(HttpMethod::Get) => "GET, OPTIONS",
+                Some(HttpMethod::Post) => "POST, OPTIONS",
</file context>
Fix with cubic


<RouteCorsOption
bind:allowed_origins
bind:error={allowedOriginsError}

@cubic-dev-ai cubic-dev-ai Bot Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When the request-options tab unmounts after an invalid allowlist, allowedOriginsError remains bound in the parent and keeps Save disabled on other tabs without a visible error. Clear allowedOriginsError when leaving that tab.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/lib/components/triggers/http/RouteEditorInner.svelte, line 1005:

<comment>When the request-options tab unmounts after an invalid allowlist, `allowedOriginsError` remains bound in the parent and keeps Save disabled on other tabs without a visible error. Clear `allowedOriginsError` when leaving that tab.</comment>

<file context>
@@ -961,6 +999,14 @@
+
+									<RouteCorsOption
+										bind:allowed_origins
+										bind:error={allowedOriginsError}
+										{instanceDefaultOrigins}
+										disabled={!can_write}
</file context>
Fix with cubic

Comment thread backend/src/monitor.rs Outdated
@hugocasa

Copy link
Copy Markdown
Collaborator Author

Round 2 nits addressed in 8a9a028 — thanks all three.

Fixed

  • isOriginRestricted diverged from effective_allowed_origins on [] (all three reviewers). Rewritten to mirror the backend's match arms directly: a route with its own list restricts, [] included; only an absent list falls back to the instance default. Verified in the UI — the CORS badge now renders for a route stored with allowed_origins = '{}'.
  • Editor collapsed an array-valued instance default to [] (Codex). New parseAllowedOriginsSetting mirrors parse_allowed_origins_setting, accepting both the comma-separated string the settings UI writes and the array the API accepts. With the setting stored as a JSON array, the label, badge and inherited-policy hint now all read it.
  • Origin validation accepted values no browser can send (Codex). The scheme must now start with a letter, and the port must be numeric — 1://host and https://example.com:not-a-port are rejected by both the trigger API and the settings API. The port split is IPv6-aware, so http://[::1] and http://[::1]:8080 still pass rather than being read as host [:. Mirrored in originError, and pinned by unit tests on both the accept and reject sides.
  • allowedOriginsError outlived the tab it was set on (Claude, Pi). Clearing the error alone would have let a half-typed list save as a real restriction, so leaving the tab now restores what the option was mounted with — which also means it can never silently drop an existing restriction. Verified: with another field edited, Save is enabled → emptying the origins field disables it → switching to Retries re-enables it with https://a.com intact.
  • Unrelated rustfmt churn in monitor.rs (Claude). Reverted; the only remaining non-feature line there is the import block this PR adds to.
  • "keeps the previous value" overstated the boot path (Claude). Reworded to say what actually guards it, which is write-time validation.

Not taking

  • Static website triggers cannot configure CORS (Codex). Correct that the middleware enforces an allowlist for them, but RouteCorsOption is not what hides it: the whole Advanced section is behind !is_static_website on main, so request_type, authentication_method and the error handler are equally unreachable there. Surfacing one option would mean building a section for static websites, which is a separate change.
  • Raw <div> for the validation and hint rows (Codex). These match the sibling trigger editors exactly — RouteEditorConfigSection.svelte:174,196, WebsocketEditorConfigSection.svelte:140,193, EmailTriggerEditorConfigSection.svelte:143 all render hint and error text as bare text-2xs divs. There is no design-system component for this slot; introducing one here would make this component the odd one out.

Left open — the middleware integration test Claude suggests is a fair gap, and I have not written it.

@github-actions

Copy link
Copy Markdown
Contributor

Codex Review

cc @hugocasa
Should address issues before merging: stale CORS rollback can widen a route policy; deny-all routes cannot be edited in the UI; origin validation still accepts impossible authorities

Findings

P1 — Invalid-edit rollback can restore another route’s CORS policyfrontend/src/lib/components/triggers/http/RouteCorsOption.svelte:103

mountedWith is captured once when the component mounts, but the drawer opens before the requested trigger finishes loading, and draft/reset operations also replace allowed_origins while the component remains mounted. The reseed effect updates the field but not this snapshot. After an invalid or empty edit, switching tabs restores the stale value; if that value is undefined, a restrictive route silently becomes unrestricted and an unrelated save persists the widening.

P2 — Valid deny-all routes cannot be edited in the UIfrontend/src/lib/components/triggers/http/RouteCorsOption.svelte:84

The backend deliberately accepts allowed_origins: [] as “deny every origin,” but mounting such a route always sets Enter at least one origin. RouteEditorInner consequently disables Save, so an API/CLI-created deny-all route cannot save unrelated changes without changing its valid CORS policy.

P2 — Origin validation still accepts authorities browsers rejectbackend/windmill-common/src/global_settings.rs:337

The final validation only requires a nonempty host and digit-only port. Values such as https://host:99999, https://[notipv6], and https://exa[mple.com therefore pass both APIs despite being invalid browser origins. The mirrored frontend validation has the same gap at RouteCorsOption.svelte:60.

Test coverage

Backend unit tests cover matching, wildcard/empty semantics, and selected validation cases, but there is still no middleware integration test covering preflight and actual-response consistency. Frontend component tests are not generally expected; the new pure parsing/restriction helpers have no focused coverage.

Manually open an unrestricted route followed by a restricted route, make the second route’s origin field invalid, switch tabs, and confirm its original policy remains intact when saving another change.

Create an allowed_origins: [] route through the API, edit an unrelated field in the UI, and confirm Save preserves the deny-all list.

Verify impossible hosts, malformed IPv6 literals, and ports above 65535 are rejected by both the trigger and settings APIs and by inline validation.

@github-actions

Copy link
Copy Markdown
Contributor

Pi Review

Good to merge.

I reviewed the full diff at head 8a9a028 (base 0f3d884c), the surrounding code, and the prior three review rounds. Every blocking finding from earlier passes is genuinely fixed at this head: Some([]) now denies rather than falling back to the instance default, the frontend isOriginRestricted mirrors effective_allowed_origins for the empty list, originError matches the backend's ASCII/scheme/port rules (verified entry-by-entry against validate_allowed_origins), routable_method folds HEAD → Get for preflight and request alike, and the cross-tab allowedOriginsError leak is closed by the unmount cleanup.

The core security properties hold. match_origin echoes the request Origin only on an exact, case-insensitive match and never reflects unchecked; a configured allowlist overrides any wm_headers value the runnable set; the preflight is decided by the same middleware path as the response; Vary: origin is appended so a shared cache can't hand one origin's answer to another; and the routers-unavailable branch omits Access-Control-Allow-Origin entirely rather than falling back to *. cors_lookup_path decodes/strips/trims to the exact key StripPath::to_path() + format!("/{}", …) produces, so the middleware and get_http_route_trigger resolve the same trigger. The instance-default read exemption for non-superadmins is read-only (write still requires require_super_admin), and the setting is validated at write time via the pre-write hook.

Considered and dismissed

  • Route-existence oracle via the preflight header — a preflight to a restricted route with a disallowed Origin omits ACAO while an unknown path (no instance default) gets *. Dismissed: route existence is already trivially observable with a non-browser client (a real GET returns the trigger's 200 vs a 404), and the differential response still fails closed — no response data reaches a disallowed origin. The PR's "not a route-existence oracle" test-plan claim only covers the unconfigured case, but the general divergence adds no meaningful information.
  • mountedWith cleanup snapshot going stale across an external cfg write — applying a draft or reset-to-deployed rewrites allowed_origins while RouteCorsOption stays mounted, so the unmount cleanup would restore the pre-draft value if the user then left the field in an error state and switched tabs. Dismissed: it only affects unsaved editor state (nothing is persisted), needs a narrow multi-step sequence, and the component's documented "never widening" intent is about the value that actually gets saved.
  • Unavailable branch still emitting Access-Control-Allow-Methods/Access-Control-Allow-Headers without Access-Control-Allow-Origin — dismissed: without ACAO the browser blocks the read regardless, so the two headers are inert.
  • Frontend parseAllowedOriginsSetting silently dropping non-string array entries while the backend errors — dismissed: it feeds only the badge/hint display, and the backend still rejects malformed arrays at write time.
  • match_origin not normalizing default ports (configured https://a.com:443 never equals the browser's https://a.com) — dismissed: fails closed (matches nothing → deny), a config footgun rather than a security issue, and already covered by the PR's own "a typo matches no request" rationale.
  • refresh_routers(db) arity — dismissed: at this head refresh_routers takes only (db); the two-arg (db, force) form is a later main-line change outside this diff.

Test coverage

  • Backend — strong. 112 unit tests cover match_origin (exact, case-insensitive, no-match, prefix non-match, missing Origin), effective_allowed_origins (route-preference, empty-list-deny, wildcard opt-out, default fallback), and validate_allowed_origins (accept and reject sides, including the scheme-first-character and numeric-port cases). The one gap the author already flagged stands: no integration test drives the middleware end-to-end against a live router (preflight + response for the resolution-order table). Given the extensive real-browser manual matrix in the test plan, this is not blocking, but a single middleware integration test pinning the route list → default → * resolution and the wm_headers-cannot-widen property would be the highest-value regression guard to add later.
  • Frontend — the new helpers (parseAllowedOrigins, parseAllowedOriginsSetting, isOriginRestricted) live in utils.ts, which has no sibling *.test.ts in this codebase, so per project convention no component/util tests are expected. No flags.
  • CI / config / docs — the OpenAPI, skills.gen.ts, workspaceToolsZod.gen.ts, summarized_schema.txt, and sqlx cache entries are regenerated consistently with the new column and field. No automated tests expected.

The author's manual verification matrix (real-browser preflight across two ports, percent-encoded path resolution, write-time rejection through both the trigger and settings APIs, fork-clone column carry, hot reload without restart) covers the surfaces that matter and was exercised against a running --features quickjs,http_trigger build. The only explicitly unexercised path — cold cache plus unreachable DB — fails closed by design (omits the header), which matches the code.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 6 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="backend/windmill-common/src/global_settings.rs">

<violation number="1" location="backend/windmill-common/src/global_settings.rs:323">
P2: Malformed host/bracket entries pass write-time validation and then can silently deny every browser origin instead of producing a configuration error. Validate the host grammar, including requiring brackets to contain a valid IPv6 literal and rejecting brackets or other invalid host characters in regular hosts.</violation>
</file>

<file name="frontend/src/lib/components/triggers/http/RouteCorsOption.svelte">

<violation number="1" location="frontend/src/lib/components/triggers/http/RouteCorsOption.svelte:103">
P1: When an existing route loads after this component mounts, `mountedWith` stays at the pre-load value. After a malformed edit and tab switch, cleanup restores that stale value, potentially changing the route's allowlist to `undefined` (allow all); update the snapshot whenever the parent replaces `allowed_origins`.</violation>

<violation number="2" location="frontend/src/lib/components/triggers/http/RouteCorsOption.svelte:103">
P2: When the prop is replaced from outside (draft apply / reset to deployed) the re-seed effect updates raw/restricted, but mountedWith keeps the value captured at component setup. If the user then types an invalid entry (setting error) and leaves the tab, the unmount cleanup restores that stale mount-time value, discarding the restriction that was just applied from the draft/deployed config. Make mountedWith reactive and re-snapshot it whenever the external-write path re-seeds, so the abandoned-edit restore preserves the latest applied value.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

// 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

@cubic-dev-ai cubic-dev-ai Bot Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: When an existing route loads after this component mounts, mountedWith stays at the pre-load value. After a malformed edit and tab switch, cleanup restores that stale value, potentially changing the route's allowlist to undefined (allow all); update the snapshot whenever the parent replaces allowed_origins.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/lib/components/triggers/http/RouteCorsOption.svelte, line 103:

<comment>When an existing route loads after this component mounts, `mountedWith` stays at the pre-load value. After a malformed edit and tab switch, cleanup restores that stale value, potentially changing the route's allowlist to `undefined` (allow all); update the snapshot whenever the parent replaces `allowed_origins`.</comment>

<file context>
@@ -86,6 +96,16 @@
+	// 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
</file context>
Fix with cubic

// 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 (host, port) = match rest.strip_prefix('[') {

@cubic-dev-ai cubic-dev-ai Bot Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Malformed host/bracket entries pass write-time validation and then can silently deny every browser origin instead of producing a configuration error. Validate the host grammar, including requiring brackets to contain a valid IPv6 literal and rejecting brackets or other invalid host characters in regular hosts.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/windmill-common/src/global_settings.rs, line 323:

<comment>Malformed host/bracket entries pass write-time validation and then can silently deny every browser origin instead of producing a configuration error. Validate the host grammar, including requiring brackets to contain a valid IPv6 literal and rejecting brackets or other invalid host characters in regular hosts.</comment>

<file context>
@@ -313,6 +316,30 @@ 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 (host, port) = match rest.strip_prefix('[') {
+            Some(after_bracket) => match after_bracket.split_once(']') {
+                Some((host, "")) => (host, None),
</file context>
Fix with cubic

// 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

@cubic-dev-ai cubic-dev-ai Bot Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When the prop is replaced from outside (draft apply / reset to deployed) the re-seed effect updates raw/restricted, but mountedWith keeps the value captured at component setup. If the user then types an invalid entry (setting error) and leaves the tab, the unmount cleanup restores that stale mount-time value, discarding the restriction that was just applied from the draft/deployed config. Make mountedWith reactive and re-snapshot it whenever the external-write path re-seeds, so the abandoned-edit restore preserves the latest applied value.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/lib/components/triggers/http/RouteCorsOption.svelte, line 103:

<comment>When the prop is replaced from outside (draft apply / reset to deployed) the re-seed effect updates raw/restricted, but mountedWith keeps the value captured at component setup. If the user then types an invalid entry (setting error) and leaves the tab, the unmount cleanup restores that stale mount-time value, discarding the restriction that was just applied from the draft/deployed config. Make mountedWith reactive and re-snapshot it whenever the external-write path re-seeds, so the abandoned-edit restore preserves the latest applied value.</comment>

<file context>
@@ -86,6 +96,16 @@
+	// 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
</file context>
Fix with cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feature: per-route CORS configuration for HTTP triggers

1 participant