-
-
Notifications
You must be signed in to change notification settings - Fork 612
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
Including CWE information #613
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a hard-coded number for the CWE, I'd like to see a constant of some kind being defined and used. Also, users will find it very beneficial to have a URL link to the cwe.mitre.org site for the specific issue found. We do this for Bandit errors also, we refer to bandit docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also elaborate the details of this feature in your PR/commit message. Typically, a feature issue is opened first and PR is linked to it. That will definitely help speed along the review for the approvers and make it much clearer to others.
Thanks a lot @ericwb for your review 👍 I addressed your comments by replacing the integer parameters for the CWEs with variables; I have also integrated a |
@julianthome Thank you! Bug what about B301-B323, B325, B401-B413,B504, B603-B607 ? |
Thanks for you response @dvyakimov. Yes, thanks for pointing this out. Going to update the MR soon 👍 |
This is an awesome improvement. Looking forward to using this soon! |
@dvyakimov I changed the MR according to your and @prabhu's comment. I put all the CWE mappings into a separate file I suppose the MR is ready for another round of review. |
bandit/core/blacklisting.py
Outdated
from bandit.core import issue | ||
|
||
|
||
def report_issue(check, name): | ||
return issue.Issue( | ||
severity=check.get('level', 'MEDIUM'), confidence='HIGH', | ||
text=check['message'].replace('{name}', name), | ||
cwe=CWEMAP[check.get("id", 'LEGACY')], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTSET or UNDEF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed NOTSET
may be more clear. Just renamed the constant from UNDEF
to NOTSET
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename LEGACY to NOTSET as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename LEGACY to NOTSET as well?
Mhh, I suppose that this would be a question for @dvyakimov and/or @ericwb as the LEGACY
key was not introduced by the current PR and I am not 100% sure if this change could have any undesirable side-effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think LEGACY is fine for now.
Any thoughts on this PR? @ericwb @dvyakimov ? |
@@ -134,6 +206,7 @@ def from_dict(self, data, with_code=True): | |||
self.code = data["code"] | |||
self.fname = data["filename"] | |||
self.severity = data["issue_severity"] | |||
self.cwe = cwe_from_dict(data["issue_cwe"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this why bandit 1.7.3 fails to load baseline data from JSON created by older bandit releases?
[manager] WARNING Failed to load baseline data: 'issue_cwe'
closes #612
This PR includes CWE mappings for every vulnerability.