From 987c321ebcd305df79fa945e941abc2b2e1d3e95 Mon Sep 17 00:00:00 2001 From: Alexander Petric Date: Thu, 8 Oct 2026 16:32:55 -0400 Subject: [PATCH] fix: uniform decrypt error and clear stored credentials on logout (#11632) * fix: uniform decrypt error and clear stored credentials on logout Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Kv72vdCDggCZEjJSmNCwnX * fix: also clear mcp-oauth-callback on logout, cover the new keys in the logout test Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Kv72vdCDggCZEjJSmNCwnX --------- Co-authored-by: Claude Opus 5.5 (1M context) --- backend/windmill-common/src/variables.rs | 42 +++++++++++++++++++----- frontend/src/lib/logout.test.ts | 16 +++++++++ frontend/src/lib/logout.ts | 16 +++++---- 3 files changed, 60 insertions(+), 14 deletions(-) diff --git a/backend/windmill-common/src/variables.rs b/backend/windmill-common/src/variables.rs index 1feb4182ab..b3774cdda4 100644 --- a/backend/windmill-common/src/variables.rs +++ b/backend/windmill-common/src/variables.rs @@ -14,7 +14,7 @@ use crate::utils::WarnAfterExt; use crate::worker::Connection; use crate::{worker::WORKER_GROUP, BASE_URL, DB}; use chrono::{SecondsFormat, Utc}; -use magic_crypt::{MagicCrypt256, MagicCryptError, MagicCryptTrait}; +use magic_crypt::{MagicCrypt256, MagicCryptTrait}; use quick_cache::sync::Cache; use serde::{Deserialize, Serialize}; @@ -375,13 +375,15 @@ pub fn encrypt(mc: &MagicCrypt256, value: &str) -> String { mc.encrypt_str_to_base64(value) } +const DECRYPT_FAILED: &str = + "Could not decrypt value. The value may have been encrypted with a different key."; + +/// Every failure returns the same message so responses carry no signal about why decryption +/// failed (padding, base64, UTF-8); the cause is only logged. pub fn decrypt(mc: &MagicCrypt256, value: String) -> error::Result { - mc.decrypt_base64_to_string(value).map_err(|e| match e { - MagicCryptError::DecryptError(_) => error::Error::internal_err( - "Could not decrypt value. The value may have been encrypted with a different key." - .to_string(), - ), - _ => error::Error::internal_err(e.to_string()), + mc.decrypt_base64_to_string(value).map_err(|e| { + tracing::warn!("decryption failed: {e}"); + error::Error::internal_err(DECRYPT_FAILED.to_string()) }) } @@ -746,7 +748,31 @@ pub async fn get_variable_or_self_as( #[cfg(test)] mod tests { - use super::{can_bind_as_prologue_const, escape_js_single_quoted, is_valid_js_identifier}; + use super::{ + can_bind_as_prologue_const, decrypt, escape_js_single_quoted, is_valid_js_identifier, + DECRYPT_FAILED, + }; + use crate::error::Error; + use magic_crypt::MagicCryptTrait; + + #[test] + fn decrypt_failures_all_return_the_same_message() { + let mc = magic_crypt::new_magic_crypt!("workspace-key", 256); + let other = magic_crypt::new_magic_crypt!("another-key", 256); + let failures = [ + "not base64 at all!".to_string(), + mc.encrypt_bytes_to_base64(&[0xff, 0xfe, 0xfd]), + other.encrypt_str_to_base64("secret"), + ]; + for value in failures { + match decrypt(&mc, value.clone()) { + Err(Error::InternalErrLoc { message, .. }) => { + assert_eq!(message, DECRYPT_FAILED, "for {value:?}") + } + other => panic!("expected the uniform decrypt error for {value:?}, got {other:?}"), + } + } + } #[test] fn is_valid_js_identifier_gates_prologue_injection() { diff --git a/frontend/src/lib/logout.test.ts b/frontend/src/lib/logout.test.ts index 160bce9934..5bdc15bac3 100644 --- a/frontend/src/lib/logout.test.ts +++ b/frontend/src/lib/logout.test.ts @@ -12,9 +12,25 @@ import { clearUser } from './logout' describe('clearUser', () => { beforeEach(() => { sessionStorage.clear() + localStorage.clear() vi.clearAllMocks() }) + it('drops credential-bearing localStorage entries and keeps preferences', async () => { + const credentialKeys = ['oauth-callback', 'mcp-oauth-callback', 'test_dev_token'] + for (const key of credentialKeys) { + localStorage.setItem(key, 'a-token') + } + localStorage.setItem('dark-mode', 'true') + + await clearUser() + + for (const key of credentialKeys) { + expect(localStorage.getItem(key)).toBeNull() + } + expect(localStorage.getItem('dark-mode')).toBe('true') + }) + // Logging out while impersonating must not leave the impersonator's own token behind. it('drops the impersonation token backup', async () => { sessionStorage.setItem('pre_impersonation_token', 'a-token') diff --git a/frontend/src/lib/logout.ts b/frontend/src/lib/logout.ts index 58fce16fe3..b4782badd3 100644 --- a/frontend/src/lib/logout.ts +++ b/frontend/src/lib/logout.ts @@ -6,15 +6,19 @@ import { clearStores } from './storeUtils' // Note: logout and logoutWithRedirect have been moved to logoutKit.ts // as they depend on SvelteKit navigation -// The impersonation backup holds the impersonator's own auth token, so it must go on -// logout too, not only when impersonation is ended from the banner — otherwise a usable -// token stays readable for the lifetime of the tab. -function clearImpersonationFromStorage(): void { +// Every browser-storage entry that can hold a credential must go on logout. The impersonation +// backup is the impersonator's own auth token; `oauth-callback` and `mcp-oauth-callback` carry a +// connected account's token response when the OAuth popup had no opener to post it to; +// `test_dev_token` is the session token of the /test_dev pages. +function clearCredentialsFromStorage(): void { try { sessionStorage.removeItem('pre_impersonation_token') sessionStorage.removeItem('pre_impersonation_email') + localStorage.removeItem('oauth-callback') + localStorage.removeItem('mcp-oauth-callback') + localStorage.removeItem('test_dev_token') } catch (e) { - console.error('error interacting with session storage', e) + console.error('error interacting with browser storage', e) } } @@ -23,7 +27,7 @@ export async function clearUser() { noteSessionEmail(undefined) accountSetup.reset() clearStores() - clearImpersonationFromStorage() + clearCredentialsFromStorage() await UserService.logout() } catch (error) {} }