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(fabric.parser): attempt to resolve some issues with regexp #7520

Merged
merged 2 commits into from
Dec 3, 2021

Conversation

asturur
Copy link
Member

@asturur asturur commented Nov 26, 2021

It looks like the regexp we use to parse CSS may be susceptible of high CPU usage with crafted payloads.
This would make servers that use fabric to parse SVGs at risk of crash or DOS.
This PR removes the regexes and uses a simpler approach to parse style rules.

@asturur
Copy link
Member Author

asturur commented Nov 26, 2021

@JamieSlome do you want to verify if this fix your concens?

@github-actions
Copy link
Contributor

github-actions bot commented Nov 26, 2021

Code Coverage Summary

> fabric@4.6.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.39 |     77.1 |   86.16 |   83.11 |                                               
 fabric.js |   83.39 |     77.1 |   86.16 |   83.11 | ...,30155,30280,30360-30425,30548,30647,30864 
-----------|---------|----------|---------|---------|-----------------------------------------------

@JamieSlome
Copy link

@asturur - thanks for the bump 🤛

I am just tagging the researcher @ready-research - can you take a look at the fix here?

@ready-research
Copy link

@asturur - Yeah, proposed changes LGTM. Sorry for the late reply.

@asturur
Copy link
Member Author

asturur commented Dec 3, 2021

why i can't merge? lol

@asturur
Copy link
Member Author

asturur commented Dec 3, 2021

@ShaMan123 can you try approve this PR?

@asturur asturur merged commit 4e0a872 into master Dec 3, 2021
@asturur asturur deleted the fix-bad-regexps branch December 3, 2021 09:50
rockerBOO pushed a commit to rockerBOO/fabric.js that referenced this pull request Jan 12, 2022
@asturur asturur mentioned this pull request Jan 26, 2022
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.

3 participants