fix(security): 2 improvements across 2 files

[tomaioo]

Summary

fix(security): 2 improvements across 2 files

Problem

Severity: Medium | File: apps/desktop/src/main/imports/safe-read.ts:L34

safeReadImportFile performs stat(path) to validate file type and size, then separately calls readFile(path). An attacker with local write access to the same filesystem location can race-replace the file between these calls (e.g., swap a small regular file for a FIFO/device/very large file), bypassing the earlier validation and potentially causing hangs or memory pressure in the main process.

Solution

Open the file once and validate/read via the same file descriptor to avoid path re-resolution races. For example: const fh = await open(path, O_RDONLY | O_NOFOLLOW) (where supported), then const st = await fh.stat(), enforce st.isFile() and size cap, then await fh.readFile('utf8'), finally close handle in finally.

Changes

• apps/desktop/src/main/imports/safe-read.ts (modified)
• apps/desktop/src/main/exporter-ipc.ts (modified)

[github-actions]

Findings

• [Blocker] open() can hang on FIFOs before the new regular-file check runs — safeReadImportFile() now calls open(path, 'r') before fileHandle.stat(), so a path swapped to a named pipe will block the Electron main process instead of returning null. I reproduced this on the Linux runner with mkfifo plus fs.promises.open(), which timed out after 2s. Evidence: apps/desktop/src/main/imports/safe-read.ts:32.
  Suggested fix:

  import { constants } from 'node:fs';
  import { open } from 'node:fs/promises';
  
  const flags = constants.O_RDONLY | (constants.O_NONBLOCK ?? 0);
  fileHandle = await open(path, flags);

  This keeps the single-descriptor flow that fixes the TOCTOU issue, but avoids blocking on FIFO/socket-style paths before fileHandle.stat() can reject them.

Summary

• Review mode: initial
• 1 blocker found in the new safeReadImportFile() flow. The TOCTOU fix is directionally right, but the current open(path, 'r') ordering reintroduces a main-process DoS path for named pipes at apps/desktop/src/main/imports/safe-read.ts:32.

Testing

• Not run (automation). Suggested regression tests: add a POSIX-only safe-read case proving a FIFO path returns null without blocking; add a parseRequest() case for oversized htmlContent.

open-codesign Bot

[github-actions]

open(path, 'r') runs before the regular-file check, so a contributor can replace this path with a FIFO and block the main process before fileHandle.stat() executes. On this Linux runner, mkfifo + fs.promises.open() hung until timeout killed it.

Suggested fix:

import { constants } from 'node:fs';
import { open } from 'node:fs/promises';

const flags = constants.O_RDONLY | (constants.O_NONBLOCK ?? 0);
fileHandle = await open(path, flags);

That preserves the single-handle TOCTOU fix while preventing FIFO/socket-style paths from hanging the process.

[github-actions]

Review mode: initial

Findings

• [Minor] Missing tests for the security fix — the TOCTOU fix in safeReadImportFile and the size limit in parseRequest are both security-sensitive paths with multiple error branches. The project uses Vitest and requires tests for changes like these. Neither function has a test file touched in this PR.
  Suggested fix: Add Vitest tests covering:

  • safeReadImportFile: ENOENT returns null, non-file handle returns null, oversized file returns null, successful file read returns content, file handle is always closed (including through error paths).
  • parseRequest: oversized htmlContent throws CodesignError, normal content passes through.

• [Nit] Unused import in safe-read.ts — the new code imports open and stat from node:fs/promises, but the stat import is unused because all stat calls go through fileHandle.stat().
  Suggested fix:

  -import { open, stat } from 'node:fs/promises';
  +import { open } from 'node:fs/promises';

Summary
The PR correctly addresses a TOCTOU race condition in safeReadImportFile by opening the file once and using the file descriptor for both stat and read. It also adds a missing size limit check for HTML export content in parseRequest. Both changes are well-motivated and implement the described fix correctly. No security regressions, no new dependencies, no hardcoded UI values. The main gap is test coverage for these security-sensitive paths.

Testing
Not run (automation). Suggested tests listed above.

Open-CoDesign Bot