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 CodeQL warning in indentation #5128

Closed
Tracked by #5205
jeddy3 opened this issue Feb 5, 2021 · 8 comments
Closed
Tracked by #5205

Fix CodeQL warning in indentation #5128

jeddy3 opened this issue Feb 5, 2021 · 8 comments
Labels
help wanted is likely non-trival and help is wanted status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule
Milestone

Comments

@jeddy3
Copy link
Member

jeddy3 commented Feb 5, 2021

The CI is failing on two CodeQL warnings:

Check failure on line 541 in lib/rules/indentation/index.js
Inefficient regular expression
This part of the regular expression may cause exponential backtracking on strings starting with '!\n' and containing many repetitions of '\n'.
Check failure on line 3808 in lib/utils/parseCalcExpression/parser.js
Inefficient regular expression
This part of the regular expression may cause exponential backtracking on strings starting with 'a(' and containing many repetitions of '''.

We'll need to fix these. The later is in generated code, so we may need to expedite #4731 (comment) and remove the function-calc-no-invalid rule.

@jeddy3 jeddy3 added type: infrastructure an improvement to devops help wanted is likely non-trival and help is wanted labels Feb 5, 2021
@jeddy3
Copy link
Member Author

jeddy3 commented Feb 6, 2021

This seems to have gone away.

If it reoccurs, there's a WIP branch to temporarily ignore the generated parser until we figure out what to do with it. The indentation regex needs tweaking in the branch.

@XhmikosR
Copy link
Member

This is the same issue I mentioned the other time @jeddy3 :) https://github.com/stylelint/stylelint/compare/XhmikosR-patch-1

https://lgtm.com/projects/g/stylelint/stylelint/?mode=list

I don't think it's a false positive, but I haven't looked at it closely.

Anyway, before ignoring CodeQL I'd make sure if the issue is valid (which it looks like it).

@jeddy3
Copy link
Member Author

jeddy3 commented Feb 11, 2021

This is the same issue I mentioned the other time @jeddy3 :)

Oh right, yes it is!

I'd make sure if the issue is valid (which it looks like it).

Yes, we should definitely fix the indentation issue. I'm not sure there's much we can do about the lib/utils/parseCalcExpression/parser.js one as that's generated code. Hence the suggestion to ignore until we can remove.

Did you want to PR your branch, or shall I port the Regex over to the fix-codeql branch?

@XhmikosR
Copy link
Member

I don't think my patch is correct, though. The original regex is supposed to match \r?\n followed by \s. The correct fix would be to actually exclude \r\n from \s or replace \s with something more specific.

Not familiar with the exact code hence why I didn't make a patch :)

@jeddy3
Copy link
Member Author

jeddy3 commented Feb 12, 2021

Regex isn't my strong suit. I've labelled the issue as "help wanted". Hopefully, someone more familiar with Regex and the rule code will jump in and update the regex in the fix-codeql branch.

@jeddy3 jeddy3 changed the title Fix CodeQL warnings Fix CodeQL warning in indentation May 12, 2021
@jeddy3 jeddy3 added type: bug a problem with a feature or rule status: ready to implement is ready to be worked on by someone and removed type: infrastructure an improvement to devops labels May 12, 2021
@jeddy3 jeddy3 mentioned this issue May 12, 2021
30 tasks
@jeddy3
Copy link
Member Author

jeddy3 commented May 18, 2021

I've deleted the fix-codeql branch.

The lib/utils/parseCalcExpression/parser.js file was removed in the v14 branch, leaving just the indentation warning to fix. Whoever picks this up, please branch off v14.

@dylmye
Copy link

dylmye commented Oct 14, 2021

Hi all, should we backport the fix created in #5539 to 13.x so users don't have to migrate to 14.x to benefit from this? Or have I got the wrong end of the stick here? Thanks

@jeddy3
Copy link
Member Author

jeddy3 commented Oct 14, 2021

14.x will likely be released next week. It's unlikely we'll backport the fix to 13.x nor maintain a 13.x branch going forward. This ReDoS issue only affects users running Stylelint in the browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted is likely non-trival and help is wanted status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule
Development

No branches or pull requests

4 participants