From d8d7332eb6d92f7a55b82890de5de4196039b40d Mon Sep 17 00:00:00 2001 From: hugocasa Date: Mon, 14 Sep 2026 11:36:01 +0200 Subject: [PATCH] feat: add per-route CORS origin allowlist for HTTP triggers (#10833) * feat: add per-route CORS origin allowlist for HTTP triggers Co-Authored-By: Claude Opus 5 * fix: fail closed on cold router cache and invalid origin input Co-Authored-By: Claude Opus 5 * fix: resolve CORS route from the decoded path like the request handler Co-Authored-By: Claude Opus 5 * feat: add instance-wide default allowed origins for HTTP routes Co-Authored-By: Claude Opus 5 * fix: let non-superadmins read the default allowed origins setting Co-Authored-By: Claude Opus 5 * feat: badge the advanced section when a route's origins are restricted Co-Authored-By: Claude Opus 5 * fix: state inherited origins on the control and use one hint row Co-Authored-By: Claude Opus 5 * fix: trim the origins tooltip and relabel the toggle when a default exists Co-Authored-By: Claude Opus 5 * fix: keep the origins format hint visible until an entry is wrong Co-Authored-By: Claude Opus 5 * fix: state the at-least-one requirement in the origins hint Co-Authored-By: Claude Opus 5 * fix: import the origins validator in the trigger-http tests Co-Authored-By: Claude Opus 5 * fix: make an empty allowlist deny rather than fall back to the default Co-Authored-By: Claude Opus 5 * fix: address review nits on origin validation and the CORS editor * fix: derive the origins error from the stored list and tighten host validation * fix: parse real IPv6 hosts and refuse a newly emptied allowlist * refactor: make origin validation advisory except for null and non-ascii * feat: let an empty allowlist be saved as deny every origin * docs: document the empty allowlist as deny every origin * fix: bound allowlists, reject commas, and decide cors after the handler * chore: revert unrelated rustfmt churn in windmill-common tests * chore: revert unrelated rustfmt churn in windmill-common * chore: drop the route types the cors restructure replaced * fix: take the stricter cors decision from before and after the handler * fix: strip runnable cors headers when the routers are unavailable * docs: document the allowlist bounds in the openapi schema * fix: let an unavailable cors read defer to one that resolved * refactor: carry the resolved cors policy from the handler to the middleware * docs: describe why an unavailable read fails closed on the paths that reach it * fix: validate the default origins on the declarative settings path * test: keep the webhook doc comment with the test it describes * fix: warn on impossible schemes and ports, and validate the instance setting * feat: treat an empty allowlist as unset at both levels * perf: decode the cors path only when the fallback needs it * docs: document the empty allowlist as unset in the api schema * docs: describe an empty allowlist as unset in the frontend comments * docs: say what a null allowlist resolves to, not what it meant before the default existed * docs: state what the validator refuses and why methods stay broad * feat: exempt static asset routes from the origin allowlist * fix: hide the origin control for every static target, not just websites * fix: exempt only static websites, not single-file static assets * fix: warn on an unclosed ipv6 host in the origins advisory * fix: require assets present, not just the static website flag --------- Co-authored-by: Claude Opus 5 --- ...871ca8667a657dfdfc2fa65e9ff1a3c0d2908.json | 87 ++++++ ...cbc2bc7235c63ed4f0a885d412793f3a3a3fc.json | 84 ++++++ ...22e8a9147d36a4b56ebd30a1aaa960156b598.json | 86 ++++++ ...ef9db3a0579c889bcae4e8bf33467ce5cebd1.json | 16 + ...1eaf40862b464af7113d2632c626e43c35c04.json | 185 ++++++++++++ ...5019_http_trigger_allowed_origins.down.sql | 2 + ...105019_http_trigger_allowed_origins.up.sql | 2 + backend/src/main.rs | 11 +- backend/src/monitor.rs | 45 ++- backend/summarized_schema.txt | 2 +- backend/tests/instance_config.rs | 59 +++- backend/windmill-api-settings/src/lib.rs | 24 +- .../windmill-api-workspaces/src/workspaces.rs | 4 +- backend/windmill-api/openapi.yaml | 24 ++ .../windmill-api/src/triggers/http/handler.rs | 284 +++++++++++++++++- .../windmill-common/src/global_settings.rs | 120 ++++++++ .../windmill-common/src/instance_config.rs | 12 +- backend/windmill-trigger-http/src/handler.rs | 91 +++--- backend/windmill-trigger-http/src/lib.rs | 218 +++++++++++++- cli/src/guidance/skills.gen.ts | 15 + .../copilot/chat/workspaceToolsZod.gen.ts | 1 + .../src/lib/components/instanceSettings.ts | 17 ++ .../triggers/http/RouteCorsOption.svelte | 132 ++++++++ .../triggers/http/RouteEditorInner.svelte | 51 +++- .../src/lib/components/triggers/http/utils.ts | 141 +++++++++ .../schemas/http_trigger.schema.yaml | 15 + 26 files changed, 1659 insertions(+), 69 deletions(-) create mode 100644 backend/.sqlx/query-2ead4c5e0fec64dfdc24431d2f4871ca8667a657dfdfc2fa65e9ff1a3c0d2908.json create mode 100644 backend/.sqlx/query-47e0f46fddb3ad1c854deb9bdbdcbc2bc7235c63ed4f0a885d412793f3a3a3fc.json create mode 100644 backend/.sqlx/query-ab752dd133b20103800554b3f6622e8a9147d36a4b56ebd30a1aaa960156b598.json create mode 100644 backend/.sqlx/query-c1976ac63f5d763b2a747ff92f5ef9db3a0579c889bcae4e8bf33467ce5cebd1.json create mode 100644 backend/.sqlx/query-c7a78d3db99e7f709479c9520471eaf40862b464af7113d2632c626e43c35c04.json create mode 100644 backend/migrations/20260825105019_http_trigger_allowed_origins.down.sql create mode 100644 backend/migrations/20260825105019_http_trigger_allowed_origins.up.sql create mode 100644 frontend/src/lib/components/triggers/http/RouteCorsOption.svelte diff --git a/backend/.sqlx/query-2ead4c5e0fec64dfdc24431d2f4871ca8667a657dfdfc2fa65e9ff1a3c0d2908.json b/backend/.sqlx/query-2ead4c5e0fec64dfdc24431d2f4871ca8667a657dfdfc2fa65e9ff1a3c0d2908.json new file mode 100644 index 0000000000..99c8262860 --- /dev/null +++ b/backend/.sqlx/query-2ead4c5e0fec64dfdc24431d2f4871ca8667a657dfdfc2fa65e9ff1a3c0d2908.json @@ -0,0 +1,87 @@ +{ + "db_name": "PostgreSQL", + "query": "\n UPDATE\n http_trigger\n SET\n route_path = $1,\n route_path_key = $2,\n workspaced_route = $3,\n wrap_body = $4,\n raw_string = $5,\n allowed_origins = $6,\n authentication_resource_path = $7,\n script_path = $8,\n path = $9,\n is_flow = $10,\n mode = $11,\n http_method = $12,\n static_asset_config = $13,\n edited_by = $14,\n permissioned_as = $15,\n request_type = $16,\n authentication_method = $17,\n summary = $18,\n description = $19,\n edited_at = now(),\n is_static_website = $20,\n error_handler_path = $21,\n error_handler_args = $22,\n retry = $23\n WHERE\n workspace_id = $24 AND\n path = $25\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Varchar", + "Bool", + "Bool", + "Bool", + "TextArray", + "Varchar", + "Varchar", + "Varchar", + "Bool", + { + "Custom": { + "name": "trigger_mode", + "kind": { + "Enum": [ + "enabled", + "disabled", + "suspended" + ] + } + } + }, + { + "Custom": { + "name": "http_method", + "kind": { + "Enum": [ + "get", + "post", + "put", + "delete", + "patch" + ] + } + } + }, + "Jsonb", + "Varchar", + "Varchar", + { + "Custom": { + "name": "request_type", + "kind": { + "Enum": [ + "sync", + "async", + "sync_sse" + ] + } + } + }, + { + "Custom": { + "name": "authentication_method", + "kind": { + "Enum": [ + "none", + "windmill", + "api_key", + "basic_http", + "custom_script", + "signature" + ] + } + } + }, + "Varchar", + "Text", + "Bool", + "Varchar", + "Jsonb", + "Jsonb", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "2ead4c5e0fec64dfdc24431d2f4871ca8667a657dfdfc2fa65e9ff1a3c0d2908" +} diff --git a/backend/.sqlx/query-47e0f46fddb3ad1c854deb9bdbdcbc2bc7235c63ed4f0a885d412793f3a3a3fc.json b/backend/.sqlx/query-47e0f46fddb3ad1c854deb9bdbdcbc2bc7235c63ed4f0a885d412793f3a3a3fc.json new file mode 100644 index 0000000000..6bec80ccb2 --- /dev/null +++ b/backend/.sqlx/query-47e0f46fddb3ad1c854deb9bdbdcbc2bc7235c63ed4f0a885d412793f3a3a3fc.json @@ -0,0 +1,84 @@ +{ + "db_name": "PostgreSQL", + "query": "\n UPDATE\n http_trigger\n SET\n wrap_body = $1,\n raw_string = $2,\n allowed_origins = $3,\n authentication_resource_path = $4,\n script_path = $5,\n path = $6,\n is_flow = $7,\n mode = $8,\n http_method = $9,\n static_asset_config = $10,\n edited_by = $11,\n permissioned_as = $12,\n request_type = $13,\n authentication_method = $14,\n summary = $15,\n description = $16,\n edited_at = now(),\n is_static_website = $17,\n error_handler_path = $18,\n error_handler_args = $19,\n retry = $20\n WHERE\n workspace_id = $21 AND\n path = $22\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Bool", + "Bool", + "TextArray", + "Varchar", + "Varchar", + "Varchar", + "Bool", + { + "Custom": { + "name": "trigger_mode", + "kind": { + "Enum": [ + "enabled", + "disabled", + "suspended" + ] + } + } + }, + { + "Custom": { + "name": "http_method", + "kind": { + "Enum": [ + "get", + "post", + "put", + "delete", + "patch" + ] + } + } + }, + "Jsonb", + "Varchar", + "Varchar", + { + "Custom": { + "name": "request_type", + "kind": { + "Enum": [ + "sync", + "async", + "sync_sse" + ] + } + } + }, + { + "Custom": { + "name": "authentication_method", + "kind": { + "Enum": [ + "none", + "windmill", + "api_key", + "basic_http", + "custom_script", + "signature" + ] + } + } + }, + "Varchar", + "Text", + "Bool", + "Varchar", + "Jsonb", + "Jsonb", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "47e0f46fddb3ad1c854deb9bdbdcbc2bc7235c63ed4f0a885d412793f3a3a3fc" +} diff --git a/backend/.sqlx/query-ab752dd133b20103800554b3f6622e8a9147d36a4b56ebd30a1aaa960156b598.json b/backend/.sqlx/query-ab752dd133b20103800554b3f6622e8a9147d36a4b56ebd30a1aaa960156b598.json new file mode 100644 index 0000000000..5a9e5da58e --- /dev/null +++ b/backend/.sqlx/query-ab752dd133b20103800554b3f6622e8a9147d36a4b56ebd30a1aaa960156b598.json @@ -0,0 +1,86 @@ +{ + "db_name": "PostgreSQL", + "query": "\n INSERT INTO http_trigger (\n workspace_id,\n path,\n route_path,\n route_path_key,\n workspaced_route,\n authentication_resource_path,\n wrap_body,\n raw_string,\n allowed_origins,\n script_path,\n summary,\n description,\n is_flow,\n mode,\n request_type,\n authentication_method,\n http_method,\n static_asset_config,\n edited_by,\n permissioned_as,\n edited_at,\n is_static_website,\n error_handler_path,\n error_handler_args,\n retry\n )\n VALUES (\n $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, now(), $21, $22, $23, $24\n )\n ", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Varchar", + "Varchar", + "Varchar", + "Bool", + "Varchar", + "Bool", + "Bool", + "TextArray", + "Varchar", + "Varchar", + "Text", + "Bool", + { + "Custom": { + "name": "trigger_mode", + "kind": { + "Enum": [ + "enabled", + "disabled", + "suspended" + ] + } + } + }, + { + "Custom": { + "name": "request_type", + "kind": { + "Enum": [ + "sync", + "async", + "sync_sse" + ] + } + } + }, + { + "Custom": { + "name": "authentication_method", + "kind": { + "Enum": [ + "none", + "windmill", + "api_key", + "basic_http", + "custom_script", + "signature" + ] + } + } + }, + { + "Custom": { + "name": "http_method", + "kind": { + "Enum": [ + "get", + "post", + "put", + "delete", + "patch" + ] + } + } + }, + "Jsonb", + "Varchar", + "Varchar", + "Bool", + "Varchar", + "Jsonb", + "Jsonb" + ] + }, + "nullable": [] + }, + "hash": "ab752dd133b20103800554b3f6622e8a9147d36a4b56ebd30a1aaa960156b598" +} diff --git a/backend/.sqlx/query-c1976ac63f5d763b2a747ff92f5ef9db3a0579c889bcae4e8bf33467ce5cebd1.json b/backend/.sqlx/query-c1976ac63f5d763b2a747ff92f5ef9db3a0579c889bcae4e8bf33467ce5cebd1.json new file mode 100644 index 0000000000..ed6a4f9b5e --- /dev/null +++ b/backend/.sqlx/query-c1976ac63f5d763b2a747ff92f5ef9db3a0579c889bcae4e8bf33467ce5cebd1.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO http_trigger (\n path, route_path, route_path_key, script_path, is_flow, workspace_id,\n edited_by, edited_at, extra_perms, authentication_method, http_method,\n static_asset_config, is_static_website, workspaced_route, wrap_body,\n raw_string, allowed_origins, authentication_resource_path, summary, description,\n error_handler_path, error_handler_args, retry, request_type, mode,\n permissioned_as, labels\n )\n SELECT\n path, route_path, route_path_key, script_path, is_flow, $1,\n edited_by, edited_at, extra_perms, authentication_method, http_method,\n static_asset_config, is_static_website, workspaced_route, wrap_body,\n raw_string, allowed_origins, authentication_resource_path, summary, description,\n error_handler_path, error_handler_args, retry, request_type, 'disabled'::TRIGGER_MODE,\n permissioned_as, labels\n FROM http_trigger\n WHERE workspace_id = $2\n AND (workspaced_route IS TRUE OR $3)", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Text", + "Bool" + ] + }, + "nullable": [] + }, + "hash": "c1976ac63f5d763b2a747ff92f5ef9db3a0579c889bcae4e8bf33467ce5cebd1" +} diff --git a/backend/.sqlx/query-c7a78d3db99e7f709479c9520471eaf40862b464af7113d2632c626e43c35c04.json b/backend/.sqlx/query-c7a78d3db99e7f709479c9520471eaf40862b464af7113d2632c626e43c35c04.json new file mode 100644 index 0000000000..7bc28f0f39 --- /dev/null +++ b/backend/.sqlx/query-c7a78d3db99e7f709479c9520471eaf40862b464af7113d2632c626e43c35c04.json @@ -0,0 +1,185 @@ +{ + "db_name": "PostgreSQL", + "query": "\n SELECT\n path,\n script_path,\n is_flow,\n route_path,\n authentication_resource_path,\n workspace_id,\n request_type AS \"request_type: _\",\n authentication_method AS \"authentication_method: _\",\n edited_by,\n permissioned_as,\n static_asset_config AS \"static_asset_config: _\",\n wrap_body,\n raw_string,\n allowed_origins,\n workspaced_route,\n is_static_website,\n error_handler_path,\n error_handler_args as \"error_handler_args: _\",\n retry as \"retry: _\",\n mode as \"mode: _\"\n FROM\n http_trigger\n WHERE\n http_method = $1 AND\n (mode = 'enabled'::TRIGGER_MODE OR mode = 'suspended'::TRIGGER_MODE)\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "script_path", + "type_info": "Varchar" + }, + { + "ordinal": 2, + "name": "is_flow", + "type_info": "Bool" + }, + { + "ordinal": 3, + "name": "route_path", + "type_info": "Varchar" + }, + { + "ordinal": 4, + "name": "authentication_resource_path", + "type_info": "Varchar" + }, + { + "ordinal": 5, + "name": "workspace_id", + "type_info": "Varchar" + }, + { + "ordinal": 6, + "name": "request_type: _", + "type_info": { + "Custom": { + "name": "request_type", + "kind": { + "Enum": [ + "sync", + "async", + "sync_sse" + ] + } + } + } + }, + { + "ordinal": 7, + "name": "authentication_method: _", + "type_info": { + "Custom": { + "name": "authentication_method", + "kind": { + "Enum": [ + "none", + "windmill", + "api_key", + "basic_http", + "custom_script", + "signature" + ] + } + } + } + }, + { + "ordinal": 8, + "name": "edited_by", + "type_info": "Varchar" + }, + { + "ordinal": 9, + "name": "permissioned_as", + "type_info": "Varchar" + }, + { + "ordinal": 10, + "name": "static_asset_config: _", + "type_info": "Jsonb" + }, + { + "ordinal": 11, + "name": "wrap_body", + "type_info": "Bool" + }, + { + "ordinal": 12, + "name": "raw_string", + "type_info": "Bool" + }, + { + "ordinal": 13, + "name": "allowed_origins", + "type_info": "TextArray" + }, + { + "ordinal": 14, + "name": "workspaced_route", + "type_info": "Bool" + }, + { + "ordinal": 15, + "name": "is_static_website", + "type_info": "Bool" + }, + { + "ordinal": 16, + "name": "error_handler_path", + "type_info": "Varchar" + }, + { + "ordinal": 17, + "name": "error_handler_args: _", + "type_info": "Jsonb" + }, + { + "ordinal": 18, + "name": "retry: _", + "type_info": "Jsonb" + }, + { + "ordinal": 19, + "name": "mode: _", + "type_info": { + "Custom": { + "name": "trigger_mode", + "kind": { + "Enum": [ + "enabled", + "disabled", + "suspended" + ] + } + } + } + } + ], + "parameters": { + "Left": [ + { + "Custom": { + "name": "http_method", + "kind": { + "Enum": [ + "get", + "post", + "put", + "delete", + "patch" + ] + } + } + } + ] + }, + "nullable": [ + false, + false, + false, + false, + true, + false, + false, + false, + false, + false, + true, + false, + false, + true, + false, + false, + true, + true, + true, + false + ] + }, + "hash": "c7a78d3db99e7f709479c9520471eaf40862b464af7113d2632c626e43c35c04" +} diff --git a/backend/migrations/20260825105019_http_trigger_allowed_origins.down.sql b/backend/migrations/20260825105019_http_trigger_allowed_origins.down.sql new file mode 100644 index 0000000000..bee3e6034c --- /dev/null +++ b/backend/migrations/20260825105019_http_trigger_allowed_origins.down.sql @@ -0,0 +1,2 @@ +-- Add down migration script here +ALTER TABLE http_trigger DROP COLUMN allowed_origins; diff --git a/backend/migrations/20260825105019_http_trigger_allowed_origins.up.sql b/backend/migrations/20260825105019_http_trigger_allowed_origins.up.sql new file mode 100644 index 0000000000..444fc46535 --- /dev/null +++ b/backend/migrations/20260825105019_http_trigger_allowed_origins.up.sql @@ -0,0 +1,2 @@ +-- Add up migration script here +ALTER TABLE http_trigger ADD COLUMN allowed_origins TEXT[]; diff --git a/backend/src/main.rs b/backend/src/main.rs index ac1d286d6e..7f2ce3cd82 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -46,7 +46,8 @@ use windmill_common::{ CUSTOM_TAGS_SETTING, DEFAULT_TAGS_PER_WORKSPACE_SETTING, DEFAULT_TAGS_WORKSPACES_SETTING, DISABLE_PASSWORD_LOGIN_SETTING, EMAIL_DOMAIN_SETTING, ENV_SETTINGS, EXPOSE_DEBUG_METRICS_SETTING, EXPOSE_METRICS_SETTING, EXTRA_PIP_INDEX_URL_SETTING, - FORK_WORKSPACE_TAG_APPEND_FORK_SUFFIX_SETTING, HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, + FORK_WORKSPACE_TAG_APPEND_FORK_SUFFIX_SETTING, + HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING, HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, HUB_API_SECRET_SETTING, HUB_BASE_URL_SETTING, INDEXER_SETTING, INSTANCE_EVENTS_WEBHOOK_SETTING, INSTANCE_PYTHON_VERSION_SETTING, JOB_DEFAULT_TIMEOUT_SECS_SETTING, JOB_ISOLATION_SETTING, JWT_SECRET_SETTING, @@ -135,7 +136,8 @@ use crate::monitor::{ reload_bun_install_min_release_age_setting, reload_bunfig_install_scopes_setting, reload_critical_alert_mute_ui_setting, reload_critical_alert_mute_zombie_job_restart_setting, reload_critical_alerts_on_token_expiry_setting, reload_critical_error_channels_setting, - reload_extra_pip_index_url_setting, reload_http_route_workspaced_route_setting, + reload_extra_pip_index_url_setting, reload_http_route_default_allowed_origins_setting, + reload_http_route_workspaced_route_setting, reload_hub_api_secret_setting, reload_hub_base_url_setting, reload_instance_events_webhook_setting, reload_job_default_timeout_setting, reload_job_isolation_setting, reload_jwt_secret_setting, reload_license_key, @@ -2145,6 +2147,11 @@ async fn process_notify_event( tracing::error!(error = %e, "Could not reload app workspaced route setting"); } } + HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING => { + if let Err(e) = reload_http_route_default_allowed_origins_setting(db).await { + tracing::error!(error = %e, "Could not reload http route default allowed origins setting"); + } + } HTTP_ROUTE_WORKSPACED_ROUTE_SETTING => { if let Err(e) = reload_http_route_workspaced_route_setting(db).await { tracing::error!(error = %e, "Could not reload http route workspaced route setting"); diff --git a/backend/src/monitor.rs b/backend/src/monitor.rs index 9f8605c17a..81571bc942 100644 --- a/backend/src/monitor.rs +++ b/backend/src/monitor.rs @@ -106,8 +106,9 @@ use windmill_common::{ use windmill_common::{ client::AuthedClient, global_settings::{ - APP_WORKSPACED_ROUTE_SETTING, HTTP_ROUTE_WORKSPACED_ROUTE, - HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, + parse_allowed_origins_setting, APP_WORKSPACED_ROUTE_SETTING, + HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS, HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING, + HTTP_ROUTE_WORKSPACED_ROUTE, HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, }, queue_metrics::{ QueueSample, QUEUE_COUNT_PREFIX, QUEUE_DELAY_PREFIX, QUEUE_DELAY_SAME_HEAD_SECS, @@ -428,6 +429,18 @@ pub async fn initial_load( pass.setting(APP_WORKSPACED_ROUTE_SETTING, false, |v| async move { apply_app_workspaced_route_setting(v) }); + pass.setting( + HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING, + false, + |v| async move { + if let Err(e) = apply_http_route_default_allowed_origins_setting(v) { + tracing::error!( + "Error reloading http route default allowed origins: {:?}", + e + ) + } + }, + ); pass.setting( HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, false, @@ -7002,6 +7015,34 @@ pub fn apply_app_workspaced_route_setting(app_workspaced_route: Option error::Result<()> { + let v = + load_value_from_global_settings(conn, HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING).await?; + apply_http_route_default_allowed_origins_setting(v) +} + +pub fn apply_http_route_default_allowed_origins_setting( + value: Option, +) -> error::Result<()> { + // A bad value leaves whatever is already loaded in place rather than + // reverting to no restriction. On the boot path that is still the empty + // default, so what keeps a stored typo from widening CORS instance-wide is + // write-time validation, not this. + let origins = match parse_allowed_origins_setting(value.as_ref()) { + Ok(origins) => origins, + Err(err) => { + tracing::error!( + "Invalid {} setting, keeping the previous value: {err:#}", + HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING + ); + return Ok(()); + } + }; + + HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS.store(std::sync::Arc::new(origins)); + Ok(()) +} + pub async fn reload_http_route_workspaced_route_setting(conn: &DB) -> error::Result<()> { let v = load_value_from_global_settings(conn, HTTP_ROUTE_WORKSPACED_ROUTE_SETTING).await?; apply_http_route_workspaced_route_setting(conn, v).await diff --git a/backend/summarized_schema.txt b/backend/summarized_schema.txt index d0f9d63647..df3e6ddbf8 100644 --- a/backend/summarized_schema.txt +++ b/backend/summarized_schema.txt @@ -120,7 +120,7 @@ group_: workspace_id(char), name(char), summary(text), extra_perms(jsonb) group_permission_history: id(bigint), workspace_id(char), group_name(char), changed_by(char), changed_at(ts), change_type(char), member_affected(char) FK: (workspace_id, group_name) -> group_(workspace_id, name) healthchecks: id(bigint), check_type(text), healthy(bool), created_at(ts) -http_trigger: path(char), route_path(char), route_path_key(char), script_path(char), is_flow(bool), workspace_id(char), edited_by(char), email(char), edited_at(ts), extra_perms(jsonb), authentication_method(authentication_method), http_method(http_method), static_asset_config(jsonb), is_static_website(bool), workspaced_route(bool), wrap_body(bool), raw_string(bool), authentication_resource_path(char), summary(char), description(text), error_handler_path(char), error_handler_args(jsonb), retry(jsonb), request_type(request_type), mode(trigger_mode), labels(text[]) +http_trigger: path(char), route_path(char), route_path_key(char), script_path(char), is_flow(bool), workspace_id(char), edited_by(char), email(char), edited_at(ts), extra_perms(jsonb), authentication_method(authentication_method), http_method(http_method), static_asset_config(jsonb), is_static_website(bool), workspaced_route(bool), wrap_body(bool), raw_string(bool), allowed_origins(text[]), authentication_resource_path(char), summary(char), description(text), error_handler_path(char), error_handler_args(jsonb), retry(jsonb), request_type(request_type), mode(trigger_mode), labels(text[]) input: id(uuid), workspace_id(char), runnable_id(char), runnable_type(runnable_type), name(text), args(jsonb), created_at(ts), created_by(char), is_public(bool) FK: (workspace_id) -> workspace(id) instance_group: name(char), summary(char), id(char), scim_display_name(char), external_id(char) diff --git a/backend/tests/instance_config.rs b/backend/tests/instance_config.rs index 76102656a5..64d7854173 100644 --- a/backend/tests/instance_config.rs +++ b/backend/tests/instance_config.rs @@ -1442,7 +1442,10 @@ async fn declarative_sync_rejects_an_unusable_webhook_base_url(db: Pool "the other settings in the same apply must not have been written either" ); } + +#[sqlx::test(fixtures("base"))] +async fn declarative_sync_rejects_an_unusable_default_allowed_origins(db: Pool) { + // The declarative writers (the sync-config CLI, the operator's ConfigMap + // sync) do not run the HTTP layer's pre-write hook, so an origin list that + // cannot be parsed would persist here, be dropped at boot, and leave the + // instance with no restriction at all. + clear_settings_and_configs(&db).await; + let before = count_global_settings(&db).await; + + for bad in [ + serde_json::json!([""]), + serde_json::json!(["https://a.example,https://b.example"]), + serde_json::json!("null"), + ] { + let mut desired = BTreeMap::new(); + desired.insert( + "http_route_default_allowed_origins".to_string(), + bad.clone(), + ); + let err = windmill_common::instance_config::sync_global_settings_declarative( + &db, + &BTreeMap::new(), + &desired, + ) + .await + .expect_err(&format!("{bad} must fail the sync")); + assert!( + err.to_string() + .contains("http_route_default_allowed_origins"), + "the error should name the offending setting, got: {err}" + ); + } + + assert_eq!( + count_global_settings(&db).await, + before, + "a rejected sync must not have persisted anything" + ); + + // A usable list still syncs. + let mut desired = BTreeMap::new(); + desired.insert( + "http_route_default_allowed_origins".to_string(), + serde_json::json!(["https://app.example.com"]), + ); + windmill_common::instance_config::sync_global_settings_declarative( + &db, + &BTreeMap::new(), + &desired, + ) + .await + .expect("a valid origin list must sync"); +} diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 0b975fda98..0fe358da1c 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -58,12 +58,13 @@ use windmill_common::{ AI_CONFIG_SETTING, APP_WORKSPACED_ROUTE_SETTING, AUTOMATE_USERNAME_CREATION_SETTING, CRITICAL_ALERT_MUTE_UI_SETTING, CUSTOM_TAGS_SETTING, DEFAULT_TAGS_WORKSPACES_SETTING, DISABLE_HUB_SETTING, EMAIL_DOMAIN_SETTING, ENV_SETTINGS, - GITHUB_APP_WEBHOOK_BASE_URL_SETTING, HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, - HUB_ACCESSIBLE_URL_SETTING, HUB_BASE_URL_SETTING, INSTANCE_BANNER_SETTING, - MAX_RETENTION_OVERRIDE_WORKSPACES, RETENTION_PERIOD_SECS_OVERRIDES_SETTING, - RUFF_CONFIG_SETTING, UNIQUE_ID_SETTING, WORKSPACE_FAIRNESS_DURATION_SECS_SETTING, - WORKSPACE_FAIRNESS_ENABLED_SETTING, WORKSPACE_FAIRNESS_MAX_PERCENT_SETTING, - WORKSPACE_FAIRNESS_MIN_TOTAL_SETTING, WS_BASE_URL_SETTING, + GITHUB_APP_WEBHOOK_BASE_URL_SETTING, HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING, + HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, HUB_ACCESSIBLE_URL_SETTING, HUB_BASE_URL_SETTING, + INSTANCE_BANNER_SETTING, MAX_RETENTION_OVERRIDE_WORKSPACES, + RETENTION_PERIOD_SECS_OVERRIDES_SETTING, RUFF_CONFIG_SETTING, UNIQUE_ID_SETTING, + WORKSPACE_FAIRNESS_DURATION_SECS_SETTING, WORKSPACE_FAIRNESS_ENABLED_SETTING, + WORKSPACE_FAIRNESS_MAX_PERCENT_SETTING, WORKSPACE_FAIRNESS_MIN_TOTAL_SETTING, + WS_BASE_URL_SETTING, }, instance_config::{self, ApplyMode, InstanceConfig}, server::Smtp, @@ -1047,6 +1048,12 @@ async fn run_setting_pre_write_hook( } } } + HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING => { + // 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 => { let serde_json::Value::Bool(workspaced_route) = value else { return Err(error::Error::BadRequest(format!( @@ -1329,6 +1336,11 @@ pub async fn get_global_setting( && key != EMAIL_DOMAIN_SETTING && key != APP_WORKSPACED_ROUTE_SETTING && key != HTTP_ROUTE_WORKSPACED_ROUTE_SETTING + // The route editor shows the inherited default to whoever is editing a + // trigger, who is usually not a superadmin. Not a secret either: any + // browser discovers the list by reading Access-Control-Allow-Origin off + // a response. + && key != HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING && key != WS_BASE_URL_SETTING && key != INSTANCE_BANNER_SETTING { diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 879e739dc0..3a2b1d715a 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -6056,7 +6056,7 @@ async fn clone_triggers_and_schedules( path, route_path, route_path_key, script_path, is_flow, workspace_id, edited_by, edited_at, extra_perms, authentication_method, http_method, static_asset_config, is_static_website, workspaced_route, wrap_body, - raw_string, authentication_resource_path, summary, description, + raw_string, allowed_origins, authentication_resource_path, summary, description, error_handler_path, error_handler_args, retry, request_type, mode, permissioned_as, labels ) @@ -6064,7 +6064,7 @@ async fn clone_triggers_and_schedules( path, route_path, route_path_key, script_path, is_flow, $1, edited_by, edited_at, extra_perms, authentication_method, http_method, static_asset_config, is_static_website, workspaced_route, wrap_body, - raw_string, authentication_resource_path, summary, description, + raw_string, allowed_origins, authentication_resource_path, summary, description, error_handler_path, error_handler_args, retry, request_type, 'disabled'::TRIGGER_MODE, permissioned_as, labels FROM http_trigger diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 9690a10663..ba4eddd97e 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -30999,6 +30999,14 @@ components: raw_string: type: boolean description: If true, passes the request body as a raw string instead of parsing as JSON + allowed_origins: + type: array + nullable: true + maxItems: 100 + items: + type: string + maxLength: 256 + description: "Origins allowed to call this route cross-origin, matched against the request's Origin header (ignoring case) and echoed back on a match. When set, the list governs both the preflight and the response, overriding any Access-Control-Allow-Origin the runnable returns via wm_headers. Use ['*'] to opt out of any restriction, including the http_route_default_allowed_origins instance setting. An empty list is not a configuration and resolves exactly as null does. When null, the instance setting applies, or Access-Control-Allow-Origin: * if it is unset. Ignored on a static website, which has no authentication of its own and so hands out public files: restricting which browsers may read them protects nothing while breaking cross-origin webfonts and fetches. A single-file static asset is not exempt, since it can carry an authentication_method." error_handler_path: type: string description: Path to a script to run when the triggered job fails. A bare @@ -31090,6 +31098,14 @@ components: raw_string: type: boolean description: If true, passes the request body as a raw string instead of parsing as JSON + allowed_origins: + type: array + nullable: true + maxItems: 100 + items: + type: string + maxLength: 256 + description: "Origins allowed to call this route cross-origin, matched against the request's Origin header (ignoring case) and echoed back on a match. When set, the list governs both the preflight and the response, overriding any Access-Control-Allow-Origin the runnable returns via wm_headers. Use ['*'] to opt out of any restriction, including the http_route_default_allowed_origins instance setting. An empty list is not a configuration and resolves exactly as null does. When null, the instance setting applies, or Access-Control-Allow-Origin: * if it is unset. Ignored on a static website, which has no authentication of its own and so hands out public files: restricting which browsers may read them protects nothing while breaking cross-origin webfonts and fetches. A single-file static asset is not exempt, since it can carry an authentication_method." error_handler_path: type: string description: Path to a script to run when the triggered job fails. A bare @@ -31188,6 +31204,14 @@ components: raw_string: type: boolean description: If true, passes the request body as a raw string instead of parsing as JSON + allowed_origins: + type: array + nullable: true + maxItems: 100 + items: + type: string + maxLength: 256 + description: "Origins allowed to call this route cross-origin, matched against the request's Origin header (ignoring case) and echoed back on a match. When set, the list governs both the preflight and the response, overriding any Access-Control-Allow-Origin the runnable returns via wm_headers. Use ['*'] to opt out of any restriction, including the http_route_default_allowed_origins instance setting. An empty list is not a configuration and resolves exactly as null does. When null, the instance setting applies, or Access-Control-Allow-Origin: * if it is unset. Ignored on a static website, which has no authentication of its own and so hands out public files: restricting which browsers may read them protects nothing while breaking cross-origin webfonts and fetches. A single-file static asset is not exempt, since it can carry an authentication_method." error_handler_path: type: string description: Path to a script to run when the triggered job fails. A bare diff --git a/backend/windmill-api/src/triggers/http/handler.rs b/backend/windmill-api/src/triggers/http/handler.rs index e58aab44a2..387ba56589 100644 --- a/backend/windmill-api/src/triggers/http/handler.rs +++ b/backend/windmill-api/src/triggers/http/handler.rs @@ -1,6 +1,7 @@ use super::{ - http_trigger_args::RawHttpTriggerArgs, refresh_routers, AuthenticationMethod, HttpMethod, - RequestType, TriggerRoute, HTTP_ACCESS_CACHE, HTTP_AUTH_CACHE, HTTP_ROUTERS_CACHE, + effective_allowed_origins, http_trigger_args::RawHttpTriggerArgs, match_origin, + refresh_routers, AuthenticationMethod, HttpMethod, RequestType, TriggerRoute, + HTTP_ACCESS_CACHE, HTTP_AUTH_CACHE, HTTP_ROUTERS_CACHE, }; use crate::{ auth::{AuthCache, OptTokened}, @@ -24,6 +25,7 @@ use std::{collections::HashMap, sync::Arc}; use windmill_common::{ db::UserDB, error::{Error, Result}, + global_settings::HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS, jobs::JobTriggerKind, triggers::{TriggerKind, TriggerMetadata}, utils::{not_found_if_none, StripPath}, @@ -37,12 +39,222 @@ use { windmill_object_store::build_object_store_client, }; +/// Which router a request's CORS decision must be looked up in. +/// +/// A preflight names the method it is asking about in +/// `Access-Control-Request-Method`; the routers are keyed by method, so without +/// that header there is nothing to look up. +fn cors_lookup_method(req: &axum::extract::Request) -> Option { + let method = req.method(); + 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(routable_method) + } else { + routable_method(method) + } +} + +/// The router key a request method maps to. `HEAD` resolves the `GET` route it +/// mirrors, and does so for a preflight naming it too: browsers send +/// `Access-Control-Request-Method: HEAD` when the HEAD carries a non-safelisted +/// header, and answering that preflight from a different route than the request +/// itself resolves is how the two come to disagree. +fn routable_method(method: &http::Method) -> Option { + if method == http::Method::HEAD { + Some(HttpMethod::Get) + } else { + HttpMethod::try_from(method).ok() + } +} + +/// The key to look a request up by, matching what `route_job` resolves it to. +/// +/// `Path` percent-decodes before `get_http_route_trigger` builds its +/// lookup key, so decoding here is what keeps the two agreeing: on the raw path, +/// `/us%65rs` misses the trigger registered at `/users` that goes on to serve the +/// request, and the response would carry the permissive default instead of that +/// trigger's allowlist. +fn cors_lookup_path(raw_path: &str) -> Option { + let decoded = urlencoding::decode(raw_path).ok()?; + // `StripPath::to_path` strips one leading slash and the handler trims + // trailing ones, before a single `/` is prefixed back on. + let stripped = decoded.strip_prefix('/').unwrap_or(&decoded); + Some(format!("/{}", stripped.trim_end_matches('/'))) +} + +/// What the middleware should stamp, decided while the routers guard is held. +/// +/// Deliberately small and owned: the allowlist itself never leaves the guard, +/// so a large one is scanned in place instead of being copied per request onto +/// a path an unauthenticated preflight can reach. +#[derive(Clone)] +enum CorsDecision { + /// No allowlist applies, so the permissive default stands. + Unrestricted, + /// An allowlist applies. `allow_origin` is the value to echo, present only + /// when the request's own `Origin` is on the list. + Restricted { route_method: Option, allow_origin: Option }, + /// The routers could not be read, so nothing is known about this path. + Unavailable, +} + +/// Whether a route actually serves a static website, rather than merely saying +/// it does. +/// +/// `is_static_website` is a caller-set flag that validation ties to nothing: a +/// route can carry it while having no assets configured and a `script_path` +/// that `route_job` runs regardless. Keying the exemption off the flag alone +/// would let one boolean disable a route's allowlist and hand its runnable back +/// the `wm_headers` escape hatch, so the assets have to be there too. +fn serves_a_static_website(trigger: &TriggerRoute) -> bool { + trigger.is_static_website && trigger.static_asset_config.is_some() +} + +/// A static website is never subject to an allowlist, its own included. It has +/// no authentication of its own — the editor does not offer any — so it hands +/// out public files that any non-browser client can already fetch, and +/// restricting which browsers may read them protects nothing while breaking the +/// cross-origin uses that do consult CORS: a webfont, a `crossorigin` asset, a +/// `fetch`. +/// +/// A single-file static asset is not exempt. That one can carry an +/// `authentication_method`, so its content need not be public, and an allowlist +/// is what keeps another origin from reading a response its own credentials +/// would not have obtained. +/// +/// The CORS verdict for a request, published by whoever resolved its trigger. +/// +/// The middleware stamps headers after the handler returns, but only the +/// handler knows which trigger it actually served. Re-deriving that from the +/// routers cache is a second lookup which can disagree with the first when a +/// route is edited, deleted or widened mid-request, and every ordering of the +/// two is wrong in some case. So the verdict travels with the request instead +/// of being worked out twice. +#[derive(Clone, Default)] +struct ResolvedCorsPolicy(std::sync::Arc>); + +impl ResolvedCorsPolicy { + /// Record what the trigger being served allows. Called once, where the + /// route is resolved, so the answer cannot drift from the response. + fn publish(&self, trigger: &TriggerRoute, method: Option, headers: &HeaderMap) { + let decision = if serves_a_static_website(trigger) { + CorsDecision::Unrestricted + } else { + let instance_default = HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS.load(); + match effective_allowed_origins( + trigger.allowed_origins.as_deref(), + instance_default.as_slice(), + ) { + None => CorsDecision::Unrestricted, + Some(allowed_origins) => CorsDecision::Restricted { + route_method: method, + allow_origin: match_origin(allowed_origins, headers.get(http::header::ORIGIN)), + }, + } + }; + let _ = self.0.set(decision); + } + + fn published(&self) -> Option { + self.0.get().cloned() + } +} + +/// Decide the CORS answer from the routers cache, for a request no handler +/// published a verdict for: a preflight, an unknown path, or any failure ahead +/// of the publish — authentication included, which runs after the route itself +/// resolves. +/// +/// Loads the routers when the cache is cold, the way `get_http_route_trigger` +/// does, so a preflight is answered from the same view of the routes as the +/// request that follows it. +async fn resolve_cors_decision( + db: &DB, + http_method: HttpMethod, + requested_path: &str, + origin: Option<&http::HeaderValue>, +) -> CorsDecision { + let routers_cache = HTTP_ROUTERS_CACHE.read().await; + + let routers_cache = if routers_cache.routers.is_empty() { + drop(routers_cache); + match refresh_routers(db, false).await { + Ok((_, routers_cache)) => routers_cache, + Err(err) => { + tracing::error!("Could not load HTTP routers to resolve CORS: {err:#}"); + return CorsDecision::Unavailable; + } + } + } else { + routers_cache + }; + + let Some(router) = routers_cache.routers.get(&http_method) else { + return CorsDecision::Unavailable; + }; + + let route = router.at(requested_path).ok(); + if route + .as_ref() + .is_some_and(|trigger| serves_a_static_website(trigger.value)) + { + return CorsDecision::Unrestricted; + } + let route_allowed_origins = route + .as_ref() + .and_then(|trigger| trigger.value.allowed_origins.as_deref()); + + let instance_default = HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS.load(); + match effective_allowed_origins(route_allowed_origins, instance_default.as_slice()) { + None => CorsDecision::Unrestricted, + Some(allowed_origins) => CorsDecision::Restricted { + route_method: route.map(|_| http_method), + allow_origin: match_origin(allowed_origins, origin), + }, + } +} + async fn conditional_cors_middleware( - req: axum::extract::Request, + Extension(db): Extension, + mut req: axum::extract::Request, next: axum::middleware::Next, ) -> Response { + 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. + // 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()); + let mut response = next.run(req).await; + let decision = match resolved.published() { + // The handler resolved a trigger and said what it served under. That is + // the policy this response was produced with, so nothing else can be + // more authoritative. + Some(decision) => decision, + // No verdict was published: a preflight, an unknown path, or a request + // that failed before reaching the publish, authentication included. No + // runnable produced this body, so reading the cache cannot contradict + // anything. + None => match lookup_method.zip(cors_lookup_path(uri.path())) { + Some((method, path)) => { + resolve_cors_decision(&db, method, &path, origin.as_ref()).await + } + // Not a preflight, not a routable method, or a path that does not + // decode. + None => CorsDecision::Unrestricted, + }, + }; + let headers = response.headers_mut(); // Check existing headers first to determine what not to insert @@ -67,18 +279,65 @@ async fn conditional_cors_middleware( } } - // Insert only the missing headers - if !not_insert_origin { - headers.insert( - http::header::ACCESS_CONTROL_ALLOW_ORIGIN, - http::HeaderValue::from_static("*"), - ); + match &decision { + CorsDecision::Restricted { allow_origin, .. } => { + // A configured allowlist decides, overriding any `wm_headers` value + // the runnable set. The preflight is answered before any code runs, + // so config is the only thing it can consult; letting the response + // widen what the preflight advertised would make the two disagree + // and leave the allowlist bounding nothing. A route escapes a + // stricter instance default — `wm_headers` included — by setting + // its own list to `*`. + match allow_origin { + Some(value) => { + headers.insert(http::header::ACCESS_CONTROL_ALLOW_ORIGIN, value.clone()) + } + // No match: omit the header entirely so the browser blocks the + // read, and drop any value the runnable set. + None => headers.remove(http::header::ACCESS_CONTROL_ALLOW_ORIGIN), + }; + // Appended, not inserted: the answer now depends on the request's + // 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")); + } + // The routers could not be read, so nothing is known about this path; + // only a preflight or an unresolved request reaches here. Answering a + // preflight permissively would let a disallowed origin go on to invoke + // a runnable whose purpose may be a side effect. + CorsDecision::Unavailable => { + headers.remove(http::header::ACCESS_CONTROL_ALLOW_ORIGIN); + } + CorsDecision::Unrestricted => { + if !not_insert_origin { + headers.insert( + http::header::ACCESS_CONTROL_ALLOW_ORIGIN, + http::HeaderValue::from_static("*"), + ); + } + } } if !not_insert_methods { + // A route accepts exactly one method, so advertising all seven + // overstates it. Only a route under an allowlist gets the narrower + // answer; an unrestricted one advertises the full supported set, since + // narrowing it would say something about a route the response is not + // otherwise willing to disclose. + let restricted_method = match &decision { + CorsDecision::Restricted { route_method, .. } => *route_method, + _ => None, + }; 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_method { + Some(HttpMethod::Get) => "GET, OPTIONS", + Some(HttpMethod::Post) => "POST, OPTIONS", + Some(HttpMethod::Put) => "PUT, OPTIONS", + Some(HttpMethod::Delete) => "DELETE, OPTIONS", + Some(HttpMethod::Patch) => "PATCH, OPTIONS", + None => "GET, POST, PUT, DELETE, PATCH, HEAD, OPTIONS", + }), ); } @@ -237,6 +496,7 @@ async fn route_job( Extension(db): Extension, Extension(user_db): Extension, Extension(auth_cache): Extension>, + Extension(cors_policy): Extension, OptTokened { token }: OptTokened, Path(route_path): Path, headers: HeaderMap, @@ -255,6 +515,10 @@ async fn route_job( .await .map_err(|e| e.into_response())?; + // Publish before anything else can fail: the CORS middleware stamps this + // response either way, and it must reflect the trigger actually served. + cors_policy.publish(&trigger, routable_method(&args.0.metadata.method), &headers); + if trigger.script_path.is_empty() && trigger.static_asset_config.is_none() { return Err(Error::NotFound(format!( "Runnable path of HTTP route at path: {}", diff --git a/backend/windmill-common/src/global_settings.rs b/backend/windmill-common/src/global_settings.rs index 1d590e38bf..c0ed63cd53 100644 --- a/backend/windmill-common/src/global_settings.rs +++ b/backend/windmill-common/src/global_settings.rs @@ -118,6 +118,7 @@ pub const OTEL_TRACING_PROXY_SETTING: &str = "otel_tracing_proxy"; pub const OTEL_TRACES_RETENTION_SECS_SETTING: &str = "otel_traces_retention_secs"; pub const APP_WORKSPACED_ROUTE_SETTING: &str = "app_workspaced_route"; pub const HTTP_ROUTE_WORKSPACED_ROUTE_SETTING: &str = "http_route_workspaced_route"; +pub const HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING: &str = "http_route_default_allowed_origins"; pub const SECRET_BACKEND_SETTING: &str = "secret_backend"; pub const MIN_KEEP_ALIVE_VERSION_SETTING: &str = "min_keep_alive_version"; pub const GITHUB_ENTERPRISE_APP_SETTING: &str = "github_enterprise_app"; @@ -362,6 +363,125 @@ use std::sync::atomic::AtomicBool; lazy_static::lazy_static! { pub static ref HTTP_ROUTE_WORKSPACED_ROUTE: AtomicBool = AtomicBool::new(false); pub static ref DISABLE_PASSWORD_LOGIN: AtomicBool = AtomicBool::new(false); + /// Origins HTTP routes allow cross-origin when they configure none of their + /// own. Empty means unset, which keeps the historical `*`. + pub static ref HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS: arc_swap::ArcSwap> = + arc_swap::ArcSwap::from_pointee(vec![]); +} + +/// Whether an allowlist places no restriction at all. +/// +/// `*` is the explicit "open on purpose" entry, and a route carrying it behaves +/// exactly as an unconfigured one: it is how a route opts out of a stricter +/// instance default, including back into the `wm_headers` escape hatch. +pub fn allows_any_origin(allowed_origins: &[String]) -> bool { + allowed_origins.iter().any(|allowed| allowed == "*") +} + +/// An allowlist is scanned on every request to a restricted route, including +/// the unauthenticated preflight, so its size is a request cost anyone can +/// trigger. +pub const MAX_ALLOWED_ORIGINS: usize = 100; +pub const MAX_ALLOWED_ORIGIN_LEN: usize = 256; + +/// Reject allowlist entries that cannot be compared, stored, or safely allowed. +/// +/// The stored string is only ever an operand: `match_origin` echoes the +/// request's own `Origin` back, never this value, so a malformed entry matches +/// nothing and fails closed. Shapes that merely cannot match are the editor's +/// business to warn about, not this function's to refuse. What is left are the +/// three cases where permissiveness costs something: `null` is what every +/// sandboxed iframe sends, so allowing it would admit any page that can open +/// one; a comma cannot survive the editor's comma-separated field, which would +/// silently split one entry into two and widen the list; and an unbounded list +/// makes every preflight pay for it. +pub fn validate_allowed_origins(allowed_origins: &[String]) -> crate::error::Result<()> { + if allowed_origins.len() > MAX_ALLOWED_ORIGINS { + return Err(crate::error::Error::BadRequest(format!( + "At most {} allowed origins, got {}.", + MAX_ALLOWED_ORIGINS, + allowed_origins.len() + ))); + } + + for origin in allowed_origins { + if origin == "*" { + continue; + } + + let invalid = |reason: &str| { + crate::error::Error::BadRequest(format!( + "Invalid allowed origin '{}': {}.", + origin, reason + )) + }; + + if origin.is_empty() { + return Err(invalid("must not be empty")); + } + if origin.len() > MAX_ALLOWED_ORIGIN_LEN { + return Err(invalid("is longer than any origin a browser sends")); + } + // The editor edits the whole list as one comma-separated field, so an + // entry carrying a comma comes back as two and widens the list. + if origin.contains(',') { + return Err(invalid("must not contain a comma, which separates entries")); + } + if origin.eq_ignore_ascii_case("null") { + return Err(invalid( + "'null' is what a sandboxed iframe sends, so allowing it would allow any page that can open one", + )); + } + // An Origin header is always visible ASCII, so a value outside it can + // never be the string this is compared against. + if !origin.chars().all(|c| c.is_ascii_graphic()) { + return Err(invalid( + "must contain only visible ASCII, with no whitespace", + )); + } + } + + Ok(()) +} + +/// Read [`HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING`] from its stored value. +/// +/// Accepts the comma-separated string the settings UI writes, or a JSON array +/// for anything setting it through the API directly. +pub fn parse_allowed_origins_setting( + value: Option<&serde_json::Value>, +) -> crate::error::Result> { + let origins = match value { + None | Some(serde_json::Value::Null) => vec![], + Some(serde_json::Value::String(raw)) => raw + .split(',') + .map(|origin| origin.trim().to_string()) + .filter(|origin| !origin.is_empty()) + .collect(), + Some(serde_json::Value::Array(entries)) => entries + .iter() + .map(|entry| match entry { + serde_json::Value::String(origin) => Ok(origin.trim().to_string()), + _ => Err(crate::error::Error::BadRequest(format!( + "{} entries must be strings", + HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING + ))), + }) + // Not filtered for empties, unlike the string form: there a + // trailing separator naturally yields an empty token, whereas an + // empty array entry is something the caller wrote and validation + // should reject rather than silently drop. + .collect::>>()?, + Some(_) => { + return Err(crate::error::Error::BadRequest(format!( + "{} expected to be a comma-separated string or an array of strings", + HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING + ))) + } + }; + + validate_allowed_origins(&origins)?; + Ok(origins) } pub const ENV_SETTINGS: &[&str] = &[ diff --git a/backend/windmill-common/src/instance_config.rs b/backend/windmill-common/src/instance_config.rs index e28bf139cf..1dd3396809 100644 --- a/backend/windmill-common/src/instance_config.rs +++ b/backend/windmill-common/src/instance_config.rs @@ -1288,8 +1288,9 @@ pub fn diff_worker_configs( ConfigsDiff { upserts, deletes } } -/// Declaratively replace the global settings, rejecting a `github_app_webhook_base_url` -/// the API would reject. +/// Declaratively replace the global settings, rejecting a +/// `github_app_webhook_base_url` or `http_route_default_allowed_origins` the +/// API would reject. /// /// Every declarative writer (the `sync-config` CLI, the Kubernetes operator's /// ConfigMap sync) MUST go through this rather than calling @@ -1348,6 +1349,13 @@ pub async fn sync_global_settings_declarative( .map_err(|e| anyhow::anyhow!("{banner_key}: {e}"))?, } + // An origin list that cannot be parsed is dropped at boot, leaving the + // empty default — which is no restriction at all. Rejecting it here is what + // keeps a typo in a ConfigMap from silently widening CORS instance-wide. + let origins_key = crate::global_settings::HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING; + crate::global_settings::parse_allowed_origins_setting(desired.get(origins_key)) + .map_err(|e| anyhow::anyhow!("{origins_key}: {e}"))?; + let diff = diff_global_settings(current, desired, ApplyMode::Replace); apply_settings_diff(db, &diff).await?; diff --git a/backend/windmill-trigger-http/src/handler.rs b/backend/windmill-trigger-http/src/handler.rs index 68986bb6f9..01ddfd4a81 100644 --- a/backend/windmill-trigger-http/src/handler.rs +++ b/backend/windmill-trigger-http/src/handler.rs @@ -9,7 +9,7 @@ use sqlx::PgConnection; use std::collections::HashSet; use windmill_api_auth::{check_scopes, ApiAuthed}; use windmill_audit::{audit_oss::audit_log, ActionKind}; -use windmill_common::global_settings::HTTP_ROUTE_WORKSPACED_ROUTE; +use windmill_common::global_settings::{validate_allowed_origins, HTTP_ROUTE_WORKSPACED_ROUTE}; use windmill_common::{ db::UserDB, error::{Error, Result}, @@ -189,6 +189,7 @@ pub async fn insert_new_trigger_into_db( authentication_resource_path, wrap_body, raw_string, + allowed_origins, script_path, summary, description, @@ -207,7 +208,7 @@ pub async fn insert_new_trigger_into_db( retry ) VALUES ( - $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, now(), $20, $21, $22, $23 + $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, now(), $21, $22, $23, $24 ) "#, w_id, @@ -218,6 +219,7 @@ pub async fn insert_new_trigger_into_db( trigger.config.authentication_resource_path, trigger.config.wrap_body.unwrap_or(false), trigger.config.raw_string.unwrap_or(false), + trigger.config.allowed_origins.as_deref(), trigger.base.script_path, trigger.config.summary, trigger.config.description, @@ -444,6 +446,7 @@ impl TriggerCrud for HttpTrigger { "workspaced_route", "wrap_body", "raw_string", + "allowed_origins", ]; fn get_deployed_object(path: String, parent_path: Option) -> DeployedObject { @@ -474,6 +477,8 @@ impl TriggerCrud for HttpTrigger { validate_authentication_method(new.authentication_method, new.raw_string)?; + validate_allowed_origins(new.allowed_origins.as_deref().unwrap_or_default())?; + Ok(()) } @@ -492,6 +497,8 @@ impl TriggerCrud for HttpTrigger { validate_authentication_method(edit.authentication_method, edit.raw_string)?; + validate_allowed_origins(edit.allowed_origins.as_deref().unwrap_or_default())?; + Ok(()) } @@ -554,33 +561,35 @@ impl TriggerCrud for HttpTrigger { workspaced_route = $3, wrap_body = $4, raw_string = $5, - authentication_resource_path = $6, - script_path = $7, - path = $8, - is_flow = $9, - mode = $10, - http_method = $11, - static_asset_config = $12, - edited_by = $13, - permissioned_as = $14, - request_type = $15, - authentication_method = $16, - summary = $17, - description = $18, + allowed_origins = $6, + authentication_resource_path = $7, + script_path = $8, + path = $9, + is_flow = $10, + mode = $11, + http_method = $12, + static_asset_config = $13, + edited_by = $14, + permissioned_as = $15, + request_type = $16, + authentication_method = $17, + summary = $18, + description = $19, edited_at = now(), - is_static_website = $19, - error_handler_path = $20, - error_handler_args = $21, - retry = $22 + is_static_website = $20, + error_handler_path = $21, + error_handler_args = $22, + retry = $23 WHERE - workspace_id = $23 AND - path = $24 + workspace_id = $24 AND + path = $25 "#, route_path, &route_path_key, Some(effective_workspaced), trigger.config.wrap_body.unwrap_or(false), trigger.config.raw_string.unwrap_or(false), + trigger.config.allowed_origins.as_deref(), trigger.config.authentication_resource_path, trigger.base.script_path, trigger.base.path, @@ -613,30 +622,32 @@ impl TriggerCrud for HttpTrigger { SET wrap_body = $1, raw_string = $2, - authentication_resource_path = $3, - script_path = $4, - path = $5, - is_flow = $6, - mode = $7, - http_method = $8, - static_asset_config = $9, - edited_by = $10, - permissioned_as = $11, - request_type = $12, - authentication_method = $13, - summary = $14, - description = $15, + allowed_origins = $3, + authentication_resource_path = $4, + script_path = $5, + path = $6, + is_flow = $7, + mode = $8, + http_method = $9, + static_asset_config = $10, + edited_by = $11, + permissioned_as = $12, + request_type = $13, + authentication_method = $14, + summary = $15, + description = $16, edited_at = now(), - is_static_website = $16, - error_handler_path = $17, - error_handler_args = $18, - retry = $19 + is_static_website = $17, + error_handler_path = $18, + error_handler_args = $19, + retry = $20 WHERE - workspace_id = $20 AND - path = $21 + workspace_id = $21 AND + path = $22 "#, trigger.config.wrap_body.unwrap_or(false), trigger.config.raw_string.unwrap_or(false), + trigger.config.allowed_origins.as_deref(), trigger.config.authentication_resource_path, trigger.base.script_path, trigger.base.path, diff --git a/backend/windmill-trigger-http/src/lib.rs b/backend/windmill-trigger-http/src/lib.rs index b78276c28d..40bab102a5 100644 --- a/backend/windmill-trigger-http/src/lib.rs +++ b/backend/windmill-trigger-http/src/lib.rs @@ -8,7 +8,7 @@ use tokio::sync::{RwLock, RwLockReadGuard}; use windmill_common::{ error::{Error, Result}, flows::Retry, - global_settings::HTTP_ROUTE_WORKSPACED_ROUTE, + global_settings::{allows_any_origin, HTTP_ROUTE_WORKSPACED_ROUTE}, utils::ExpiringCacheEntry, worker::CLOUD_HOSTED, DB, @@ -51,6 +51,7 @@ pub struct TriggerRoute { pub workspaced_route: bool, pub wrap_body: bool, pub raw_string: bool, + pub allowed_origins: Option>, pub error_handler_path: Option, pub error_handler_args: Option>>, pub retry: Option>, @@ -127,6 +128,7 @@ pub struct HttpConfig { pub workspaced_route: bool, pub wrap_body: bool, pub raw_string: bool, + pub allowed_origins: Option>, } #[derive(Debug, Clone, Serialize)] @@ -144,6 +146,7 @@ pub struct HttpConfigRequest { pub workspaced_route: Option, pub wrap_body: Option, pub raw_string: Option, + pub allowed_origins: Option>, } #[derive(Deserialize)] @@ -162,6 +165,7 @@ struct HttpConfigRequestHelper { workspaced_route: Option, wrap_body: Option, raw_string: Option, + allowed_origins: Option>, } impl<'de> Deserialize<'de> for HttpConfigRequest { @@ -197,6 +201,7 @@ impl<'de> Deserialize<'de> for HttpConfigRequest { workspaced_route: helper.workspaced_route, wrap_body: helper.wrap_body, raw_string: helper.raw_string, + allowed_origins: helper.allowed_origins, }) } } @@ -216,6 +221,50 @@ pub struct RouteExists { pub workspaced_route: Option, } +/// The allowlist that governs a route: its own when it has one, otherwise the +/// instance-wide default. `None` means nothing is configured at either level, so +/// the route keeps the historical permissive behaviour. +/// +/// A list containing `*` is treated as no restriction, which is how a route opts +/// out of a stricter instance default. +pub fn effective_allowed_origins<'a>( + route_allowed_origins: Option<&'a [String]>, + instance_default: &'a [String], +) -> Option<&'a [String]> { + // An empty list is not a configuration. It reads exactly as never having set + // one, so such a route still inherits the instance default rather than + // skipping it, which is what would make `[]` more permissive than `NULL`. + match route_allowed_origins.filter(|list| !list.is_empty()) { + // `*` is the opt-out, including out of a stricter instance default. + Some(list) if allows_any_origin(list) => None, + Some(list) => Some(list), + None => (!instance_default.is_empty() && !allows_any_origin(instance_default)) + .then_some(instance_default), + } +} + +/// Resolve the `Access-Control-Allow-Origin` value for a request, or `None` to +/// omit the header so the browser blocks the read. +/// +/// The request's `Origin` is echoed back only on a match against the allowlist. +/// Reflecting it unchecked is the classic way this feature turns into no +/// restriction at all. +/// +/// The comparison ignores ASCII case because a browser lowercases the scheme and +/// host it sends, so a configured `https://App.Example.com` would otherwise name +/// a real origin and still match nothing. +pub fn match_origin( + allowed_origins: &[String], + origin: Option<&http::HeaderValue>, +) -> Option { + let origin = origin?; + let origin_str = origin.to_str().ok()?; + allowed_origins + .iter() + .any(|allowed| allowed.eq_ignore_ascii_case(origin_str)) + .then(|| origin.clone()) +} + pub fn validate_authentication_method( authentication_method: AuthenticationMethod, raw_string: Option, @@ -276,6 +325,7 @@ pub async fn refresh_routers( static_asset_config AS "static_asset_config: _", wrap_body, raw_string, + allowed_origins, workspaced_route, is_static_website, error_handler_path, @@ -384,6 +434,10 @@ pub struct HttpTrigger; #[cfg(test)] mod tests { use super::*; + // Not used by the lib itself, only exercised here. + use windmill_common::global_settings::{ + validate_allowed_origins, MAX_ALLOWED_ORIGINS, MAX_ALLOWED_ORIGIN_LEN, + }; #[test] fn test_request_type_backward_compatibility() { @@ -578,6 +632,168 @@ mod tests { assert!(validate_authentication_method(AuthenticationMethod::Signature, None).is_ok()); } + // --- CORS allowed origins --- + + fn origin(value: &str) -> http::HeaderValue { + http::HeaderValue::from_str(value).unwrap() + } + + #[test] + fn test_match_origin_exact_match_echoes_request_origin() { + let allowed = vec!["https://a.com".to_string(), "https://b.com".to_string()]; + assert_eq!( + match_origin(&allowed, Some(&origin("https://b.com"))), + Some(origin("https://b.com")) + ); + } + + #[test] + fn test_match_origin_ignores_case() { + let allowed = vec!["https://App.Example.com".to_string()]; + assert_eq!( + match_origin(&allowed, Some(&origin("https://app.example.com"))), + Some(origin("https://app.example.com")) + ); + } + + #[test] + fn test_match_origin_no_match_omits_header() { + let allowed = vec!["https://a.com".to_string()]; + assert_eq!( + match_origin(&allowed, Some(&origin("https://evil.com"))), + None + ); + // A prefix of an allowed origin must not match: https://a.com.evil.com + // is a different site entirely. + assert_eq!( + match_origin(&allowed, Some(&origin("https://a.com.evil.com"))), + None + ); + } + + #[test] + fn test_wildcard_entry_means_unrestricted() { + // `*` is handled before matching: it means "no restriction", which is + // how a route opts out of a stricter instance default. + assert!(allows_any_origin(&["*".to_string()])); + assert!(allows_any_origin(&[ + "https://a.com".to_string(), + "*".to_string() + ])); + assert!(!allows_any_origin(&["https://a.com".to_string()])); + assert_eq!( + effective_allowed_origins(Some(&["*".to_string()]), &[]), + None + ); + } + + #[test] + fn test_effective_allowed_origins_prefers_the_route() { + let route = ["https://a.com".to_string()]; + let default = ["https://default.com".to_string()]; + assert_eq!( + effective_allowed_origins(Some(&route), &default), + Some(&route[..]) + ); + // No route list: the instance default applies. + assert_eq!( + effective_allowed_origins(None, &default), + Some(&default[..]) + ); + // No route list and no instance default: nothing is restricted, so the + // historical permissive behaviour is kept. + assert_eq!(effective_allowed_origins(None, &[]), None); + // A route opting out with `*` escapes a stricter instance default. + assert_eq!( + effective_allowed_origins(Some(&["*".to_string()]), &default), + None + ); + // An empty route list is not a configuration: it resolves exactly as + // `NULL` does, so it inherits the instance default rather than skipping + // it and becoming more permissive than an unset one. + assert_eq!( + effective_allowed_origins(Some(&[]), &default), + Some(&default[..]) + ); + assert_eq!(effective_allowed_origins(Some(&[]), &[]), None); + } + + #[test] + fn test_match_origin_missing_origin_header_omits_header() { + let allowed = vec!["https://a.com".to_string()]; + assert_eq!(match_origin(&allowed, None), None); + } + + #[test] + fn test_validate_allowed_origins_accepts_anything_comparable() { + // A shape that cannot match simply matches nothing, so it is the + // editor's job to warn and not this one's to refuse. What is refused is + // narrower: `null`, values that are not header-comparable, entries that + // cannot round-trip the editor's comma-separated field, and lists past + // the size a request can afford to scan. + let allowed = vec![ + "https://app.example.com".to_string(), + "http://localhost:3000".to_string(), + "http://[::1]:8080".to_string(), + "chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai".to_string(), + // Never matches, but that is the caller's problem, not an error. + "https://app.example.com/".to_string(), + "https://app.example.com:99999".to_string(), + "not-an-origin".to_string(), + "*".to_string(), + ]; + assert!(validate_allowed_origins(&allowed).is_ok()); + assert!(validate_allowed_origins(&[]).is_ok()); + } + + #[test] + fn test_parse_allowed_origins_setting_rejects_empty_array_entries() { + use windmill_common::global_settings::parse_allowed_origins_setting; + // A trailing separator in the string form is a typing artifact and is + // dropped; an empty array entry is something the caller wrote, so it + // must reach validation rather than be filtered away into an empty + // (and therefore unrestricted) default. + assert!(parse_allowed_origins_setting(Some(&serde_json::json!("https://a.com,"))).is_ok()); + assert!(parse_allowed_origins_setting(Some(&serde_json::json!([""]))).is_err()); + assert!( + parse_allowed_origins_setting(Some(&serde_json::json!(["https://a.com", ""]))).is_err() + ); + } + + #[test] + fn test_validate_allowed_origins_bounds_the_list() { + // An allowlist is scanned on every request to a restricted route, the + // unauthenticated preflight included, so its size is a cost anyone can + // trigger. + let too_many = vec!["https://a.com".to_string(); MAX_ALLOWED_ORIGINS + 1]; + assert!(validate_allowed_origins(&too_many).is_err()); + assert!(validate_allowed_origins(&too_many[..MAX_ALLOWED_ORIGINS]).is_ok()); + let too_long = format!("https://{}.com", "a".repeat(MAX_ALLOWED_ORIGIN_LEN)); + assert!(validate_allowed_origins(&[too_long]).is_err()); + } + + #[test] + fn test_validate_allowed_origins_rejects_null_and_uncomparable() { + for invalid in [ + // Every sandboxed iframe sends `Origin: null`, so allowing it would + // grant access to any page that can open one. + "null", + "NULL", // Cannot be the string an Origin header is compared against. + "https://a b.com", + "https://app.example.com ", + "https://exämple.com", + // The editor edits the list as one comma-separated field, so an + // entry carrying a comma would come back as two and widen the list. + "https://a.com,https://b.com", + "", + ] { + assert!( + validate_allowed_origins(&[invalid.to_string()]).is_err(), + "expected {invalid} to be rejected" + ); + } + } + // --- Route path regex --- #[test] diff --git a/cli/src/guidance/skills.gen.ts b/cli/src/guidance/skills.gen.ts index 0c7b91887d..b8b71b1de9 100644 --- a/cli/src/guidance/skills.gen.ts +++ b/cli/src/guidance/skills.gen.ts @@ -8525,6 +8525,21 @@ properties: type: boolean description: If true, passes the request body as a raw string instead of parsing as JSON + allowed_origins: + type: array + items: + type: string + description: 'Origins allowed to call this route cross-origin, matched against + the request''s Origin header (ignoring case) and echoed back on a match. When + set, the list governs both the preflight and the response, overriding any Access-Control-Allow-Origin + the runnable returns via wm_headers. Use [''*''] to opt out of any restriction, + including the http_route_default_allowed_origins instance setting. An empty + list is not a configuration and resolves exactly as null does. When null, the + instance setting applies, or Access-Control-Allow-Origin: * if it is unset. + Ignored on a static website, which has no authentication of its own and so hands + out public files: restricting which browsers may read them protects nothing + while breaking cross-origin webfonts and fetches. A single-file static asset + is not exempt, since it can carry an authentication_method.' error_handler_path: type: string description: Path to a script to run when the triggered job fails. A bare path, diff --git a/frontend/src/lib/components/copilot/chat/workspaceToolsZod.gen.ts b/frontend/src/lib/components/copilot/chat/workspaceToolsZod.gen.ts index 42773873c2..fc019ee983 100644 --- a/frontend/src/lib/components/copilot/chat/workspaceToolsZod.gen.ts +++ b/frontend/src/lib/components/copilot/chat/workspaceToolsZod.gen.ts @@ -69,6 +69,7 @@ export const httpTriggerRequestSchema = z.object({ "wrap_body": z.boolean().describe("If true, wraps the request body in a 'body' parameter").optional(), "mode": z.enum(["enabled", "disabled", "suspended"]).describe("job trigger mode").optional(), "raw_string": z.boolean().describe("If true, passes the request body as a raw string instead of parsing as JSON").optional(), + "allowed_origins": z.array(z.string()).describe("Origins allowed to call this route cross-origin, matched against the request's Origin header (ignoring case) and echoed back on a match. When set, the list governs both the preflight and the response, overriding any Access-Control-Allow-Origin the runnable returns via wm_headers. Use ['*'] to opt out of any restriction, including the http_route_default_allowed_origins instance setting. An empty list is not a configuration and resolves exactly as null does. When null, the instance setting applies, or Access-Control-Allow-Origin: * if it is unset. Ignored on a static website, which has no authentication of its own and so hands out public files: restricting which browsers may read them protects nothing while breaking cross-origin webfonts and fetches. A single-file static asset is not exempt, since it can carry an authentication_method.").nullable().optional(), "error_handler_path": z.string().describe("Path to a script to run when the triggered job fails. A bare path, without the script/ or flow/ prefix a schedule error handler takes; it cannot be a flow.").optional(), "error_handler_args": z.record(z.string(), z.any()).describe("Arguments to pass to the error handler").optional(), "retry": z.object({ diff --git a/frontend/src/lib/components/instanceSettings.ts b/frontend/src/lib/components/instanceSettings.ts index 0548d3ded5..0677648978 100644 --- a/frontend/src/lib/components/instanceSettings.ts +++ b/frontend/src/lib/components/instanceSettings.ts @@ -1,4 +1,5 @@ import type { ButtonType } from './common/button/model' +import { allowedOriginsSettingError } from './triggers/http/utils' import { z } from 'zod' import { instanceBannerFormError } from './instanceBanner' import { writable } from 'svelte/store' @@ -281,6 +282,22 @@ export const settings: Record = { ee_only: '', hideInQuickSetup: true }, + { + label: 'HTTP route default allowed origins', + 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 unset for no instance-wide default, so every route is callable from any origin unless it restricts itself.', + key: 'http_route_default_allowed_origins', + fieldType: 'text', + placeholder: 'https://app.example.com, https://admin.example.com', + storage: 'setting', + error: + '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: unknown) => allowedOriginsSettingError(value) === undefined, + ee_only: '', + hideInQuickSetup: true + }, { label: 'Audit log retention (days)', key: 'audit_log_retention_days', diff --git a/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte new file mode 100644 index 0000000000..50b09a108d --- /dev/null +++ b/frontend/src/lib/components/triggers/http/RouteCorsOption.svelte @@ -0,0 +1,132 @@ + + + + diff --git a/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte b/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte index 5a32120ca2..eb046f380e 100644 --- a/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte +++ b/frontend/src/lib/components/triggers/http/RouteEditorInner.svelte @@ -12,6 +12,7 @@ import ScriptPicker from '$lib/components/ScriptPicker.svelte' import { HttpTriggerService, + SettingService, VariableService, type AuthenticationMethod, type ErrorHandler, @@ -46,9 +47,18 @@ import ResourcePicker from '$lib/components/ResourcePicker.svelte' import ItemPicker from '../../ItemPicker.svelte' import { Popover } from '$lib/components/meltComponents' - import { HUB_SCRIPT_ID, saveHttpRouteFromCfg, SECRET_KEY_PATH } from './utils' + import { + HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING, + HUB_SCRIPT_ID, + allowedOriginsError, + isOriginRestricted, + parseAllowedOriginsSetting, + saveHttpRouteFromCfg, + SECRET_KEY_PATH + } from './utils' import { HubFlow } from '$lib/hub' import RouteBodyTransformerOption from './RouteBodyTransformerOption.svelte' + import RouteCorsOption from './RouteCorsOption.svelte' import TestingBadge from '../testingBadge.svelte' import TriggerEditorToolbar from '../TriggerEditorToolbar.svelte' import PermissionedAsLine from '../PermissionedAsLine.svelte' @@ -110,6 +120,27 @@ let workspaced_route = $state(false) 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. An empty list is not an error + // either, since it resolves as an unset one, so only what the API refuses + // blocks the save. + 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([]) + async function loadInstanceDefaultOrigins() { + try { + const setting = await SettingService.getGlobal({ + key: HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING + }) + instanceDefaultOrigins = parseAllowedOriginsSetting(setting) + } catch { + instanceDefaultOrigins = [] + } + } + loadInstanceDefaultOrigins() let drawerLoading = $state(true) let showLoader = $state(false) let authentication_resource_path = $state('') @@ -160,6 +191,7 @@ !can_write || pathError != '' || !isValid || + originsError != undefined || (!static_asset_config && emptyString(script_path)) || !hasChanged ) @@ -295,6 +327,7 @@ signature_options_type = defaultValues?.signature_options_type ?? 'custom_signature' raw_string = defaultValues?.raw_string ?? false wrap_body = defaultValues?.wrap_body ?? false + allowed_origins = defaultValues?.allowed_origins ?? undefined summary = defaultValues?.summary ?? '' routeDescription = defaultValues?.description ?? '' error_handler_path = defaultValues?.error_handler_path ?? undefined @@ -323,6 +356,7 @@ workspaced_route = cfg?.workspaced_route ?? false wrap_body = cfg?.wrap_body ?? false raw_string = cfg?.raw_string ?? false + allowed_origins = cfg?.allowed_origins ?? undefined summary = cfg?.summary ?? '' mode = cfg?.mode ?? 'enabled' routeDescription = cfg?.description ?? '' @@ -423,6 +457,7 @@ mode, wrap_body, raw_string, + allowed_origins, authentication_resource_path, authentication_method: auth_method, static_asset_config, @@ -758,7 +793,11 @@ extraBadges={[ { name: 'Async', active: request_type === 'async' }, { name: 'SSE', active: request_type === 'sync_sse' }, - { name: 'Authentication', active: authentication_method !== 'none' } + { name: 'Authentication', active: authentication_method !== 'none' }, + { + name: 'CORS', + active: isOriginRestricted(allowed_origins, instanceDefaultOrigins) + } ]} /> {/snippet} @@ -961,6 +1000,14 @@ disabled={!can_write} {testingBadge} /> + + {:else} origin.trim()) + .filter((origin) => origin !== '') +} + +/** + * Entries the API refuses, mirroring `validate_allowed_origins`. + * + * Deliberately short: a stored origin is only ever compared against the + * request's `Origin`, so a shape that cannot match is dead config rather than a + * risk. `null` is the exception, since it is what every sandboxed iframe sends. + */ +export function 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` + return undefined +} + +/** Kept in step with `MAX_ALLOWED_ORIGIN{,S}` in windmill-common. */ +export const MAX_ALLOWED_ORIGINS = 100 +export const MAX_ALLOWED_ORIGIN_LEN = 256 + +/** + * The first entry the API would refuse, if any. Derived from the stored list + * rather than the field, so it stays correct while the editor is on another tab + * and the field is not mounted. An empty list is not an error: it resolves as an + * unset one, so there is nothing in it to refuse. + * + * A comma-bearing entry does reach this, through the settings path where a list + * can be given as an array. It cannot arrive from the origins field, which + * splits on commas before this ever sees it. + */ +export function allowedOriginsError(allowed_origins: string[] | undefined): string | undefined { + if (allowed_origins !== undefined && allowed_origins.length > MAX_ALLOWED_ORIGINS) + return `At most ${MAX_ALLOWED_ORIGINS} origins, got ${allowed_origins.length}` + return allowed_origins?.map(allowedOriginRejection).find((message) => message !== undefined) +} + +/** + * Shapes that save fine but can never equal an `Origin` header, so the route + * would read as configured while allowing nothing. + * + * Advisory only. What a browser sends is the caller's to know, so this points + * at the usual slips rather than deciding which origins are legitimate. + */ +export function allowedOriginWarning(origin: string): string | undefined { + if (origin === '*' || allowedOriginRejection(origin) !== undefined) return undefined + const separator = origin.indexOf('://') + if (separator <= 0) return `'${origin}' has no scheme, such as https://` + const rest = origin.slice(separator + 3) + if (rest === '') return `'${origin}' has no host` + if (/[/?#]/.test(rest)) + return `'${origin}' should be scheme://host[:port], with no path, query or fragment` + if (rest.includes('@')) return `'${origin}' should not contain userinfo` + // Only the port is checked past this point. The host is left alone on + // 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. + if (rest.startsWith(':')) return `'${origin}' has no host` + // An unclosed bracket would otherwise leave `portStart` at zero, which reads + // as "no port" and lets the entry through unremarked. + if (rest.startsWith('[') && !rest.includes(']')) + return `'${origin}' has an unclosed IPv6 host` + 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 + * API accepts an array too. + */ +export function parseAllowedOriginsSetting(setting: unknown): string[] { + if (typeof setting === 'string') return parseAllowedOrigins(setting) + if (Array.isArray(setting)) + return setting + .filter((origin): origin is string => typeof origin === 'string') + .map((origin) => origin.trim()) + .filter((origin) => origin !== '') + return [] +} + +/** + * Whether a route is restricted to specific origins, mirroring + * `effective_allowed_origins` in windmill-trigger-http: a route with a non-empty + * list of its own restricts, `*` in it is the opt-out, and anything else — an + * empty list included, since that is not a configuration — falls back to the + * instance default. + */ +export function isOriginRestricted( + allowed_origins: string[] | undefined, + instanceDefaultOrigins: string[] +): boolean { + if (allowed_origins !== undefined && allowed_origins.length > 0) + return !allowed_origins.includes('*') + return instanceDefaultOrigins.length > 0 && !instanceDefaultOrigins.includes('*') +} + export const SECRET_KEY_PATH = 'secret_key_path' export const HUB_SCRIPT_ID = 19670 export const SIGNATURE_TEMPLATE_SCRIPT_HUB_PATH: string = `hub/${HUB_SCRIPT_ID}` @@ -56,6 +196,7 @@ export async function saveHttpRouteFromCfg( authentication_resource_path: routeCfg.authentication_resource_path, wrap_body: routeCfg.wrap_body, raw_string: routeCfg.raw_string, + allowed_origins: routeCfg.allowed_origins, description: routeCfg.description, summary: routeCfg.summary, error_handler_path: routeCfg.error_handler_path, diff --git a/system_prompts/auto-generated/schemas/http_trigger.schema.yaml b/system_prompts/auto-generated/schemas/http_trigger.schema.yaml index 14628d4001..f484366783 100644 --- a/system_prompts/auto-generated/schemas/http_trigger.schema.yaml +++ b/system_prompts/auto-generated/schemas/http_trigger.schema.yaml @@ -95,6 +95,21 @@ properties: type: boolean description: If true, passes the request body as a raw string instead of parsing as JSON + allowed_origins: + type: array + items: + type: string + description: 'Origins allowed to call this route cross-origin, matched against + the request''s Origin header (ignoring case) and echoed back on a match. When + set, the list governs both the preflight and the response, overriding any Access-Control-Allow-Origin + the runnable returns via wm_headers. Use [''*''] to opt out of any restriction, + including the http_route_default_allowed_origins instance setting. An empty + list is not a configuration and resolves exactly as null does. When null, the + instance setting applies, or Access-Control-Allow-Origin: * if it is unset. + Ignored on a static website, which has no authentication of its own and so hands + out public files: restricting which browsers may read them protects nothing + while breaking cross-origin webfonts and fetches. A single-file static asset + is not exempt, since it can carry an authentication_method.' error_handler_path: type: string description: Path to a script to run when the triggered job fails. A bare path,