Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix regex in utils.stackTraceFilter to prevent ReDoS #3416 #3686

Merged
merged 1 commit into from Jan 30, 2019
Merged

fix regex in utils.stackTraceFilter to prevent ReDoS #3416 #3686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions lib/utils.js
View file
Open in desktop
Expand Up @@ -671,8 +671,6 @@ exports.stackTraceFilter = function() {
function isMochaInternal(line) {
return (
~line.indexOf('node_modules' + slash + 'mocha' + slash) ||
~line.indexOf('node_modules' + slash + 'mocha.js') ||
plroebuck marked this conversation as resolved.
Show resolved Hide resolved
~line.indexOf('bower_components' + slash + 'mocha.js') ||
~line.indexOf(slash + 'mocha.js')
plroebuck marked this conversation as resolved.
Show resolved Hide resolved
plroebuck marked this conversation as resolved.
Show resolved Hide resolved
);
}
Expand Down Expand Up @@ -701,7 +699,7 @@ exports.stackTraceFilter = function() {
}

// Clean up cwd(absolute)
if (/\(?.+:\d+:\d+\)?$/.test(line)) {
if (/:\d+:\d+\)?$/.test(line)) {
line = line.replace('(' + cwd, '(');
plroebuck marked this conversation as resolved.
Show resolved Hide resolved
plroebuck marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down
57 changes: 57 additions & 0 deletions test/unit/runner.spec.js
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ var Runner = mocha.Runner;
var Test = mocha.Test;
var Hook = mocha.Hook;
var path = require('path');
var fs = require('fs');
var noop = mocha.utils.noop;

describe('Runner', function() {
Expand Down Expand Up @@ -496,6 +497,62 @@ describe('Runner', function() {
runner.failHook(hook, err);
});
});

describe('hugeStackTrace', function() {
beforeEach(function() {
if (path.sep !== '/') {
this.skip();
}
});

it('should not hang if the error message is ridiculously long single line', function(done) {
var hook = new Hook();
hook.parent = suite;
var data = [];
// mock a long message
for (var i = 0; i < 10000; i++) data[i] = {a: 1};
var message = JSON.stringify(data);
plroebuck marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var err = new Error();
// Fake stack-trace
plroebuck marked this conversation as resolved.
Show resolved Hide resolved
err.stack = [message].concat(stack).join('\n');

runner.on('fail', function(hook, err) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')
          );

Copy link
Contributor Author

@cyjake cyjake Jan 30, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

expect(
err.stack
.split('\n')
.slice(1)
.join('\n'),
'to be',
stack.slice(0, 3).join('\n')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cyjake cyjake Jan 30, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
done();
});
runner.failHook(hook, err);
});
plroebuck marked this conversation as resolved.
Show resolved Hide resolved

it('should not hang if error message is ridiculously long multiple lines either', function(done) {
var hook = new Hook();
hook.parent = suite;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

var fpath = path.join(__dirname, '../../mocha.js');
var message = fs.readFileSync(fpath, 'utf8');
var err = new Error();
// Fake stack-trace
err.stack = [message].concat(stack).join('\n');

runner.on('fail', function(hook, err) {
expect(
err.stack
.split('\n')
.slice(-3)
.join('\n'),
'to be',
stack.slice(0, 3).join('\n')
Copy link
Contributor

@plroebuck plroebuck Jan 30, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See prettyErrStack comment above.

);
done();
});
runner.failHook(hook, err);
});
});
});

describe('abort', function() {
Expand Down