-
Notifications
You must be signed in to change notification settings - Fork 516
feat: add --exclude flag to skip paths during scanning #2458
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
base: main
Are you sure you want to change the base?
feat: add --exclude flag to skip paths during scanning #2458
Conversation
Add --exclude/-e CLI flag that accepts glob patterns to exclude files and directories from vulnerability scanning. This addresses user requests to easily skip test files and documentation during scans. Changes: - Add ExcludePatterns field to ScannerActions struct - Add --exclude flag to scan source command - Wire ExcludePatterns to scalibr's SkipDirGlob option - Document the new flag in scan-source.md Fixes google#2324
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.
This is definitely something we want, though might need a bit more design before including it. It is really hard for us to justify removing a flag once it's added.
I want a way for people to specify regex patterns without having to add another separate flag. Maybe something like if it is wrapped /<regex>/ like in javascript?
…at/exclude-paths-flag
- Replace regexp import with cachedregexp to satisfy depguard rules - Add Regexp type alias and Compile function to cachedregexp package - Remove duplicate package doc comment (godoclint) - Add blank line before return (nlreturn) - Add t.Parallel() calls to test functions (paralleltest)
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.
Appreciate your work on this!
Could you please:
- move the flag to be
--experimental-(this is mainly because right now scalibr only supports skipping directories, not files which'll hopefully be able to change but in the meantime this will mean we can do breaking changes if we go another direction instead) - update the docs to reflect that this only applies to directories, not files
- add a couple of cmd tests
- update the syntax to use
:, matching our--lockfileflag, which'll also let us useDirsToSkiptoo- if the string is prefixed with
:or nothing, it should be applied as a directory (DirsToSkip) - if the string is prefixed with
g:, it should be applied as a glob (SkipDirGlob) - if the string is prefixed with
r:, it should be applied as a regex (SkipDirRegex)
- if the string is prefixed with
to clarify, also if it is not prefixed, use DirsToSkip, the prefixing with |
…it glob, regex, and exact match patterns.
…or exact, glob, and regex patterns, and update `go.mod` to directly import `gobwas/glob`.
…-dir arguments and patterns.
|
@another-rex sir, I have done the changes suggested you. |
…o exclude directories during scanning.
… to experimental actions.
… integration test.
|
I initially removed the type alias as you suggested, but CI failed with a depguard error: The Should I look into adding an exception to the depguard config for this file, or is the type alias acceptable given this constraint? |
|
right yeah of course - I think for now let's go with adding the exception as that mirrors what we're doing elsewhere for this situation, though I think I'll revisit that after this as it'll probably be better to just re-export stuff from (I had forgotten we pretty much don't use regexp outside of |
…r's `regexp` package usage check.
G-Rath
left a comment
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.
looking good, we just also do want to change the cmd flag to use exclude too
Co-authored-by: Gareth Jones <3151613+G-Rath@users.noreply.github.com>
Co-authored-by: Gareth Jones <3151613+G-Rath@users.noreply.github.com>
…e` and update its usage and documentation.
… multiple flags.
…parseExcludePatterns` test.
another-rex
left a comment
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.
Looks good!
|
/gemini review |
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.
Code Review
This pull request introduces a new experimental flag --experimental-exclude to allow excluding paths from scanning. The implementation is well-structured, with new logic for parsing exclusion patterns, comprehensive tests, and clear documentation. I've found one issue with how absolute paths are handled, which could lead to silent failures. My detailed feedback is in the review comments. Also, note that the PR description and title mention --exclude, but the implementation uses --experimental-exclude, which is correctly reflected in the documentation.
|
Hmm the requested interactions not found errors are a bit odd in the CI, because it looks like you have added the new interactions to the cassettes. @G-Rath Can you take a look at why that could be happening? |
|
I think they might just need to be regenerated, as they're still using the old First I'd do is just revert all changes to |
Overview
Issue: #2324
This PR adds a
--excludeCLI flag that allows users to specify glob patterns to exclude files and directories from vulnerability scanning.Fixes #2324
Details
Problem
Users scanning large repositories often want to exclude test files, documentation, and other non-production code from vulnerability scans. While
.gitignoreis respected by default and osv-scanner.toml can be used, there was no quick CLI option to exclude paths.Solution
Added
--exclude/-eflag that accepts glob patterns: