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

Add bandit scan to pre-commit hooks #297

Closed
wants to merge 1 commit into from

Conversation

sandrobonazzola
Copy link
Contributor

Adding Bandit scan to pre-commit hooks.

Signed-off-by: Sandro Bonazzola sbonazzo@redhat.com

Copy link
Collaborator

@kwk kwk 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 adding this security scanner. But we cannot enable it unless the pre-commit issues that it finds are dealt with. There are clearly false-positives when it comes to accidentally shared secrets. Here's an example bandit finding for a piece of code that I've contributed recently:

>> Issue: [B107:hardcoded_password_default] Possible hardcoded password: 'token'
   Severity: Low   Confidence: Medium
   CWE: CWE-259 (https://cwe.mitre.org/data/definitions/259.html)
   Location: did/base.py:478:0
   More Info: https://bandit.readthedocs.io/en/0.0.0/plugins/b107_hardcoded_password_default.html
477	
478	def get_token(
479	        config: dict,
480	        token_key: str = "token",
481	        token_file_key: str = "token_file") -> str:
482	    """

I suggest to add exceptions or find another solution to similar findings.

bandit.yml Outdated Show resolved Hide resolved
Adding bandit scanning to pre-commit hook and
addressing initially reported issues

Signed-off-by: Sandro Bonazzola <sbonazzo@redhat.com>
Co-authored-by: Konrad Kleine <konrad.kleine@posteo.de>
Copy link
Collaborator

@kwk kwk 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 addressing my concern but it gets quite noisy and I'm completely uncertain if this is really what we want: everytime the word token is used, we turn off security by given a comment # nosec. Not only is this noise but maybe bad if there was really a security problem. I mean what happens the next token function is implemented? Do we disable security there as well? That's quite unfortunate, don't you think?

@sandrobonazzola
Copy link
Contributor Author

abandoning

@sandrobonazzola sandrobonazzola deleted the bandit-config branch March 8, 2023 13:47
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.

2 participants