Merge pull request #4 from Trott/fix-it-again

fix: remediate ReDOS further

Author: stevemao

index.js

@@ -1,7 +1,16 @@
 'use strict';
 
-var regex = /^(?:\r|\n)+|(?:\r|\n)+$/g;
+var regex = /[^\r\n]/;
 
 module.exports = function (str) {
-	return str.replace(regex, '');
+	var result = str.match(regex);
+	if (!result) {
+		return '';
+	}
+	var firstIndex = result.index;
+	var lastIndex = str.length - 1;
+	while (str[lastIndex] === '\r' || str[lastIndex] === '\n') {
+		lastIndex--;
+	}
+	return str.substring(firstIndex, lastIndex + 1);
 };

package.json

@@ -35,7 +35,7 @@
     "delete"
   ],
   "devDependencies": {
-    "mocha": "*",
+    "mocha": "^3.5.3",
     "xo": "^0.17.1"
   },
   "xo": {

test.js

@@ -21,8 +21,71 @@ it('should trim off \\r\\n', function () {
 });
 
 it('should not be susceptible to exponential backtracking', function () {
+	var redosString = 'a';
+	var count = 1000;
+	while (count) {
+		redosString += '\r\n';
+		count--;
+	}
+	redosString += 'a';
+
+	var longerRedosString = redosString;
+	count = 1000;
+	while (count) {
+		longerRedosString += redosString;
+		count--;
+	}
+
+	var start = Date.now();
+	trimOffNewlines(redosString);
+	trimOffNewlines(longerRedosString);
+	var end = Date.now();
+	assert.ok(end - start < 1000, 'took too long, susceptible to ReDoS?');
+});
+
+it('should be performant on very long strings', function () {
+	var longOrdinaryString = 'aa';
+	var count = 27;
+	while (count) {
+		longOrdinaryString += longOrdinaryString;
+		count--;
+	}
+	assert.strictEqual(longOrdinaryString.length, 268435456);
+
 	var start = Date.now();
-	trimOffNewlines('a' + '\r\n'.repeat(1000) + 'a');
+	trimOffNewlines(longOrdinaryString);
 	var end = Date.now();
-	assert.ok(end - start < 1000, 'took too long, probably susceptible to ReDOS');
+	assert.ok(end - start < 1000, 'took too long, performance issue?');
+});
+
+it('should be performant in worst-case', function () {
+	// In the current algorithm, this is likely a worst-case:
+	// non-newline character followed by many newline characters.
+
+	this.timeout(10000);
+
+	var worstCaseString = '\r\n';
+	var count = 27;
+	while (count) {
+		worstCaseString += worstCaseString;
+		count--;
+	}
+	worstCaseString = 'a' + worstCaseString;
+	assert.strictEqual(worstCaseString.length, 268435457);
+	var start = Date.now();
+	trimOffNewlines(worstCaseString);
+	var end = Date.now();
+	assert.ok(end - start < 5000, 'worst case took too long, performance issue?');
+});
+
+it('should leave newlines in the middle of a string alone', function () {
+	assert.strictEqual(trimOffNewlines('Come on,\nFhqwhgads.'), 'Come on,\nFhqwhgads.');
+});
+
+it('should leave spaces at start and end alone', function () {
+	assert.strictEqual(trimOffNewlines(' fhqwhgads '), ' fhqwhgads ');
+});
+
+it('should return an empty string if there are only \\r and \\n', function () {
+	assert.strictEqual(trimOffNewlines('\r\n\r\r\n\n'), '');
 });