fix: compare fork secret variables by decrypted value (#11952)

* fix: compare fork secret variables by decrypted value

* test: restore MIN_VERSION even when the secret compare test fails

* fix: re-evaluate cached variable diffs once after the secret compare fix

* refactor: decrypt already-loaded secret rows when comparing variables

* docs: state the authorization contract of decrypt_secret_value

* fix: compare external-backend secrets by their resolved values

---------

Co-authored-by: Ruben Fiszel <ruben@windmill.dev>

backend/.sqlx/query-e1d7ca4dfbc5eeb6eb60e48a13029ac1a2714b78cc88969604dbd6c44984831f.json

@@ -0,0 +1,12 @@
+{
+  "db_name": "PostgreSQL",
+  "query": "DELETE FROM workspace_diff WHERE fork_workspace_id = 'wm-fork-secrets'",
+  "describe": {
+    "columns": [],
+    "parameters": {
+      "Left": []
+    },
+    "nullable": []
+  },
+  "hash": "e1d7ca4dfbc5eeb6eb60e48a13029ac1a2714b78cc88969604dbd6c44984831f"
+}

backend/.sqlx/query-e57039a483e96afa69f96e36be7666e83c6ccdea5087eb194d1de17086c58952.json

@@ -0,0 +1,12 @@
+{
+  "db_name": "PostgreSQL",
+  "query": "INSERT INTO workspace_diff\n         (source_workspace_id, fork_workspace_id, path, kind, ahead, behind, has_changes)\n         VALUES\n         ('test-workspace', 'wm-fork-secrets', 'u/test-user/same', 'variable', 1, 1, NULL),\n         ('test-workspace', 'wm-fork-secrets', 'u/test-user/different', 'variable', 1, 1, NULL)",
+  "describe": {
+    "columns": [],
+    "parameters": {
+      "Left": []
+    },
+    "nullable": []
+  },
+  "hash": "e57039a483e96afa69f96e36be7666e83c6ccdea5087eb194d1de17086c58952"
+}

backend/.sqlx/query-ecd08125821a21842bad7d328ea83cbaf7676cbed2f0c3a433699578da88e8ae.json

@@ -0,0 +1,20 @@
+{
+  "db_name": "PostgreSQL",
+  "query": "SELECT COUNT(DISTINCT value) FROM variable WHERE path = 'u/test-user/same'",
+  "describe": {
+    "columns": [
+      {
+        "ordinal": 0,
+        "name": "count",
+        "type_info": "Int8"
+      }
+    ],
+    "parameters": {
+      "Left": []
+    },
+    "nullable": [
+      null
+    ]
+  },
+  "hash": "ecd08125821a21842bad7d328ea83cbaf7676cbed2f0c3a433699578da88e8ae"
+}

backend/migrations/20261022163514_reset_cached_variable_workspace_diffs.down.sql

@@ -0,0 +1 @@
+-- Add down migration script here

backend/migrations/20261022163514_reset_cached_variable_workspace_diffs.up.sql

@@ -0,0 +1,3 @@
+-- Secrets compared by ciphertext were cached as changed even when both sides held the same value,
+-- and a cached verdict is trusted until the next write. Re-evaluate every variable row once.
+UPDATE workspace_diff SET has_changes = NULL WHERE kind = 'variable';

backend/windmill-api-integration-tests/tests/workspace_comparison.rs

@@ -2635,3 +2635,114 @@ async fn test_compare_null_description_equals_empty(db: Pool<Postgres>) -> anyho
 
     Ok(())
 }
+
+/// Runs its closure when dropped, so process-wide state is put back even when an assertion
+/// fails.
+struct OnDrop<F: FnMut()>(F);
+impl<F: FnMut()> Drop for OnDrop<F> {
+    fn drop(&mut self) {
+        (self.0)()
+    }
+}
+
+/// Secrets are encrypted with a fresh nonce on every write, so a secret holding the same value on
+/// both sides must compare by plaintext, not ciphertext.
+#[sqlx::test(migrations = "../migrations", fixtures("base"))]
+async fn test_compare_secret_variables_by_value(db: Pool<Postgres>) -> anyhow::Result<()> {
+    initialize_tracing().await;
+
+    let server = ApiServer::start(db.clone()).await?;
+    let port = server.addr.port();
+    let client = windmill_api_client::create_client(
+        &format!("http://localhost:{port}"),
+        "SECRET_TOKEN".to_string(),
+    );
+    let base_url = format!("http://localhost:{port}/api");
+
+    let resp = client
+        .client()
+        .post(&format!(
+            "{base_url}/w/test-workspace/workspaces/create_fork"
+        ))
+        .json(&json!({ "id": "wm-fork-secrets", "name": "Secrets Fork" }))
+        .send()
+        .await?;
+    assert!(
+        resp.status().is_success(),
+        "fork creation failed: {}",
+        resp.status()
+    );
+
+    // Secrets are written in CBC, which is deterministic, until every worker reads AES-GCM.
+    use windmill_common::min_version::{MIN_VERSION, MIN_VERSION_SUPPORTS_AEAD_SECRETS};
+    let before = MIN_VERSION.load_full();
+    let _restore = OnDrop(move || MIN_VERSION.store(before.clone()));
+    MIN_VERSION.store(std::sync::Arc::new(
+        MIN_VERSION_SUPPORTS_AEAD_SECRETS.version().clone(),
+    ));
+
+    for (ws, path, value) in [
+        ("test-workspace", "u/test-user/same", "s3cret"),
+        ("wm-fork-secrets", "u/test-user/same", "s3cret"),
+        ("test-workspace", "u/test-user/different", "one"),
+        ("wm-fork-secrets", "u/test-user/different", "two"),
+    ] {
+        let resp = client
+            .client()
+            .post(&format!("{base_url}/w/{ws}/variables/create"))
+            .json(&json!({ "path": path, "value": value, "is_secret": true, "description": "" }))
+            .send()
+            .await?;
+        assert_eq!(resp.status(), 201, "{}", resp.text().await?);
+    }
+    let stored = sqlx::query_scalar!(
+        "SELECT COUNT(DISTINCT value) FROM variable WHERE path = 'u/test-user/same'"
+    )
+    .fetch_one(&db)
+    .await?;
+    assert_eq!(
+        stored,
+        Some(2),
+        "the same secret must be stored as two different ciphertexts"
+    );
+
+    sqlx::query!("DELETE FROM workspace_diff WHERE fork_workspace_id = 'wm-fork-secrets'")
+        .execute(&db)
+        .await?;
+    sqlx::query!(
+        "INSERT INTO workspace_diff
+         (source_workspace_id, fork_workspace_id, path, kind, ahead, behind, has_changes)
+         VALUES
+         ('test-workspace', 'wm-fork-secrets', 'u/test-user/same', 'variable', 1, 1, NULL),
+         ('test-workspace', 'wm-fork-secrets', 'u/test-user/different', 'variable', 1, 1, NULL)"
+    )
+    .execute(&db)
+    .await?;
+
+    let comparison: serde_json::Value = client
+        .client()
+        .get(&format!(
+            "{base_url}/w/test-workspace/workspaces/compare/wm-fork-secrets"
+        ))
+        .send()
+        .await?
+        .json()
+        .await?;
+    let diffs = comparison["diffs"].as_array().unwrap();
+    let listed = |path: &str| {
+        diffs
+            .iter()
+            .any(|d| d["path"] == path && d["kind"] == "variable")
+    };
+
+    assert!(
+        !listed("u/test-user/same"),
+        "equal secrets must compare as unchanged: {comparison}"
+    );
+    assert!(
+        listed("u/test-user/different"),
+        "different secrets must compare as changed: {comparison}"
+    );
+
+    Ok(())
+}

backend/windmill-api-workspaces/src/workspaces.rs

@@ -39,7 +39,7 @@ use windmill_common::global_settings::HTTP_ROUTE_WORKSPACED_ROUTE;
 use windmill_common::query_builders::{render_db_quoted_identifier, DbType};
 use windmill_common::users::username_to_permissioned_as;
 use windmill_common::variables::{
-    crypt_from_key_with_suffix, decrypt, encrypt, WORKSPACE_CRYPT_CACHE,
+    crypt_from_key_with_suffix, decrypt, decrypt_secret_value, encrypt, WORKSPACE_CRYPT_CACHE,
 };
 use windmill_common::worker::{to_raw_value, CLOUD_HOSTED};
 use windmill_common::workspaces::GitRepositorySettings;
@@ -76,7 +76,9 @@ use hyper::StatusCode;
 use serde::{Deserialize, Serialize};
 use sqlx::{FromRow, Postgres, Row, Transaction};
 use windmill_common::oauth2::InstanceEvent;
-use windmill_common::secret_backend::{get_secret_backend, is_vault_backend_configured};
+use windmill_common::secret_backend::{
+    get_secret_backend, is_external_stored_value, is_vault_backend_configured,
+};
 use windmill_common::utils::not_found_if_none;
 
 lazy_static::lazy_static! {
@@ -14524,11 +14526,22 @@ async fn compare_two_variables(
 
     // Check metadata differences
     if let (Some(source), Some(target)) = (&source_variable, &target_variable) {
-        if source.is_secret != target.is_secret
-            || source.value != target.value
-            || source.description != target.description
-        {
+        if source.is_secret != target.is_secret || source.description != target.description {
             has_changes = true;
+        } else if source.value != target.value
+            || (source.is_secret && is_external_stored_value(&source.value))
+        {
+            // A secret's ciphertext is randomized on every write, so equal secrets are compared
+            // by plaintext, each decrypted with its own workspace's key. An external backend
+            // reference is the same text on both sides but names a per-workspace entry.
+            has_changes = !source.is_secret
+                || !matches!(
+                    tokio::try_join!(
+                        decrypt_secret_value(db, source_workspace_id, path, source.value.clone()),
+                        decrypt_secret_value(db, fork_workspace_id, path, target.value.clone()),
+                    ),
+                    Ok((a, b)) if a == b
+                );
         }
     } else if source_variable.is_some() || target_variable.is_some() {
         // The variable exists in one of source or target, but not the other, this is considered as a change

backend/windmill-common/src/variables.rs

@@ -465,28 +465,32 @@ pub async fn get_secret_value_as_admin(
         )));
     };
 
-    let r = if variable.is_secret {
-        let value = variable.value;
-        if !value.is_empty() {
-            if is_external_stored_value(&value) {
-                get_secret_value(db, w_id, &variable.path, &value).await?
-            } else {
-                let mc = build_crypt(db, w_id).await?;
-                decrypt(&mc, value).map_err(|e| {
-                    crate::error::Error::internal_err(format!(
-                        "Error decrypting variable {}: {}",
-                        variable.path, e
-                    ))
-                })?
-            }
-        } else {
-            "".to_string()
-        }
+    if variable.is_secret {
+        decrypt_secret_value(db, w_id, &variable.path, variable.value).await
     } else {
-        variable.value
-    };
+        Ok(variable.value)
+    }
+}
 
-    Ok(r)
+/// Plaintext of a secret variable's stored `value`, read from the external secret backend when it
+/// holds a reference to one. Bypasses variable permissions: callers MUST have authorized the
+/// reader before returning the plaintext to them.
+pub async fn decrypt_secret_value(
+    db: &DB,
+    w_id: &str,
+    path: &str,
+    value: String,
+) -> crate::error::Result<String> {
+    if value.is_empty() {
+        Ok(value)
+    } else if is_external_stored_value(&value) {
+        get_secret_value(db, w_id, path, &value).await
+    } else {
+        let mc = build_crypt(db, w_id).await?;
+        decrypt(&mc, value).map_err(|e| {
+            crate::error::Error::internal_err(format!("Error decrypting variable {}: {}", path, e))
+        })
+    }
 }
 
 /// Marks a workspace setting's secret written into the settings document encrypted with the