-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat: adds includePattern option #63
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
# IDE configs | ||
.idea | ||
|
||
# gitignore | ||
node_modules | ||
lib | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import transformSvg from './transformSvg'; | |
import fileExistsWithCaseSync from './fileExistsWithCaseSync'; | ||
|
||
let ignoreRegex; | ||
let includeRegex; | ||
|
||
export default declare(({ | ||
assertVersion, | ||
|
@@ -49,9 +50,17 @@ export default declare(({ | |
if (typeof importPath !== 'string') { | ||
throw new TypeError('`applyPlugin` `importPath` must be a string'); | ||
} | ||
const { ignorePattern, caseSensitive, filename: providedFilename } = state.opts; | ||
const { ignorePattern, includePattern, caseSensitive, filename: providedFilename } = state.opts; | ||
const { file, filename } = state; | ||
if (ignorePattern) { | ||
if (includePattern) { | ||
// Only set the includeRegex once: | ||
includeRegex = includeRegex || new RegExp(includePattern); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's dangerous to pass user input into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok cool I'll give that a shot There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// Test if we should ignore this: | ||
if (!includeRegex.test(importPath)) { | ||
return; | ||
} | ||
} | ||
else if (ignorePattern) { | ||
// Only set the ignoreRegex once: | ||
ignoreRegex = ignoreRegex || new RegExp(ignorePattern); | ||
// Test if we should ignore this: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
individual IDE configs should go in your global gitignore, not in every project you happen to touch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best advice I've gotten all day.