Skip to content

Commit

Permalink
Security fix for ReDoS (#3980)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ready-research
ready-research committed Aug 30, 2021
1 parent 5bc9ea2 commit 5b45711
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion lib/utils.js
View file
Expand Up @@ -185,7 +185,7 @@ function isURLSearchParams(val) {
* @returns {String} The String freed of excess whitespace
*/
function trim(str) {
return str.replace(/^\s*/, '').replace(/\s*$/, '');
return str.trim ? str.trim() : str.replace(/^\s+|\s+$/g, '');
}

/**
Expand Down

7 comments on commit 5b45711

@chiefkana
Copy link

Choose a reason for hiding this comment

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

What is the usage of self made trim function?

@muditjuneja
Copy link

Choose a reason for hiding this comment

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

Something related to this : https://app.snyk.io/vuln/SNYK-JS-AXIOS-1579269?

@vargaurav
Copy link

Choose a reason for hiding this comment

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

This is getting flagged in snyk.

@ErickRodrCodes
Copy link

@ErickRodrCodes ErickRodrCodes commented on 5b45711 Sep 6, 2021 •
edited

Choose a reason for hiding this comment

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

What is the usage of self made trim function?

Probably an intended custom made trim function with the intention to be faster... but ended in bloating resources...

@catscarlet
Copy link
Contributor

@catscarlet catscarlet commented on 5b45711 Sep 8, 2021 •
edited

Choose a reason for hiding this comment

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

But, but str.trim() should not only deal with \s but also deal with \uFEFF and \xA0.

The trim Polyfill was:

if (!String.prototype.trim) {
  String.prototype.trim = function () {
    return this.replace(/^[\s\uFEFF\xA0]+|[\s\uFEFF\xA0]+$/g, '');
  };
}

MDN(en) removed this part because of "outdated with WebView Android 37".
The other languages still have this Polyfill part.

See mdn/content#7602

@Teej42
Copy link

@Teej42 Teej42 commented on 5b45711 Sep 9, 2021

Choose a reason for hiding this comment

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

It is not clear to me, but was this fix added in v0.21.4 release, or will be added in the next release?

@jasonsaayman
Copy link
Member

Choose a reason for hiding this comment

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

Already added :) I think the custom trim function was used like this incase a browser or version of node did not have native support. I don't think we can drop it just yet due to supporting a pretty large range of browsers. However I will review some of that code when I have a chance and see if it would be possible to get rid of it.

Please sign in to comment.