Skip to content

Add CodeQL Action#85

Merged
jonschlinkert merged 1 commit intomicromatch:masterfrom
XhmikosR:codeql
Nov 21, 2021
Merged

Add CodeQL Action#85
jonschlinkert merged 1 commit intomicromatch:masterfrom
XhmikosR:codeql

Conversation

@XhmikosR
Copy link
Copy Markdown
Contributor

This should flag a potential Polynomial ReDos issue I've been seeing on lgtm.com.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 92.325% when pulling d302a5c on XhmikosR:codeql into 6879725 on micromatch:master.

@XhmikosR
Copy link
Copy Markdown
Contributor Author

BTW @jonschlinkert how about disabling Coveralls comments? You can configure this on the Coveralls project page.

@jonschlinkert
Copy link
Copy Markdown
Member

potential Polynomial ReDos issue

Nice thanks for this PR! This might confirm my hunch for what was causing issues in Jest. I wasn't sure where specifically, but my guess was either backslashes (not from escaping per se, but users defining windows-style paths as globs, which is invalid), or negative lookbehinds.

However, shouldn't we only be testing the library itself? Meaning, is there an option to avoid flagging unit test fixtures as problematic? IMHO, for a couple of reasons it doesn't make sense to flag those fixtures:

  1. They're contrived and are intended to test coverage
  2. They're based on the bash tests
  3. Most of the fixtures themselves aren't regular expressions, they're globs that will be converted to regex.
  4. In some cases, I'm specifically testing for catastrophic backtracking or malicious regex.

Thoughts?

@jonschlinkert
Copy link
Copy Markdown
Member

how about disabling Coveralls comments

I don't really have an opinion on that, but I'd support whatever you want to do.

@XhmikosR
Copy link
Copy Markdown
Contributor Author

You can ignore any issues that you want later through the security repo tab. It should be possible to limit what CodeQL scans but this is simpler.

I don't really have an opinion on that, but I'd support whatever you want to do.

I'd definitely disable comments because they are just noise since there is a status check already :)

@jonschlinkert
Copy link
Copy Markdown
Member

Another side note, while we're on this topic of ReDoS... Have you seen any other JavaScript-based libraries or solutions besides safe-regex for checking for regex vulnerabilities? I don't think safe-regex is doing star-heigh calculations correctly. I'm not sure about it's dependency (vuln-regex-detector), but at a quick glance it also seems to be doing naive calculations that will result in false positives and negatives.

@XhmikosR
Copy link
Copy Markdown
Contributor Author

@jonschlinkert not sure how to mitigate the issues, but I'd start by using CodeQL and/or LGTM.

@jonschlinkert
Copy link
Copy Markdown
Member

sorry for taking such a long time on this. I had some unexpected changes this past year, but I'm getting back up to speed.

@jonschlinkert jonschlinkert merged commit 719d348 into micromatch:master Nov 21, 2021
@XhmikosR XhmikosR deleted the codeql branch November 23, 2021 08:18
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