diff --git a/lib/collectors/codebase.js b/lib/collectors/codebase.js index 1a8c7a1..957337c 100644 --- a/lib/collectors/codebase.js +++ b/lib/collectors/codebase.js @@ -11,6 +11,7 @@ const fs = require('fs'); const path = require('path'); +const { readFileWithLimit } = require('../utils/fs-safe'); const DEFAULT_OPTIONS = { depth: 'thorough', @@ -194,10 +195,10 @@ function scanFileSymbols(basePath, topLevelDirs) { if (entry.name.includes('.test.') || entry.name.includes('.spec.')) continue; try { - const stat = fs.statSync(fullPath); - if (stat.size > MAX_FILE_SIZE) continue; - - const content = fs.readFileSync(fullPath, 'utf8'); + // Open once and read through the fd (size check + read on the + // same inode) to avoid a stat/read TOCTOU. Oversized or + // unreadable files throw and are skipped by the catch below. + const content = readFileWithLimit(fullPath, MAX_FILE_SIZE); const symbols = extractSymbols(content); if (symbols.functions.length || symbols.classes.length || symbols.exports.length) { diff --git a/lib/enhance/auto-suppression.js b/lib/enhance/auto-suppression.js index 911a433..1f373a6 100644 --- a/lib/enhance/auto-suppression.js +++ b/lib/enhance/auto-suppression.js @@ -14,6 +14,8 @@ const fs = require('fs'); const path = require('path'); const { execFileSync } = require('child_process'); +const { readFileWithLimit } = require('../utils/fs-safe'); +const { writeJsonAtomic } = require('../utils/atomic-write'); // Try to import cross-platform helpers let getSuppressionPath; @@ -488,19 +490,20 @@ function saveAutoSuppressions(suppressionPath, projectId, findings) { */ function clearAutoSuppressions(suppressionPath, projectId) { try { - if (!fs.existsSync(suppressionPath)) return; - - const data = JSON.parse(fs.readFileSync(suppressionPath, 'utf8')); + // Read via fd (no existsSync pre-check) and write atomically, so neither + // the read nor the write races against a swap of the path. A missing file + // throws ENOENT and is swallowed below. + const data = JSON.parse(readFileWithLimit(suppressionPath)); if (data.projects?.[projectId]?.auto_learned) { data.projects[projectId].auto_learned = { patterns: {}, stats: { totalSuppressed: 0, lastAnalysis: new Date().toISOString() } }; - fs.writeFileSync(suppressionPath, JSON.stringify(data, null, 2)); + writeJsonAtomic(suppressionPath, data); } } catch { - // Ignore errors + // Missing file or write error: ignore. } } diff --git a/lib/enhance/cross-file-analyzer.js b/lib/enhance/cross-file-analyzer.js index eea0de1..236f620 100644 --- a/lib/enhance/cross-file-analyzer.js +++ b/lib/enhance/cross-file-analyzer.js @@ -10,6 +10,7 @@ const fs = require('fs'); const path = require('path'); +const { readFileWithLimit } = require('../utils/fs-safe'); const { parseMarkdownFrontmatter } = require('./agent-analyzer'); const { crossFilePatterns, loadKnownTools } = require('./cross-file-patterns'); @@ -474,8 +475,9 @@ function loadAllSkills(rootDir, options = {}) { if (!isPathWithinRoot(skillPath, rootDir)) continue; try { - fs.accessSync(skillPath); - const content = fs.readFileSync(skillPath, 'utf8'); + // Read through a single fd (no separate access check) to avoid an + // access/read TOCTOU; a missing/unreadable file throws and is skipped. + const content = readFileWithLimit(skillPath); const { frontmatter, body } = parseMarkdownFrontmatter(content); skills.push({ diff --git a/lib/enhance/docs-analyzer.js b/lib/enhance/docs-analyzer.js index e927a68..7601ab2 100644 --- a/lib/enhance/docs-analyzer.js +++ b/lib/enhance/docs-analyzer.js @@ -6,6 +6,7 @@ const fs = require('fs'); const path = require('path'); +const { writeFileAtomic } = require('../utils/atomic-write'); const { getPatternsForMode, estimateTokens } = require('./docs-patterns'); function analyzeDoc(docPath, options = {}) { @@ -252,6 +253,9 @@ function applyDocsFixes(issues, options = {}) { } let content = fs.readFileSync(filePath, 'utf8'); + // Snapshot the on-disk content now so the backup does not re-read + // filePath right before the write (which would be a read/write TOCTOU). + const originalContent = content; const appliedToFile = []; for (const issue of fileIssues) { @@ -283,9 +287,9 @@ function applyDocsFixes(issues, options = {}) { // Write changes if (!dryRun && appliedToFile.length > 0) { if (backup) { - fs.writeFileSync(`${filePath}.backup`, fs.readFileSync(filePath, 'utf8'), 'utf8'); + writeFileAtomic(`${filePath}.backup`, originalContent); } - fs.writeFileSync(filePath, content, 'utf8'); + writeFileAtomic(filePath, content); } results.applied.push(...appliedToFile); diff --git a/lib/enhance/fixer.js b/lib/enhance/fixer.js index bb2fb7c..35f5b40 100644 --- a/lib/enhance/fixer.js +++ b/lib/enhance/fixer.js @@ -6,6 +6,7 @@ const fs = require('fs'); const path = require('path'); +const { writeFileAtomic } = require('../utils/atomic-write'); /** * Reject symlinks before read/write operations. @@ -195,12 +196,12 @@ function applyFixes(issues, options = {}) { } else { newContent = JSON.stringify(modified, null, 2); } - // Re-check immediately before write. Narrows the TOCTOU window - // between the initial lstat and this writeFileSync (an attacker - // who swaps the regular file for a symlink between calls will - // be caught here). + // Refuse a symlink swap (explicit, early), then write atomically via + // a temp file + rename. The rename replaces the path entry itself and + // never follows a symlink, so the write cannot be redirected through a + // swapped link - closing the lstat/write TOCTOU window. assertNotSymlink(filePath); - fs.writeFileSync(filePath, newContent, 'utf8'); + writeFileAtomic(filePath, newContent); } results.applied.push(...appliedToFile); diff --git a/lib/enhance/prompt-analyzer.js b/lib/enhance/prompt-analyzer.js index 792d463..93e7ac4 100644 --- a/lib/enhance/prompt-analyzer.js +++ b/lib/enhance/prompt-analyzer.js @@ -8,6 +8,7 @@ const fs = require('fs'); const path = require('path'); +const { writeFileAtomic } = require('../utils/atomic-write'); const { promptPatterns, estimateTokens, extractCodeBlocks } = require('./prompt-patterns'); const reporter = require('./reporter'); @@ -344,6 +345,9 @@ function applyFixes(results, options = {}) { } let content = fs.readFileSync(filePath, 'utf8'); + // Snapshot the on-disk content now so the backup does not re-read + // filePath right before the write (which would be a read/write TOCTOU). + const originalContent = content; const appliedToFile = []; for (const issue of fileIssues) { @@ -369,9 +373,9 @@ function applyFixes(results, options = {}) { // Write changes if (!dryRun && appliedToFile.length > 0) { if (backup) { - fs.writeFileSync(`${filePath}.backup`, fs.readFileSync(filePath, 'utf8'), 'utf8'); + writeFileAtomic(`${filePath}.backup`, originalContent); } - fs.writeFileSync(filePath, content, 'utf8'); + writeFileAtomic(filePath, content); } fixResults.applied.push(...appliedToFile); diff --git a/lib/enhance/suppression.js b/lib/enhance/suppression.js index 536d26e..15a86be 100644 --- a/lib/enhance/suppression.js +++ b/lib/enhance/suppression.js @@ -5,6 +5,7 @@ const fs = require('fs'); const path = require('path'); +const { readFileWithLimit } = require('../utils/fs-safe'); /** * Default suppression config @@ -34,20 +35,18 @@ function loadConfig(projectRoot) { ]; for (const configPath of configPaths) { - if (fs.existsSync(configPath)) { - try { - // Check file size before reading to prevent DoS - const stats = fs.statSync(configPath); - if (stats.size > MAX_CONFIG_SIZE) { - console.error(`[WARN] Config file too large (${stats.size} bytes), using defaults`); - continue; - } - const content = fs.readFileSync(configPath, 'utf8'); - const userConfig = JSON.parse(content); - return mergeConfig(DEFAULT_CONFIG, userConfig); - } catch (err) { - // Invalid config, use defaults + try { + // Open once and read through the fd (size check + read on the same + // inode) to avoid an exists/stat/read TOCTOU. A missing file throws + // ENOENT and falls through to the next candidate / defaults. + const content = readFileWithLimit(configPath, MAX_CONFIG_SIZE); + const userConfig = JSON.parse(content); + return mergeConfig(DEFAULT_CONFIG, userConfig); + } catch (err) { + if (err && err.code === 'EFBIG') { + console.error('[WARN] Config file too large, using defaults'); } + // Missing or invalid config: try the next candidate / defaults. } } diff --git a/lib/patterns/slop-analyzers.js b/lib/patterns/slop-analyzers.js index 52b0957..3cda1f6 100644 --- a/lib/patterns/slop-analyzers.js +++ b/lib/patterns/slop-analyzers.js @@ -858,15 +858,22 @@ function countEntryPointExports(repoPath, options = {}) { for (const entry of ENTRY_POINTS) { const fullPath = path.join(repoPath, entry); try { - const stat = fs.statSync(fullPath); - if (stat.isFile()) { - const content = fs.readFileSync(fullPath, 'utf8'); - const lang = detectLanguage(entry); - const exports = countExportsInContent(content, lang); - if (exports > 0) { - foundEntries.push(entry); - count += exports; + // Open once and check/read through the fd so the is-file check and the + // read apply to the same inode (no stat/read TOCTOU). Uses the injected + // fs so mocked tests keep working. + const fd = fs.openSync(fullPath, 'r'); + try { + if (fs.fstatSync(fd).isFile()) { + const content = fs.readFileSync(fd, 'utf8'); + const lang = detectLanguage(entry); + const exports = countExportsInContent(content, lang); + if (exports > 0) { + foundEntries.push(entry); + count += exports; + } } + } finally { + fs.closeSync(fd); } } catch { // File doesn't exist, try next diff --git a/lib/utils/fs-safe.js b/lib/utils/fs-safe.js new file mode 100644 index 0000000..9c3c6ee --- /dev/null +++ b/lib/utils/fs-safe.js @@ -0,0 +1,42 @@ +'use strict'; + +const fs = require('fs'); + +/** + * Read a file with an optional size cap, free of check-then-use (TOCTOU) + * races. + * + * The path is opened once and every subsequent operation (the regular-file + * check, the size check, the read) acts on that file descriptor. Because the + * descriptor is bound to a single inode, swapping the path for another file or + * a symlink between calls cannot redirect the read - which a separate + * `fs.statSync(path)` / `fs.existsSync(path)` followed by `fs.readFileSync(path)` + * cannot guarantee. + * + * @param {string} filePath + * @param {number} [maxSize] - byte ceiling; larger files throw `EFBIG` + * @param {BufferEncoding} [encoding='utf8'] + * @returns {string|Buffer} file contents (string when an encoding is given) + * @throws if the path is missing, not a regular file, too large, or unreadable + */ +function readFileWithLimit(filePath, maxSize, encoding = 'utf8') { + const fd = fs.openSync(filePath, 'r'); + try { + const stat = fs.fstatSync(fd); + if (!stat.isFile()) { + const err = new Error(`Not a regular file: ${filePath}`); + err.code = 'ENOTFILE'; + throw err; + } + if (typeof maxSize === 'number' && stat.size > maxSize) { + const err = new Error(`File too large: ${stat.size} > ${maxSize} bytes`); + err.code = 'EFBIG'; + throw err; + } + return fs.readFileSync(fd, encoding); + } finally { + fs.closeSync(fd); + } +} + +module.exports = { readFileWithLimit };