feat: adds includePattern option #63
Conversation
There was a problem hiding this comment.
See #62 (comment); this seems like something your babel config can do without any individual transform needing to participate.
| @@ -1,3 +1,6 @@ | |||
| # IDE configs | |||
| .idea | |||
There was a problem hiding this comment.
individual IDE configs should go in your global gitignore, not in every project you happen to touch.
There was a problem hiding this comment.
Best advice I've gotten all day.
| if (ignorePattern) { | ||
| if (includePattern) { | ||
| // Only set the includeRegex once: | ||
| includeRegex = includeRegex || new RegExp(includePattern); |
There was a problem hiding this comment.
it's dangerous to pass user input into RegExp; this is a DOS attack vector, for example.
There was a problem hiding this comment.
@ljharb In case you come around to the idea of adding this option, might you suggest how I could resolve this issue? I've copied the behavior of the ignorePattern almost exactly: https://github.com/airbnb/babel-plugin-inline-react-svg/pull/63/files#diff-1fdf421c05c1140f6d71444ea2b27638R65
There was a problem hiding this comment.
Instead of supporting regex, it should only support globs (gitignore syntax) - you can use https://npmjs.com/glob for that, i think
There was a problem hiding this comment.
Actually I've been trying to find some literature about this vulnerability - this is the best I can do so far: https://www.owasp.org/index.php/Regular_expression_Denial_of_Service_-_ReDoS.
It doesn't seem to me that this is a DOS risk (not like that at least). Is there precedent for protecting the engineer from doing this to his/herself? I suppose another babel preset could end up doing this - but still I'd have to have deliberately installed a malicious preset, right? Thoughts?
There was a problem hiding this comment.
In general, I’d consider a self-DOS to be a non-problem - however, avoiding footguns is a UX concern, and i consider regexes in configs to be an attractive pit of failure.
|
@mrassili sure. You're going to go to the trouble of using |
|
It’s about the UX to me more than a vulnerability. |
|
That seems reasonable. @mrassili you can have it if you want. It might be a week or so before I'd have the time. |
|
Unfortunately the fork was deleted, so this PR is unrecoverable. |
Closes #62