feat(docker): implement exclude paths functionality #4057
Conversation
Add support for excluding paths in Docker source scanning: - Add ExcludePaths field to Docker protobuf - Implement path exclusion logic in docker.go - Add comprehensive test coverage for exact and wildcard path matching - Update engine to pass exclude paths configuration - Add CLI support for --exclude-paths flag The implementation supports: - Exact path matching (e.g., /var/log/test) - Wildcard path matching (e.g., /var/log/test/*) - Multiple exclude paths Tests ensure proper handling of: - Exact path exclusions - Wildcard exclusions - Edge cases and similar paths
|
|
|
Thank you for opening this PR @tannerjones4075. Can you sign the CLA? |
I have signed the CLA :) |
There was a problem hiding this comment.
Thanks for this PR! It's a very clear and straightforward improvement. In particular, thank you for the detailed description and the tests. I've found a couple of small improvements I'd like to make before getting this in that I think will substantially reduce our future maintenance burden - please see my inline comments.
|
I appreciate the feedback. I'll make some changes and add another commit. |
…on to compile once per pattern.
…075/trufflehog into docker-exclude-path
| pattern = "^" + pattern + "$" | ||
| regex, err := regexp.Compile(pattern) | ||
| if err != nil { | ||
| return fmt.Errorf("error compiling exclude path regex for %q: %w", path, err) |
There was a problem hiding this comment.
You might also consider adding the pattern being compiled to this error message; if the error is caused by something added during pattern generation, users are likely to be quite confused.
| for _, tf := range testFiles { | ||
| excluded := false | ||
| for _, excludePath := range s.excludePaths { | ||
| if tf.path == excludePath { |
There was a problem hiding this comment.
You're only checking for exact matches here, which means that you're not actually exercising the match logic you've added to the source. This test is effectively adding a single item to a slice and then asserting that a few other items are not in the slice. You don't need a whole source implementation to test that! I think you can just remove this test entirely.
| // Test if the path should be excluded | ||
| excluded := false | ||
| for _, path := range tt.excludePaths { | ||
| if tt.testPath == path { | ||
| excluded = true | ||
| break | ||
| } | ||
| // Convert wildcard pattern to regex | ||
| pattern := strings.ReplaceAll(path, "*", ".*") | ||
| pattern = "^" + pattern + "$" | ||
| isMatch, err := regexp.MatchString(pattern, tt.testPath) | ||
| assert.NoError(t, err) | ||
| if isMatch { | ||
| excluded = true | ||
| break | ||
| } | ||
| } | ||
| assert.Equal(t, tt.want, excluded) |
There was a problem hiding this comment.
I love the idea of this new test! But notice how s isn't involved in any of this test code. That means that it's not actually testing the source at all! This is why I suggested creating a method on Source that actually checks for exclusion. If you do that, you can use it in the source and also test it here.
| } | ||
| } | ||
|
|
||
| func TestDockerScanWithExclusions(t *testing.T) { |
|
One thing I didn't mention in earlier reviews - that I probably should have - is that the existing codebase has glob inclusion/exclusion implementation logic you could possibly reuse instead of adding this new regex-based technique. I'm not going to insist on it (because I didn't bring it up earlier) but it's probably worth at least giving it a glance to see if it would be helpful. |
Description:
Add support for excluding paths in Docker source scanning:
The implementation supports:
Tests ensure proper handling of:
References:
https://github.com/trufflesecurity/trufflehog/issues/2216?utm_source=chatgpt.com
Checklist:
make test-community)?make lintthis requires golangci-lint)?