From 5af7b5c32fb51a58249d622ccc0750e77aa858ef Mon Sep 17 00:00:00 2001 From: Sucukdeluxe Date: Tue, 11 Aug 2026 21:50:02 +0200 Subject: [PATCH] fix(security): purge credentials from settings backups Write the newly protected settings payload to both sync and async config backups so disabling credential persistence or losing encryption availability cannot leave previously stored provider secrets behind. Replace content-based prefix detection with a typed, versioned safeStorage envelope, preserve arbitrary legacy plaintext for migration, and emit field-only diagnostics for encryption failures without exposing credentials or ciphertext. Add regression coverage for both save paths, both persistence-off conditions, and prefix-shaped plaintext values. --- src/main/credential-protection.ts | 63 ++++++++++++++++++++------- src/main/storage.ts | 67 +++++++++++++++++------------ tests/credential-protection.test.ts | 54 ++++++++++++++++++++++- tests/storage.test.ts | 54 ++++++++++++++++++++++- 4 files changed, 192 insertions(+), 46 deletions(-) diff --git a/src/main/credential-protection.ts b/src/main/credential-protection.ts index 66673b0..ad2d225 100644 --- a/src/main/credential-protection.ts +++ b/src/main/credential-protection.ts @@ -1,4 +1,5 @@ import { AppSettings } from "../shared/types"; +import { logger } from "./logger"; export interface CredentialProtector { isEncryptionAvailable(): boolean; @@ -6,7 +7,6 @@ export interface CredentialProtector { decryptString(value: Buffer): string; } -const PROTECTED_VALUE_PREFIX = "mdd-safe-storage:v1:"; const MASKED_CREDENTIAL = "••••••••"; const CREDENTIAL_KEYS = [ "token", @@ -24,6 +24,17 @@ const CREDENTIAL_KEYS = [ "linkSnappyLogin", "linkSnappyPassword" ] as const satisfies readonly (keyof AppSettings)[]; +type CredentialKey = typeof CREDENTIAL_KEYS[number]; + +interface PersistedCredentialEnvelope { + type: "safe-storage"; + version: 1; + payload: string; +} + +export type PersistedAppSettings = Omit & { + [K in CredentialKey]: string | PersistedCredentialEnvelope; +}; let credentialProtector: CredentialProtector = { isEncryptionAvailable: () => false, @@ -39,12 +50,20 @@ function isEncryptionAvailable(): boolean { try { return credentialProtector.isEncryptionAvailable(); } catch { + logger.warn("Credential-Verschlüsselungsverfügbarkeit konnte nicht ermittelt werden"); return false; } } -function isProtectedValue(value: string): boolean { - return value.startsWith(PROTECTED_VALUE_PREFIX); +function isPersistedCredentialEnvelope(value: unknown): value is PersistedCredentialEnvelope { + if (!value || typeof value !== "object" || Array.isArray(value)) { + return false; + } + const envelope = value as Partial; + return envelope.type === "safe-storage" + && envelope.version === 1 + && typeof envelope.payload === "string" + && envelope.payload.length > 0; } function clearCredentials(settings: AppSettings): AppSettings { @@ -55,61 +74,73 @@ function clearCredentials(settings: AppSettings): AppSettings { return cleared; } -export function protectPersistedSettings(settings: AppSettings): AppSettings { +export function protectPersistedSettings(settings: AppSettings): PersistedAppSettings { if (settings.rememberToken === false || !isEncryptionAvailable()) { return clearCredentials(settings); } - const protectedSettings = { ...settings }; + const protectedSettings = { ...settings } as PersistedAppSettings; for (const key of CREDENTIAL_KEYS) { const value = typeof settings[key] === "string" ? settings[key] : ""; - if (!value || isProtectedValue(value)) { + if (!value) { protectedSettings[key] = value; continue; } try { - protectedSettings[key] = `${PROTECTED_VALUE_PREFIX}${credentialProtector.encryptString(value).toString("base64")}`; + protectedSettings[key] = { + type: "safe-storage", + version: 1, + payload: credentialProtector.encryptString(value).toString("base64") + }; } catch { protectedSettings[key] = ""; + logger.warn(`Credential-Verschlüsselung fehlgeschlagen: ${key}`); } } return protectedSettings; } -export function restorePersistedSettings(settings: AppSettings): AppSettings { +export function restorePersistedSettings(settings: AppSettings | PersistedAppSettings): AppSettings { if (settings.rememberToken === false) { - return clearCredentials(settings); + return clearCredentials(settings as AppSettings); } - const restored = { ...settings }; + const restored = { ...settings } as AppSettings; for (const key of CREDENTIAL_KEYS) { - const value = typeof settings[key] === "string" ? settings[key] : ""; - if (!isProtectedValue(value)) { + const value = settings[key]; + if (typeof value === "string") { restored[key] = value; continue; } + if (!isPersistedCredentialEnvelope(value)) { + restored[key] = ""; + continue; + } if (!isEncryptionAvailable()) { restored[key] = ""; continue; } try { - restored[key] = credentialProtector.decryptString(Buffer.from(value.slice(PROTECTED_VALUE_PREFIX.length), "base64")); + restored[key] = credentialProtector.decryptString(Buffer.from(value.payload, "base64")); } catch { restored[key] = ""; + logger.warn(`Credential-Entschlüsselung fehlgeschlagen: ${key}`); } } return restored; } -export function needsPersistedSettingsRewrite(settings: AppSettings): boolean { - const values = CREDENTIAL_KEYS.map((key) => typeof settings[key] === "string" ? settings[key] : "").filter(Boolean); +export function needsPersistedSettingsRewrite(settings: AppSettings | PersistedAppSettings): boolean { + const values = CREDENTIAL_KEYS.map((key) => settings[key]).filter((value) => { + return typeof value === "string" ? value.length > 0 : value !== null && value !== undefined; + }); if (values.length === 0) { return false; } if (settings.rememberToken === false || !isEncryptionAvailable()) { return true; } - return values.some((value) => !isProtectedValue(value)); + return values.some((value) => !isPersistedCredentialEnvelope(value)); } export function projectSettingsForRenderer(settings: AppSettings): AppSettings { diff --git a/src/main/storage.ts b/src/main/storage.ts index 60eaf2c..a1f197e 100644 --- a/src/main/storage.ts +++ b/src/main/storage.ts @@ -1050,40 +1050,51 @@ function readSessionFile(filePath: string): SessionState | null { } export function saveSettings(paths: StoragePaths, settings: AppSettings): void { - syncSettingsSaveGeneration += 1; - ensureBaseDir(paths.baseDir); - if (fs.existsSync(paths.configFile)) { - try { - fs.copyFileSync(paths.configFile, `${paths.configFile}.bak`); - } catch { - } - } + syncSettingsSaveGeneration += 1; + ensureBaseDir(paths.baseDir); const payload = settingsPayload(settings); - const tempPath = `${paths.configFile}.tmp`; - try { - fs.writeFileSync(tempPath, payload, "utf8"); - syncRenameWithExdevFallback(tempPath, paths.configFile); - } catch (error) { - try { fs.rmSync(tempPath, { force: true }); } catch { } - throw error; - } -} + if (fs.existsSync(paths.configFile)) { + writeSettingsFileAtomically(`${paths.configFile}.bak`, payload); + } + writeSettingsFileAtomically(paths.configFile, payload); +} let asyncSettingsSaveRunning = false; let asyncSettingsSaveQueued: { paths: StoragePaths; payload: string; generation: number } | null = null; let syncSettingsSaveGeneration = 0; -async function writeSettingsPayload(paths: StoragePaths, payload: string, generation: number): Promise { - await fs.promises.mkdir(paths.baseDir, { recursive: true }); - await fsp.copyFile(paths.configFile, `${paths.configFile}.bak`).catch(() => {}); - const tempPath = `${paths.configFile}.settings.tmp`; - await fsp.writeFile(tempPath, payload, "utf8"); - if (generation < syncSettingsSaveGeneration) { - await fsp.rm(tempPath, { force: true }).catch(() => {}); - return; - } - try { - await fsp.rename(tempPath, paths.configFile); +async function writeSettingsPayload(paths: StoragePaths, payload: string, generation: number): Promise { + await fs.promises.mkdir(paths.baseDir, { recursive: true }); + const tempPath = `${paths.configFile}.settings.tmp`; + await fsp.writeFile(tempPath, payload, "utf8"); + if (generation < syncSettingsSaveGeneration) { + await fsp.rm(tempPath, { force: true }).catch(() => {}); + return; + } + if (fs.existsSync(paths.configFile)) { + const backupTempPath = `${paths.configFile}.bak.settings.tmp`; + await fsp.writeFile(backupTempPath, payload, "utf8"); + if (generation < syncSettingsSaveGeneration) { + await Promise.all([ + fsp.rm(tempPath, { force: true }).catch(() => {}), + fsp.rm(backupTempPath, { force: true }).catch(() => {}) + ]); + return; + } + try { + await fsp.rename(backupTempPath, `${paths.configFile}.bak`); + } catch (renameError: unknown) { + if (renameError && typeof renameError === "object" && "code" in renameError && (renameError as NodeJS.ErrnoException).code === "EXDEV") { + await fsp.copyFile(backupTempPath, `${paths.configFile}.bak`); + await fsp.rm(backupTempPath, { force: true }).catch(() => {}); + } else { + await fsp.rm(backupTempPath, { force: true }).catch(() => {}); + throw renameError; + } + } + } + try { + await fsp.rename(tempPath, paths.configFile); } catch (renameError: unknown) { if (renameError && typeof renameError === "object" && "code" in renameError && (renameError as NodeJS.ErrnoException).code === "EXDEV") { if (generation < syncSettingsSaveGeneration) { diff --git a/tests/credential-protection.test.ts b/tests/credential-protection.test.ts index 076fe09..0f64f8c 100644 --- a/tests/credential-protection.test.ts +++ b/tests/credential-protection.test.ts @@ -1,5 +1,6 @@ -import { beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { defaultSettings } from "../src/main/constants"; +import { logger } from "../src/main/logger"; import { configureCredentialProtector, CredentialProtector, @@ -21,6 +22,10 @@ describe("credential protection", () => { configureCredentialProtector(createProtector()); }); + afterEach(() => { + vi.restoreAllMocks(); + }); + it("protects remembered provider values and restores them for the main process", () => { const input = { ...defaultSettings(), @@ -53,6 +58,53 @@ describe("credential protection", () => { expect(protectPersistedSettings(restorePersistedSettings(input)).token).not.toBe(input.token); }); + it("protects plaintext credentials that begin with the legacy marker text", () => { + const input = { + ...defaultSettings(), + rememberToken: true, + token: "mdd-safe-storage:v1:literal-credential" + }; + + const persisted = protectPersistedSettings(input); + + expect(persisted.token).not.toBe(input.token); + expect(JSON.stringify(persisted)).not.toContain(input.token); + expect(restorePersistedSettings(persisted).token).toBe(input.token); + }); + + it("reports encryption failures without logging credential data", () => { + const value = "credential-value-not-for-logs"; + const warn = vi.spyOn(logger, "warn").mockImplementation(() => {}); + configureCredentialProtector({ + isEncryptionAvailable: () => true, + encryptString: () => { throw new Error("encryption failed"); }, + decryptString: () => "" + }); + + const persisted = protectPersistedSettings({ ...defaultSettings(), token: value }); + + expect(persisted.token).toBe(""); + expect(warn).toHaveBeenCalledWith("Credential-Verschlüsselung fehlgeschlagen: token"); + expect(JSON.stringify(warn.mock.calls)).not.toContain(value); + }); + + it("reports decryption failures without logging persisted data", () => { + const persisted = protectPersistedSettings({ ...defaultSettings(), token: "value-to-encrypt" }); + const serialized = JSON.stringify(persisted.token); + const warn = vi.spyOn(logger, "warn").mockImplementation(() => {}); + configureCredentialProtector({ + isEncryptionAvailable: () => true, + encryptString: () => Buffer.alloc(0), + decryptString: () => { throw new Error("decryption failed"); } + }); + + const restored = restorePersistedSettings(persisted); + + expect(restored.token).toBe(""); + expect(warn).toHaveBeenCalledWith("Credential-Entschlüsselung fehlgeschlagen: token"); + expect(JSON.stringify(warn.mock.calls)).not.toContain(serialized); + }); + it("does not persist provider values when encryption is unavailable", () => { configureCredentialProtector(createProtector(false)); const persisted = protectPersistedSettings({ diff --git a/tests/storage.test.ts b/tests/storage.test.ts index e13a9c7..d441b57 100644 --- a/tests/storage.test.ts +++ b/tests/storage.test.ts @@ -8,7 +8,7 @@ import { getProviderUsageDayKey } from "../src/shared/provider-daily-limits"; import { AppSettings } from "../src/shared/types"; import { defaultSettings } from "../src/main/constants"; import { configureCredentialProtector } from "../src/main/credential-protection"; -import { addHistoryEntryForRetention, createStoragePaths, emptySession, loadHistory, loadHistoryForRetention, loadSession, loadSettings, normalizeLoadedSession, normalizeSettings, resetHistoryForRetention, saveHistory, saveSession, saveSessionAsync, saveSettings } from "../src/main/storage"; +import { addHistoryEntryForRetention, createStoragePaths, emptySession, loadHistory, loadHistoryForRetention, loadSession, loadSettings, normalizeLoadedSession, normalizeSettings, resetHistoryForRetention, saveHistory, saveSession, saveSessionAsync, saveSettings, saveSettingsAsync } from "../src/main/storage"; const tempDirs: string[] = []; @@ -195,6 +195,58 @@ describe("settings storage", () => { expect(loaded.allDebridToken).toBe("all-token"); }); + it.each(["sync", "async"] as const)("clears previously stored credentials from the backup when remembering is disabled during a %s save", async (mode) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "rd-store-")); + tempDirs.push(dir); + const paths = createStoragePaths(dir); + const remembered = { + ...defaultSettings(), + rememberToken: true, + token: "stored-value-before-clear" + }; + saveSettings(paths, remembered); + + const cleared = { ...remembered, rememberToken: false }; + if (mode === "sync") { + saveSettings(paths, cleared); + } else { + await saveSettingsAsync(paths, cleared); + } + + const primary = JSON.parse(fs.readFileSync(paths.configFile, "utf8")) as Record; + const backup = JSON.parse(fs.readFileSync(`${paths.configFile}.bak`, "utf8")) as Record; + expect(primary.token).toBe(""); + expect(backup.token).toBe(""); + }); + + it.each(["sync", "async"] as const)("clears previously stored credentials from the backup when encryption is unavailable during a %s save", async (mode) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "rd-store-")); + tempDirs.push(dir); + const paths = createStoragePaths(dir); + const remembered = { + ...defaultSettings(), + rememberToken: true, + token: "stored-value-before-unavailable" + }; + saveSettings(paths, remembered); + configureCredentialProtector({ + isEncryptionAvailable: () => false, + encryptString: () => Buffer.alloc(0), + decryptString: () => "" + }); + + if (mode === "sync") { + saveSettings(paths, remembered); + } else { + await saveSettingsAsync(paths, remembered); + } + + const primary = JSON.parse(fs.readFileSync(paths.configFile, "utf8")) as Record; + const backup = JSON.parse(fs.readFileSync(`${paths.configFile}.bak`, "utf8")) as Record; + expect(primary.token).toBe(""); + expect(backup.token).toBe(""); + }); + it("migrates remembered plaintext provider values without retaining plaintext in config backups", () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), "rd-store-")); tempDirs.push(dir);