From e663f76534ecf2fd1428c3435db4a458cabf9a97 Mon Sep 17 00:00:00 2001 From: Alexander Petric Date: Mon, 12 Oct 2026 22:26:04 +0000 Subject: [PATCH] fix: cap legacy secrets at 15000 chars with a check constraint so every writer is covered Co-Authored-By: Claude Opus 5.5 (1M context) --- ...261012213357_widen_variable_value.down.sql | 1 + ...20261012213357_widen_variable_value.up.sql | 5 ++ backend/windmill-common/src/utils.rs | 3 + backend/windmill-store/src/variables.rs | 83 +++++++------------ 4 files changed, 38 insertions(+), 54 deletions(-) diff --git a/backend/migrations/20261012213357_widen_variable_value.down.sql b/backend/migrations/20261012213357_widen_variable_value.down.sql index b4140ed521..48b61c3f65 100644 --- a/backend/migrations/20261012213357_widen_variable_value.down.sql +++ b/backend/migrations/20261012213357_widen_variable_value.down.sql @@ -8,4 +8,5 @@ BEGIN END IF; END $$; +ALTER TABLE variable DROP CONSTRAINT IF EXISTS variable_value_length; ALTER TABLE variable ALTER COLUMN value TYPE VARCHAR(15000); diff --git a/backend/migrations/20261012213357_widen_variable_value.up.sql b/backend/migrations/20261012213357_widen_variable_value.up.sql index 6ef69448d6..06dc8a191a 100644 --- a/backend/migrations/20261012213357_widen_variable_value.up.sql +++ b/backend/migrations/20261012213357_widen_variable_value.up.sql @@ -1,3 +1,8 @@ -- AES-256-GCM ciphertext is 32 bytes (magic, nonce, tag) longer than legacy CBC before base64, -- so the longest CBC value that fits 15000 chars is exactly 15040 chars once re-encrypted. ALTER TABLE variable ALTER COLUMN value TYPE VARCHAR(15040); + +-- Only GCM secrets (base64 of the `wmA2` magic starts `d21BM`) may use the extra room: a longer +-- CBC value could never be re-encrypted to fit, and would fail every rotation of its workspace key. +ALTER TABLE variable ADD CONSTRAINT variable_value_length + CHECK (length(value) <= 15000 OR (is_secret AND value LIKE 'd21BM%')); diff --git a/backend/windmill-common/src/utils.rs b/backend/windmill-common/src/utils.rs index af6bdfe2b3..116a15303e 100644 --- a/backend/windmill-common/src/utils.rs +++ b/backend/windmill-common/src/utils.rs @@ -281,6 +281,9 @@ pub fn sanitize_db_error(e: sqlx::Error) -> Error { return Error::from(e); }; match db_err.code().as_deref() { + Some("23514") if db_err.constraint() == Some("variable_value_length") => { + Error::BadRequest("A field exceeds its maximum length".to_string()) + } // check_violation — `proper_id` / `proper_name` and friends Some("23514") => Error::BadRequest( "Invalid path or name: it does not match the required format".to_string(), diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 1d07fd57fe..151b75bd85 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -57,7 +57,6 @@ use lazy_static::lazy_static; use serde::Deserialize; use sqlx::{Acquire, Postgres, Transaction}; use windmill_common::variables::encrypt; -use windmill_common::workspace_crypt::V2_MAGIC; use windmill_git_sync::{handle_deployment_metadata, DeployedObject}; lazy_static! { @@ -587,41 +586,6 @@ fn validate_already_encrypted_secret(path: &str, value: &str) -> Result<()> { Ok(()) } -async fn secret_value_to_store( - db: &DB, - w_id: &str, - path: &str, - value: String, - already_encrypted: bool, -) -> Result { - let stored = if !already_encrypted { - // A restored draft sends the `$encrypted:` marker as-is; decrypt it back - // (validating against the workspace key) before the secret backend re-stores it. - let plain = if value.starts_with(ENCRYPTED_DRAFT_PREFIX) { - decrypt_draft_secret_value(db, w_id, &value).await? - } else { - value - }; - // Use secret backend for encryption (supports both DB and Vault) - store_secret_value(db, w_id, path, &plain).await? - } else { - // Stored verbatim, so it must be ciphertext and not plaintext mislabeled as encrypted. - validate_already_encrypted_secret(path, &value)?; - value - }; - // Only v2 may use the column's 15040: a longer CBC value could never be re-encrypted to fit. - if stored.len() > 15000 - && !STANDARD - .decode(&stored) - .is_ok_and(|bytes| bytes.starts_with(&V2_MAGIC)) - { - return Err(Error::BadRequest( - "A field exceeds its maximum length".to_string(), - )); - } - Ok(stored) -} - async fn create_variable( authed: ApiAuthed, Extension(db): Extension, @@ -662,16 +626,22 @@ async fn create_variable( let authed = maybe_refresh_folders(&variable.path, &w_id, authed, &db).await; check_path_conflict(&db, &w_id, &variable.path).await?; - let value = if variable.is_secret { - secret_value_to_store( - &db, - &w_id, - &variable.path, - variable.value, - already_encrypted.unwrap_or(false), - ) - .await? + let value = if variable.is_secret && !already_encrypted.unwrap_or(false) { + // A restored draft sends the `$encrypted:` marker as-is; decrypt it back + // (validating against the workspace key) before the secret backend re-stores it. + let plain = if variable.value.starts_with(ENCRYPTED_DRAFT_PREFIX) { + decrypt_draft_secret_value(&db, &w_id, &variable.value).await? + } else { + variable.value.clone() + }; + // Use secret backend for encryption (supports both DB and Vault) + store_secret_value(&db, &w_id, &variable.path, &plain).await? } else { + if variable.is_secret { + // already_encrypted == true: value is stored verbatim, so it must be + // ciphertext and not plaintext mislabeled as encrypted. + validate_already_encrypted_secret(&variable.path, &variable.value)?; + } variable.value }; @@ -1160,17 +1130,22 @@ async fn update_variable( .unwrap_or(false) }; - let value = if is_secret { + let value = if is_secret && !already_encrypted.unwrap_or(false) { + // Decrypt a restored draft's `$encrypted:` marker before re-storing; see create_variable. + let plain = if nvalue.starts_with(ENCRYPTED_DRAFT_PREFIX) { + decrypt_draft_secret_value(&db, &w_id, &nvalue).await? + } else { + nvalue + }; + // Use secret backend for encryption (supports both DB and Vault) // Store at target_path (new path if renaming, otherwise current path) - secret_value_to_store( - &db, - &w_id, - target_path, - nvalue, - already_encrypted.unwrap_or(false), - ) - .await? + store_secret_value(&db, &w_id, target_path, &plain).await? } else { + if is_secret { + // already_encrypted == true: value is stored verbatim, so it must + // be ciphertext and not plaintext mislabeled as encrypted. + validate_already_encrypted_secret(target_path, &nvalue)?; + } nvalue }; sqlb.set_str("value", &value);