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

Do not require leading quotes for high-entropy strings #697

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

robinbowes
Copy link

@robinbowes robinbowes commented Jun 5, 2023

  • 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 Most secrets are not detected #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 Most secrets are not detected #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
Copy link
Author

I have not yet added any tests.

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

@lorenzodb1
Copy link
Member

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?

@alexrepetskyi
Copy link

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

@lorenzodb1
Copy link
Member

@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.

@robinbowes
Copy link
Author

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

@lorenzodb1
Copy link
Member

@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

@robinbowes
Copy link
Author

@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!

@lorenzodb1
Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

3 participants