🛡️ Sentinel: [Hardening] Refactor synchronous FS and exec calls to async in pipeline handlers#347
Conversation
…ync in pipeline handlers - **🚨 Severity:** LOW / HARDENING - **💡 Issue:** Pipeline handlers (`src/tools/handlers/pipeline-handlers.ts`) used blocking, synchronous operations like `fs.readdirSync`, `fs.readFileSync`, `fs.accessSync`, and `execSync` to locate the UnrealBuildTool. - **🎯 Impact:** These operations block the Node.js event loop, preventing the MCP server from processing concurrent requests or heartbeats, which could lead to unresponsiveness or Denial of Service (DoS) if directories are large or drives are slow. - **🔧 Fix:** Refactored `tryUbtpath` and `findUbtExecutable` to use their asynchronous equivalents (`fs.promises.access`, `fs.promises.readdir`, `fs.promises.readFile`, and `util.promisify(exec)`). - **✅ Verification:** `npm run lint`, `npm run type-check`, `npm run test:unit`, and `npm run build` pass without issue. - **📝 Pattern Used:** Used `fs.promises` and `util.promisify` consistent with asynchronous event loop best practices in Node.js.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Refactors UnrealBuildTool (UBT) discovery in pipeline handlers to use asynchronous filesystem and process execution APIs to avoid blocking the Node.js event loop.
Changes:
- Converted UBT probing/discovery helpers (
tryUbtpath,findUbtExecutable) from sync (*Sync,execSync) to async (fs.promises.*, promisifiedexec). - Updated
run_ubtflow to await async UBT discovery and switched remaining directory reads to async equivalents. - Added a new Sentinel knowledge-base entry documenting the event-loop blocking risk from synchronous I/O in handlers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tools/handlers/pipeline-handlers.ts | Replaces synchronous UBT discovery and directory/file checks with async equivalents and updates call sites accordingly. |
| .jules/sentinel.md | Documents the event-loop blocking vulnerability pattern and its prevention guidance. |
Comments suppressed due to low confidence (1)
src/tools/handlers/pipeline-handlers.ts:103
- The UBT discovery logic was substantially refactored to async (fs.promises + exec promisified), but there are no unit tests covering the discovery paths (env var,
.uprojectEngineAssociation/DefaultEngine.ini, PATH fallback). Adding targeted tests (mockingfs.promisesandchild_process.exec/spawn) would help prevent regressions across platforms.
async function findUbtExecutable(): Promise<string> {
// ─── Strategy 1: Explicit environment variable ────────────────────────
// UE_ENGINE_PATH is the convention in this project (see AGENTS.md / README).
// The path may point to either:
// • .../UE_5.x/Engine (already includes "Engine" suffix)
// • .../UE_5.x (root directory without "Engine")
const enginePath =
process.env.UE_ENGINE_PATH ??
process.env.UNREAL_ENGINE_PATH ??
undefined;
| async function tryUbtpath(candidate: string): Promise<string | undefined> { | ||
| // On Windows F_OK is sufficient for executability; X_OK is a no-op on fs.access | ||
| const mode = process.platform === 'win32' | ||
| ? fs.constants.F_OK | ||
| : fs.constants.F_OK | fs.constants.X_OK; | ||
| try { | ||
| fs.accessSync(candidate, mode); | ||
| await fs.promises.access(candidate, mode); | ||
| return candidate; | ||
| } catch { /* not usable */ } |
There was a problem hiding this comment.
On non-Windows platforms, tryUbtpath() requires X_OK for all candidates. That will likely reject UnrealBuildTool.dll (typically not marked executable) even though the code later supports running a .dll via dotnet. Consider relaxing the access mode for .dll candidates (e.g., check F_OK/readability) while still requiring X_OK for true executables.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
(#347 (review))
And mention today's date in sentinel.md file.
There was a problem hiding this comment.
I have addressed the feedback by relaxing the X_OK requirement on Windows and non-Windows dlls, testing via fs.constants.F_OK. I have also updated the date in the sentinel.md file as requested.
There was a problem hiding this comment.
Done in commit 1e83ccc.
tryUbtpath()now usesF_OKfor.dllcandidates on non-Windows (since dlls are not executable — they're invoked viadotnet), while still requiringF_OK | X_OKfor true executables like.exeor bare binaries.- Updated the sentinel.md entry date to
2026-04-10.
…e sentinel.md date Agent-Logs-Url: https://github.com/ChiR24/Unreal_mcp/sessions/50f83e4e-aea5-489f-aa86-dfbe1b87be29 Co-authored-by: ChiR24 <125826529+ChiR24@users.noreply.github.com>
…ync in pipeline handlers - **🚨 Severity:** LOW / HARDENING - **💡 Issue:** Pipeline handlers (`src/tools/handlers/pipeline-handlers.ts`) used blocking, synchronous operations like `fs.readdirSync`, `fs.readFileSync`, `fs.accessSync`, and `execSync` to locate the UnrealBuildTool. - **🎯 Impact:** These operations block the Node.js event loop, preventing the MCP server from processing concurrent requests or heartbeats, which could lead to unresponsiveness or Denial of Service (DoS) if directories are large or drives are slow. - **🔧 Fix:** Refactored `tryUbtpath` and `findUbtExecutable` to use their asynchronous equivalents (`fs.promises.access`, `fs.promises.readdir`, `fs.promises.readFile`, and `util.promisify(exec)`). Fixed bug where `tryUbtpath` was incorrectly returning an error on Linux machines by rejecting .dlls with `fs.constants.X_OK`. - **✅ Verification:** `npm run lint`, `npm run type-check`, `npm run test:unit`, and `npm run build` pass without issue. - **📝 Pattern Used:** Used `fs.promises` and `util.promisify` consistent with asynchronous event loop best practices in Node.js.
src/tools/handlers/pipeline-handlers.ts) used blocking, synchronous operations likefs.readdirSync,fs.readFileSync,fs.accessSync, andexecSyncto locate the UnrealBuildTool.tryUbtpathandfindUbtExecutableto use their asynchronous equivalents (fs.promises.access,fs.promises.readdir,fs.promises.readFile, andutil.promisify(exec)).npm run lint,npm run type-check,npm run test:unit, andnpm run buildpass without issue.fs.promisesandutil.promisifyconsistent with asynchronous event loop best practices in Node.js.PR created automatically by Jules for task 5414201795584645671 started by @ChiR24