feat(desktop): replace keychain storage with encrypted file storage#366
feat(desktop): replace keychain storage with encrypted file storage#366saddlepaddle merged 2 commits intomainfrom
Conversation
Replace Electron's safeStorage (which triggers macOS Keychain prompts) with custom AES-256-GCM encryption using machine-derived keys. - Add crypto-storage.ts with machine ID-based key derivation - Update token-storage.ts to use new encryption approach - Supports macOS (IOPlatformUUID), Linux (/etc/machine-id), Windows (MachineGuid) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughAdds a new machine-bound AES-256-GCM crypto module and replaces Electron safeStorage usage in token storage so tokens are consistently encrypted/decrypted using the new machine-derived key approach. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
apps/desktop/src/main/lib/auth/token-storage.ts (1)
26-34:load()swallows all errors—consider distinguishing “missing file” vs “corrupt/undecryptable”
Returningnullis fine for UX, but silently masking “corrupted file” makes support/debug harder. Consider narrowing the catch (e.g., missing file) and optionally logging a debug-level message for decryption/JSON failures (without including decrypted data).apps/desktop/src/main/lib/auth/crypto-storage.ts (3)
54-57: Fallback machine-id is predictable and may break decryptability if user/home changes
hostname()+homedir()can change (renamed host, moved home, domain join), causing permanent token loss. Consider generating a random “device key” once and storing it in~/.superset/device-keywith0600perms as the fallback input toscrypt(stability improves; security is not worse than the current fallback in the realistic “disk is readable” threat model).
62-65:scryptSyncon the main thread may be noticeable; consider async
If this runs in the Electron main process during startup, considerscrypt(async) to avoid blocking, or at least leave a comment that this is sync-by-design and should be revisited if startup regressions appear.
71-85: Add a small format header/version to support future migrations
Right now the file is “just bytes”; adding a magic + version (even 5–8 bytes) makes it much easier to rotate algorithms/params safely later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/main/lib/auth/crypto-storage.ts(1 hunks)apps/desktop/src/main/lib/auth/token-storage.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/lib/auth/crypto-storage.tsapps/desktop/src/main/lib/auth/token-storage.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/lib/auth/crypto-storage.tsapps/desktop/src/main/lib/auth/token-storage.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/main/lib/auth/crypto-storage.tsapps/desktop/src/main/lib/auth/token-storage.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/main/lib/auth/crypto-storage.tsapps/desktop/src/main/lib/auth/token-storage.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/auth/token-storage.ts (2)
apps/desktop/src/main/lib/auth/crypto-storage.ts (2)
encrypt(71-85)decrypt(90-108)apps/desktop/src/shared/auth.ts (1)
AuthSession(6-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Deploy Web
- GitHub Check: Deploy Admin
- GitHub Check: Build
| import { execSync } from "node:child_process"; | ||
| import { | ||
| createCipheriv, | ||
| createDecipheriv, | ||
| randomBytes, | ||
| scryptSync, | ||
| } from "node:crypto"; | ||
| import { homedir, hostname, platform } from "node:os"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electron main process best practices execSync vs execFileSync spawnSync blocking
💡 Result:
Short guidance for Electron main (concise):
-
Don’t block the main/renderer event loop. All synchronous child_process variants (execSync, execFileSync, spawnSync) block the Node/Electron event loop — avoid them except for short startup scripts. [1][2]
-
Prefer async APIs:
- Use spawn / execFile / fork (async) so you can stream stdout/stderr, set timeouts, and handle errors via callbacks/events. Use spawn for large/streaming output, execFile for running binaries without a shell, fork for Node child processes with IPC. [1][6]
-
Use execFile over exec when you don’t need a shell (safer vs shell injection and faster). Use spawn when you need streaming or continuous output (e.g., ffmpeg). [1][6]
-
If you must run blocking work, move it off the main process:
- Spawn a dedicated helper process (child process or separate binary) or use Worker Threads / a background Node helper to avoid freezing UI. Use IPC (or Electron’s built-in messaging) to communicate results. [1][9]
-
Control stdio and output size to avoid deadlocks: stream output or set stdio/options; synchronous calls buffer output and can block if buffers fill. Use {stdio:'ignore'} or streaming via spawn when output is large. [1]
-
Security & packaging notes: avoid shell=true unless needed; ASAR packaging can affect how binaries are executed — execFile is ASAR-friendly; check Electron docs/issues when packaging. [3]
-
Error handling & timeouts: always handle exit/error/timeout and sanitize args when invoking external commands. Prefer explicit paths and set env/cwd options. [1][6]
If you want, I can:
- show a small code example migrating execSync -> spawn with streaming and timeout, or
- review specific code you have and recommend changes.
🌐 Web query:
Electron official documentation avoiding blocking operations main process child_process
💡 Result:
Short answer — follow Electron's guidance: never run blocking/synchronous work in the main process; use async Node APIs, worker threads, renderer-side workers, or spawn separate processes (prefer Electron's UtilityProcess or async child_process APIs) and avoid sync child_process/Sync and synchronous IPC. References:
- Electron performance guide: avoid blocking the main process; prefer async I/O, worker threads or spawning dedicated processes; avoid synchronous IPC and sync Node APIs. [1]
- UtilityProcess API: a recommended, Electron-provided child-process API (utilityProcess.fork) for offloading CPU‑heavy or crash‑prone tasks from the main process. [2]
- Electron process model: describes the utility process and using it instead of forking from main when appropriate. [3]
- Node.js child_process docs: the Sync variants (spawnSync, execSync, execFileSync) block the event loop — do not use them in the main process. [4]
- Practical caveats about spawn/fork in Electron (ASAR, ELECTRON_RUN_AS_NODE, renderer vs main spawning) — be careful which process you spawn from and prefer utilityProcess or worker threads when you need Electron integration. [5]
If you want, I can extract specific code examples (utilityProcess.fork, worker_threads, non‑blocking child_process.spawn with stdio handling) for your use case.
Avoid execSync in Electron main process and use async alternatives instead
execSync blocks the main process event loop and will freeze the UI. Per Electron guidance, avoid all synchronous child_process variants (execSync, execFileSync, spawnSync). Use async alternatives like spawn() or execFile() instead. If this operation is performance-critical on startup, consider moving it to a Worker Thread or UtilityProcess to keep the main process responsive.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/auth/crypto-storage.ts around lines 1 to 8, the use
of execSync blocks the Electron main process; replace synchronous child_process
usage with an asynchronous alternative (e.g., spawn/execFile wrapped in a
Promise or util.promisify(execFile)) and await the result, handle errors via
callbacks/promises, and ensure any CPU-bound work is offloaded to a Worker
Thread or UtilityProcess if it runs on startup to avoid freezing the UI; update
import and call sites to the async API and add proper try/catch and timeout
handling.
| /** | ||
| * Securely stores authentication session using Electron's safeStorage API | ||
| * Session data is encrypted at rest using the OS keychain | ||
| * Securely stores authentication session using machine-derived encryption. | ||
| * Session data is encrypted at rest using AES-256-GCM with a key derived | ||
| * from the machine's hardware identifier. | ||
| */ |
There was a problem hiding this comment.
Docstring overstates security guarantees (“Securely stores…”)
Given the key is derived from machine identifiers (and may fall back to predictable properties), consider rewording to “encrypts at rest” / “discourages casual inspection” to avoid implying keychain/HSM-like protection.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/auth/token-storage.ts around lines 9 to 13, the
docstring overstates security by claiming "Securely stores…", so update the
comment to remove absolute security claims and accurately describe the
protection: state that session data is encrypted at rest using AES-256-GCM with
a key derived from machine identifiers (and may fallback to less-unique
properties), and replace wording with safer phrasing such as "encrypts at rest"
or "helps discourage casual inspection" and explicitly note it is not equivalent
to keychain/HSM protection.
| async save(session: AuthSession): Promise<void> { | ||
| if (!safeStorage.isEncryptionAvailable()) { | ||
| console.warn( | ||
| "[auth] Secure storage not available, session will not be persisted", | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const encrypted = safeStorage.encryptString(JSON.stringify(session)); | ||
| const encrypted = encrypt(JSON.stringify(session)); | ||
| await fs.writeFile(this.filePath, encrypted); | ||
| } |
There was a problem hiding this comment.
Harden save(): ensure dir exists, set restrictive perms, and avoid partial writes
Right now writeFile() may fail if SUPERSET_HOME_DIR doesn’t exist, will use default perms, and can leave a truncated file on crash/power loss. Consider mkdir + mode: 0o600 + atomic temp+rename.
async save(session: AuthSession): Promise<void> {
+ await fs.mkdir(SUPERSET_HOME_DIR, { recursive: true });
const encrypted = encrypt(JSON.stringify(session));
- await fs.writeFile(this.filePath, encrypted);
+ const tmpPath = `${this.filePath}.tmp`;
+ await fs.writeFile(tmpPath, encrypted, { mode: 0o600 });
+ // Best-effort atomic replace (Windows rename doesn’t overwrite)
+ await fs.rm(this.filePath, { force: true }).catch(() => {});
+ await fs.rename(tmpPath, this.filePath);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async save(session: AuthSession): Promise<void> { | |
| if (!safeStorage.isEncryptionAvailable()) { | |
| console.warn( | |
| "[auth] Secure storage not available, session will not be persisted", | |
| ); | |
| return; | |
| } | |
| const encrypted = safeStorage.encryptString(JSON.stringify(session)); | |
| const encrypted = encrypt(JSON.stringify(session)); | |
| await fs.writeFile(this.filePath, encrypted); | |
| } | |
| async save(session: AuthSession): Promise<void> { | |
| await fs.mkdir(SUPERSET_HOME_DIR, { recursive: true }); | |
| const encrypted = encrypt(JSON.stringify(session)); | |
| const tmpPath = `${this.filePath}.tmp`; | |
| await fs.writeFile(tmpPath, encrypted, { mode: 0o600 }); | |
| // Best-effort atomic replace (Windows rename doesn't overwrite) | |
| await fs.rm(this.filePath, { force: true }).catch(() => {}); | |
| await fs.rename(tmpPath, this.filePath); | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/auth/token-storage.ts around lines 21-24, the
save(session) implementation should ensure the parent directory exists, use
restrictive file permissions, and perform an atomic write to avoid
partial/truncated files: create the SUPERSET_HOME_DIR (mkdir with recursive
true) before writing, serialize and encrypt the session to a temp file placed in
the same directory (unique temp name), write the temp file with mode 0o600,
fsync the file handle after write, rename the temp file to the final filePath to
atomically replace it, and optionally fsync the parent directory to ensure the
rename is persisted; handle and surface errors appropriately.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
- Add data length validation in decrypt() to reject truncated buffers - Use execFileSync instead of execSync (safer, no shell injection) - Use static import for readFileSync instead of require() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/desktop/src/main/lib/auth/crypto-storage.ts (3)
22-64: Recommended: Add error logging and validate machine IDs for debuggability.Empty catch blocks swallow errors silently, making it impossible to debug why platform-specific methods fail. Also, there's no validation that machine IDs are non-empty before returning them.
function getMachineId(): string { try { const os = platform(); if (os === "darwin") { // macOS: Use IOPlatformUUID (hardware UUID) - const output = execFileSync( - "ioreg", - ["-rd1", "-c", "IOPlatformExpertDevice"], - { encoding: "utf8" }, - ); + let output: string; + try { + output = execFileSync( + "ioreg", + ["-rd1", "-c", "IOPlatformExpertDevice"], + { encoding: "utf8" }, + ); + } catch (err) { + console.error("[crypto-storage] ioreg failed:", err); + throw err; + } const match = output.match(/"IOPlatformUUID"\s*=\s*"([^"]+)"/); - if (match?.[1]) return match[1]; + const uuid = match?.[1]; + if (uuid && uuid.length > 0) return uuid; + console.error("[crypto-storage] IOPlatformUUID not found in ioreg output"); } else if (os === "linux") { // Linux: Use machine-id try { - return readFileSync("/etc/machine-id", "utf8").trim(); + const id = readFileSync("/etc/machine-id", "utf8").trim(); + if (id.length > 0) return id; } catch { - return readFileSync("/var/lib/dbus/machine-id", "utf8").trim(); + const id = readFileSync("/var/lib/dbus/machine-id", "utf8").trim(); + if (id.length > 0) return id; } + console.error("[crypto-storage] machine-id files not found or empty"); } else if (os === "win32") { // Similar logging for Windows... } - } catch { + } catch (err) { + console.error("[crypto-storage] Failed to get machine ID:", err); // Fallback if platform-specific method fails }
69-72: Optional: Add salt validation for defensive coding.
deriveKeydoesn't validate that the salt is non-empty or of the expected length. While the current callers always pass valid salts, defensive validation would make this safer to refactor later.function deriveKey(salt: Buffer): Buffer { + if (salt.length !== SALT_LENGTH) { + throw new Error(`Invalid salt length: expected ${SALT_LENGTH}, got ${salt.length}`); + } const machineId = getMachineId(); + if (!machineId || machineId.length === 0) { + throw new Error("Machine ID is empty"); + } return scryptSync(machineId, salt, KEY_LENGTH); }
99-121: Recommended: Add try-catch for clearer decryption failure messages.If the auth tag verification fails or the ciphertext is corrupted,
decipher.final()throws a generic crypto error. Wrapping in try-catch would let you provide a more user-friendly message (e.g., "Failed to decrypt: data may be corrupted or encrypted with a different machine ID").export function decrypt(data: Buffer): string { if (data.length < MIN_ENCRYPTED_LENGTH) { throw new Error("Encrypted data too short"); } // Extract components const salt = data.subarray(0, SALT_LENGTH); const iv = data.subarray(SALT_LENGTH, SALT_LENGTH + IV_LENGTH); const authTag = data.subarray( SALT_LENGTH + IV_LENGTH, SALT_LENGTH + IV_LENGTH + AUTH_TAG_LENGTH, ); const ciphertext = data.subarray(SALT_LENGTH + IV_LENGTH + AUTH_TAG_LENGTH); const key = deriveKey(salt); const decipher = createDecipheriv(ALGORITHM, key, iv); decipher.setAuthTag(authTag); + try { return Buffer.concat([ decipher.update(ciphertext), decipher.final(), ]).toString("utf8"); + } catch (err) { + throw new Error( + "Failed to decrypt: data may be corrupted or encrypted with a different machine ID", + { cause: err } + ); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/main/lib/auth/crypto-storage.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/lib/auth/crypto-storage.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/lib/auth/crypto-storage.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/main/lib/auth/crypto-storage.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/main/lib/auth/crypto-storage.ts
🧠 Learnings (2)
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx,js,jsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/main/lib/auth/crypto-storage.ts
📚 Learning: 2025-12-12T05:45:09.686Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.686Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx,js,jsx} apps/desktop/src/lib/**/*.{ts,tsx,js,jsx} apps/desktop/src/shared/**/*.{ts,tsx,js,tsx} : Never import Node.js modules in renderer process or shared code, only in main process
Applied to files:
apps/desktop/src/main/lib/auth/crypto-storage.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/main/lib/auth/crypto-storage.ts (2)
78-92: LGTM: Encryption implementation is cryptographically sound.The function correctly:
- Generates random salt and IV for each encryption (prevents key/IV reuse)
- Uses AES-256-GCM with proper auth tag handling
- Returns a deterministic buffer format for decryption
100-102: Good: Buffer length validation addresses previous review concern.The explicit length check prevents parsing truncated buffers and provides a clear error message.
| import { execFileSync } from "node:child_process"; | ||
| import { | ||
| createCipheriv, | ||
| createDecipheriv, | ||
| randomBytes, | ||
| scryptSync, | ||
| } from "node:crypto"; | ||
| import { readFileSync } from "node:fs"; | ||
| import { homedir, hostname, platform } from "node:os"; |
There was a problem hiding this comment.
Critical: Still using blocking sync APIs in Electron main process despite previous review.
The previous review flagged execSync blocking issues, but the code still uses synchronous APIs (execFileSync, readFileSync, scryptSync). In particular:
scryptSyncis CPU-intensive key derivation that will freeze the UI for 100ms+execFileSyncblocks during subprocess executionreadFileSyncblocks during disk I/O
These will be called on every encrypt/decrypt operation, causing noticeable UI freezes.
Migrate to async APIs:
-import { execFileSync } from "node:child_process";
+import { execFile } from "node:child_process";
+import { promisify } from "node:util";
import {
createCipheriv,
createDecipheriv,
randomBytes,
- scryptSync,
+ scrypt,
} from "node:crypto";
-import { readFileSync } from "node:fs";
+import { readFile } from "node:fs/promises";
import { homedir, hostname, platform } from "node:os";
+
+const execFileAsync = promisify(execFile);
+const scryptAsync = promisify(scrypt);Then update getMachineId(), deriveKey(), encrypt(), and decrypt() to be async and return Promises. This is essential for Electron main process responsiveness.
Based on learnings, Electron IPC should use tRPC, so ensure the consumers of this module (token-storage.ts) expose async tRPC procedures.
Committable suggestion skipped: line range outside the PR's diff.
| // Fallback: Use a combination of stable system properties | ||
| // This is less secure but ensures the app still works | ||
| return `${hostname()}-${homedir()}-superset-fallback`; |
There was a problem hiding this comment.
Major: Fallback machine ID is weak and causes data loss on hostname/homedir changes.
The fallback ${hostname()}-${homedir()}-superset-fallback has two problems:
- Security: hostname is user-settable and homedir is easily discoverable, providing minimal protection compared to hardware UUIDs
- Data loss: If a user renames their machine or moves their home directory, all stored tokens become permanently inaccessible
Consider:
- Adding persistent storage of the derived machine ID on first run
- Or failing loudly rather than silently falling back to weak identifiers
- Or generating and storing a random UUID on first launch as the stable machine ID
// Fallback: Use a combination of stable system properties
- // This is less secure but ensures the app still works
- return `${hostname()}-${homedir()}-superset-fallback`;
+ // Generate and persist a stable random ID on first run
+ // (implementation would check for ~/.superset/machine-id and create if missing)
+ throw new Error(
+ "Unable to determine machine identifier. Platform-specific commands failed."
+ );Committable suggestion skipped: line range outside the PR's diff.
1 participant
Summary
safeStorageAPI with custom AES-256-GCM encryptionTest plan
~/.superset/auth-session.enc🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.