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: Ensure globbing doesn't include subdirectories #16272
Changes from 4 commits
46017f1
315dd71
dd9612c
1f9ccdc
8fc387c
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 |
---|---|---|
|
@@ -15,6 +15,7 @@ const fsp = fs.promises; | |
const isGlob = require("is-glob"); | ||
const globby = require("globby"); | ||
const hash = require("../cli-engine/hash"); | ||
const minimatch = require("minimatch"); | ||
|
||
//----------------------------------------------------------------------------- | ||
// Errors | ||
|
@@ -126,7 +127,7 @@ async function findFiles({ | |
const filePaths = patterns.map(filePath => path.resolve(cwd, filePath)); | ||
const stats = await Promise.all( | ||
filePaths.map( | ||
filePath => fsp.stat(filePath).catch(() => {}) | ||
filePath => fsp.stat(filePath).catch(() => { }) | ||
) | ||
); | ||
|
||
|
@@ -157,6 +158,11 @@ async function findFiles({ | |
return false; | ||
} | ||
|
||
// patterns starting with ** always apply | ||
if (filePattern.startsWith("**")) { | ||
return true; | ||
} | ||
|
||
// patterns ending with * are not used for file search | ||
if (filePattern.endsWith("*")) { | ||
return false; | ||
|
@@ -167,11 +173,27 @@ async function findFiles({ | |
return false; | ||
} | ||
|
||
// check if the pattern would be inside the cwd or not | ||
// check if the pattern would be inside the config base path or not | ||
const fullFilePattern = path.join(cwd, filePattern); | ||
const relativeFilePattern = path.relative(configs.basePath, fullFilePattern); | ||
const patternRelativeToConfigBasePath = path.relative(configs.basePath, fullFilePattern); | ||
|
||
if (patternRelativeToConfigBasePath.startsWith("..")) { | ||
return false; | ||
} | ||
|
||
// check if the pattern matches | ||
if (minimatch(filePath, path.dirname(fullFilePattern))) { | ||
return true; | ||
} | ||
|
||
// check if the pattern is inside the directory or not | ||
const patternRelativeToFilePath = path.relative(filePath, fullFilePattern); | ||
|
||
if (patternRelativeToFilePath.startsWith("..")) { | ||
return false; | ||
} | ||
Comment on lines
+189
to
+194
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. This won't work well when For example: // eslint.config.js
module.exports = {
files: ["foo/**/*.md"],
// ...configuration for .md files
}; Command 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 also doesn't work when the pattern starts with a non-literal (except for the special-cased For example, if the config entry has 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. Hmmm, yeah, this is the trickiest part of all of this. Do you have any suggestions? 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. For this step (filtering out patterns that can't match inside the given directory), I think we could use minimatch with 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. Some alternative approaches:
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. Thanks for the suggestions. These were super helpful. I went with a |
||
|
||
return !relativeFilePattern.startsWith(".."); | ||
return true; | ||
}) | ||
.map(filePattern => { | ||
if (filePattern.startsWith("**")) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
module.exports = [ | ||
{ | ||
files: ["subdir/*.js"] | ||
} | ||
]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = /* intentional syntax error */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
function( {} // intentional syntax error |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
foo; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = true; |
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.
I still think we would need
{ partial: true }
for this approach.Consider the following example.
Config:
Command:
The
"t*/internal-rules/*.js"
pattern from the config should not be filtered out, because it can match something under thetools
directory?Current behavior:
I think what we need is:
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.
Ah gotcha. I think I misunderstood the purpose of
partial: true
.