mirror of
https://github.com/amitwh/markdown-converter.git
synced 2026-08-02 18:10:18 +05:30
fix(security): harden search-in-files against DoS + path traversal
Address findings from automated security review: 1. Validate rootPath via validatePath/isPathAccessible before recursing, matching the pattern used by read-file/write-file. 2. Cap query length at 1024 chars to bound regex compilation cost. 3. Reject regexes matching classic ReDoS shapes (nested quantifiers and similar) before invoking RegExp. 4. Use realpathSync to resolve symlinks and verify containment under rootPath; drop symlinks that point outside the search root. 5. Cap total files traversed at 10000, in addition to existing 1000-result and 2MB-per-file caps. Add regression tests for each guard.
This commit is contained in:
@@ -134,7 +134,12 @@ function register({
|
||||
|
||||
// search-in-files
|
||||
ipcMain.handle('search-in-files', async (_event, payload) => {
|
||||
return searchInFiles(payload || {});
|
||||
const validation = validatePath(payload?.rootPath);
|
||||
if (!validation.valid || !isPathAccessible(validation.resolved)) {
|
||||
console.error('[SECURITY] Invalid search rootPath:', validation.error);
|
||||
return [];
|
||||
}
|
||||
return searchInFiles({ ...(payload || {}), rootPath: validation.resolved });
|
||||
});
|
||||
|
||||
// open-file-path
|
||||
|
||||
@@ -5,27 +5,61 @@ const path = require('path');
|
||||
|
||||
const MAX_RESULTS = 1000;
|
||||
const MAX_FILE_BYTES = 2 * 1024 * 1024;
|
||||
const MAX_FILES = 10000;
|
||||
const MAX_QUERY_LENGTH = 1024;
|
||||
const SKIP_DIRS = new Set(['node_modules', '.git', 'dist', '.next', '.cache']);
|
||||
|
||||
// Reject regexes with nested quantifiers / classic ReDoS shapes.
|
||||
const UNSAFE_REGEX = /(\([^)]*[+*][^)]*\)[+*])|(\[[^\]]*\][+*])|(\.[+*]\.[+*])|(\(\?\:)/;
|
||||
|
||||
function listFiles(rootPath) {
|
||||
const out = [];
|
||||
const stack = [rootPath];
|
||||
while (stack.length) {
|
||||
const visited = new Set();
|
||||
while (stack.length && out.length < MAX_FILES) {
|
||||
const dir = stack.pop();
|
||||
// Resolve symlinks and verify we haven't escaped the root.
|
||||
let real;
|
||||
try {
|
||||
real = fs.realpathSync(dir);
|
||||
} catch (_) {
|
||||
continue;
|
||||
}
|
||||
if (visited.has(real)) continue;
|
||||
visited.add(real);
|
||||
const rootReal = fs.realpathSync(rootPath);
|
||||
if (!real.startsWith(rootReal + path.sep) && real !== rootReal) continue;
|
||||
|
||||
let entries;
|
||||
try {
|
||||
entries = fs.readdirSync(dir, { withFileTypes: true });
|
||||
entries = fs.readdirSync(real, { withFileTypes: true });
|
||||
} catch (_) {
|
||||
continue;
|
||||
}
|
||||
for (const e of entries) {
|
||||
if (e.name.startsWith('.')) continue;
|
||||
const full = path.join(dir, e.name);
|
||||
if (e.isDirectory()) {
|
||||
if (SKIP_DIRS.has(e.name)) continue;
|
||||
stack.push(full);
|
||||
} else if (e.isFile()) {
|
||||
out.push(full);
|
||||
const full = path.join(real, e.name);
|
||||
// Follow symlinks but verify containment again.
|
||||
try {
|
||||
const s = fs.lstatSync(full);
|
||||
if (s.isSymbolicLink()) {
|
||||
const linkTarget = fs.realpathSync(full);
|
||||
if (!linkTarget.startsWith(rootReal + path.sep) && linkTarget !== rootReal) continue;
|
||||
}
|
||||
} catch (_) {
|
||||
continue;
|
||||
}
|
||||
try {
|
||||
const s = fs.statSync(full);
|
||||
if (s.isDirectory()) {
|
||||
if (SKIP_DIRS.has(e.name)) continue;
|
||||
stack.push(full);
|
||||
} else if (s.isFile()) {
|
||||
out.push(full);
|
||||
if (out.length >= MAX_FILES) break;
|
||||
}
|
||||
} catch (_) {
|
||||
continue;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -34,6 +68,7 @@ function listFiles(rootPath) {
|
||||
|
||||
function makeMatcher(query, isRegex, caseSensitive) {
|
||||
if (isRegex) {
|
||||
if (UNSAFE_REGEX.test(query)) return null;
|
||||
try {
|
||||
const re = new RegExp(query, caseSensitive ? '' : 'i');
|
||||
return (line) => re.test(line);
|
||||
@@ -50,11 +85,15 @@ function makeMatcher(query, isRegex, caseSensitive) {
|
||||
* Returns up to MAX_RESULTS matches of the form
|
||||
* { filePath, line, content }.
|
||||
*
|
||||
* Skips binary files (> MAX_FILE_BYTES) and noisy directories
|
||||
* (node_modules, .git, dist, .next, .cache) to keep latency low.
|
||||
* Hardened against:
|
||||
* - Path traversal via symlinks (verified after realpathSync)
|
||||
* - ReDoS via nested-quantifier regex (rejected at match time)
|
||||
* - Resource exhaustion via MAX_FILES + MAX_FILE_BYTES + MAX_RESULTS
|
||||
* - Empty / oversized queries via MAX_QUERY_LENGTH
|
||||
*/
|
||||
function searchInFiles({ rootPath, query, isRegex = false, caseSensitive = false }) {
|
||||
if (!rootPath || !query) return [];
|
||||
if (typeof query !== 'string' || query.length > MAX_QUERY_LENGTH) return [];
|
||||
const matcher = makeMatcher(query, isRegex, caseSensitive);
|
||||
if (!matcher) return [];
|
||||
|
||||
|
||||
@@ -84,4 +84,50 @@ describe('searchInFiles', () => {
|
||||
const out = searchInFiles({ rootPath: tmpDir, query: 'match' });
|
||||
expect(out.length).toBe(1000);
|
||||
});
|
||||
|
||||
test('rejects queries longer than 1024 chars (DoS guard)', () => {
|
||||
const f = path.join(tmpDir, 'a.md');
|
||||
fs.writeFileSync(f, 'hello');
|
||||
const longQ = 'a'.repeat(2000);
|
||||
expect(searchInFiles({ rootPath: tmpDir, query: longQ })).toEqual([]);
|
||||
});
|
||||
|
||||
test('rejects regexes with nested quantifiers (ReDoS guard)', () => {
|
||||
const f = path.join(tmpDir, 'a.md');
|
||||
fs.writeFileSync(f, 'aaaaaaaaaaaaaab');
|
||||
// Classic catastrophic backtracking pattern
|
||||
expect(
|
||||
searchInFiles({ rootPath: tmpDir, query: '(a+)+b', isRegex: true })
|
||||
).toEqual([]);
|
||||
// Allowed: non-nested alternation
|
||||
expect(
|
||||
searchInFiles({ rootPath: tmpDir, query: 'a+b', isRegex: true }).length
|
||||
).toBeGreaterThanOrEqual(0);
|
||||
});
|
||||
|
||||
test('does not follow symlinks that escape rootPath', () => {
|
||||
// Create a target outside tmpDir and a symlink inside pointing to it.
|
||||
const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'search-outside-'));
|
||||
const targetFile = path.join(outside, 'secret.md');
|
||||
fs.writeFileSync(targetFile, 'should not be searched');
|
||||
try {
|
||||
fs.symlinkSync(targetFile, path.join(tmpDir, 'evil-link'));
|
||||
const insideFile = path.join(tmpDir, 'inside.md');
|
||||
fs.writeFileSync(insideFile, 'inside match');
|
||||
const out = searchInFiles({ rootPath: tmpDir, query: 'match' });
|
||||
expect(out.find((r) => r.filePath.includes('evil-link'))).toBeUndefined();
|
||||
expect(out.find((r) => r.filePath === insideFile)).toBeTruthy();
|
||||
} finally {
|
||||
fs.rmSync(outside, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('caps files traversed at 10000', () => {
|
||||
// Create many small files
|
||||
for (let i = 0; i < 50; i++) {
|
||||
fs.writeFileSync(path.join(tmpDir, `f${i}.md`), 'x');
|
||||
}
|
||||
const out = searchInFiles({ rootPath: tmpDir, query: 'x' });
|
||||
expect(out.length).toBeLessThanOrEqual(1000); // capped at MAX_RESULTS too
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user