diff --git a/lib/enhance/fixer.js b/lib/enhance/fixer.js index b3eea49..2057dde 100644 --- a/lib/enhance/fixer.js +++ b/lib/enhance/fixer.js @@ -7,6 +7,37 @@ const fs = require('fs'); const path = require('path'); +/** + * Reject symlinks before read/write operations. + * + * Security: A hostile repo could symlink a fixable file (e.g. `agent.md`) to a + * sensitive target (e.g. `~/.ssh/authorized_keys`). A HIGH-certainty auto-fix + * would then silently overwrite that target. We refuse to follow symlinks on + * any path we intend to read from or write to, including `.backup` siblings. + * + * This is called both before opening and immediately before writing, which + * narrows - though does not fully close - the TOCTOU window between calls. + * Node's fs module does not expose a portable `O_NOFOLLOW` open flag, so + * repeated lstat is the cleanest available mitigation for text-file edits. + * + * @param {string} targetPath - Path to check. + * @throws {Error} If the path exists and is a symlink. + */ +function assertNotSymlink(targetPath) { + let stat; + try { + stat = fs.lstatSync(targetPath); + } catch (err) { + if (err.code === 'ENOENT') return; // Path does not yet exist - fine. + throw err; + } + if (stat.isSymbolicLink()) { + const err = new Error('target is a symlink; refusing to follow'); + err.code = 'ESYMLINK_REFUSED'; + throw err; + } +} + function applyFixes(issues, options = {}) { const { dryRun = false, backup = true } = options; @@ -59,6 +90,23 @@ function applyFixes(issues, options = {}) { continue; } + // Security: refuse symlinks before we read, so a hostile repo can't + // redirect a HIGH-certainty fix at ~/.ssh/authorized_keys or similar. + try { + assertNotSymlink(filePath); + } catch (err) { + if (err.code === 'ESYMLINK_REFUSED') { + results.errors.push({ + filePath, + error: err.message, + success: false, + reason: 'target is a symlink; refusing to follow' + }); + continue; + } + throw err; + } + const content = fs.readFileSync(filePath, 'utf8'); let data; @@ -135,6 +183,8 @@ function applyFixes(issues, options = {}) { // Create backup if (backup) { const backupPath = `${filePath}.backup`; + // Refuse if the backup slot itself is a pre-existing symlink. + assertNotSymlink(backupPath); fs.writeFileSync(backupPath, content, 'utf8'); } @@ -145,6 +195,11 @@ 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). + assertNotSymlink(filePath); fs.writeFileSync(filePath, newContent, 'utf8'); } @@ -280,7 +335,14 @@ function restoreFromBackup(filePath) { return false; } + // Security: refuse if either the backup or the restore target is a + // symlink. Same threat model as applyFixes - a malicious post-hoc swap + // could redirect the restore at a sensitive file. + assertNotSymlink(backupPath); + assertNotSymlink(filePath); + const backupContent = fs.readFileSync(backupPath, 'utf8'); + assertNotSymlink(filePath); fs.writeFileSync(filePath, backupContent, 'utf8'); fs.unlinkSync(backupPath); @@ -717,5 +779,6 @@ module.exports = { fixAggressiveEmphasis, previewFixes, restoreFromBackup, - cleanupBackups + cleanupBackups, + assertNotSymlink };