feat: improve file type detection; consolidate file content functionality - #1246
Conversation
…lity Signed-off-by: egibs <20933572+egibs@users.noreply.github.com>
Signed-off-by: egibs <20933572+egibs@users.noreply.github.com>
Signed-off-by: egibs <20933572+egibs@users.noreply.github.com>
Signed-off-by: egibs <20933572+egibs@users.noreply.github.com>
| // typically, JSON and YAML files are data files only scanned via --all, but we want to support the NPM ecosystem | ||
| if strings.HasSuffix(path, "package.json") || strings.HasSuffix(path, "package-lock.json") { | ||
| return &FileType{ | ||
| Ext: ext, | ||
| MIME: "application/json", | ||
| } | ||
| } | ||
|
|
||
| if strings.HasSuffix(path, "pnpm-lock.yaml") || | ||
| strings.HasSuffix(path, "pnpm-workspace.yaml") || | ||
| strings.HasSuffix(path, "yarn.lock") { | ||
| return &FileType{ | ||
| Ext: ext, | ||
| MIME: "application/x-yaml", | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Not a blocker on this PR, but would it be better to split this out into a separate function; this would (a) isolate future changes for the ecosystem, as they decide on different tools, (b) you could have unit tests for it, and (c) if we decide to add similar functionality for other language ecosystems, it will give us a similar pattern to follow.
| // create a buffer sized to the minimum of the file's size or the default ReadBuffer | ||
| // only do so if we actually need to retrieve the file's contents | ||
| buf := readPool.Get(min(size, file.ReadBuffer)) //nolint:nilaway // the buffer pool is created above |
There was a problem hiding this comment.
Not needed for this PR (since this is not an issue added by this PR), but is the thing the linter whinging about here about failing to catch the potential failure of the pool Get() call, say in a resource constrained environment?
There was a problem hiding this comment.
That could be the case. The possibility of false positives when running nilaway is documented so it's likely not detecting that the pool is initialized previously (this is a common theme across the codebase). There could be cases where readPool may still be nil post-initialization but we always attempt this before ever running Get().
3 participants
Follow-up for #1234
This PR improves the filetype detection by scanning the entirety of a given file instead of a limited subset (previously 512 bytes). There are cases when a file's magic bytes are located near the end of the file or at arbitrary locations within it which means a partial approach won't be adequate in all cases.
As opposed to the previous PR, this PR maintains the separate
programkind.goandscan.goretrievals since we may not end up generating a file report once we determine a file's type (e.g., we may scan a file and subsequently filter it if the risk falls below the configured threshold).If we do end up generating a report, we'll retrieve the file contents again.
The alternatives here are:
File()File()and use the results where necessaryFile()Both would require updates in several places, so this is the best compromise between cleanliness and performance. I ran
make refresh-sample-testdataon a loop for a while and each run took 28-32 seconds which is about what we'd expect.I also added some more logic for NPM lockfiles, specifically:
package-lock.jsonpnpm-lock.yamlpnpm-workspace.yamlyarn.lockPreviously, these files would be considered data files and would not be scanned unless
--allwas used.