diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 7d47bbcdeb..834c636f0a 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -15189,12 +15189,14 @@ dependencies = [ "anyhow", "async-recursion", "axum 0.8.9", + "base64 0.22.1", "chrono", "futures", "hex", "http 1.4.2", "hyper 1.10.1", "lazy_static", + "magic-crypt", "quick_cache", "reqwest 0.13.1", "serde", diff --git a/backend/windmill-store/Cargo.toml b/backend/windmill-store/Cargo.toml index b3aca5e669..fdd82a7e5d 100644 --- a/backend/windmill-store/Cargo.toml +++ b/backend/windmill-store/Cargo.toml @@ -53,3 +53,7 @@ futures.workspace = true chrono.workspace = true reqwest.workspace = true anyhow.workspace = true +base64.workspace = true + +[dev-dependencies] +magic-crypt.workspace = true diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index d5a906392d..55047ec0c2 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -14,8 +14,8 @@ use windmill_common::db::DB; use windmill_common::workspaces::{check_deploy_rules, RuleCheckResult}; use crate::secret_backend_ext::{ - delete_secret_from_backend, get_secret_value, is_vault_stored_value, rename_vault_secret, - store_secret_value, + delete_secret_from_backend, get_secret_value, is_external_stored_value, is_vault_stored_value, + rename_vault_secret, store_secret_value, }; use windmill_common::utils::{escape_ilike_pattern, BulkDeleteRequest}; use windmill_common::webhook::{WebhookMessage, WebhookShared}; @@ -25,6 +25,7 @@ use axum::{ routing::{delete, get, post}, Json, Router, }; +use base64::{engine::general_purpose::STANDARD, Engine as _}; use futures::future::try_join_all; use hyper::StatusCode; use serde_json::Value; @@ -535,6 +536,35 @@ async fn check_path_conflict(db: &DB, w_id: &str, path: &str) -> Result<()> { return Ok(()); } +/// Reject a secret value flagged as already-encrypted (`already_encrypted=true`) +/// that is not actually workspace-key ciphertext — e.g. plaintext mistakenly +/// pushed as encrypted. Storing plaintext in the encrypted `value` column +/// silently bricks the variable: every later read fails to decrypt it. +/// +/// The check is purely structural and never decrypts, so it cannot act as a +/// decryption/padding oracle for a caller who can write but not read secrets. +/// `encrypt` (AES-256-CBC) always yields standard base64 decoding to a non-zero +/// multiple of the 16-byte block size; anything else cannot be our ciphertext. +/// Values stored by an external backend ($vault:/$aws_sm:/$azure_kv: markers) +/// are not workspace ciphertext and are passed through untouched. +fn validate_already_encrypted_secret(path: &str, value: &str) -> Result<()> { + if is_external_stored_value(value) { + return Ok(()); + } + let looks_like_ciphertext = STANDARD + .decode(value) + .map(|bytes| !bytes.is_empty() && bytes.len() % 16 == 0) + .unwrap_or(false); + if !looks_like_ciphertext { + return Err(Error::BadRequest(format!( + "Variable {path} was sent as already-encrypted (already_encrypted=true) but its \ + value is not valid workspace-encrypted ciphertext. To push a plaintext secret, \ + send it without already_encrypted (CLI: use --plain-secrets) so it gets encrypted." + ))); + } + Ok(()) +} + async fn create_variable( authed: ApiAuthed, Extension(db): Extension, @@ -585,6 +615,11 @@ async fn create_variable( // 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 }; @@ -1082,6 +1117,11 @@ async fn update_variable( // Store at target_path (new path if renaming, otherwise current path) 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); @@ -1513,3 +1553,61 @@ pub async fn get_value_internal<'a>( Ok(r) } + +#[cfg(test)] +mod tests { + use super::*; + use magic_crypt::MagicCryptTrait; + + #[test] + fn accepts_real_workspace_ciphertext() { + // The exact shape produced by `encrypt` (AES-256-CBC, base64). + let mc = magic_crypt::new_magic_crypt!("a-test-workspace-key", 256); + for plain in [ + "", + "original-secret", + "some: plaintext\n", + "a".repeat(500).as_str(), + ] { + let ciphertext = mc.encrypt_str_to_base64(plain); + assert!( + validate_already_encrypted_secret("f/x/cfg", &ciphertext).is_ok(), + "should accept genuine ciphertext for plaintext {plain:?}: {ciphertext}" + ); + } + } + + #[test] + fn rejects_plaintext_mislabeled_as_encrypted() { + // Plaintext mislabeled as encrypted: storing it verbatim would make the + // variable undecryptable on every read, so it must be rejected. + for plaintext in [ + "some: plaintext\n", + "original-secret", + "hunter2", + "{\"a\": 1}", + "not base64!!", + " leading-space", + ] { + assert!( + validate_already_encrypted_secret("f/x/cfg", plaintext).is_err(), + "should reject plaintext mislabeled as encrypted: {plaintext:?}" + ); + } + } + + #[test] + fn rejects_empty_and_non_block_aligned() { + // Valid base64 but not a whole number of AES blocks -> cannot be our ciphertext. + assert!(validate_already_encrypted_secret("p", "").is_err()); + assert!(validate_already_encrypted_secret("p", "dGVzdA==").is_err()); // "test" -> 4 bytes + } + + #[test] + fn passes_through_external_backend_markers() { + // External secret backends store $-prefixed markers, not workspace ciphertext. + for marker in ["$vault:f/x/cfg", "$aws_sm:f/x/cfg", "$azure_kv:f/x/cfg"] { + assert!(validate_already_encrypted_secret("f/x/cfg", marker).is_ok()); + } + } +} diff --git a/cli/src/commands/variable/variable.ts b/cli/src/commands/variable/variable.ts index dec35fc4f0..2d885c9dd5 100644 --- a/cli/src/commands/variable/variable.ts +++ b/cli/src/commands/variable/variable.ts @@ -99,6 +99,31 @@ export interface VariableFile { is_oauth?: boolean; } +/** + * Whether `value` has the structural shape of a workspace-encrypted secret + * (the form produced by `sync pull` without --plain-secrets), as opposed to a + * plaintext value a user authored by hand. + * + * Mirrors the server guard (windmill-store/src/variables.rs): workspace + * ciphertext (AES-256-CBC, base64) is standard base64 decoding to a non-zero + * multiple of the 16-byte block size. External secret-backend markers + * ($vault:/$aws_sm:/$azure_kv:) are stored verbatim too, so they count as + * already-encrypted. This is a shape check only — it never decrypts. + */ +export function looksLikeWorkspaceCiphertext(value: string): boolean { + if ( + value.startsWith("$vault:") || + value.startsWith("$aws_sm:") || + value.startsWith("$azure_kv:") + ) { + return true; + } + if (value.length === 0 || value.length % 4 !== 0) return false; + if (!/^[A-Za-z0-9+/]+={0,2}$/.test(value)) return false; + const decodedLen = Buffer.from(value, "base64").length; + return decodedLen > 0 && decodedLen % 16 === 0; +} + export async function pushVariable( workspace: string, remotePath: string, @@ -106,6 +131,11 @@ export async function pushVariable( localVariable: VariableFile, plainSecrets: boolean, wsSpecific?: boolean, + // Whether a secret->non-secret downgrade may be applied. Only an authoritative + // single-file `variable push` sets this. Bulk `sync push` leaves it false: a + // pulled secret's spec value is ciphertext, and demoting it would store that + // ciphertext verbatim as a visible non-secret value. + allowSecretDowngrade: boolean = false, ): Promise { remotePath = removeType(remotePath, "variable"); log.debug(`Processing local variable ${remotePath}`); @@ -130,14 +160,26 @@ export async function pushVariable( log.debug(`Variable ${remotePath} is not up-to-date, updating`); + // Apply is_secret only when it differs from the remote (the value is always + // sent, so the server allows the flag change). Upgrades (non-secret->secret) + // always apply; downgrades only when explicitly allowed (single-file push) — + // see allowSecretDowngrade. `undefined` leaves the flag untouched. + let nextIsSecret: boolean | undefined = undefined; + if (localVariable.is_secret !== variable.is_secret) { + if (localVariable.is_secret) { + nextIsSecret = true; + } else if (allowSecretDowngrade) { + nextIsSecret = false; + } + } + await wmill.updateVariable({ workspace, path: remotePath.replaceAll(SEP, "/"), alreadyEncrypted: !plainSecrets, requestBody: { ...localVariable, - is_secret: - localVariable.is_secret && !variable.is_secret ? true : undefined, + is_secret: nextIsSecret, ...(wsSpecific !== undefined ? { ws_specific: wsSpecific } : {}), }, }); @@ -174,12 +216,40 @@ async function push( log.info(colors.bold.yellow("Pushing variable...")); + const local = parseFromFile(filePath) as VariableFile; + + // A secret value in a single-file push is authored by the user and is + // therefore plaintext that must be encrypted server-side — unless it has the + // shape of workspace ciphertext (a value round-tripped from `sync pull`). + // Pushing plaintext as already-encrypted would brick the variable. An explicit + // --plain-secrets always forces the plaintext (encrypt) path. + let plainSecrets = opts.plainSecrets ?? false; + if (opts.plainSecrets === undefined && local.is_secret) { + if (!looksLikeWorkspaceCiphertext(local.value)) { + log.info( + colors.yellow( + "Secret value is not in encrypted form; pushing as plaintext to be encrypted server-side (pass --plain-secrets to silence)." + ) + ); + plainSecrets = true; + } else { + // The value has the shape of workspace ciphertext, so it's stored as-is. + // A plaintext secret that coincidentally looks like ciphertext (e.g. a + // base64 token) would be stored unreadable, so surface the assumption. + log.warn( + "Secret value looks already-encrypted; pushing it as-is. If it is a plaintext secret, re-run with --plain-secrets so it gets encrypted." + ); + } + } + await pushVariable( workspace.workspaceId, remotePath, undefined, - parseFromFile(filePath), - opts.plainSecrets ?? false + local, + plainSecrets, + undefined, + true // single-file push is authoritative: allow secret->non-secret downgrade ); log.info(colors.bold.underline.green(`Variable ${remotePath} pushed`)); } diff --git a/cli/src/core/auth.ts b/cli/src/core/auth.ts index b3496d3cb8..bee70aef28 100644 --- a/cli/src/core/auth.ts +++ b/cli/src/core/auth.ts @@ -104,25 +104,25 @@ export async function requireLogin( // 403 means the token authenticated but lacks scope — re-issuing // won't help. Keep this distinct from the 401 message so the user // doesn't waste time reproducing the token. - log.info(colors.red( + log.infoStderr(colors.red( `Permission denied: the token is valid but lacks the required scope.${bodyStr ? `\n${bodyStr}` : ""}` )); } else if (status === 401) { - log.info(colors.red( + log.infoStderr(colors.red( `Could not authenticate with the provided credentials. Please check your --token and --base-url and try again.${bodyStr ? `\n${bodyStr}` : ""}` )); } else { - log.info(colors.red( + log.infoStderr(colors.red( `Request failed (${status ?? "unknown"}): ${bodyStr}` )); } return process.exit(1); } - log.info(colors.red("Could not authenticate with the provided credentials. Please check your --token and --base-url and try again.")); + log.infoStderr(colors.red("Could not authenticate with the provided credentials. Please check your --token and --base-url and try again.")); return process.exit(1); } - log.info( + log.infoStderr( "! Could not reach API given existing credentials. Attempting to reauth..." ); const newToken = await loginInteractive(workspace.remote); diff --git a/cli/src/core/context.ts b/cli/src/core/context.ts index a4ec705473..929a3728e1 100644 --- a/cli/src/core/context.ts +++ b/cli/src/core/context.ts @@ -57,7 +57,7 @@ async function selectFromMultipleProfiles( (p) => p.name === lastUsedProfileName ); if (lastUsedProfile) { - log.info( + log.infoStderr( colors.green( `Using last used profile '${lastUsedProfile.name}' for ${context}` ) @@ -69,7 +69,7 @@ async function selectFromMultipleProfiles( // No last used or it no longer exists - prompt for selection if (!!!process.stdin.isTTY || !!!process.stdout.isTTY) { const selectedProfile = profiles[0]; - log.info( + log.infoStderr( colors.yellow( `Multiple profiles found for ${context}. Using first available profile: '${selectedProfile.name}'` ) @@ -87,7 +87,7 @@ async function selectFromMultipleProfiles( return selectedProfile; } - log.info( + log.infoStderr( colors.yellow(`\nMultiple workspace profiles found for ${context}:`) ); @@ -125,14 +125,14 @@ async function createWorkspaceProfileInteractively( ): Promise { // Log appropriate message based on context if (!context.isForked) { - log.info( + log.infoStderr( colors.yellow( `\nNo workspace profile found for branch '${context.rawBranch}'\n` + `(${normalizedBaseUrl}, ${workspaceId})` ) ); } else { - log.info( + log.infoStderr( colors.yellow( `\nNo workspace profile was found for this forked workspace\n` + `(${normalizedBaseUrl}, ${workspaceId})` @@ -141,7 +141,7 @@ async function createWorkspaceProfileInteractively( } if (!!!process.stdin.isTTY || !!!process.stdout.isTTY) { - log.info( + log.infoStderr( "Not a TTY, cannot create profile interactively. Use 'wmill workspace add' first." ); return undefined; @@ -187,12 +187,12 @@ async function createWorkspaceProfileInteractively( opts.configDir ); - log.info( + log.infoStderr( colors.green( `✓ Created profile '${profileName}' for ${workspaceId} on ${normalizedBaseUrl}` ) ); - log.info(colors.green(`✓ Profile '${profileName}' is now active`)); + log.infoStderr(colors.green(`✓ Profile '${profileName}' is now active`)); return newWorkspace; } @@ -244,7 +244,7 @@ async function tryResolveWorkspace( `workspace '${opts.workspace}'`, opts.configDir ); - log.info( + log.infoStderr( colors.green( `Using workspace profile '${selected.name}' for workspace '${opts.workspace}' (${workspaceId} on ${normalizedBaseUrl})` ) @@ -254,7 +254,7 @@ async function tryResolveWorkspace( } // No matching profile — offer to create one - log.info( + log.infoStderr( `No profile found for workspace '${opts.workspace}' (${workspaceId} on ${normalizedBaseUrl})` ); const ws = await createWorkspaceProfileInteractively( @@ -309,7 +309,7 @@ export async function tryResolveBranchWorkspace( wsEntry = config.workspaces?.[workspaceNameOverride] as WorkspaceEntryConfig | undefined; if (wsEntry) { wsName = workspaceNameOverride; - log.info(`Using workspace override: ${workspaceNameOverride}`); + log.infoStderr(`Using workspace override: ${workspaceNameOverride}`); } } else { // Only try branch-based resolution if in a Git repository @@ -328,7 +328,7 @@ export async function tryResolveBranchWorkspace( const branchToLookup = originalBranchIfForked ?? rawBranch; if (originalBranchIfForked) { - log.info( + log.infoStderr( `Using original branch \`${originalBranchIfForked}\` for finding workspace from workspaces section in wmill.yaml` ); } @@ -346,7 +346,7 @@ export async function tryResolveBranchWorkspace( if (!wsEntry.baseUrl) { if (workspaceNameOverride) { // User explicitly asked for this workspace but it has no baseUrl - log.warn( + log.warnStderr( `⚠️ Workspace '${wsName}' has no baseUrl configured. Cannot resolve a profile.\n` + ` Add baseUrl to workspace '${wsName}' in wmill.yaml, or use --base-url flag.` ); @@ -370,7 +370,7 @@ export async function tryResolveBranchWorkspace( reason = `matched current git branch '${rawBranch}'`; } - log.info( + log.infoStderr( `Using workspace '${wsName}' (${reason}) → ${workspaceId} on ${baseUrl}` ); @@ -406,7 +406,7 @@ export async function tryResolveBranchWorkspace( if (matchingProfiles.length === 1) { selectedProfile = matchingProfiles[0]; - log.info( + log.infoStderr( colors.green( `Using workspace profile '${selectedProfile.name}' for workspace '${wsName}' with workspace id \`${workspaceId}\`` ) @@ -424,7 +424,7 @@ export async function tryResolveBranchWorkspace( (p) => p.name === lastUsedName ); if (lastUsedProfile) { - log.info( + log.infoStderr( colors.green( `Using workspace profile '${lastUsedProfile.name}' for workspace '${wsName}' (last used)` ) @@ -449,7 +449,7 @@ export async function tryResolveBranchWorkspace( opts.configDir ); - log.info( + log.infoStderr( colors.green( `Using workspace profile '${selectedProfile.name}' for workspace '${wsName}'` ) @@ -459,7 +459,7 @@ export async function tryResolveBranchWorkspace( if (workspaceIdIfForked) { selectedProfile.name = `${selectedProfile.name}/${workspaceIdIfForked}`; selectedProfile.workspaceId = workspaceIdIfForked; - log.info( + log.infoStderr( `Using fork workspace \`${workspaceIdIfForked}\` (parent: \`${workspaceId}\`) from branch \`${rawBranch}\`` ); } @@ -480,7 +480,7 @@ export async function resolveWorkspace( try { normalizedBaseUrl = new URL(opts.baseUrl).toString(); } catch (error) { - log.info(colors.red(`Invalid base URL: ${opts.baseUrl}`)); + log.infoStderr(colors.red(`Invalid base URL: ${opts.baseUrl}`)); return process.exit(-1); } @@ -514,7 +514,7 @@ export async function resolveWorkspace( if (existingWorkspace) { if (existingWorkspace.remote !== normalizedBaseUrl) { - log.info( + log.infoStderr( colors.red( `Base URL mismatch: --base-url is ${normalizedBaseUrl} but workspace profile "${opts.workspace}" uses ${existingWorkspace.remote}` ) @@ -535,7 +535,7 @@ export async function resolveWorkspace( token: opts.token, }; } else { - log.info( + log.infoStderr( colors.red( "If you specify a base URL with --base-url, you must also specify a workspace (--workspace) and token (--token)." ) @@ -555,7 +555,7 @@ export async function resolveWorkspace( if (workspaceNameOverride || opts.workspace || !branch || !branch.startsWith(WM_FORK_PREFIX)) { return workspace; } else { - log.info( + log.infoStderr( `Found an active workspace \`${workspace.name}\` but the branch name indicates this is a forked workspace. Ignoring active workspace and trying to resolve the correct workspace from the branch name \`${branch}\`. Use --workspace to override.` ); } @@ -572,9 +572,9 @@ export async function resolveWorkspace( if (suggestions.length > 0) { msg += ` Did you mean: ${suggestions.map((s) => `"${s.name}"`).join(", ")}?`; } - log.info(colors.red.bold(msg)); + log.infoStderr(colors.red.bold(msg)); if (profiles.length > 0) { - log.info("\nAvailable workspaces:"); + log.infoStderr("\nAvailable workspaces:"); new Table() .header(["name", "remote", "workspace id"]) .padding(2) @@ -620,12 +620,12 @@ export async function resolveWorkspace( if (wsNames.length === 1) { pickedWsName = wsNames[0]; - log.info( + log.infoStderr( `Auto-selected workspace '${pickedWsName}' (only workspace in config).\n` + `Use --workspace to override or 'wmill workspace bind' to add more workspaces.` ); } else if (process.stdin.isTTY) { - log.info( + log.infoStderr( `Multiple workspaces configured but none matched the current context.\n` + `Configured workspaces:\n${wsListStr}\n` + `Use --workspace to skip this prompt.` @@ -675,7 +675,7 @@ export async function resolveWorkspace( try { normalizedBaseUrl = new URL(envBaseUrl).toString(); } catch { - log.info(colors.red(`Invalid BASE_INTERNAL_URL: ${envBaseUrl}`)); + log.infoStderr(colors.red(`Invalid BASE_INTERNAL_URL: ${envBaseUrl}`)); return process.exit(-1); } log.debug( @@ -691,7 +691,7 @@ export async function resolveWorkspace( return ws; } - log.info(colors.red.bold("No workspace given and no default set. Run 'wmill workspace add' to configure one.")); + log.infoStderr(colors.red.bold("No workspace given and no default set. Run 'wmill workspace add' to configure one.")); return process.exit(-1); } @@ -746,7 +746,7 @@ export async function tryResolveVersion( export function validatePath(path: string): boolean { if (!(path.startsWith("g") || path.startsWith("u") || path.startsWith("f"))) { - log.info( + log.infoStderr( colors.red( "Given remote path looks invalid. Remote paths are typically of the form //..." ) diff --git a/cli/src/core/log.ts b/cli/src/core/log.ts index 034e13e3e1..e78450597f 100644 --- a/cli/src/core/log.ts +++ b/cli/src/core/log.ts @@ -21,11 +21,26 @@ export function info(msg: unknown) { console.log(`\x1b[34m${String(msg)}\x1b[39m`); } +// Like `info` but written to stderr, for diagnostics (e.g. the workspace-profile +// banner printed on every command) that must not pollute stdout when a command's +// data output is piped or redirected (e.g. `wmill variable get path > file`). +export function infoStderr(msg: unknown) { + if (silentMode) return; + console.error(`\x1b[34m${String(msg)}\x1b[39m`); +} + export function warn(msg: unknown) { if (silentMode) return; console.log(`\x1b[33m${String(msg)}\x1b[39m`); } +// Like `warn` but written to stderr; see `infoStderr` for why diagnostics must +// not land on stdout. +export function warnStderr(msg: unknown) { + if (silentMode) return; + console.error(`\x1b[33m${String(msg)}\x1b[39m`); +} + export function error(msg: unknown) { console.error(`\x1b[31m${String(msg)}\x1b[39m`); } diff --git a/cli/src/core/login.ts b/cli/src/core/login.ts index f6034b3add..e6a06e8383 100644 --- a/cli/src/core/login.ts +++ b/cli/src/core/login.ts @@ -10,7 +10,7 @@ import * as http from "node:http"; export async function loginInteractive(remote: string) { let token: string | undefined; if (!process.stdin.isTTY) { - log.info("Not a TTY, can't login interactively."); + log.infoStderr("Not a TTY, can't login interactively."); return undefined; } if ( @@ -55,7 +55,7 @@ export async function browserLogin( const port = await getPort.default({ port: env }); if (port == undefined) { - log.info(colors.red.underline("failed to aquire port")); + log.infoStderr(colors.red.underline("failed to aquire port")); return undefined; } @@ -79,7 +79,7 @@ export async function browserLogin( }); const url = `${baseUrl}user/cli?port=${port}`; - log.info(`Login by going to ${url}`); + log.infoStderr(`Login by going to ${url}`); try { open.default(url).catch((error) => { @@ -88,7 +88,7 @@ export async function browserLogin( ); }); - log.info("Opened browser for you"); + log.infoStderr("Opened browser for you"); } catch (error) { console.error( `Failed to open browser, please navigate to ${url}, error: ${error}` diff --git a/cli/test/variable_ciphertext_shape_unit.test.ts b/cli/test/variable_ciphertext_shape_unit.test.ts new file mode 100644 index 0000000000..0113929164 --- /dev/null +++ b/cli/test/variable_ciphertext_shape_unit.test.ts @@ -0,0 +1,42 @@ +import { expect, test } from "bun:test"; + +import { looksLikeWorkspaceCiphertext } from "../src/commands/variable/variable.ts"; + +// ============================================================================= +// looksLikeWorkspaceCiphertext drives whether single-file `variable push` treats +// a secret's value as already-encrypted (store verbatim) or as plaintext to be +// encrypted server-side. It must agree with the server guard +// (validate_already_encrypted_secret in windmill-store/src/variables.rs): a value +// is "ciphertext shaped" iff it is an external-backend marker, or standard base64 +// decoding to a non-zero multiple of the AES block size (16 bytes). +// ============================================================================= + +test("treats workspace-ciphertext-shaped values as already-encrypted", () => { + const ciphertextShaped = [ + "MpYeXnSBBF7dzI6K8J89xQ==", // real magic_crypt output: 16 bytes + Buffer.alloc(16, 7).toString("base64"), // 16 bytes + Buffer.alloc(32, 7).toString("base64"), // 32 bytes + "$vault:f/x/cfg", + "$aws_sm:f/x/cfg", + "$azure_kv:f/x/cfg", + ]; + for (const value of ciphertextShaped) { + expect(looksLikeWorkspaceCiphertext(value)).toBe(true); + } +}); + +test("treats hand-authored plaintext as NOT already-encrypted", () => { + const plaintext = [ + "some: plaintext\n", // space, colon, newline + "original-secret", // hyphen, not length % 4 + "hunter2", + '{"a": 1}', + "", // empty + "dGVzdA==", // valid base64 but decodes to 4 bytes (not % 16) + Buffer.alloc(17, 7).toString("base64"), // 17 bytes (not % 16) + "$omething-plain", // starts with $ but is not a real backend marker + ]; + for (const value of plaintext) { + expect(looksLikeWorkspaceCiphertext(value)).toBe(false); + } +}); diff --git a/cli/test/variable_resource_push.test.ts b/cli/test/variable_resource_push.test.ts index e8b0dc9e26..e6bc84c326 100644 --- a/cli/test/variable_resource_push.test.ts +++ b/cli/test/variable_resource_push.test.ts @@ -219,6 +219,103 @@ describe("variable", () => { }); }); + test("push encrypts a plaintext secret value (no --plain-secrets) and round-trips", async () => { + await withTestBackend(async (backend, tempDir) => { + await setupWorkspaceProfile(backend); + + const uniqueId = Date.now(); + const varPath = `f/test/sec_push_${uniqueId}`; + + // Existing secret variable (server-encrypted). + const createResp = await backend.apiRequest!( + `/api/w/${backend.workspace}/variables/create`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + path: varPath, + value: "original-secret", + is_secret: true, + description: "", + }), + } + ); + expect(createResp.status).toBeLessThan(300); + await createResp.text(); + + // A hand-authored spec file: plaintext value, is_secret: true. Pushing it + // without --plain-secrets must encrypt the value server-side, not store the + // plaintext verbatim as ciphertext (which would make every read fail). + const specPath = join(tempDir, "v.yaml"); + await writeFile( + specPath, + `value: |\n some: plaintext\nis_secret: true\ndescription: ""\n`, + "utf-8" + ); + + const pushResult = await backend.runCLICommand( + ["variable", "push", specPath, varPath], + tempDir + ); + expect(pushResult.code).toEqual(0); + + // The value must decrypt cleanly to the pushed plaintext. + const apiResp = await backend.apiRequest!( + `/api/w/${backend.workspace}/variables/get/${varPath}?decrypt_secret=true` + ); + expect(apiResp.status).toEqual(200); + const varData = await apiResp.json(); + expect(varData.is_secret).toBe(true); + expect(varData.value).toBe("some: plaintext\n"); + }); + }); + + test("push flips is_secret from true to false", async () => { + await withTestBackend(async (backend, tempDir) => { + await setupWorkspaceProfile(backend); + + const uniqueId = Date.now(); + const varPath = `f/test/sec_down_${uniqueId}`; + + const createResp = await backend.apiRequest!( + `/api/w/${backend.workspace}/variables/create`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + path: varPath, + value: "original-secret", + is_secret: true, + description: "", + }), + } + ); + expect(createResp.status).toBeLessThan(300); + await createResp.text(); + + const specPath = join(tempDir, "v_down.yaml"); + await writeFile( + specPath, + `value: "now-public"\nis_secret: false\ndescription: ""\n`, + "utf-8" + ); + + const pushResult = await backend.runCLICommand( + ["variable", "push", specPath, varPath], + tempDir + ); + expect(pushResult.code).toEqual(0); + + const apiResp = await backend.apiRequest!( + `/api/w/${backend.workspace}/variables/get/${varPath}?decrypt_secret=true` + ); + expect(apiResp.status).toEqual(200); + const varData = await apiResp.json(); + expect(varData.is_secret).toBe(false); + expect(varData.value).toBe("now-public"); + }); + }); + test("pull retrieves variables into local files", async () => { await withTestBackend(async (backend, tempDir) => { await setupWorkspaceProfile(backend);