mirror of
https://github.com/amitwh/markdown-converter.git
synced 2026-08-02 10:00:17 +05:30
fix(security): strengthen ReDoS denylist in search-in-files
Follow-up to the push-sweep review: the previous UNSAFE_REGEX used JS bitwise OR (|) between regex literals, accidentally OR-ing the RegExp objects instead of combining alternatives into one pattern. Replace with a single RegExp that catches: - nested quantifiers (a+)+, [a-z]*+ - class + quantifier - dot-quantifier followed by dot-quantifier - lookahead / lookbehind (?=, ?!) - backrefs \1..\9 - alternation + quantifier Also add MAX_REGEX_LENGTH=200 hard cap. The denylist is still defense-in-depth — the length, files-traversed, and per-file-byte caps are the primary defense.
This commit is contained in:
@@ -7,10 +7,31 @@ const MAX_RESULTS = 1000;
|
|||||||
const MAX_FILE_BYTES = 2 * 1024 * 1024;
|
const MAX_FILE_BYTES = 2 * 1024 * 1024;
|
||||||
const MAX_FILES = 10000;
|
const MAX_FILES = 10000;
|
||||||
const MAX_QUERY_LENGTH = 1024;
|
const MAX_QUERY_LENGTH = 1024;
|
||||||
|
const MAX_REGEX_LENGTH = 200;
|
||||||
const SKIP_DIRS = new Set(['node_modules', '.git', 'dist', '.next', '.cache']);
|
const SKIP_DIRS = new Set(['node_modules', '.git', 'dist', '.next', '.cache']);
|
||||||
|
|
||||||
// Reject regexes with nested quantifiers / classic ReDoS shapes.
|
// Reject regexes with classic ReDoS shapes. This is a defense-in-depth
|
||||||
const UNSAFE_REGEX = /(\([^)]*[+*][^)]*\)[+*])|(\[[^\]]*\][+*])|(\.[+*]\.[+*])|(\(\?\:)/;
|
// denylist, not a proof of safety — the hard caps on length, files, and
|
||||||
|
// results are the primary defense.
|
||||||
|
const UNSAFE_REGEX = new RegExp(
|
||||||
|
// nested quantifiers: (a+)+, [a-z]*+
|
||||||
|
'\\([^)]*[+*][^)]*\\)[+*?]' +
|
||||||
|
'|' +
|
||||||
|
// class with quantifier: [a-z]+
|
||||||
|
'\\[[^\\]]*\\][+*]' +
|
||||||
|
'|' +
|
||||||
|
// dot-quantifier followed by dot-quantifier
|
||||||
|
'\\.[+*]\\s*\\.[+*]' +
|
||||||
|
'|' +
|
||||||
|
// lookahead / lookbehind
|
||||||
|
'\\(\\?[=!]' +
|
||||||
|
'|' +
|
||||||
|
// backrefs
|
||||||
|
'\\\\[1-9]' +
|
||||||
|
'|' +
|
||||||
|
// alternation with quantifier
|
||||||
|
'\\([^)]*\\|[^)]*\\)[+*]'
|
||||||
|
);
|
||||||
|
|
||||||
function listFiles(rootPath) {
|
function listFiles(rootPath) {
|
||||||
const out = [];
|
const out = [];
|
||||||
@@ -68,6 +89,7 @@ function listFiles(rootPath) {
|
|||||||
|
|
||||||
function makeMatcher(query, isRegex, caseSensitive) {
|
function makeMatcher(query, isRegex, caseSensitive) {
|
||||||
if (isRegex) {
|
if (isRegex) {
|
||||||
|
if (query.length > MAX_REGEX_LENGTH) return null;
|
||||||
if (UNSAFE_REGEX.test(query)) return null;
|
if (UNSAFE_REGEX.test(query)) return null;
|
||||||
try {
|
try {
|
||||||
const re = new RegExp(query, caseSensitive ? '' : 'i');
|
const re = new RegExp(query, caseSensitive ? '' : 'i');
|
||||||
|
|||||||
@@ -105,6 +105,43 @@ describe('searchInFiles', () => {
|
|||||||
).toBeGreaterThanOrEqual(0);
|
).toBeGreaterThanOrEqual(0);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('rejects regexes with lookahead / lookbehind', () => {
|
||||||
|
fs.writeFileSync(path.join(tmpDir, 'a.md'), 'hello');
|
||||||
|
expect(
|
||||||
|
searchInFiles({ rootPath: tmpDir, query: '(?=hello)h', isRegex: true })
|
||||||
|
).toEqual([]);
|
||||||
|
expect(
|
||||||
|
searchInFiles({ rootPath: tmpDir, query: '(?!foo)h', isRegex: true })
|
||||||
|
).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('rejects regexes with backrefs', () => {
|
||||||
|
fs.writeFileSync(path.join(tmpDir, 'a.md'), 'hello hello');
|
||||||
|
expect(
|
||||||
|
searchInFiles({ rootPath: tmpDir, query: '(hello)\\1', isRegex: true })
|
||||||
|
).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('rejects regexes with class + quantifier', () => {
|
||||||
|
fs.writeFileSync(path.join(tmpDir, 'a.md'), 'abc');
|
||||||
|
expect(
|
||||||
|
searchInFiles({ rootPath: tmpDir, query: '[a-z]+', isRegex: true })
|
||||||
|
).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('rejects regexes longer than 200 chars', () => {
|
||||||
|
fs.writeFileSync(path.join(tmpDir, 'a.md'), 'x');
|
||||||
|
const longRe = 'a'.repeat(250);
|
||||||
|
expect(
|
||||||
|
searchInFiles({ rootPath: tmpDir, query: longRe, isRegex: true })
|
||||||
|
).toEqual([]);
|
||||||
|
// But 200 chars is fine
|
||||||
|
const okRe = 'a'.repeat(200);
|
||||||
|
expect(
|
||||||
|
searchInFiles({ rootPath: tmpDir, query: okRe, isRegex: true }).length
|
||||||
|
).toBeGreaterThanOrEqual(0);
|
||||||
|
});
|
||||||
|
|
||||||
test('does not follow symlinks that escape rootPath', () => {
|
test('does not follow symlinks that escape rootPath', () => {
|
||||||
// Create a target outside tmpDir and a symlink inside pointing to it.
|
// Create a target outside tmpDir and a symlink inside pointing to it.
|
||||||
const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'search-outside-'));
|
const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'search-outside-'));
|
||||||
|
|||||||
Reference in New Issue
Block a user