Skip to content
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

Feat: exclude flag #199

Closed
wants to merge 10 commits into from
Closed

Conversation

Tzurrr
Copy link

@Tzurrr Tzurrr commented Jan 14, 2024

Closes #194

Proposed Changes

I've added the option to use "--exclude" flag. this flag allows the user to write file/folder name to exclude.
I was debating whether to do seperated flags for folders and files, but it seems to me like too much duplicated code.
Because I didnt seperated it - now if a user have a file within a folder that named the same and the he wants to exclude only the file - the whole folder will be excluded. I would like to hear your opinion about the case I mentioned above
I submit this contribution under the Apache-2.0 license.

@baruchiro baruchiro self-requested a review January 15, 2024 17:53
Copy link
Contributor

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Many thanks!!

Sorry for the delay, I'm on reserve duty.

Thank you for joining to our project.

cmd/main.go Outdated Show resolved Hide resolved
secrets/secrets.go Outdated Show resolved Hide resolved
secrets/secrets.go Outdated Show resolved Hide resolved
Copy link
Contributor

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Many thanks!!

Sorry for the delay, I'm on reserve duty.

Thank you for joining to our project.

@Tzurrr Tzurrr changed the title Feature/exclude flag Feat: exclude flag Jan 15, 2024
@Tzurrr
Copy link
Author

Tzurrr commented Jan 15, 2024

still WIP, I have to add actual support on glob and comma-separated list and fix the "totalitemsscanned" bug

@baruchiro baruchiro marked this pull request as draft January 16, 2024 07:07
@baruchiro
Copy link
Contributor

still WIP, I have to add actual support on glob and comma-separated list and fix the "totalitemsscanned" bug

Converted to a draft PR.

@Tzurrr Tzurrr marked this pull request as ready for review January 17, 2024 19:15
@Tzurrr Tzurrr requested a review from baruchiro January 17, 2024 19:16
@Tzurrr
Copy link
Author

Tzurrr commented Jan 17, 2024

@baruchiro Hey, I made the changes you suggested

@Tzurrr
Copy link
Author

Tzurrr commented Jan 22, 2024

@baruchiro

@baruchiro
Copy link
Contributor

I will be back next week, sorry.

Copy link
Contributor

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work. This feature is more complicated than we thought.

I played with your implementation, and first, I didn't manage to make it work, I tried these options:

$ go run . filesystem --path . --exclude-paths secrets
$ go run . filesystem --path . --exclude-paths ./secrets
$ go run . filesystem --path . --exclude-paths ./secrets/*
$ go run . filesystem --path . --exclude-paths ./secrets/**/*

The main issue here, I think, is that you're testing the filepath.Glob(filename) on the current working directory, which is not always the scanning directory.

If you think about that more, you will understand that every plugin will consider the "working directory" differently, or even will not use a directory at all (such as the Confluence plugin, which is accessing through API).
It is what I mentioned here. I think you need to start with implementing it only for the filesystem and the git plugins, where we know for sure this filter needs to work.

@@ -89,7 +92,7 @@ func Execute() (int, error) {
rootCmd.PersistentFlags().StringSliceVar(&secretsConfigVar.SpecialList, specialRulesFlagName, []string{}, "special (non-default) rules to apply.\nThis list is not affected by the --rule and --ignore-rule flags.")
rootCmd.PersistentFlags().Var(&ignoreOnExitVar, ignoreOnExitFlagName, "defines which kind of non-zero exits code should be ignored\naccepts: all, results, errors, none\nexample: if 'results' is set, only engine errors will make 2ms exit code different from 0")
rootCmd.PersistentFlags().IntVar(&secretsConfigVar.MaxTargetMegabytes, maxTargetMegabytesFlagName, 0, "files larger than this will be skipped.\nOmit or set to 0 to disable this check.")

rootCmd.PersistentFlags().StringSliceVar(&excludedPathsVar, excludedPathsFlagName, []string{}, "exclude paths from scan. supports glob and can be provided multiple times or as a quoted comma separated string example: './shouldNotScan/*,somefile.txt'")
rootCmd.AddCommand(secrets.GetRulesCommand(&secretsConfigVar))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rootCmd.AddCommand(secrets.GetRulesCommand(&secretsConfigVar))
rootCmd.PersistentFlags().StringSliceVar(&excludedPathsVar, excludedPathsFlagName, []string{}, "exclude paths from scan.\nsupports glob and can be provided multiple times or as a quoted comma separated string\nexample: './shouldNotScan/*,somefile.txt'")

Make it multiline

Comment on lines +165 to +178
func resolveGlobExpression(excludedFileNames []string) map[string]bool {
var matchedFileNames = make(map[string]bool)
for _, filename := range excludedFileNames {
matched, err := filepath.Glob(filename)
if err != nil {
log.Error().Msgf("failed to get exclude path %s: %s", filename, err)
} else {
for _, value := range matched {
matchedFileNames[value] = true
}
}
}
return matchedFileNames
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move it to a new file called glob.go or something under the lib folder.

@baruchiro
Copy link
Contributor

Hi @Tzurrr, as we discussed on #194, this feature should be implemented for each relevant plugin. It will not be a global argument.

It is already implemented for the filesystem plugin, and we need to implement it for git, confluence, and paligo.
I suggest that each plugin will be implemented in a separate PR.

Let me know what you think, thank you very much for your work, and sorry about the changes.

You can contact me at [email protected] or on Discord.

@baruchiro baruchiro self-requested a review February 28, 2024 11:17
@baruchiro baruchiro self-assigned this Feb 28, 2024
@baruchiro baruchiro mentioned this pull request Feb 28, 2024
@baruchiro
Copy link
Contributor

Closing this, thank you @Tzurrr.

I'm inviting you to continue the work in a new PR, and implement the filter for the git plugin.

@baruchiro baruchiro closed this Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

File exclusion
2 participants