Skip to content

fix(security): 2 improvements across 2 files #228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomaioo wants to merge 2 commits into
base: main
Choose a base branch
from
+43 −21
Open

fix(security): 2 improvements across 2 files #228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59daeeb
fix(security): 2 improvements across 2 files
tomaioo Apr 24, 2026
c7a9ff4
fix(security): 2 improvements across 2 files
tomaioo Apr 24, 2026
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
5 changes: 5 additions & 0 deletions apps/desktop/src/main/exporter-ipc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ const FORMAT_FILTERS: Record<ExporterFormat, Electron.FileFilter[]> = {
markdown: [{ name: 'Markdown', extensions: ['md'] }],
};

const MAX_HTML_CONTENT_BYTES = 10 * 1024 * 1024;

export interface ExportRequest {
format: ExporterFormat;
htmlContent: string;
Expand Down Expand Up @@ -46,6 +48,9 @@ export function parseRequest(raw: unknown): ExportRequest {
if (typeof html !== 'string' || html.length === 0) {
throw new CodesignError('export requires non-empty htmlContent', ERROR_CODES.IPC_BAD_INPUT);
}
if (Buffer.byteLength(html, 'utf8') > MAX_HTML_CONTENT_BYTES) {
throw new CodesignError('export htmlContent exceeds size limit', ERROR_CODES.IPC_BAD_INPUT);
}
const out: ExportRequest = { format, htmlContent: html };
if (typeof defaultFilename === 'string' && defaultFilename.length > 0) {
out.defaultFilename = defaultFilename;
Expand Down
59 changes: 38 additions & 21 deletions apps/desktop/src/main/imports/safe-read.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { readFile, stat } from 'node:fs/promises';
import { open, stat } from 'node:fs/promises';
import { getLogger } from '../logger';

/**
Expand All @@ -11,9 +11,10 @@ import { getLogger } from '../logger';
* - plant a 10-GB file at the same path and exhaust the heap;
* - point the path at a socket / FIFO / directory to hang `readFile`.
*
* `safeReadImportFile` stats first, rejects anything that isn't a regular
* file or that exceeds `MAX_IMPORT_FILE_BYTES`, and falls through to
* `readFile` only when the stat says it's safe. Returns `null` on
* `safeReadImportFile` opens the path once, stats via that handle,
* rejects anything that isn't a regular file or that exceeds
* `MAX_IMPORT_FILE_BYTES`, and reads only when the stat says it's safe.
* Returns `null` on
* missing/too-big/not-a-file so callers keep their existing "no config
* found" branches; logs the reason so diagnostics bundles capture a
* rejection trail.
Expand All @@ -26,28 +27,44 @@ const log = getLogger('import-read');
export const MAX_IMPORT_FILE_BYTES = 256 * 1024;

export async function safeReadImportFile(path: string): Promise<string | null> {
let stats: Awaited<ReturnType<typeof stat>>;
let fileHandle: Awaited<ReturnType<typeof open>>;
try {
stats = await stat(path);
fileHandle = await open(path, 'r');
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 24, 2026

Choose a reason for hiding this comment

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

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.

} catch (err) {
const code = (err as NodeJS.ErrnoException).code;
if (code === 'ENOENT') return null;
log.warn('safe_read.stat_failed', { path, code: code ?? 'unknown' });
log.warn('safe_read.open_failed', { path, code: code ?? 'unknown' });
return null;
}
if (!stats.isFile()) {
// Symlink to /dev/zero, symlink to a directory, named pipe, etc.
// `isFile()` follows symlinks, so a symlink to a regular file is fine.
log.warn('safe_read.not_regular_file', { path });
return null;
}
if (stats.size > MAX_IMPORT_FILE_BYTES) {
log.warn('safe_read.size_exceeded', {
path,
size: stats.size,
cap: MAX_IMPORT_FILE_BYTES,
});
return null;

try {
let stats: Awaited<ReturnType<typeof stat>>;
try {
stats = await fileHandle.stat();
} catch (err) {
const code = (err as NodeJS.ErrnoException).code;
if (code === 'ENOENT') return null;
log.warn('safe_read.stat_failed', { path, code: code ?? 'unknown' });
return null;
}

if (!stats.isFile()) {
// Symlink to /dev/zero, symlink to a directory, named pipe, etc.
// `isFile()` follows symlinks, so a symlink to a regular file is fine.
log.warn('safe_read.not_regular_file', { path });
return null;
}
if (stats.size > MAX_IMPORT_FILE_BYTES) {
log.warn('safe_read.size_exceeded', {
path,
size: stats.size,
cap: MAX_IMPORT_FILE_BYTES,
});
return null;
}

return fileHandle.readFile('utf8');
} finally {
await fileHandle.close();
}
return readFile(path, 'utf8');
}
Loading