Skip to content

Commit 25246c6

Browse files
committed
Fix ReDoS vulnerability for the .end() method
The other methods do not have this vulnerability, but I’m refactoring them while I’m at it.
1 parent 7bd2272 commit 25246c6

2 files changed

Lines changed: 71 additions & 3 deletions

File tree

index.js

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,35 @@
11
export default function trimNewlines(string) {
2-
return string.replace(/^[\r\n]+|[\r\n]+$/g, '');
2+
let start = 0;
3+
let end = string.length;
4+
5+
while (start < end && (string[start] === '\r' || string[start] === '\n')) {
6+
start++;
7+
}
8+
9+
while (end > 0 && (string[end - 1] === '\r' || string[end - 1] === '\n')) {
10+
end--;
11+
}
12+
13+
return (start > 0 || end < string.length) ? string.slice(start, end) : string;
314
}
415

5-
trimNewlines.start = string => string.replace(/^[\r\n]+/, '');
6-
trimNewlines.end = string => string.replace(/[\r\n]+$/, '');
16+
trimNewlines.start = string => {
17+
const end = string.length;
18+
let start = 0;
19+
20+
while (start < end && (string[start] === '\r' || string[start] === '\n')) {
21+
start++;
22+
}
23+
24+
return start > 0 ? string.slice(start, end) : string;
25+
};
26+
27+
trimNewlines.end = string => {
28+
let end = string.length;
29+
30+
while (end > 0 && (string[end - 1] === '\r' || string[end - 1] === '\n')) {
31+
end--;
32+
}
33+
34+
return end < string.length ? string.slice(0, end) : string;
35+
};

test.js

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,19 @@ import test from 'ava';
22
import trimNewlines from './index.js';
33

44
test('main', t => {
5+
t.is(trimNewlines(''), '');
6+
t.is(trimNewlines(' '), ' ');
7+
t.is(trimNewlines('\n\n\r'), '');
58
t.is(trimNewlines('\nx\n'), 'x');
69
t.is(trimNewlines('\n\n\nx\n\n\n'), 'x');
710
t.is(trimNewlines('\r\nx\r\n'), 'x');
811
t.is(trimNewlines('\n\r\n\nx\n\r\n\n'), 'x');
912
});
1013

1114
test('start', t => {
15+
t.is(trimNewlines.start(''), '');
16+
t.is(trimNewlines.start(' '), ' ');
17+
t.is(trimNewlines.start('\n\n\r'), '');
1218
t.is(trimNewlines.start('\nx'), 'x');
1319
t.is(trimNewlines.start('\r\nx'), 'x');
1420
t.is(trimNewlines.start('\n\n\n\nx'), 'x');
@@ -17,9 +23,42 @@ test('start', t => {
1723
});
1824

1925
test('end', t => {
26+
t.is(trimNewlines.end(''), '');
27+
t.is(trimNewlines.end(' '), ' ');
28+
t.is(trimNewlines.end('\n\n\r'), '');
2029
t.is(trimNewlines.end('x\n'), 'x');
2130
t.is(trimNewlines.end('x\r\n'), 'x');
2231
t.is(trimNewlines.end('x\n\n\n\n'), 'x');
2332
t.is(trimNewlines.end('x\n\n\r\n\n'), 'x');
2433
t.is(trimNewlines.end('\n\n\r\n\nx'), '\n\n\r\n\nx');
2534
});
35+
36+
test('main - does not have exponential performance', t => {
37+
for (let index = 0; index < 45000; index += 1000) {
38+
const string = String(Array.from({length: index}).fill('\n').join('')) + 'a' + String(Array.from({length: index}).fill('\n').join(''));
39+
const start = Date.now();
40+
trimNewlines(string);
41+
const difference = Date.now() - start;
42+
t.true(difference < 10, `Execution time: ${difference}`);
43+
}
44+
});
45+
46+
test('start - does not have exponential performance', t => {
47+
for (let index = 0; index < 45000; index += 1000) {
48+
const string = String(Array.from({length: index}).fill('\n').join('')) + 'a';
49+
const start = Date.now();
50+
trimNewlines.start(string);
51+
const difference = Date.now() - start;
52+
t.true(difference < 10, `Execution time: ${difference}`);
53+
}
54+
});
55+
56+
test('end - does not have exponential performance', t => {
57+
for (let index = 0; index < 45000; index += 1000) {
58+
const string = 'a' + String(Array.from({length: index}).fill('\n').join(''));
59+
const start = Date.now();
60+
trimNewlines.end(string);
61+
const difference = Date.now() - start;
62+
t.true(difference < 10, `Execution time: ${difference}`);
63+
}
64+
});

0 commit comments

Comments
 (0)