From 5e76d77ece20e55dad6bd9a00f966b64c5e295b3 Mon Sep 17 00:00:00 2001 From: Amit Haridas Date: Thu, 23 Jul 2026 01:02:02 +0530 Subject: [PATCH] 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. --- src/main/files/index.js | 7 ++- src/main/files/search-in-files.js | 59 +++++++++++++++---- tests/unit/main/files/search-in-files.test.js | 46 +++++++++++++++ 3 files changed, 101 insertions(+), 11 deletions(-) diff --git a/src/main/files/index.js b/src/main/files/index.js index 3896992..79f7d77 100644 --- a/src/main/files/index.js +++ b/src/main/files/index.js @@ -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 diff --git a/src/main/files/search-in-files.js b/src/main/files/search-in-files.js index f95150d..c933fff 100644 --- a/src/main/files/search-in-files.js +++ b/src/main/files/search-in-files.js @@ -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 []; diff --git a/tests/unit/main/files/search-in-files.test.js b/tests/unit/main/files/search-in-files.test.js index 89064c7..62e5ed5 100644 --- a/tests/unit/main/files/search-in-files.test.js +++ b/tests/unit/main/files/search-in-files.test.js @@ -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 + }); });