From c39ee07c0bcd2249dd19ffa5cd988125eefc6c9f Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 19 Jun 2026 16:54:04 +0200 Subject: [PATCH] fix: validate websocket trigger urls and gate trigger test route (#9682) * fix: validate websocket trigger urls and gate trigger test route Co-Authored-By: Claude Opus 4.8 (1M context) * docs: clarify validate_websocket_url_for_ssrf call sites Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- backend/THREAT_MODEL.md | 2 +- .../windmill-trigger-websocket/src/handler.rs | 16 +++- backend/windmill-trigger-websocket/src/lib.rs | 89 +++++++++++++++++++ .../src/listener.rs | 5 +- backend/windmill-trigger/src/handler.rs | 4 + 5 files changed, 111 insertions(+), 5 deletions(-) diff --git a/backend/THREAT_MODEL.md b/backend/THREAT_MODEL.md index 8cc71ec71c..dc98a5a625 100644 --- a/backend/THREAT_MODEL.md +++ b/backend/THREAT_MODEL.md @@ -97,7 +97,7 @@ published advisory history (73 GHSA advisories, several rated 9.9 critical). | id | threat | actor | surface | asset | impact | likelihood | status | controls | evidence | |---|---|---|---|---|---|---|---|---|---| | T1 | SQL injection in app/internal query builders and trigger clauses compromises the metadata DB and connected databases | remote_auth | EP8 | Database, downstream connected systems | critical | almost_certain | partially_mitigated | sqlx parameterized queries elsewhere; query-builder safety reviews | GHSA-225c-j3xq-g6x6, GHSA-78p7-jc72-gv66, GHSA-hvc7-f67h-jx3g, GHSA-wrrg-f89m-f84q, GHSA-79vf-3qwm-2w64, GHSA-55p6-fxj4-v983, GHSA-5g4v-49rj-r52r, GHSA-x6cq-7xr8-53x3, 2cf4bb180b | -| T2 | Server-side request forgery via proxies/executors reaches cloud metadata, internal network, and downstream credentials | remote_auth | EP6, EP7 | Cloud metadata, internal network, downstream connected systems, resource creds | critical | almost_certain | partially_mitigated | SSRF URL validation + redirect-following disabled added piecemeal; MCP private URL access requires the instance-wide `ALLOW_PRIVATE_MCP_SERVER_URLS` opt-in; outbound network isolation (`clone_newnet`) is opt-in and off by default | GHSA-3ggp-h37f-5qfw, GHSA-98qq-g8rh-xhff, GHSA-hfw8-27mx-63jm, GHSA-3r59-qvvc-774j, GHSA-4pj9-w5jc-g8w7, GHSA-8hh3-jf25-78j5, GHSA-3pjm-4w7f-3r2w, GHSA-f44c-x9hq-h68r, GHSA-j4h4-f8fj-3m3c, 4b06881918, 96a8eb63d4, dbd3942ef3 | +| T2 | Server-side request forgery via proxies/executors reaches cloud metadata, internal network, and downstream credentials | remote_auth | EP6, EP7 | Cloud metadata, internal network, downstream connected systems, resource creds | critical | almost_certain | partially_mitigated | SSRF URL validation + redirect-following disabled added piecemeal; MCP private URL access requires the instance-wide `ALLOW_PRIVATE_MCP_SERVER_URLS` opt-in; WebSocket trigger URLs (stored, test, and runnable-resolved) are SSRF-validated at connect time behind the `ALLOW_PRIVATE_WEBSOCKET_URLS` opt-in, and the trigger test route now requires `:write` scope; outbound network isolation (`clone_newnet`) is opt-in and off by default | GHSA-3ggp-h37f-5qfw, GHSA-98qq-g8rh-xhff, GHSA-hfw8-27mx-63jm, GHSA-3r59-qvvc-774j, GHSA-4pj9-w5jc-g8w7, GHSA-8hh3-jf25-78j5, GHSA-3pjm-4w7f-3r2w, GHSA-f44c-x9hq-h68r, GHSA-j4h4-f8fj-3m3c, 4b06881918, 96a8eb63d4, dbd3942ef3 | | T3 | Broken authorization / IDOR lets a scoped token or low-privilege member read scripts, job data, and secrets across folders and workspaces | remote_auth | EP5, EP2, EP1 | Scripts, job data, secrets, isolation | critical | almost_certain | partially_mitigated | RLS, token scopes, folder ACLs, view-token HMAC (added incrementally); on managed, sensitive tenants can opt into dedicated DB/worker/namespace, but the shared tier IS the software boundary | GHSA-qfg7-x243-5hg4, GHSA-8x8x-88qc-qp4r, GHSA-2ppx-66jv-wpw5, GHSA-x3x7-g97v-mp59, GHSA-j276-g4h8-g6h5, GHSA-8mv7-hmrg-96xv, GHSA-x2wf-f962-7frq, GHSA-qc7c-gcw6-h4xp, GHSA-vxc5-w28p-m9xw, GHSA-2g34-wfvr-5qqj, GHSA-w7p6-wpxm-pp66, 7edf3f0212, 89a7a37776, ab11c7747a, 664edcdfb7 | | T4 | Remote code execution by injecting attacker-controlled identifiers into generated worker wrappers | remote_auth | EP10 | Worker host, isolation, downstream | critical | likely | partially_mitigated | entrypoint/env-var-name validation added | GHSA-wxjq-w5pj-jqhx, GHSA-5f5q-2vg2-r2x4, GHSA-8q8j-mm3g-5c2q (CVE-2026-33881), bf93657fee, bd05bcadde, 22ec4da5f0 | | T5 | Worker compromise & cross-tenant access via weak-by-default isolation (nsjail off by default → user code runs with only PID-ns `unshare`); sandbox escape where nsjail/dind/podman is enabled | remote_auth | EP9, EP15 | Worker host, isolation, downstream | critical | likely | unmitigated | nsjail off by default everywhere (`DISABLE_NSJAIL=true`); shipped compose gives PID-ns `unshare` only (`FAVOR_UNSHARE_PID=true`), bare installs get no isolation. Where nsjail enabled: read-only remounts, jail-tmp refusal, podman socket gating | GHSA-6qr8-xhg4-453q, GHSA-3vpp-vf62-wqp6, f8467f38c8, df5aec0f5d, f1b6746e0e | diff --git a/backend/windmill-trigger-websocket/src/handler.rs b/backend/windmill-trigger-websocket/src/handler.rs index dd6b9f4363..ed8e95b164 100644 --- a/backend/windmill-trigger-websocket/src/handler.rs +++ b/backend/windmill-trigger-websocket/src/handler.rs @@ -15,8 +15,8 @@ use windmill_git_sync::DeployedObject; use windmill_trigger::{Trigger, TriggerCrud, TriggerData}; use super::{ - get_url_from_runnable_value, proxy::connect_async_with_proxy, TestWebsocketConfig, - WebsocketConfig, WebsocketConfigRequest, WebsocketTrigger, + get_url_from_runnable_value, proxy::connect_async_with_proxy, validate_websocket_url_for_ssrf, + TestWebsocketConfig, WebsocketConfig, WebsocketConfigRequest, WebsocketTrigger, }; #[async_trait] @@ -28,7 +28,8 @@ impl TriggerCrud for WebsocketTrigger { const TABLE_NAME: &'static str = "websocket_trigger"; const TRIGGER_TYPE: &'static str = "websocket"; - const DRAFT_KIND: windmill_common::user_drafts::UserDraftItemKind = windmill_common::user_drafts::UserDraftItemKind::TriggerWebsocket; + const DRAFT_KIND: windmill_common::user_drafts::UserDraftItemKind = + windmill_common::user_drafts::UserDraftItemKind::TriggerWebsocket; const SUPPORTS_SERVER_STATE: bool = true; const SUPPORTS_TEST_CONNECTION: bool = true; const ROUTE_PREFIX: &'static str = "/websocket_triggers"; @@ -61,6 +62,13 @@ impl TriggerCrud for WebsocketTrigger { )); } + // Reject SSRF targets at save time for static URLs. A `$flow:`/`$script:` + // URL is only known at runtime, so it is validated at connect time + // instead (in the listener and test handler). + if !config.url.starts_with('$') { + validate_websocket_url_for_ssrf(&config.url).await?; + } + if let Some(args) = &config.url_runnable_args { if !args.is_object() { return Err(Error::BadRequest( @@ -277,6 +285,8 @@ impl TriggerCrud for WebsocketTrigger { Cow::Borrowed(&url) }; + validate_websocket_url_for_ssrf(&connect_url).await?; + connect_async_with_proxy(&*connect_url) .await .map_err(|err| { diff --git a/backend/windmill-trigger-websocket/src/lib.rs b/backend/windmill-trigger-websocket/src/lib.rs index e61c067479..4111606b2b 100644 --- a/backend/windmill-trigger-websocket/src/lib.rs +++ b/backend/windmill-trigger-websocket/src/lib.rs @@ -104,6 +104,58 @@ pub fn value_to_args_hashmap( Ok(args) } +/// Env var that opts a deployment out of SSRF validation for WebSocket trigger +/// URLs, permitting connections to private/internal addresses. Off by default. +pub const ALLOW_PRIVATE_WEBSOCKET_URLS_ENV: &str = "ALLOW_PRIVATE_WEBSOCKET_URLS"; + +/// Reject WebSocket URLs that target (or resolve to) a private/internal address, +/// blocking SSRF probes of the host's internal network and cloud metadata +/// endpoints. +/// +/// `ws://`/`wss://` are mapped to `http`/`https` so the shared +/// `validate_url_for_ssrf` host + DNS-resolution checks apply. The +/// security-critical call sites are the outbound connects (the test handler and +/// every listener (re)connect): validating the *resolved* URL there means a +/// `$flow:`/`$script:` URL is checked on its returned value and re-checked on +/// each reconnect (DNS rebinding). `validate_config` also calls this at save +/// time to reject static URLs early. +pub async fn validate_websocket_url_for_ssrf(url: &str) -> Result<()> { + if std::env::var(ALLOW_PRIVATE_WEBSOCKET_URLS_ENV) + .ok() + .is_some_and(|v| v == "true" || v == "1") + { + return Ok(()); + } + + // `ws`/`wss` aren't recognised by `validate_url_for_ssrf`'s scheme check, so + // map them to the http(s) equivalent the same connection would tunnel over. + // The prefixes are ASCII, so byte-slicing at their length stays on a char + // boundary. + let lower = url.to_ascii_lowercase(); + let http_url = if lower.starts_with("wss://") { + format!("https://{}", &url["wss://".len()..]) + } else if lower.starts_with("ws://") { + format!("http://{}", &url["ws://".len()..]) + } else { + url.to_string() + }; + + windmill_common::ssrf::validate_url_for_ssrf(&http_url) + .await + .map_err(|e| match e { + // The env-var hint is only actionable for a well-formed URL blocked + // for targeting a private address; a malformed URL or bad scheme + // surfaces its real error so the user fixes the URL (see #9171). + e @ windmill_common::ssrf::SsrfValidationError::Private { .. } => { + Error::BadRequest(format!( + "{e}. If you need to connect to private/internal WebSocket endpoints, \ + set the {ALLOW_PRIVATE_WEBSOCKET_URLS_ENV}=true environment variable" + )) + } + e => Error::from(e), + }) +} + pub async fn get_url_from_runnable_value( path: &str, is_flow: bool, @@ -144,3 +196,40 @@ pub async fn get_url_from_runnable_value( )) }) } + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn ssrf_blocks_private_and_metadata_ws_urls() { + // ws:// → http:// mapping must still reach the IP-literal block. + let err = validate_websocket_url_for_ssrf("ws://127.0.0.1:6379/") + .await + .unwrap_err(); + assert!(matches!(err, Error::BadRequest(_))); + // Private errors carry the opt-out hint so operators can allow internal + // targets deliberately. + assert!(err.to_string().contains(ALLOW_PRIVATE_WEBSOCKET_URLS_ENV)); + + // wss:// → https:// mapping blocks the cloud metadata endpoint. + assert!( + validate_websocket_url_for_ssrf("wss://169.254.169.254/latest/meta-data") + .await + .is_err() + ); + assert!(validate_websocket_url_for_ssrf("ws://10.0.0.5:6379/") + .await + .is_err()); + } + + #[tokio::test] + async fn ssrf_rejects_non_ws_scheme_without_private_hint() { + // A non-ws scheme isn't mapped and fails the scheme check; it must not + // get the "set ALLOW_PRIVATE_WEBSOCKET_URLS" hint (issue #9171). + let err = validate_websocket_url_for_ssrf("file:///etc/passwd") + .await + .unwrap_err(); + assert!(!err.to_string().contains(ALLOW_PRIVATE_WEBSOCKET_URLS_ENV)); + } +} diff --git a/backend/windmill-trigger-websocket/src/listener.rs b/backend/windmill-trigger-websocket/src/listener.rs index 54dd4ddf60..31e3f83995 100644 --- a/backend/windmill-trigger-websocket/src/listener.rs +++ b/backend/windmill-trigger-websocket/src/listener.rs @@ -1,5 +1,6 @@ use super::{ - get_url_from_runnable_value, proxy::connect_async_with_proxy, WebsocketConfig, WebsocketTrigger, + get_url_from_runnable_value, proxy::connect_async_with_proxy, validate_websocket_url_for_ssrf, + WebsocketConfig, WebsocketTrigger, }; use anyhow::Context; use async_trait::async_trait; @@ -173,6 +174,8 @@ impl Listener for WebsocketTrigger { Cow::Borrowed(&url) }; + validate_websocket_url_for_ssrf(&connect_url).await?; + let connection = connect_async_with_proxy(&*connect_url) .await .map(|conn| Some(conn)) diff --git a/backend/windmill-trigger/src/handler.rs b/backend/windmill-trigger/src/handler.rs index 4f3d381569..65072bc9ab 100644 --- a/backend/windmill-trigger/src/handler.rs +++ b/backend/windmill-trigger/src/handler.rs @@ -1057,6 +1057,10 @@ async fn test_connection( Path(workspace_id): Path, Json(config): Json, ) -> Result<()> { + // Test connection opens an outbound connection to a caller-supplied target, + // so gate it behind write access like the other mutating trigger routes. + check_scopes(&authed, || format!("{}:write", T::scope_domain_name()))?; + let connect_f = async move { handler .test_connection(&db, &authed, &user_db, &workspace_id, config)