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

Respect plugin list from baseline #121

Closed
killuazhu opened this issue Jan 31, 2019 · 7 comments
Closed

Respect plugin list from baseline #121

killuazhu opened this issue Jan 31, 2019 · 7 comments
Assignees

Comments

@killuazhu
Copy link
Contributor

Some of the plugins, in particular, entropy-based and keyword plugins, can generate a relatively high number of false positives. When some of our teams are using detect-secrets, they choose to exclude certain plugins (with or without the combination of excluding some files). Currently, if you run a scan with --no-xxx-scan option, the used plugin list would be persisted in the baseline file.

If some developer or automation system picks up the repo, have no pre-commit hook setup and also unaware of the exclude list, they could run into the issue that they issue detect-secrets --update baseline, then the baseline file is regenerated with all plugins used.

Would the community entertain the idea that detect-secrets --update baseline scan use the plugin list from baseline instead of all plugins (default setting)? Some additional options can be added if you want to use more plugins than baseline ones to scan the repo.

We have something implemented in our fork (offline in our GHE), we'd like to hear some feedback on the problem before submitting a big PR.

@killuazhu
Copy link
Contributor Author

CC @jribm

@KevinHock
Copy link
Collaborator

KevinHock commented Jan 31, 2019

That certainly sounds reasonable @killuazhu, let me chat with @domanchi about it IRL and I'll update this afterwards.

The only "Pro" for having --update use all the plugins is that if a new plugin was added since the baseline was created, the new plugins are used automatically. We can probably solve this with --update-with-all-plugins and --update-with-new-plugins options like you mentioned.

@domanchi
Copy link
Contributor

Hey @killuazhu,

That's a really good idea. I think --update adding back all the plugins was partially a design choice (so people will get the new plugins), but also partially a bug (because if people already specify the plugins they care about, then we should respect that). After all, that is one of the reasons why we decided to publish a plugins list with the baseline.

We would definitely welcome the addition of a new --update-all flag, where this new entry will get all latest plugins, while --update only gets the current plugins that are in the baseline!

@killuazhu
Copy link
Contributor Author

Hey @KevinHock , @domanchi thanks for looking into the issue.

So instead of using --update-all, how about using a new option called --use-all-plugins. It would be easier to align with detect-secrets-hook. Somehow the hook command is using --baseline <baseline> to update the baseline, instead of --update 😕

The updated behavior would be

  • created a new baseline with all plugins
    • detect-secrets scan
  • created a new baseline with fewer plugins
    • detect-secrets scan --no-xxx-scan
  • update baseline with detect-secrets with exact plugins in baseline
    • detect-secrets scan --update baseline
  • update baseline with detect-secrets with all plugins
    • detect-secrets scan --update baseline --use-all-plugins
  • update baseline with detect-secrets and use more plugins
    • detect-secrets scan --update baseline --use-all-plugins --no-xxx-scan
    • need to know the exact plugins to get rid of
    • the plugin to be removed will use all plugins as starting point.
  • update baseline with detect-secrets and use fewer plugins
    • detect-secrets scan --update baseline --no-xxx-scan
    • the plugin to be removed will use plugins in baseline as starting point.
  • update baseline with pre-commit to use all plugins
    • detect-secrets-hook --baseline baseline --use-all-plugins
  • update baseline with pre-commit to use exact plugins
    • detect-secrets-hook --baseline baseline
  • update baseline with pre-commit hook to use more plugins
    • detect-secrets-hook --baseline baseline --use-all-plugins --no-xxx-scan
    • the plugin to be removed will use all plugins as starting point.
  • update baseline with pre-commit hook to use fewer plugins
    • detect-secrets-hook --baseline baseline --no-xxx-scan
    • the plugin to be removed will use plugins in baseline as starting point.

For users who want to always stay at the latest, they can use detect-secrets-hook --baseline baseline --use-all-plugins and detect-secrets scan --update baseline --use-all-plugins

If you guys think this makes sense, I can put something together. It's a little bit different from our current implementation, but I think it's actually cleaner. Our current implementation introduces a bunch of --add-xxx-scan options which allows you to adjust plugins base on all plugins or baseline plugins. The main drawback is that it would hard for user to know when need to add new plugin and what are these new plugins.

@domanchi
Copy link
Contributor

domanchi commented Feb 1, 2019

SGTM. Looking forward to your improvements!

@killuazhu
Copy link
Contributor Author

This issue is implemented through #124

@asdrubalos
Copy link

Help me please.
how write # pragma: whitelist secret in java?

I have 2 false positive:
1)
this.clientSecret = clientSecret; //# pragma: whitelist secret
2)
String x =", clientSecret='" + this.clientSecret +"'"; //# pragma: whitelist secret

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants