detect-secrets icon indicating copy to clipboard operation
detect-secrets copied to clipboard

Published 20 hours ago • verified publisher icon Yelp

Do not require leading quotes for high-entropy strings

Open robinbowes opened this issue 1 year ago • 8 comments

  • Please check if the PR fulfills these requirements
  • [ ] Tests for the changes have been added
  • [ ] Docs have been added / updated
  • [ ] All CI checks are green
  • What kind of change does this PR introduce? Make the quotes around high-entropy strings optional

We found that secrets are being missed if they are not quoted.

  • What is the current behavior? High-entropy strings are currently required to be quoted "to reduce noise". This results in unquoted secrets being ignored, as reported in #458

  • What is the new behavior (if this is a feature change)? With this change, high-entropy strings are no longer required to be quoted. 3 of the secrets not detected in #458 are detected with this change.

  • Does this PR introduce a breaking change?

Users may find previously-undetected secrets. They may also run into additional false positives when scanning.

  • Other information:

robinbowes avatar Jun 05 '23 17:06 robinbowes

I have not yet added any tests.

I am happy to do so if this change is likely to be accepted.

robinbowes avatar Jun 05 '23 17:06 robinbowes

Hi @robinbowes, thank you for your contribution 😄 I'll have to look a bit into this to make sure everything is right. In the meantime, could you please merge master into your branch so you get the latest fixes I pushed?

lorenzodb1 avatar Nov 16 '23 00:11 lorenzodb1

@lorenzodb1 Any news about this PR, I still have issue with secrets detect with " and without ".

alexrepetskyi avatar Mar 29 '24 09:03 alexrepetskyi

@alexrepetskyi unfortunately I'd need the author of this PR (@robinbowes) to merge master to this branch so that the CI can run tests before approval. As the author hasn't really replied in a while, you're more than welcome to create a PR yourself with the changes contained in this branch and we can take it from there. If you can do this by the end of this month we'll include the changes in the next release.

lorenzodb1 avatar Apr 12 '24 21:04 lorenzodb1

Apologies for the delay - my branch is now up-to-date.

robinbowes avatar Apr 12 '24 21:04 robinbowes

@robinbowes looks like some tests are failing. If you could take care of them before the end of the month I'll make sure to include these changes in the coming-up release

lorenzodb1 avatar Apr 16 '24 18:04 lorenzodb1

@robinbowes looks like some tests are failing. If you could take care of them before the end of the month I'll make sure to include these changes in the coming-up release

I'll take a look when I get a moment. At first glance, it seems that it will just need some of the tests tweaking.

This could be quite an intrusive change, btw. Existing users could hit many more false positives. Conversely, they could find many more secrets that were not previously detected!

robinbowes avatar Apr 16 '24 23:04 robinbowes

This could be quite an intrusive change, btw. Existing users could hit many more false positives.

I guess we'll have to be careful about how we release these changes. Once this PR it's ready to be merged, @jpdakran and I will discuss this and release it in the least disruptive way possible.

lorenzodb1 avatar Apr 18 '24 17:04 lorenzodb1