fix regex in utils.stackTraceFilter to prevent ReDoS #3416

[cyjake]

if the stack trace begins with a large error message (>= 20k charactors), and user leaves --full-trace disabled, utils.stackTraceFilter() takes ages to finish. Large error messages is quite possible when user makes containment assertions such as expect(content).to.contain(word).

Description of the Change

simplified the regex used in utils.stackTraceFilter() to boost performance therefore prevent ReDoS if the stack contains large error message.

Alternate Designs

It's possible to skip check error message in stack by skipping the first line but the regex fix won't change any behavior hence it is chosen.

Why should this be in core?

well, it's a bug introduced by the stack filtering feature.

Benefits

mocha won't hang even if users make assertions with large error messages, which is quite possible because containment check is used a lot.

Possible Drawbacks

None

Applicable issues

#3416 looks like one but it was because of an external library (still quite possible if chai makes large error message and assert.deepEqual don't, didn't check).

[coveralls]

Coverage increased (+0.04%) to 91.159% when pulling ec7699b on cyjake:master into 7fee3a3 on mochajs:master.

[boneskull]

thanks! LGTM

[boneskull]

will wait for someone else to review, but fine by me if it's merged thereafter

[juergba]

@cyjake I feel uncomfortable with this err.stack stuff, since I still don't understand it clearly. My platform is windows.

• I run your test case and your "end-to-end benchmark" example, both work with and without your changes. I didn't check the time though, but no obvious difference.
• Error.prototype.message is a standard property which contains a brief description of the error (definition of Mozilla.org). Do assertion errors (ab)use this property to log huge diffs? And when getting too large we have those hangers?
• why not pass err.message as second parameter to stackTraceFilter() and omit that regex?

[plroebuck]

OK. Think I'm good with the regex change to code in "lib/utils.js". But uncomfortable with your test here, which seems at odds with your issue. Why doesn't it just create a multi-line AssertionError.message and replace stack[0]? The for loop code seems... excessive-? to that task, and its contents are non-reproducible. Shouldn't message be of type String?

[cyjake]

@plroebuck ah, good catch. There actually are two factors that might slow down the regex test, a long single line (>= 40k takes ~1600ms), or a large message with multiple lines. The test case I constructed was trying to mimic the first case but it was done incorrectly. I've pushed the fix.

The for loop is the simplest way I can come up with to create a long and non-repeating text. Using fs.readFileSync(path.join(__dirname, '../mocha.js'), 'utf8') suffices the task but I'm not sure this unit test is node.js only or not.

[cyjake]

ha, turns out I wasn't very wrong.

> a = [1,2]
[ 1, 2 ]
> [a]
[ [ 1, 2 ] ]
> [a].concat(3).join('\n')
'1,2\n3'

[message].concat(stack).join(\n') just yields a large err.message with lots of ,. Anyway, I've updated the test code.

[plroebuck]

Could you add a test case for a large multi-line message to cover both triggers?

// L491
 describe('when the stacktrace error message is ridiculously long', function() {
   it('should not hang temporarily if error message is single line', function(done) {
     // existing test here
   });
   it('should not hang temporarily if error message is multiple lines', function(done) {
     // requested test here
   });
 });

[cyjake]

ah, guess I missed this one. I'm on it.

[plroebuck]

On L450 and L471, can we change the skip condition to be more apparent?

 if (path.sep !== '/') {

would become

 if (process.platform === 'win32') {

[boneskull]

yes, but it doesn't need to happen in this PR.

[cyjake]

@juergba Sorry for not catching your comment. Here's my two cents.

• You might've got a computer faster than mine. It should fail if you bump the length in the for loop of the mock function. Run the test with shorter timeout shall suffice too, such as ./bin/mocha.js test/unit/runner.spec.js --timeout 500.
• Yes, AssertionError use err.message to display failure message which might be (accidentally) very large because expect(content).to.contain(string) is quite common, at least in my projects.
• I did consider the approach of separating err.message from err.stack, but the changes involved is disproportionate compared to this one. The regex needs fix anyway, /\(?.+/ simply matches any character which makes no point.

[plroebuck]

        var fpath = path.join(__dirname, '..', '..', 'mocha.js');

[cyjake]

https://nodejs.org/api/path.html#path_path_sep path actually accepts both back and forward slash, manually separating them into parameters is unnecessary.

[plroebuck]

assertion is comparing the last three lines of err.stack against the first three lines of stack?

[cyjake]

I think I need to explain L546 a bit. err.stack is huge because of the enormous err.message, which makes cleaning the stack to be comparable to stack.slice(0, 3) a bit complicated. In theory, if err.stack is correctly filtered, the last 3 lines should match the first thee of stack, because the rest of stack is meant to be filtered out.

[plroebuck]

See prettyErrStack comment above.

[plroebuck]

assertion is comparing err.stack sans message against the first three lines of stack?

[cyjake]

yes, because the rest of stack should be filtered out.

[plroebuck]

function genMultiLineMessage() {
    var fpath = path.join(__dirname, '..', '..', 'mocha.js');
    return fs.readFileSync(fpath, 'utf8');
}
var message = genMultiLineMessage();

[plroebuck]

function genSingleLineMessage() {
  var data = [];
  for (var i = 0; i < 10000; i++) data[i] = {a: 1};
  return JSON.stringify(data);
}
var message = genSingleLineMessage();

[cyjake]

I think the test case title is self explanatory enough. The test case body isn't that long that needs this level of refactor. Can I pass on this one?

[plroebuck]

Each function would live outside the corresponding it() replacing the how with what was generated. Prefer self-documenting code (especially considering the number of questions this small PR has already brought up). But whatever...

[cyjake]

Let's just discuss in this thread. I believe the changes you proposed are all quite similar. Extracting three lines into a function, breaking one line into multiple or joining multiple into one, these are all personal preferences. This repo has already configured quite strict (far too strict in my opinion) rules, and I've done the changes to comply with them already.

And about the number of questions this small PR has already brought up , at least half of them were because you guys didn't read my PR description or the code carefully to understand in the first place. A week has passed since I requested this PR, four reviewers have reviewed the code, more than 55 comments were made, the real change that actually matters is to remove five characters \(?.+, and guess what, it is still not merged.

I understand that people get busy and stuff, but I'm done. Take it or leave it.

[plroebuck]

And about the number of questions this small PR has already brought up , at least half of
them were because you guys didn't read my PR description or the code carefully to
understand in the first place. A week has passed since I requested this PR, four reviewers
have reviewed the code, more than 55 comments were made, the real change that actually
matters is to remove five characters \(?.+, and guess what, it is still not merged.

I read your PR description. Problem is that you're making the assumption we're extremely familiar with the entirety of the codebase. Most of us aren't. As such, we have to learn what the original code does, figure out what changes you made, whether it could have been done differently, what is tested (or wasn't), and what possible breakage could come. We also have to maintain it afterwards, typically only having gained cursory knowledge of the original problem.
And all that takes unpaid time away from our families... Sorry we're too slow for your taste.

[plroebuck]

How about this to make it more clear:

          var prettyErrStack = err.stack.split('\n').slice(1)
          expect(
            prettyErrStack.join('\n'),
            'to be',
            stack.slice(0, 3).join('\n')
          );

[cyjake]

Actually I wrote a one-liner in the first place err.stack.split('\n').slice(1).join('\n'), which is frowned upon by the prettier pre-check and forced me to broke them into lines. Assigning an extra variable does reduce the lines but I do still prefer err.stack.split('\n').slice(1).join('\n') in the first place.

Anyway, var prettyErrStack = err.stack.split('\n').slice(1) is good, var prettyErrStack = err.stack.split('\n').slice(1).join('\n') is bad. This makes no sense.

[plroebuck]

Was trying to annotate (albeit poorly) that err.stack had changed behind the scenes by using different variable with "pretty" in its name.

Line length probably triggers Prettier to break things up (but I don't know what column).

[juergba]

@cyjake I didn't get your description at first, especially this Error.stack is non standard and quite fuzzy to catch. A few more words in your description would have helped me a lot. And it's true, most of us don't know every line of code in every detail.

@plroebuck what about one more round of reviewing, then we merge this PR and you do your adaptions later?

[plroebuck]

@juergba, I've said what I was going to. You have anything else? If not, let's just merge it and I'll make my changes to the tests afterwards.

[juergba]

@plroebuck I didn't follow all the latest comments, but yes let's merge it.