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

🔒 fix: CVE-2023-26115 #33

Merged
merged 2 commits into from
Jul 18, 2023
Merged

🔒 fix: CVE-2023-26115 #33

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
235cfd4
:lock: fix: CVE-2023-26115
aashutoshrathi Mar 25, 2023
d6e8514
fix: settle for new regex to support lower node versions
aashutoshrathi Jun 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*!
* word-wrap <https://github.com/jonschlinkert/word-wrap>
*
* Copyright (c) 2014-2017, Jon Schlinkert.
* Copyright (c) 2014-2023, Jon Schlinkert.
* Released under the MIT License.
*/

Expand Down Expand Up @@ -36,7 +36,7 @@ module.exports = function(str, options) {
}).join(newline);

if (options.trim === true) {
result = result.replace(/[ \t]*$/gm, '');
Copy link

Choose a reason for hiding this comment

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

I guess /\s+$/gm works faster

Copy link
Contributor Author

@aashutoshrathi aashutoshrathi Mar 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, let me check on that.
But ideally, I would like to not use regex for the operation that can be easily handled by native trimEnd, right?

Copy link

Choose a reason for hiding this comment

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

maybe or native better regex can be faster. need to bench to check that

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @aashutoshrathi, sorry for the long delay, and thanks for the PR. I'll try to get this merged in ASAP.

I guess /\s+$/gm works faster

Yes, let's use this pattern without the m flag. Using + will also be a bit safer since it won't allow that pattern to match an empty string.

Copy link

@sig-holec sig-holec Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

/[ \t]+$/g is slightly better than /\s+$/g it would seem. Unfortunately both regexes are suspectable to redos, as opposed to trimEnd();

$ time node -e 'payloadstr=""; for ( i=0; i < 100000; i++ ) payloadstr+="\t"; payloadstr+="a"; payloadstr.replace(/[ \t]+$/gm,"");'
real 0m21.084s
$ time node -e 'payloadstr=""; for ( i=0; i < 100000; i++ ) payloadstr+="\t"; payloadstr+="a"; payloadstr.replace(/\s+$/g,"");'
real 0m8.587s
$ time node -e 'payloadstr=""; for ( i=0; i < 100000; i++ ) payloadstr+="\t"; payloadstr+="a"; payloadstr.replace(/[ \t]+$/g,"");'
real 0m6.169s
$ time node -e 'payloadstr=""; for ( i=0; i < 100000; i++ ) payloadstr+="\t"; payloadstr+="a"; payloadstr.trimEnd();'
real 0m0.082s

Copy link

Choose a reason for hiding this comment

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

@jonschlinkert Any update when this PR will be merged ?

result = result.replace(/\s+$/g, '');
Copy link

Choose a reason for hiding this comment

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

Not sure if you guys consider but, according to couple of checkers, this also a vulnerable regexp
https://devina.io/redos-checker
https://makenowjust-labs.github.io/recheck/playground/
image

Copy link

Choose a reason for hiding this comment

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

Introducing a new linter rule could be a necessary step to move forward
https://www.npmjs.com/package/eslint-plugin-redos

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 we can't do this with regex

Copy link

Choose a reason for hiding this comment

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

String.trimEnd

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 also think that the first commit was better.
But I think Node engines below v10 doesn't support stuff

Copy link

Choose a reason for hiding this comment

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

@aashutoshrathi @jonschlinkert

Given we don't have a non-vulnerable regex solution and there's concern for older versions of Node.js, can we use this instead?

while (result.substring(result.length - 1) === ' ' || result.substring(result.length - 1) === '\t') {
    result = result.substring(0, result.length - 1)
}

Copy link

Choose a reason for hiding this comment

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

@aashutoshrathi @jonschlinkert @seahindeniz

How about this?

result = result.replace(/([^ \t])([ \t]*)$/gm, '$1');

This assumes every line will start with something other than a space or tab. The $1 will preserve whatever that character is. I confirmed that this is not a vulnerable regex and that all unit tests pass.

Copy link

Choose a reason for hiding this comment

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

@aashutoshrathi have you tried this suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not yet, will do

Copy link

@sig-holec sig-holec Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This works just as well as .trimEnd();
$ time node -e 'payloadstr=""; for ( i=0; i < 100000; i++ ) payloadstr+="\t"; payloadstr+="a"; payloadstr.replace(/([^ \t])([ \t]*)$/gm, "$1");'
real 0m0.087s

}
return result;
};
Expand Down