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: CVE-2023-26115 #33

Merged
merged 2 commits into from Jul 18, 2023
Merged

🔒 fix: CVE-2023-26115 #33

Changes from 1 commit
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions index.js
View file
Open in desktop
@@ -1,10 +1,16 @@
/*!
* 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.
*/

function trimTabAndSpaces(str) {
const lines = str.split('\n');
Copy link

Choose a reason for hiding this comment

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

may be using method chaining eliminates the need to create an intermediate variable ??
return str.split('\n').map(line => line.trimEnd()).join('\n');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup! can be done. I once just need verification the approach or whether it is important or not

Copy link

Choose a reason for hiding this comment

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

utilizing native String.prototype.trimEnd() would require the node-engine to be at least on version 10.0.0.

Ref: String.prototype.trimEnd() - JavaScript | MDN

This should probably considered a breaking-change which requires new major version release.
Hence I would recommend to utilize custom implementation for this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh! I can write trimEnd myself if that's the case

Copy link
Owner

Choose a reason for hiding this comment

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

For the reason mentioned by @mscheid-sf, for now let's merge in the regex I suggested in my other comment. After that I'd be happy to take another PR that replaces the regex with .trimEnd() (Using .trimEnd would indeed qualify as a breaking change according to semver, and also it's generally a good practice to minimize potential for regressions when releasing patches).

const trimmedLines = lines.map((line) => line.trimEnd());
return trimmedLines.join('\n');
}

module.exports = function(str, options) {
options = options || {};
if (str == null) {
Expand Down Expand Up @@ -36,7 +42,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

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

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 = trimTabAndSpaces(result);
}
return result;
};
Expand Down