Add glob support for lcov-file-paths - #18
Conversation
| for (const pattern of patterns) { | ||
| assertSafePattern(pattern); | ||
|
|
||
| const globber = await glob.create(pattern, { | ||
| followSymbolicLinks: false, | ||
| matchDirectories: false, | ||
| }); | ||
| const matches = await globber.glob(); | ||
|
|
||
| if (matches.length === 0) { | ||
| throw new Error(`No file(s) found matching "${pattern}"`); |
There was a problem hiding this comment.
🔵 Low - Brace-expansion glob patterns are broken by input tokenization
The new resolver receives patterns after readInputs has split the input on commas, so a standard glob such as coverage/{unit,integration}/lcov.info becomes two malformed patterns (coverage/{unit and integration}/lcov.info). The resolver then treats each fragment as a separate required match and fails with a no-files-found error, preventing coverage upload even though the original glob would match valid reports. This makes a common glob expression unusable under the newly advertised glob support.
Show fix
Preserve glob expressions while parsing the input, for example by using newline-delimited entries as the unambiguous separator or by parsing commas only when they are outside brace expressions.
More info - Reply on this comment to give feedback or ignore the issue.
There was a problem hiding this comment.
This is a fair point, and the README only documents newlines as a separator, so not sure if we should just not separate on commas.
There was a problem hiding this comment.
Turns out this isn't an issue in @actions/glob:
https://github.com/actions/toolkit/blob/193fa46c20fde8b0ed54194bc08b841c78c0776d/packages/glob/src/internal-pattern.ts#L127
119dd6c to
c0943f8
Compare
c0943f8 to
b7bdf2f
Compare
| throw new Error('Invalid file path'); | ||
| } | ||
|
|
||
| contents.push(await fs.readFile(path.resolve(inputPath), 'utf8')); |
There was a problem hiding this comment.
Potential file inclusion attack via reading file - medium severity
If an attacker can control the input leading into the ReadFile function, they might be able to read sensitive files and launch further attacks with that information.
Show fix
Remediation: Ignore this issue only after you've verified or sanitized the input going into this function. This issue is only relevant in the backend, not in the frontend!
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| const resolvedPath = path.resolve(match); | ||
| const relativePath = path.relative(cwd, resolvedPath); | ||
|
|
||
| if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { |
There was a problem hiding this comment.
The relativePath.startsWith('..') guard can reject valid in-workspace paths like ..foo/..., but reports them as outside workspace. The boundary check logic is broader than the stated condition.
| if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { | |
| if (relativePath === '..' || relativePath.startsWith(`..${path.sep}`) || path.isAbsolute(relativePath)) { |
Details
✨ AI Reasoning
The code is trying to block matches that resolve outside the workspace. However, the outside-workspace check treats any relative path starting with two dots as invalid. A valid in-workspace path segment can also begin with two dots (for example, a directory named with that prefix), so this condition can reject valid files while reporting they are outside. That makes the control-flow assumption behind the error message too broad and logically inconsistent with the intended boundary check.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| const matches = await globber.glob(); | ||
|
|
||
| if (matches.length === 0) { | ||
| throw new Error(`No file(s) found matching "${pattern}"`); | ||
| } | ||
|
|
||
| return Promise.all( | ||
| matches.sort().map(async (match) => { | ||
| const resolvedPath = path.resolve(match); |
There was a problem hiding this comment.
🟡 Medium - Unbounded glob expansion can exhaust the runner while reading coverage inputs
The new resolver materializes every file matched by a user-supplied pattern, sorts and lstats the entire result, and applies no match-count or size limit. A broad pattern such as **/* consequently sends all workspace files into mergeLcov, which reads every file fully into the retained contents array before parsing, with additional gzip/base64 payload amplification during upload. On ordinary monorepos containing dependencies or build artifacts, this can consume substantial memory and CPU or cause the action to be killed, turning a malformed or overly broad coverage pattern into a CI availability failure.
Show fix
Bound the number of matches and total/per-file bytes before reading, and restrict glob results to plausible LCOV report files (or otherwise reject overly broad patterns). Stream or incrementally process inputs rather than retaining every matched file and the encoded upload payload simultaneously.
More info - Reply on this comment to give feedback or ignore the issue.
| const resolvedPath = path.resolve(match); | ||
| const relativePath = path.relative(cwd, resolvedPath); | ||
|
|
||
| if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { | ||
| throw new Error(`Invalid file path: "${pattern}" resolved outside the workspace`); | ||
| } | ||
|
|
||
| // followSymbolicLinks: false above only stops glob from descending into symlinked | ||
| // directories; it still returns a symlinked file as a match, so check explicitly. | ||
| if ((await fs.lstat(resolvedPath)).isSymbolicLink()) { | ||
| throw new Error(`Invalid file path: "${pattern}" matched a symlink, which is not allowed`); | ||
| } | ||
|
|
||
| return resolvedPath; |
There was a problem hiding this comment.
🟡 Medium - Symlinked parent directories bypass the workspace containment check
The resolver validates only the final matched entry with lstat, while the path.relative check is purely lexical and does not canonicalize parent components. A workflow pattern such as link-to-outside/secret.info can therefore match a regular file through a symlinked directory, pass both checks, and then be dereferenced by fs.readFile in main.js or mergeLcov. This lets a matched path read and upload a file outside the workspace despite the new symlink protection.
Show fix
| const resolvedPath = path.resolve(match); | |
| const relativePath = path.relative(cwd, resolvedPath); | |
| if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { | |
| throw new Error(`Invalid file path: "${pattern}" resolved outside the workspace`); | |
| } | |
| // followSymbolicLinks: false above only stops glob from descending into symlinked | |
| // directories; it still returns a symlinked file as a match, so check explicitly. | |
| if ((await fs.lstat(resolvedPath)).isSymbolicLink()) { | |
| throw new Error(`Invalid file path: "${pattern}" matched a symlink, which is not allowed`); | |
| } | |
| return resolvedPath; | |
| const resolvedPath = path.resolve(match); | |
| // Reject symlinked files, and canonicalize parent components before checking containment. | |
| if ((await fs.lstat(resolvedPath)).isSymbolicLink()) { | |
| throw new Error(`Invalid file path: "${pattern}" matched a symlink, which is not allowed`); | |
| } | |
| const canonicalPath = await fs.realpath(resolvedPath); | |
| const relativePath = path.relative(cwd, canonicalPath); | |
| if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { | |
| throw new Error(`Invalid file path: "${pattern}" resolved outside the workspace`); | |
| } | |
| return canonicalPath; |
More info - Reply on this comment to give feedback or ignore the issue.
Make inputs for complex monorepo setups simpler by accepting a glob pattern for
lcov-file-paths.Example of what this helps with here.