Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 64 additions & 1 deletion lib/enhance/fixer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing TOCTOU re-check before backup file write

Low Severity

The backupPath write at fs.writeFileSync(backupPath, content, 'utf8') only gets a single assertNotSymlink check before it, with no immediate re-check before the write. In contrast, the main filePath write has an explicit re-check pattern to narrow the TOCTOU window. In the hostile-repo threat model, content (original file content) is attacker-controlled, so a race swapping backupPath for a symlink between check and write poses the same risk the re-check mitigates for filePath.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 32858df. Configure here.

fs.writeFileSync(backupPath, content, 'utf8');
}

Expand All @@ -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');
}

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -717,5 +779,6 @@ module.exports = {
fixAggressiveEmphasis,
previewFixes,
restoreFromBackup,
cleanupBackups
cleanupBackups,
assertNotSymlink
};
Loading