From 773e80a20a06293036fd5ff04b841c6b0a299a46 Mon Sep 17 00:00:00 2001
From: Ruben Fiszel <ruben@windmill.dev>
Date: Fri, 4 Sep 2026 17:53:19 +0200
Subject: [PATCH] fix: require HTTPS for a guest JWKS URL; serialize the
 env-var tests

validate_guest_jwks_url accepted http://, but the JWKS supplies the keys that
authenticate guest JWTs, so an on-path attacker replacing a plaintext response
could forge accepted tokens. Require https by default; allow http only under the
existing ALLOW_PRIVATE_GUEST_JWKS_URLS opt-in (dev/loopback). Also lock the two
JWKS tests that mutate that process-wide env var behind a shared mutex, as the
ssrf tests do, so a concurrent run cannot clear it out from under one another.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3
---
 backend/windmill-common/src/guest_jwt.rs | 18 ++++++++++++++++++
 backend/windmill-common/src/ssrf.rs      | 15 ++++++++++-----
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs
index 298524c8d1..17c75bcf2c 100644
--- a/backend/windmill-common/src/guest_jwt.rs
+++ b/backend/windmill-common/src/guest_jwt.rs
@@ -538,6 +538,10 @@ pub async fn verify_for_workspace(db: &DB, w_id: &str, token: &str) -> Result<Gu
 mod tests {
     use super::*;
 
+    /// Serializes the tests that mutate the process-wide `ALLOW_PRIVATE_GUEST_JWKS_URLS`, so a
+    /// concurrent run cannot clear it out from under another (mirrors `ssrf.rs`'s test lock).
+    static TEST_ENV_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(());
+
     fn jwk(v: serde_json::Value) -> Jwk {
         serde_json::from_value(v).unwrap()
     }
@@ -780,6 +784,7 @@ y9rTR828ADcaZ63Ej1oL4GcqmGhODxCLy1YKKcy0FHzChqPMV6g=\n\
     async fn a_concurrent_cold_burst_makes_one_jwks_fetch() {
         use std::sync::atomic::{AtomicUsize, Ordering};
         use tokio::io::AsyncWriteExt;
+        let _env = TEST_ENV_LOCK.lock().await;
         // The stub listens on loopback, which SSRF validation refuses without this.
         unsafe { std::env::set_var("ALLOW_PRIVATE_GUEST_JWKS_URLS", "true") };
         let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
@@ -830,6 +835,7 @@ y9rTR828ADcaZ63Ej1oL4GcqmGhODxCLy1YKKcy0FHzChqPMV6g=\n\
 
     #[tokio::test]
     async fn stale_jwks_keys_stop_being_served_past_the_grace_window() {
+        let _env = TEST_ENV_LOCK.lock().await;
         unsafe { std::env::set_var("ALLOW_PRIVATE_GUEST_JWKS_URLS", "true") };
         // A dead loopback port, so every refresh fails (connection refused).
         let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
@@ -874,4 +880,16 @@ y9rTR828ADcaZ63Ej1oL4GcqmGhODxCLy1YKKcy0FHzChqPMV6g=\n\
         );
         unsafe { std::env::remove_var("ALLOW_PRIVATE_GUEST_JWKS_URLS") };
     }
+
+    #[tokio::test]
+    async fn a_plaintext_http_jwks_url_is_refused_by_default() {
+        let _env = TEST_ENV_LOCK.lock().await;
+        unsafe { std::env::remove_var("ALLOW_PRIVATE_GUEST_JWKS_URLS") };
+        // The JWKS supplies the keys that authenticate guest JWTs; without the operator opt-in,
+        // a plaintext URL (which an on-path attacker could replace) is refused for its scheme.
+        assert!(matches!(
+            crate::ssrf::validate_guest_jwks_url("http://issuer.example.com/jwks.json").await,
+            Err(crate::ssrf::SsrfValidationError::DisallowedScheme(_))
+        ));
+    }
 }
diff --git a/backend/windmill-common/src/ssrf.rs b/backend/windmill-common/src/ssrf.rs
index b31dcaa0df..460db53f90 100644
--- a/backend/windmill-common/src/ssrf.rs
+++ b/backend/windmill-common/src/ssrf.rs
@@ -222,17 +222,22 @@ pub async fn validate_guest_jwks_url(url: &str) -> Result<ValidatedTarget, SsrfV
     let parsed =
         url::Url::parse(url).map_err(|e| SsrfValidationError::InvalidUrl(e.to_string()))?;
 
+    let allow_private = std::env::var(ALLOW_PRIVATE_GUEST_JWKS_URLS_ENV)
+        .ok()
+        .is_some_and(|v| v == "true" || v == "1");
+
     match parsed.scheme() {
-        "http" | "https" => {}
+        "https" => {}
+        // Plaintext HTTP only under the explicit operator opt-in that also allows private
+        // hosts (dev/loopback): the JWKS supplies the keys that authenticate guest JWTs, so an
+        // on-path attacker who could replace an http response could forge accepted tokens.
+        "http" if allow_private => {}
         scheme => return Err(SsrfValidationError::DisallowedScheme(scheme.to_string())),
     }
 
     let host = parsed.host_str().ok_or(SsrfValidationError::MissingHost)?;
 
-    if std::env::var(ALLOW_PRIVATE_GUEST_JWKS_URLS_ENV)
-        .ok()
-        .is_some_and(|v| v == "true" || v == "1")
-    {
+    if allow_private {
         return Ok(ValidatedTarget::unpinned(host));
     }