From e11077b35b59cf1d6af9a7f1200d10db36803924 Mon Sep 17 00:00:00 2001 From: abhigyantrumio Date: Tue, 12 May 2026 19:30:54 +0530 Subject: [PATCH] fix(hooks): inspect slot mtime + content via single fd (codeql TOCTOU) CodeQL flagged the stale-takeover path in acquireHookSlot as a potential filesystem race (js/file-system-race): statSync(slotPath) followed by readFileSync(slotPath) gives a TOCTOU window where the file could be swapped between the metadata check and the content read. Replace the two separate path-based calls with a single openSync + fstatSync + readSync + closeSync sequence. Both mtime and owner PID now come from the same file descriptor, so the operations are atomic on one inode. No behavioral change beyond closing the race. Applied to all three hook variants (CJS, Plugin, Cursor). Co-Authored-By: Claude Opus 4.7 (1M context) --- gitnexus-claude-plugin/hooks/gitnexus-hook.js | 23 +++++++++++++++---- .../hooks/gitnexus-hook.cjs | 23 +++++++++++++++---- gitnexus/hooks/claude/gitnexus-hook.cjs | 23 +++++++++++++++---- 3 files changed, 54 insertions(+), 15 deletions(-) diff --git a/gitnexus-claude-plugin/hooks/gitnexus-hook.js b/gitnexus-claude-plugin/hooks/gitnexus-hook.js index 1c618d8aa..5d0c722e1 100644 --- a/gitnexus-claude-plugin/hooks/gitnexus-hook.js +++ b/gitnexus-claude-plugin/hooks/gitnexus-hook.js @@ -221,15 +221,22 @@ function acquireHookSlot(gitNexusDir) { return release; } catch { // Slot exists. Decide whether to take it over. - let stat; + // Open once and inspect mtime + content via the same fd so there's + // no TOCTOU between the metadata check and the content read + // (codeql js/file-system-race). + let fd; try { - stat = fs.statSync(slotPath); + fd = fs.openSync(slotPath, 'r'); } catch { - continue; // Vanished between EEXIST and stat — retry this slot. + continue; // Vanished between EEXIST and open — retry this slot. } let isLive = false; + let mtimeMs = Date.now(); try { - const ownerStr = fs.readFileSync(slotPath, 'utf-8').trim(); + mtimeMs = fs.fstatSync(fd).mtimeMs; + const buf = Buffer.alloc(32); + const n = fs.readSync(fd, buf, 0, 32, 0); + const ownerStr = buf.slice(0, n).toString('utf-8').trim(); if (ownerStr === '') { // Owner created the file but hasn't written its PID yet. The // wx open+write window is microseconds; give it the benefit @@ -248,11 +255,17 @@ function acquireHookSlot(gitNexusDir) { } } catch { /* unreadable — treat as dead */ + } finally { + try { + fs.closeSync(fd); + } catch { + /* already closed */ + } } // PID-liveness wins over age (avoids evicting a slow-but-alive hook). // Age check is a safety net against PID reuse on long-abandoned slots: // 30s >> the 7s augment timeout, so a healthy run never hits it. - if (isLive && Date.now() - stat.mtimeMs > HOOK_LOCK_STALE_MS) { + if (isLive && Date.now() - mtimeMs > HOOK_LOCK_STALE_MS) { isLive = false; } if (isLive) break; // Try the next slot. diff --git a/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs b/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs index e7f758116..245c5fcfc 100644 --- a/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs +++ b/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs @@ -251,15 +251,22 @@ function acquireHookSlot(gitNexusDir) { return release; } catch { // Slot exists. Decide whether to take it over. - let stat; + // Open once and inspect mtime + content via the same fd so there's + // no TOCTOU between the metadata check and the content read + // (codeql js/file-system-race). + let fd; try { - stat = fs.statSync(slotPath); + fd = fs.openSync(slotPath, 'r'); } catch { - continue; // Vanished between EEXIST and stat — retry this slot. + continue; // Vanished between EEXIST and open — retry this slot. } let isLive = false; + let mtimeMs = Date.now(); try { - const ownerStr = fs.readFileSync(slotPath, 'utf-8').trim(); + mtimeMs = fs.fstatSync(fd).mtimeMs; + const buf = Buffer.alloc(32); + const n = fs.readSync(fd, buf, 0, 32, 0); + const ownerStr = buf.slice(0, n).toString('utf-8').trim(); if (ownerStr === '') { // Owner created the file but hasn't written its PID yet. The // wx open+write window is microseconds; give it the benefit @@ -278,11 +285,17 @@ function acquireHookSlot(gitNexusDir) { } } catch { /* unreadable — treat as dead */ + } finally { + try { + fs.closeSync(fd); + } catch { + /* already closed */ + } } // PID-liveness wins over age (avoids evicting a slow-but-alive hook). // Age check is a safety net against PID reuse on long-abandoned slots: // 30s >> the 7s augment timeout, so a healthy run never hits it. - if (isLive && Date.now() - stat.mtimeMs > HOOK_LOCK_STALE_MS) { + if (isLive && Date.now() - mtimeMs > HOOK_LOCK_STALE_MS) { isLive = false; } if (isLive) break; // Try the next slot. diff --git a/gitnexus/hooks/claude/gitnexus-hook.cjs b/gitnexus/hooks/claude/gitnexus-hook.cjs index fca5a84cc..7db0e23e1 100755 --- a/gitnexus/hooks/claude/gitnexus-hook.cjs +++ b/gitnexus/hooks/claude/gitnexus-hook.cjs @@ -221,15 +221,22 @@ function acquireHookSlot(gitNexusDir) { return release; } catch { // Slot exists. Decide whether to take it over. - let stat; + // Open once and inspect mtime + content via the same fd so there's + // no TOCTOU between the metadata check and the content read + // (codeql js/file-system-race). + let fd; try { - stat = fs.statSync(slotPath); + fd = fs.openSync(slotPath, 'r'); } catch { - continue; // Vanished between EEXIST and stat — retry this slot. + continue; // Vanished between EEXIST and open — retry this slot. } let isLive = false; + let mtimeMs = Date.now(); try { - const ownerStr = fs.readFileSync(slotPath, 'utf-8').trim(); + mtimeMs = fs.fstatSync(fd).mtimeMs; + const buf = Buffer.alloc(32); + const n = fs.readSync(fd, buf, 0, 32, 0); + const ownerStr = buf.slice(0, n).toString('utf-8').trim(); if (ownerStr === '') { // Owner created the file but hasn't written its PID yet. The // wx open+write window is microseconds; give it the benefit @@ -248,11 +255,17 @@ function acquireHookSlot(gitNexusDir) { } } catch { /* unreadable — treat as dead */ + } finally { + try { + fs.closeSync(fd); + } catch { + /* already closed */ + } } // PID-liveness wins over age (avoids evicting a slow-but-alive hook). // Age check is a safety net against PID reuse on long-abandoned slots: // 30s >> the 7s augment timeout, so a healthy run never hits it. - if (isLive && Date.now() - stat.mtimeMs > HOOK_LOCK_STALE_MS) { + if (isLive && Date.now() - mtimeMs > HOOK_LOCK_STALE_MS) { isLive = false; } if (isLive) break; // Try the next slot.