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.

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(rtrim): remove regex to prevent ReDOS attack #1738

Merged
merged 1 commit into from Nov 1, 2021

Conversation

tux-tn
Copy link
Member

@tux-tn tux-tn commented Sep 27, 2021

This PR fixes a potential ReDOS in rtrim sanitizer. A try has been made in #1603 to fix the same vulnerability but it looks like we failed to prevent it.

The new implementation is not based on regex and is inspired by Steven Levithan's blog post and trim package implementation.

Thanks to @yetingli for discovering the vulnerability and huntr.dev for reporting it

Checklist

  • PR contains only changes related; no stray files, etc.

@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #1738 (b21879c) into master (c899b31) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1738   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          102       102           
  Lines         2015      2020    +5     
  Branches       454       454           
=========================================
+ Hits          2015      2020    +5     
Impacted Files Coverage Δ
src/lib/rtrim.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c899b31...b21879c. Read the comment docs.

return str.replace(pattern, '');
}
// Use a faster and more safe than regex trim method https://blog.stevenlevithan.com/archives/faster-trim-javascript
let strIndex = str.length - 1;
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering why this is not in the else statement, but I might less understand that because we have done return already.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's exactly that. If an if block contains a return statement, the else block is unnecessary and the instructions after the block will only be executed when the if condition is not met!

@profnandaa
Copy link
Member

@tux-tn -- sorry missed on this one, will get it in for the release. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants