From eee418bee9fa762ec356587f90f78855fe0b5ac1 Mon Sep 17 00:00:00 2001 From: ravjotb Date: Fri, 18 Sep 2026 12:42:13 -0700 Subject: [PATCH 1/4] Fail closed instead of storing a cleartext password without a keystore When the OS has no secure store (e.g. headless/minimal Linux without gnome-keyring or KWallet), safeStorage.isEncryptionAvailable() is false and the encrypt IPC previously returned the plaintext password, which the renderer then persisted to localStorage while the UI implied it was protected. Fail closed: - The encrypt handler now returns { ok, value } and never returns plaintext; adds a secure-storage:is-encryption-available IPC. - The connect/save paths keep the entered plaintext in memory so the current session still connects, mark the connection isPasswordEncrypted = false, and warn the user (with an actionable keyring hint). - persistConnections is the single localStorage write path: a connection whose password could not be encrypted is written with password undefined (and the transient flag dropped), so no cleartext reaches disk. On next use the connection re-prompts for the password. Passwordless connections are unaffected (encryption is never invoked). Adds wrapper tests for the new structured contract and persistConnections strip tests. Signed-off-by: ravjotb --- apps/frontend/electron.main.js | 12 +++- apps/frontend/preload.js | 1 + .../src/components/ValkeyReconnect.tsx | 9 ++- .../cluster-topology/cluster-node-row.tsx | 9 ++- .../src/components/connection/Connection.tsx | 9 ++- .../src/components/ui/connection-form.tsx | 20 +++++-- apps/frontend/src/components/ui/edit-form.tsx | 15 +++-- .../state/epics/valkeyEpics.persist.test.ts | 58 +++++++++++++++++++ apps/frontend/src/state/epics/valkeyEpics.ts | 25 +++++++- .../connection/connectionSlice.ts | 4 ++ .../valkey-features/topology/topologySlice.ts | 6 +- apps/frontend/src/utils/secureStorage.test.ts | 51 ++++++++++------ apps/frontend/src/utils/secureStorage.ts | 29 +++++++--- 13 files changed, 195 insertions(+), 53 deletions(-) create mode 100644 apps/frontend/src/state/epics/valkeyEpics.persist.test.ts diff --git a/apps/frontend/electron.main.js b/apps/frontend/electron.main.js index 544a3151..c1a4d8ba 100644 --- a/apps/frontend/electron.main.js +++ b/apps/frontend/electron.main.js @@ -126,10 +126,16 @@ powerMonitor.on("resume", () => { }) }) +ipcMain.handle("secure-storage:is-encryption-available", async () => safeStorage.isEncryptionAvailable()) + ipcMain.handle("secure-storage:encrypt", async (event, password) => { - if (!password || !safeStorage.isEncryptionAvailable()) return password - const encrypted = safeStorage.encryptString(password) - return encrypted.toString("base64") + if (!password) return { ok: true, value: "" } + // Fail closed: never return the plaintext password when the OS has no secure + // store, so the renderer can't persist an unprotected secret while implying it + // is encrypted. The caller keeps the plaintext for the live connection and marks + // it do-not-persist. + if (!safeStorage.isEncryptionAvailable()) return { ok: false } + return { ok: true, value: safeStorage.encryptString(password).toString("base64") } }) ipcMain.handle("secure-storage:decrypt", async (event, encryptedBase64) => { diff --git a/apps/frontend/preload.js b/apps/frontend/preload.js index c081778d..82b504f3 100644 --- a/apps/frontend/preload.js +++ b/apps/frontend/preload.js @@ -4,6 +4,7 @@ const { contextBridge, ipcRenderer } = require("electron") contextBridge.exposeInMainWorld("secureStorage", { encrypt: (password) => ipcRenderer.invoke("secure-storage:encrypt", password), decrypt: (encrypted) => ipcRenderer.invoke("secure-storage:decrypt", encrypted), + isEncryptionAvailable: () => ipcRenderer.invoke("secure-storage:is-encryption-available"), }) contextBridge.exposeInMainWorld("electronNavigation", { diff --git a/apps/frontend/src/components/ValkeyReconnect.tsx b/apps/frontend/src/components/ValkeyReconnect.tsx index 242bfd24..3ddea1c3 100644 --- a/apps/frontend/src/components/ValkeyReconnect.tsx +++ b/apps/frontend/src/components/ValkeyReconnect.tsx @@ -4,11 +4,12 @@ import { useNavigate, useParams } from "react-router" import { CONNECTED, CONNECTING, ERROR } from "@common/src/constants" import { Loader2, Database, AlertCircle } from "lucide-react" import * as R from "ramda" +import { toast } from "sonner" import RetryProgress from "./ui/retry-progress" import { PasswordPromptModal } from "./ui/password-prompt-modal" import type { RootState } from "@/store" import { connectPending } from "@/state/valkey-features/connection/connectionSlice" -import { secureStorage } from "@/utils/secureStorage" +import { secureStorage, PASSWORD_NOT_STORED_WARNING } from "@/utils/secureStorage" export function ValkeyReconnect() { const dispatch = useDispatch() @@ -51,10 +52,12 @@ export function ValkeyReconnect() { const handlePasswordSubmit = async (password: string) => { if (!connection) return - const encryptedPassword = await secureStorage.encryptIfAvailable(password) + const result = await secureStorage.encryptForStorage(password) + if (!result.ok) toast.warning(PASSWORD_NOT_STORED_WARNING) dispatch(connectPending({ connectionId: id!, - connectionDetails: { ...connection.connectionDetails, password: encryptedPassword }, + connectionDetails: { ...connection.connectionDetails, password: result.ok ? result.value : password }, + isPasswordEncrypted: result.ok, })) } diff --git a/apps/frontend/src/components/cluster-topology/cluster-node-row.tsx b/apps/frontend/src/components/cluster-topology/cluster-node-row.tsx index 77516533..5f33623b 100644 --- a/apps/frontend/src/components/cluster-topology/cluster-node-row.tsx +++ b/apps/frontend/src/components/cluster-topology/cluster-node-row.tsx @@ -8,6 +8,7 @@ import { buildConnectionId } from "@common/src/connection-id.ts" import { calculateHitRatio } from "@common/src/cache-hit-ratio.ts" import { formatBytes } from "@common/src/bytes-conversion.ts" import { TooltipProvider } from "@radix-ui/react-tooltip" +import { toast } from "sonner" import { Badge } from "../ui/badge" import { CustomTooltip } from "../ui/tooltip" import { Button } from "../ui/button" @@ -23,7 +24,7 @@ import { useAppDispatch } from "@/hooks/hooks" import { selectIsAtConnectionLimit, selectEncryptedPassword, selectClusterDb } from "@/state/valkey-features/connection/connectionSelectors" -import { secureStorage } from "@/utils/secureStorage.ts" +import { secureStorage, PASSWORD_NOT_STORED_WARNING } from "@/utils/secureStorage.ts" import { cn } from "@/lib/utils" const UTILIZATION_BADGE: Record = { @@ -126,14 +127,16 @@ export function ClusterNodeRow({ } const handlePasswordSubmit = async (password: string) => { - const encryptedPw = await secureStorage.encryptIfAvailable(password) + const result = await secureStorage.encryptForStorage(password) + if (!result.ok) toast.warning(PASSWORD_NOT_STORED_WARNING) dispatch(connectPending({ connectionId, connectionDetails: { ...baseDetails, username: primaryConfig.username ?? "", - password: encryptedPw, + password: result.ok ? result.value : password, }, + isPasswordEncrypted: result.ok, })) } diff --git a/apps/frontend/src/components/connection/Connection.tsx b/apps/frontend/src/components/connection/Connection.tsx index c5bc9eb1..01247fba 100644 --- a/apps/frontend/src/components/connection/Connection.tsx +++ b/apps/frontend/src/components/connection/Connection.tsx @@ -2,6 +2,7 @@ import { useState } from "react" import { useSelector } from "react-redux" import { HousePlug } from "lucide-react" import { CONNECTED, CONNECTING, MAX_CONNECTIONS, RECONNECTING } from "@common/src/constants.ts" +import { toast } from "sonner" import ConnectionForm from "../ui/connection-form.tsx" import EditForm from "../ui/edit-form.tsx" import { PasswordPromptModal } from "../ui/password-prompt-modal.tsx" @@ -17,7 +18,7 @@ import { selectConnections } from "@/state/valkey-features/connection/connection import { ConnectionEntry } from "@/components/connection/ConnectionEntry.tsx" import { ClusterConnectionGroup } from "@/components/connection/ClusterConnectionGroup.tsx" import { useAppDispatch } from "@/hooks/hooks.ts" -import { secureStorage } from "@/utils/secureStorage.ts" +import { secureStorage, PASSWORD_NOT_STORED_WARNING } from "@/utils/secureStorage.ts" const matchesSearch = (q: string, connection: ConnectionState) => connection.searchableText.includes(q) @@ -49,10 +50,12 @@ export function Connection() { if (!passwordPromptConnectionId) return const connection = connections[passwordPromptConnectionId] if (!connection) return - const encryptedPassword = await secureStorage.encryptIfAvailable(password) + const result = await secureStorage.encryptForStorage(password) + if (!result.ok) toast.warning(PASSWORD_NOT_STORED_WARNING) dispatch(connectPending({ connectionId: passwordPromptConnectionId, - connectionDetails: { ...connection.connectionDetails, password: encryptedPassword }, + connectionDetails: { ...connection.connectionDetails, password: result.ok ? result.value : password }, + isPasswordEncrypted: result.ok, preservedHistory: connection.connectionHistory, })) } diff --git a/apps/frontend/src/components/ui/connection-form.tsx b/apps/frontend/src/components/ui/connection-form.tsx index c6e19524..fc479c7d 100644 --- a/apps/frontend/src/components/ui/connection-form.tsx +++ b/apps/frontend/src/components/ui/connection-form.tsx @@ -2,6 +2,7 @@ import { type FormEvent, useEffect, useState } from "react" import { useSelector } from "react-redux" import { buildConnectionId, isValidDatabaseIndex } from "@common/src/connection-id.ts" import { CONNECTED, CONNECTING, ERROR } from "@common/src/constants.ts" +import { toast } from "sonner" import { ConnectionModal } from "./connection-modal.tsx" import { useAppDispatch, useAppSelector } from "@/hooks/hooks" import { connectPending, type ConnectionDetails } from "@/state/valkey-features/connection/connectionSlice.ts" @@ -10,7 +11,7 @@ import { discoveryEndpointPending, clearEndpointDiscovery } from "@/state/valkey-features/topology/topologySlice.ts" -import { secureStorage } from "@/utils/secureStorage.ts" +import { secureStorage, PASSWORD_NOT_STORED_WARNING } from "@/utils/secureStorage.ts" interface ConnectionFormProps { onClose: () => void @@ -87,22 +88,29 @@ function ConnectionForm({ onClose }: ConnectionFormProps) { awsReplicationGroupId: connectionDetails.awsReplicationGroupId?.trim(), } - const detailsToDispatch = connectionDetails.password - ? { ...trimmed, password: await secureStorage.encryptIfAvailable(connectionDetails.password) } - : trimmed + let isPasswordEncrypted: boolean | undefined + let detailsToDispatch = trimmed + if (connectionDetails.password) { + const result = await secureStorage.encryptForStorage(connectionDetails.password) + isPasswordEncrypted = result.ok + // On failure keep the plaintext so this session can connect; it will not be + // persisted (see the persistence layer), and the user is warned. + detailsToDispatch = { ...trimmed, password: result.ok ? result.value : connectionDetails.password } + if (!result.ok) toast.warning(PASSWORD_NOT_STORED_WARNING) + } if (trimmed.endpointType === "cluster-endpoint") { const newDiscoveryId = `discovery-${buildConnectionId(trimmed.host, trimmed.port, 0)}` setDiscoveryId(newDiscoveryId) setConnectionId(null) - dispatch(discoveryEndpointPending({ discoveryId: newDiscoveryId, connectionDetails: detailsToDispatch })) + dispatch(discoveryEndpointPending({ discoveryId: newDiscoveryId, connectionDetails: detailsToDispatch, isPasswordEncrypted })) return } const newConnectionId = buildConnectionId(trimmed.host, trimmed.port, trimmed.db) setConnectionId(newConnectionId) setDiscoveryId(null) - dispatch(connectPending({ connectionId: newConnectionId, connectionDetails: detailsToDispatch })) + dispatch(connectPending({ connectionId: newConnectionId, connectionDetails: detailsToDispatch, isPasswordEncrypted })) } return ( diff --git a/apps/frontend/src/components/ui/edit-form.tsx b/apps/frontend/src/components/ui/edit-form.tsx index 6957b225..9300ff88 100644 --- a/apps/frontend/src/components/ui/edit-form.tsx +++ b/apps/frontend/src/components/ui/edit-form.tsx @@ -2,6 +2,7 @@ import { type FormEvent, useState, useEffect, useCallback } from "react" import { useSelector } from "react-redux" import { buildConnectionId, isValidDatabaseIndex } from "@common/src/connection-id.ts" import { CONNECTED } from "@common/src/constants" +import { toast } from "sonner" import { ConnectionModal } from "./connection-modal.tsx" import { updateConnectionDetails, @@ -16,7 +17,7 @@ import { selectIsAtConnectionLimit } from "@/state/valkey-features/connection/connectionSelectors" import { useAppDispatch } from "@/hooks/hooks" -import { secureStorage } from "@/utils/secureStorage.ts" +import { secureStorage, PASSWORD_NOT_STORED_WARNING } from "@/utils/secureStorage.ts" interface EditFormProps { onClose: () => void @@ -146,15 +147,21 @@ function EditForm({ onClose, connectionId }: EditFormProps) { dispatch(deleteConnection({ connectionId, silent: true })) // Encrypt password only if user typed a new one; otherwise it's already encrypted from Redux - const detailsToDispatch = passwordChanged && connectionDetails.password - ? { ...trimmed, password: await secureStorage.encryptIfAvailable(connectionDetails.password) } - : trimmed + let isPasswordEncrypted: boolean | undefined + let detailsToDispatch = trimmed + if (passwordChanged && connectionDetails.password) { + const result = await secureStorage.encryptForStorage(connectionDetails.password) + isPasswordEncrypted = result.ok + detailsToDispatch = { ...trimmed, password: result.ok ? result.value : connectionDetails.password } + if (!result.ok) toast.warning(PASSWORD_NOT_STORED_WARNING) + } dispatch( connectPending({ connectionId: newConnectionId, connectionDetails: detailsToDispatch, isEdit: true, + isPasswordEncrypted, preservedHistory: connectionHistory, }), ) diff --git a/apps/frontend/src/state/epics/valkeyEpics.persist.test.ts b/apps/frontend/src/state/epics/valkeyEpics.persist.test.ts new file mode 100644 index 00000000..4b73d50f --- /dev/null +++ b/apps/frontend/src/state/epics/valkeyEpics.persist.test.ts @@ -0,0 +1,58 @@ +import { LOCAL_STORAGE } from "@common/src/constants" +import { persistConnections } from "./valkeyEpics" +import type { ConnectionState } from "@/state/valkey-features/connection/connectionSlice" + +// persistConnections is the single write path to localStorage. Its security job: +// never write a password that could not be encrypted (isPasswordEncrypted === false). +describe("persistConnections", () => { + beforeEach(() => localStorage.clear()) + + const read = () => JSON.parse(localStorage.getItem(LOCAL_STORAGE.VALKEY_CONNECTIONS) ?? "{}") + + const conn = (overrides: Partial): ConnectionState => ({ + status: "NOT_CONNECTED" as ConnectionState["status"], + errorMessage: null, + searchableText: "", + connectionDetails: { + host: "h", port: "6379", tls: false, verifyTlsCertificate: false, endpointType: "node", db: 0, + password: "secret", + }, + ...overrides, + }) + + it("strips the password and drops the flag when the password is unencrypted", () => { + persistConnections({ a: conn({ isPasswordEncrypted: false }) }) + + const stored = read().a + expect(stored.connectionDetails.password).toBeUndefined() + expect("isPasswordEncrypted" in stored).toBe(false) + }) + + it("preserves an encrypted password (flag true)", () => { + persistConnections({ a: conn({ isPasswordEncrypted: true, connectionDetails: { + host: "h", port: "6379", tls: false, verifyTlsCertificate: false, endpointType: "node", db: 0, + password: "ciphertext", + } }) }) + + expect(read().a.connectionDetails.password).toBe("ciphertext") + }) + + it("preserves a password when the flag is absent (existing/normal connections)", () => { + persistConnections({ a: conn({}) }) + expect(read().a.connectionDetails.password).toBe("secret") + }) + + it("only strips the flagged connection, leaving others intact", () => { + persistConnections({ + bad: conn({ isPasswordEncrypted: false }), + good: conn({ isPasswordEncrypted: true, connectionDetails: { + host: "g", port: "6379", tls: false, verifyTlsCertificate: false, endpointType: "node", db: 0, + password: "keepme", + } }), + }) + + const stored = read() + expect(stored.bad.connectionDetails.password).toBeUndefined() + expect(stored.good.connectionDetails.password).toBe("keepme") + }) +}) diff --git a/apps/frontend/src/state/epics/valkeyEpics.ts b/apps/frontend/src/state/epics/valkeyEpics.ts index 427f111e..a4c1502e 100644 --- a/apps/frontend/src/state/epics/valkeyEpics.ts +++ b/apps/frontend/src/state/epics/valkeyEpics.ts @@ -51,6 +51,24 @@ const getCurrentConnections = () => R.pipe( (s) => (s === null ? {} : JSON.parse(s)), )(LOCAL_STORAGE.VALKEY_CONNECTIONS) +// Single write path to localStorage. A password that could not be encrypted +// (isPasswordEncrypted === false) is never written to disk: it is dropped to +// undefined so the connection re-prompts on next use, and the transient flag is +// not persisted either. All persistence writes must go through this. +export const persistConnections = (connections: Record) => { + const safe = Object.fromEntries( + Object.entries(connections).map(([id, conn]) => { + if (conn?.isPasswordEncrypted === false) { + const stripped = { ...conn, connectionDetails: { ...conn.connectionDetails, password: undefined } } + delete stripped.isPasswordEncrypted + return [id, stripped] + } + return [id, conn] + }), + ) + localStorage.setItem(LOCAL_STORAGE.VALKEY_CONNECTIONS, JSON.stringify(safe)) +} + export const connectionEpic = (store: Store) => merge( action$.pipe( @@ -100,7 +118,7 @@ export const connectionEpic = (store: Store) => } currentConnections[payload.connectionId] = connectionToSave - localStorage.setItem(LOCAL_STORAGE.VALKEY_CONNECTIONS, JSON.stringify(currentConnections)) + persistConnections(currentConnections) if (baseConnectionDetails?.host?.includes(".serverless.")) { toast.warning( @@ -156,6 +174,7 @@ export const connectionEpic = (store: Store) => port: String(firstNode.port), endpointType: "node", }, + isPasswordEncrypted: discovery.isPasswordEncrypted, })) // store the connectionId in the discovery state so we can show the correct connection status store.dispatch(discoveryNodeConnecting({ discoveryId, connectionId })) @@ -372,7 +391,7 @@ export const deleteConnectionEpic = () => const currentConnections = getCurrentConnections() if (currentConnections[connectionId]) { currentConnections[connectionId].userDisconnected = true - localStorage.setItem(LOCAL_STORAGE.VALKEY_CONNECTIONS, JSON.stringify(currentConnections)) + persistConnections(currentConnections) } } catch (e) { console.error(e) @@ -415,7 +434,7 @@ export const updateConnectionDetailsEpic = (store: Store) => currentConnections[connectionId].connectionDetails = connection.connectionDetails currentConnections[connectionId].connectionHistory = connection.connectionHistory || [] currentConnections[connectionId].searchableText = connection.searchableText ?? "" - localStorage.setItem(LOCAL_STORAGE.VALKEY_CONNECTIONS, JSON.stringify(currentConnections)) + persistConnections(currentConnections) } } catch (e) { console.error(e) diff --git a/apps/frontend/src/state/valkey-features/connection/connectionSlice.ts b/apps/frontend/src/state/valkey-features/connection/connectionSlice.ts index 9db74e15..f448c1e9 100644 --- a/apps/frontend/src/state/valkey-features/connection/connectionSlice.ts +++ b/apps/frontend/src/state/valkey-features/connection/connectionSlice.ts @@ -72,6 +72,7 @@ export interface ConnectionState { connectionHistory?: ConnectionHistoryEntry[]; wasEdit?: boolean; userDisconnected?: boolean; + isPasswordEncrypted?: boolean; // Set when a connect is automatic (refresh resume / socket-drop reconnect) autoConnect?: boolean; } @@ -138,6 +139,7 @@ const connectionSlice = createSlice({ isRetry?: boolean; isResume?: boolean; isEdit?: boolean; + isPasswordEncrypted?: boolean; autoConnect?: boolean; preservedHistory?: ConnectionHistoryEntry[]; }>, @@ -147,6 +149,7 @@ const connectionSlice = createSlice({ connectionDetails, isRetry = false, isEdit = false, + isPasswordEncrypted, autoConnect = false, preservedHistory, } = action.payload @@ -166,6 +169,7 @@ const connectionSlice = createSlice({ }, searchableText: buildSearchableText(connectionId, connectionDetails), wasEdit: isEdit, + isPasswordEncrypted, autoConnect, ...(isRetry && existingConnection?.reconnect && { reconnect: existingConnection.reconnect, diff --git a/apps/frontend/src/state/valkey-features/topology/topologySlice.ts b/apps/frontend/src/state/valkey-features/topology/topologySlice.ts index 7cc75648..22bf9554 100644 --- a/apps/frontend/src/state/valkey-features/topology/topologySlice.ts +++ b/apps/frontend/src/state/valkey-features/topology/topologySlice.ts @@ -12,6 +12,7 @@ export type DiscoveryStatus = "pending" | "fulfilled" | "node_connecting" | "rej export interface DiscoveryState { status: DiscoveryStatus connectionDetails: ConnectionDetails + isPasswordEncrypted?: boolean clusterNodes?: Record errorMessage?: string nodeConnectionId?: string @@ -31,12 +32,13 @@ const topologySlice = createSlice({ reducers: { discoveryEndpointPending: ( state, - action: PayloadAction<{ discoveryId: string; connectionDetails: ConnectionDetails }>, + action: PayloadAction<{ discoveryId: string; connectionDetails: ConnectionDetails; isPasswordEncrypted?: boolean }>, ) => { - const { discoveryId, connectionDetails } = action.payload + const { discoveryId, connectionDetails, isPasswordEncrypted } = action.payload state.discoveries[discoveryId] = { status: "pending", connectionDetails, + isPasswordEncrypted, } }, discoveryEndpointFulfilled: ( diff --git a/apps/frontend/src/utils/secureStorage.test.ts b/apps/frontend/src/utils/secureStorage.test.ts index b5078495..8d7a47af 100644 --- a/apps/frontend/src/utils/secureStorage.test.ts +++ b/apps/frontend/src/utils/secureStorage.test.ts @@ -3,58 +3,68 @@ import { secureStorage } from "./secureStorage" // Tests for the renderer-side secureStorage wrapper (secureStorage.ts). // This wrapper delegates to window.secureStorage (exposed via contextBridge) -// and gracefully falls back when running outside Electron. +// and gracefully fails closed when running outside Electron. describe("secureStorage wrapper", () => { beforeEach(() => { delete (window as Record).secureStorage }) // Outside Electron (e.g. in tests or a browser), window.secureStorage is - // undefined. The wrapper should return empty string to prevent unencrypted persistence. + // undefined. Persistence encryption must fail closed so no cleartext is stored. describe("when window.secureStorage is not available", () => { - it("encrypt returns empty string", async () => { - expect(await secureStorage.encrypt("password")).toBe("") + it("encryptForStorage fails closed for a real password", async () => { + expect(await secureStorage.encryptForStorage("password")).toEqual({ ok: false }) + }) + + it("encryptForStorage reports ok for empty input (nothing to protect)", async () => { + expect(await secureStorage.encryptForStorage("")).toEqual({ ok: true, value: "" }) }) it("decrypt returns empty string", async () => { expect(await secureStorage.decrypt("encrypted")).toBe("") }) - it("encrypt returns empty string for empty input", async () => { - expect(await secureStorage.encrypt("")).toBe("") + it("isEncryptionAvailable is false", async () => { + expect(await secureStorage.isEncryptionAvailable()).toBe(false) }) - it("decrypt returns empty string for empty input", async () => { - expect(await secureStorage.decrypt("")).toBe("") + it("isAvailable is false", () => { + expect(secureStorage.isAvailable()).toBe(false) }) }) describe("when window.secureStorage is available", () => { let mockEncrypt: ReturnType let mockDecrypt: ReturnType + let mockIsEncryptionAvailable: ReturnType beforeEach(() => { mockEncrypt = vi.fn() mockDecrypt = vi.fn() + mockIsEncryptionAvailable = vi.fn() Object.defineProperty(window, "secureStorage", { - value: { encrypt: mockEncrypt, decrypt: mockDecrypt }, + value: { encrypt: mockEncrypt, decrypt: mockDecrypt, isEncryptionAvailable: mockIsEncryptionAvailable }, writable: true, configurable: true, }) }) - it("encrypt delegates to window.secureStorage.encrypt", async () => { - mockEncrypt.mockResolvedValue("base64encrypted") - const result = await secureStorage.encrypt("mypassword") + it("encryptForStorage returns the structured success result from the bridge", async () => { + mockEncrypt.mockResolvedValue({ ok: true, value: "base64encrypted" }) + const result = await secureStorage.encryptForStorage("mypassword") expect(mockEncrypt).toHaveBeenCalledWith("mypassword") - expect(result).toBe("base64encrypted") + expect(result).toEqual({ ok: true, value: "base64encrypted" }) + }) + + it("encryptForStorage surfaces a failure (encryption unavailable) from the bridge", async () => { + mockEncrypt.mockResolvedValue({ ok: false }) + expect(await secureStorage.encryptForStorage("mypassword")).toEqual({ ok: false }) }) - // Empty passwords should short-circuit without invoking the IPC channel - it("encrypt returns empty string for empty input", async () => { - const result = await secureStorage.encrypt("") + it("encryptForStorage short-circuits empty input without invoking the bridge", async () => { + const result = await secureStorage.encryptForStorage("") expect(mockEncrypt).not.toHaveBeenCalled() - expect(result).toBe("") + expect(result).toEqual({ ok: true, value: "" }) }) it("decrypt delegates to window.secureStorage.decrypt", async () => { @@ -69,6 +79,13 @@ describe("secureStorage wrapper", () => { expect(mockDecrypt).not.toHaveBeenCalled() expect(result).toBe("") }) + + it("isEncryptionAvailable reflects the real OS backend", async () => { + mockIsEncryptionAvailable.mockResolvedValue(true) + expect(await secureStorage.isEncryptionAvailable()).toBe(true) + mockIsEncryptionAvailable.mockResolvedValue(false) + expect(await secureStorage.isEncryptionAvailable()).toBe(false) + }) }) }) diff --git a/apps/frontend/src/utils/secureStorage.ts b/apps/frontend/src/utils/secureStorage.ts index 457db5e4..6698216e 100644 --- a/apps/frontend/src/utils/secureStorage.ts +++ b/apps/frontend/src/utils/secureStorage.ts @@ -1,7 +1,17 @@ +type EncryptResult = { ok: true; value: string } | { ok: false } + +export const PASSWORD_NOT_STORED_WARNING = + "This system has no secure credential store, so the password can't be saved and will be requested " + + "on the next connection. Install or unlock your system keyring (e.g. gnome-keyring or KWallet) to " + + "enable saved passwords." + export const secureStorage = { - encrypt: async (unencrypted: string): Promise => { - if (!unencrypted || !window.secureStorage) return "" - return await window.secureStorage.encrypt(unencrypted) + // Encrypt for PERSISTENCE. Reports whether real encryption happened so the + // caller can refuse to persist an unprotected secret. + encryptForStorage: async (password: string): Promise => { + if (!password) return { ok: true, value: "" } + if (!window.secureStorage) return { ok: false } + return await window.secureStorage.encrypt(password) }, decrypt: async (encrypted: string): Promise => { @@ -9,11 +19,11 @@ export const secureStorage = { return await window.secureStorage.decrypt(encrypted) }, - encryptIfAvailable: async (password: string): Promise => { - if (password.length > 0 && secureStorage.isAvailable()) { - return await secureStorage.encrypt(password) - } - return password + // True only when the OS actually has a secure store (not merely that we are in + // Electron). Reflects safeStorage.isEncryptionAvailable() in the main process. + isEncryptionAvailable: async (): Promise => { + if (!window.secureStorage?.isEncryptionAvailable) return false + return await window.secureStorage.isEncryptionAvailable() }, isAvailable: (): boolean => { @@ -24,8 +34,9 @@ export const secureStorage = { declare global { interface Window { secureStorage?: { - encrypt: (password: string) => Promise + encrypt: (password: string) => Promise decrypt: (encrypted: string) => Promise + isEncryptionAvailable: () => Promise } } } From 722b6dc47a65cc7aba8da8dcd8e817bef0023da2 Mon Sep 17 00:00:00 2001 From: ravjotb Date: Fri, 18 Sep 2026 12:50:49 -0700 Subject: [PATCH 2/4] Close flag-drop paths that bypassed the persistence strip Self-review of the fail-closed change found three paths where the isPasswordEncrypted marking was lost while the plaintext password remained, so persistConnections would not strip it: - The connect-success epic rebuilt the saved entry without the flag, so the primary path (connect then persist) still wrote plaintext. - updateConnectionDetailsEpic copied connectionDetails onto the stored entry without the flag. - The connectPending reducer took the flag only from the payload, so retry/resume/auto-reconnect re-dispatches (which omit it) wiped the marking in Redux. Carry the flag through all three, preserving an existing marking when a re-dispatch omits it while still letting an explicit value override it. Adds reducer regression tests for the preserve/override behavior. Signed-off-by: ravjotb --- apps/frontend/src/state/epics/valkeyEpics.ts | 2 + .../connection/connectionSlice.test.ts | 37 +++++++++++++++++++ .../connection/connectionSlice.ts | 4 +- 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/apps/frontend/src/state/epics/valkeyEpics.ts b/apps/frontend/src/state/epics/valkeyEpics.ts index a4c1502e..81d34536 100644 --- a/apps/frontend/src/state/epics/valkeyEpics.ts +++ b/apps/frontend/src/state/epics/valkeyEpics.ts @@ -115,6 +115,7 @@ export const connectionEpic = (store: Store) => status: NOT_CONNECTED, connectionHistory: connection?.connectionHistory ?? [], searchableText: connection?.searchableText ?? "", + isPasswordEncrypted: connection?.isPasswordEncrypted, } currentConnections[payload.connectionId] = connectionToSave @@ -434,6 +435,7 @@ export const updateConnectionDetailsEpic = (store: Store) => currentConnections[connectionId].connectionDetails = connection.connectionDetails currentConnections[connectionId].connectionHistory = connection.connectionHistory || [] currentConnections[connectionId].searchableText = connection.searchableText ?? "" + currentConnections[connectionId].isPasswordEncrypted = connection.isPasswordEncrypted persistConnections(currentConnections) } } catch (e) { diff --git a/apps/frontend/src/state/valkey-features/connection/connectionSlice.test.ts b/apps/frontend/src/state/valkey-features/connection/connectionSlice.test.ts index c19bc061..d4a1f6b6 100644 --- a/apps/frontend/src/state/valkey-features/connection/connectionSlice.test.ts +++ b/apps/frontend/src/state/valkey-features/connection/connectionSlice.test.ts @@ -22,6 +22,43 @@ describe("connectionSlice", () => { }) describe("connectPending", () => { + it("preserves isPasswordEncrypted=false across re-dispatches that omit the flag (retry/resume)", () => { + const details = { + host: "localhost", port: "6379", username: "admin", password: "plaintext", + tls: false, verifyTlsCertificate: false, alias: "Test", + } + // Initial connect marks the password as unencrypted (no OS keystore). + let state = connectionReducer( + initialState, + connectPending({ connectionId: "conn-1", connectionDetails: details, isPasswordEncrypted: false }), + ) + expect(state.connections["conn-1"].isPasswordEncrypted).toBe(false) + + // A retry re-dispatch carries the same details but no flag — the marking must survive, + // otherwise the persistence strip would be bypassed and plaintext could reach disk. + state = connectionReducer( + state, + connectPending({ connectionId: "conn-1", connectionDetails: details, isRetry: true }), + ) + expect(state.connections["conn-1"].isPasswordEncrypted).toBe(false) + }) + + it("lets a new explicit flag override the preserved one (password re-entered with keystore available)", () => { + const details = { + host: "localhost", port: "6379", username: "admin", password: "ciphertext", + tls: false, verifyTlsCertificate: false, alias: "Test", + } + let state = connectionReducer( + initialState, + connectPending({ connectionId: "conn-1", connectionDetails: details, isPasswordEncrypted: false }), + ) + state = connectionReducer( + state, + connectPending({ connectionId: "conn-1", connectionDetails: details, isPasswordEncrypted: true }), + ) + expect(state.connections["conn-1"].isPasswordEncrypted).toBe(true) + }) + it("should create connection with CONNECTING status and store details with no password", () => { const state = connectionReducer( initialState, diff --git a/apps/frontend/src/state/valkey-features/connection/connectionSlice.ts b/apps/frontend/src/state/valkey-features/connection/connectionSlice.ts index f448c1e9..bd87cb3d 100644 --- a/apps/frontend/src/state/valkey-features/connection/connectionSlice.ts +++ b/apps/frontend/src/state/valkey-features/connection/connectionSlice.ts @@ -169,7 +169,9 @@ const connectionSlice = createSlice({ }, searchableText: buildSearchableText(connectionId, connectionDetails), wasEdit: isEdit, - isPasswordEncrypted, + // Re-dispatches (retry/resume/auto-reconnect) omit the flag but carry the + // same in-memory password, so keep the existing marking. + isPasswordEncrypted: isPasswordEncrypted ?? existingConnection?.isPasswordEncrypted, autoConnect, ...(isRetry && existingConnection?.reconnect && { reconnect: existingConnection.reconnect, From 5a37f3da871f951ed3f229b428198076b7f6ed5d Mon Sep 17 00:00:00 2001 From: ravjotb Date: Fri, 18 Sep 2026 15:21:19 -0700 Subject: [PATCH 3/4] Carry the password marking through cluster-node password reuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cluster node connect flow reuses a sibling connection's stored password via selectEncryptedPassword, which returned only the password string. With the fail-closed change that value can now be plaintext (marked isPasswordEncrypted=false on its source connection), and the reuse dispatch targeted a new connectionId with no marking — so the persistence strip was bypassed and the plaintext could be written to disk as if encrypted. Rename the selector to selectClusterPassword and return the password together with its source connection's isPasswordEncrypted marking, and include that marking in the connectPending dispatch. Signed-off-by: ravjotb --- .../cluster-topology/cluster-node-row.tsx | 16 +++++++++------- .../connection/connectionSelectors.ts | 9 ++++++--- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/apps/frontend/src/components/cluster-topology/cluster-node-row.tsx b/apps/frontend/src/components/cluster-topology/cluster-node-row.tsx index 5f33623b..cad71dd8 100644 --- a/apps/frontend/src/components/cluster-topology/cluster-node-row.tsx +++ b/apps/frontend/src/components/cluster-topology/cluster-node-row.tsx @@ -22,7 +22,7 @@ import { getUtilizationLevel, type UtilizationLevel } from "@/state/valkey-featu import { connectPending, type ConnectionDetails } from "@/state/valkey-features/connection/connectionSlice.ts" import { useAppDispatch } from "@/hooks/hooks" import { - selectIsAtConnectionLimit, selectEncryptedPassword, selectClusterDb + selectIsAtConnectionLimit, selectClusterPassword, selectClusterDb } from "@/state/valkey-features/connection/connectionSelectors" import { secureStorage, PASSWORD_NOT_STORED_WARNING } from "@/utils/secureStorage.ts" import { cn } from "@/lib/utils" @@ -80,9 +80,9 @@ export function ClusterNodeRow({ const isDisabled = useSelector(selectIsAtConnectionLimit) - // Look up encrypted password from an existing connection in the same cluster. - // Available when secureStorage was active during the original connection. - const encryptedPassword = useSelector(selectEncryptedPassword(clusterId)) + // Look up a stored password from an existing connection in the same cluster, + // together with its isPasswordEncrypted marking. + const clusterPassword = useSelector(selectClusterPassword(clusterId)) const [showPasswordModal, setShowPasswordModal] = useState(false) @@ -110,15 +110,17 @@ export function ClusterNodeRow({ awsReplicationGroupId: primaryConfig.awsReplicationGroupId, }, })) - } else if (R.isNotNil(encryptedPassword)) { - // Password already encrypted from existing cluster connection — do NOT re-encrypt + } else if (R.isNotNil(clusterPassword)) { + // Reuse the sibling connection's stored password, carrying its + // isPasswordEncrypted marking so an unencrypted one is still never persisted. dispatch(connectPending({ connectionId, connectionDetails: { ...baseDetails, username: primaryConfig.username ?? "", - password: encryptedPassword, + password: clusterPassword.password, }, + isPasswordEncrypted: clusterPassword.isPasswordEncrypted, })) } else { // No stored password — prompt for password diff --git a/apps/frontend/src/state/valkey-features/connection/connectionSelectors.ts b/apps/frontend/src/state/valkey-features/connection/connectionSelectors.ts index 78b9c924..c38dd759 100644 --- a/apps/frontend/src/state/valkey-features/connection/connectionSelectors.ts +++ b/apps/frontend/src/state/valkey-features/connection/connectionSelectors.ts @@ -18,10 +18,13 @@ export const selectIsAnyConnecting = (state: RootState) => Object.values(selectConnections(state)).some((c) => c.status === CONNECTING) export const selectJsonModuleAvailable = (id: string) => (state: RootState) => atId(id, state)?.connectionDetails?.jsonModuleAvailable ?? false -export const selectEncryptedPassword = (clusterId: string) => (state: RootState) => - Object.values(state.valkeyConnection?.connections ?? {}).find( +export const selectClusterPassword = (clusterId: string) => (state: RootState) => { + const source = Object.values(state.valkeyConnection?.connections ?? {}).find( (c) => c.connectionDetails?.clusterId === clusterId && R.isNotNil(c.connectionDetails?.password), - )?.connectionDetails?.password + ) + if (!source) return undefined + return { password: source.connectionDetails.password, isPasswordEncrypted: source.isPasswordEncrypted } +} export const selectClusterDb = (clusterId: string) => (state: RootState) => Object.values(state.valkeyConnection?.connections ?? {}).find( From 29b1c14f04a46655e0e173f5144b5a7ac2ab0354 Mon Sep 17 00:00:00 2001 From: ravjotb Date: Fri, 18 Sep 2026 16:04:53 -0700 Subject: [PATCH 4/4] Treat absent password markings conservatively on edit and reuse Two more paths could persist a plaintext password because an absent isPasswordEncrypted marker is treated as safe: - A core-field edit with an unchanged password deletes the old connection and reconnects under a new connectionId with no marker, so the reducer has nothing to inherit from. Initialize the marker from the source connection when the password is unchanged. - selectClusterPassword could return a password whose source marker was absent (e.g. a connection restored from localStorage, including legacy plaintext persisted before this fix). Default the marker to false so reuse can never persist plaintext; a restored ciphertext reused this way just re-prompts on the next session instead of being saved. Signed-off-by: ravjotb --- apps/frontend/src/components/ui/edit-form.tsx | 4 +++- .../state/valkey-features/connection/connectionSelectors.ts | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/frontend/src/components/ui/edit-form.tsx b/apps/frontend/src/components/ui/edit-form.tsx index 9300ff88..ac392af6 100644 --- a/apps/frontend/src/components/ui/edit-form.tsx +++ b/apps/frontend/src/components/ui/edit-form.tsx @@ -147,7 +147,9 @@ function EditForm({ onClose, connectionId }: EditFormProps) { dispatch(deleteConnection({ connectionId, silent: true })) // Encrypt password only if user typed a new one; otherwise it's already encrypted from Redux - let isPasswordEncrypted: boolean | undefined + // and carries the source connection's marking (the connect below targets a new + // connectionId, so the reducer can't inherit it). + let isPasswordEncrypted = passwordChanged ? undefined : fullConnection?.isPasswordEncrypted let detailsToDispatch = trimmed if (passwordChanged && connectionDetails.password) { const result = await secureStorage.encryptForStorage(connectionDetails.password) diff --git a/apps/frontend/src/state/valkey-features/connection/connectionSelectors.ts b/apps/frontend/src/state/valkey-features/connection/connectionSelectors.ts index c38dd759..3d153278 100644 --- a/apps/frontend/src/state/valkey-features/connection/connectionSelectors.ts +++ b/apps/frontend/src/state/valkey-features/connection/connectionSelectors.ts @@ -23,7 +23,9 @@ export const selectClusterPassword = (clusterId: string) => (state: RootState) = (c) => c.connectionDetails?.clusterId === clusterId && R.isNotNil(c.connectionDetails?.password), ) if (!source) return undefined - return { password: source.connectionDetails.password, isPasswordEncrypted: source.isPasswordEncrypted } + // Conservative default: an absent marker (e.g. a connection restored from + // localStorage) is treated as unencrypted so reuse can never persist plaintext. + return { password: source.connectionDetails.password, isPasswordEncrypted: source.isPasswordEncrypted ?? false } } export const selectClusterDb = (clusterId: string) => (state: RootState) =>