fix: address P0 critical issues in security, stability, and correctness#116
Open
fix: address P0 critical issues in security, stability, and correctness#116
Conversation
- Add unhandledRejection handler in index.ts to prevent silent crashes - Enable SQLite foreign_keys and WAL journal mode for data integrity - Remove broken transactionAsync that ran async code inside sync transaction - Fix mutex race condition: replace polling loop with proper queue-based implementation - Validate PIDs as positive integers before shell interpolation in SessionManager and AbstractExecutor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #115
Summary
This PR fixes five confirmed P0 issues identified through static analysis of the codebase.
Changes
1. Missing
unhandledRejectionhandler (index.ts)Without this handler, unhandled promise rejections silently crash the Electron main process in Node.js 15+. Added a handler that logs the rejection reason.
2. SQLite missing
foreign_keysandjournal_modepragmas (database.ts)SQLite disables foreign key enforcement by default, allowing orphaned records and referential integrity violations. WAL mode also improves concurrent read performance and crash safety. Both pragmas are now set in the constructor.
3. Removed broken
transactionAsync(database.ts)The method wrapped an async function inside a
better-sqlite3synchronous transaction callback. Sincebetter-sqlite3transactions are synchronous, the transaction committed before the async work completed — making the transaction semantics completely ineffective. The method was unused, so it was removed.4. Mutex race condition fix (
mutex.ts)The previous implementation used a polling loop (
setTimeout(10ms)) to wait for lock release. This has a TOCTOU race: two concurrent callers could both observe the lock as free between thewhilecheck and thelocks.setcall. Replaced with a proper queue-based implementation using promise callbacks — no polling, no race condition, and waiters are notified immediately on release.5. PID validation before shell interpolation (
SessionManager.ts,AbstractExecutor.ts)parentPidandpidvalues were interpolated directly into shell command strings (wmic,ps,taskkill) without validation. A non-integer value could inject arbitrary shell commands. Added integer validation (Math.floor+Number.isInteger+> 0) before any shell interpolation.