Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions apps/desktop/src/main/lib/auth/crypto-storage.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
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";
Comment on lines +1 to +9
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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:

  • scryptSync is CPU-intensive key derivation that will freeze the UI for 100ms+
  • execFileSync blocks during subprocess execution
  • readFileSync blocks 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.


const ALGORITHM = "aes-256-gcm";
const KEY_LENGTH = 32;
const SALT_LENGTH = 16;
const IV_LENGTH = 12;
const AUTH_TAG_LENGTH = 16;

/**
* Gets a stable machine identifier for key derivation.
* This provides "good enough" protection for local credential storage
* without requiring OS keychain access.
*/
function getMachineId(): string {
try {
const os = platform();

if (os === "darwin") {
// macOS: Use IOPlatformUUID (hardware UUID)
const output = execFileSync(
"ioreg",
["-rd1", "-c", "IOPlatformExpertDevice"],
{ encoding: "utf8" },
);
const match = output.match(/"IOPlatformUUID"\s*=\s*"([^"]+)"/);
if (match?.[1]) return match[1];
} else if (os === "linux") {
// Linux: Use machine-id
try {
return readFileSync("/etc/machine-id", "utf8").trim();
} catch {
return readFileSync("/var/lib/dbus/machine-id", "utf8").trim();
}
} else if (os === "win32") {
// Windows: Use MachineGuid from registry
const output = execFileSync(
"reg",
[
"query",
"HKLM\\SOFTWARE\\Microsoft\\Cryptography",
"/v",
"MachineGuid",
],
{ encoding: "utf8" },
);
const match = output.match(/MachineGuid\s+REG_SZ\s+(\S+)/);
if (match?.[1]) return match[1];
}
} catch {
// Fallback if platform-specific method fails
}

// Fallback: Use a combination of stable system properties
// This is less secure but ensures the app still works
return `${hostname()}-${homedir()}-superset-fallback`;
Comment on lines +61 to +63
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Major: Fallback machine ID is weak and causes data loss on hostname/homedir changes.

The fallback ${hostname()}-${homedir()}-superset-fallback has two problems:

  1. Security: hostname is user-settable and homedir is easily discoverable, providing minimal protection compared to hardware UUIDs
  2. 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.

}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Derives an encryption key from the machine ID and a salt.
*/
function deriveKey(salt: Buffer): Buffer {
const machineId = getMachineId();
return scryptSync(machineId, salt, KEY_LENGTH);
}

/**
* Encrypts a string using AES-256-GCM with a machine-derived key.
* Returns: salt (16) + iv (12) + authTag (16) + ciphertext
*/
export function encrypt(plaintext: string): Buffer {
const salt = randomBytes(SALT_LENGTH);
const key = deriveKey(salt);
const iv = randomBytes(IV_LENGTH);

const cipher = createCipheriv(ALGORITHM, key, iv);
const encrypted = Buffer.concat([
cipher.update(plaintext, "utf8"),
cipher.final(),
]);
const authTag = cipher.getAuthTag();

// Combine all components: salt + iv + authTag + ciphertext
return Buffer.concat([salt, iv, authTag, encrypted]);
}

const MIN_ENCRYPTED_LENGTH = SALT_LENGTH + IV_LENGTH + AUTH_TAG_LENGTH + 1;

/**
* Decrypts data encrypted with the encrypt function.
*/
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);

return Buffer.concat([
decipher.update(ciphertext),
decipher.final(),
]).toString("utf8");
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
22 changes: 6 additions & 16 deletions apps/desktop/src/main/lib/auth/token-storage.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import fs from "node:fs/promises";
import { join } from "node:path";
import { safeStorage } from "electron";
import type { AuthSession } from "shared/auth";
import { SUPERSET_HOME_DIR } from "../app-environment";
import { decrypt, encrypt } from "./crypto-storage";

const SESSION_FILE_NAME = "auth-session.enc";

/**
* 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.
*/
Comment on lines 9 to 13
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

class TokenStorage {
private readonly filePath: string;
Expand All @@ -18,25 +19,14 @@ class TokenStorage {
}

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);
}
Comment on lines 21 to 24
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.


async load(): Promise<AuthSession | null> {
if (!safeStorage.isEncryptionAvailable()) {
return null;
}

try {
const encrypted = await fs.readFile(this.filePath);
const decrypted = safeStorage.decryptString(encrypted);
const decrypted = decrypt(encrypted);
return JSON.parse(decrypted) as AuthSession;
} catch {
// File doesn't exist or can't be decrypted
Expand Down