Add CWE taxonomy to SARIF report #2410
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- added CWE 4.10 JSON to /etc - added classes for the CWE weaknesses - added the SARIF taxon element - extended Rule, SarifBugReporter and BugCollectionAnalyser classes to include CWE details - added a GUIDCalculator which implements UUIDv5
- fixed issues detected while testing - New BugCollectionAnalyserTest - added test for CWE taxonomies to SarifBugReporterTest - applied spotless rules to source code - made Taxon comparable
- CWEs without a severity level have no severity level assigned to them - convert CWE severity levels to SARIF severity levels - the taxon id and relationship id are now strings rather than integers
Jeeppler
changed the title
hazendaz
reviewed
Apr 23, 2023
spotbugs/src/main/java/edu/umd/cs/findbugs/cwe/WeaknessCatalog.java
Outdated
Show resolved
Hide resolved
ThrawnCA
approved these changes
Apr 23, 2023
ThrawnCA
previously requested changes
Apr 26, 2023
spotbugs/src/main/java/edu/umd/cs/findbugs/cwe/WeaknessCatalog.java
Outdated
Show resolved
Hide resolved
Sure on including but not sure where to place it. Make your best guess :) |
|
I used the REUSE specification header comments for the CWE conversion script to put it under the proper license.
|
|
Yes but not on logger. Long disputed special case. Has to do with high frequency usage of loggers.
Sent from my Verizon, Samsung Galaxy smartphone
Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: ThrawnCA ***@***.***>
Sent: Thursday, April 27, 2023 7:30:18 PM
To: spotbugs/spotbugs ***@***.***>
Cc: Jeremy Landis ***@***.***>; Mention ***@***.***>
Subject: Re: [spotbugs/spotbugs] Add CWE taxonomy to SARIF report (PR #2410)
@ThrawnCA commented on this pull request.
________________________________
In spotbugs/src/main/java/edu/umd/cs/findbugs/cwe/WeaknessCatalog.java<#2410 (comment)>:
/**
* The weakness catalog contains a number of weaknesses
*
* @author Jeremias Eppler
* @see Weakness
*/
public class WeaknessCatalog {
+ private static final Logger LOG = LoggerFactory.getLogger(AbstractBugReporter.class);
It's a static final field. The Google Java style guide<https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names> still says to use UPPER_SNAKE_CASE for those, and so does Carnegie Mellon<https://www.cs.cmu.edu/~rdriley/121/resources/styleguide/#s5.2.4-constant-names>, and also Oracle<https://www.oracle.com/java/technologies/javase/codeconventions-namingconventions.html>. What Java style guide says otherwise?
—
Reply to this email directly, view it on GitHub<#2410 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHODIZEBAQY3C76SNN3DUDXDL6QVANCNFSM6AAAAAAXIBPPDM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I don't know why, this project is really old so just maybe not something that was generally thought of. I'd have all files marked with license headers if this were maven but its gradle and my skills there are very low. If you understand gradle enough and know how to get that to be turned on and automated so files auto do it across the board, I think that would be a separate welcomed PR. I'm sure there are plugins for gradle to do same as there are for maven so its not something we have to think about :) |
|
@ThrawnCA I think I'm good here. WDYT? |
hazendaz
approved these changes
Apr 28, 2023
|
2009 same statement. https://stackoverflow.com/questions/1417190/should-a-static-final-logger-be-declared-in-upper-case
Look at follow up 2020. Look at fact it's defined in jave style docs.
Then look again at Google. They in fact have it lower cased.
https://google.github.io/styleguide/javaguide.html#s5.2-specific-identifier-names
Look specifically at what they call constants. Logger is not.
Sent from my Verizon, Samsung Galaxy smartphone
Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: ThrawnCA ***@***.***>
Sent: Friday, April 28, 2023 6:25:30 PM
To: spotbugs/spotbugs ***@***.***>
Cc: Jeremy Landis ***@***.***>; Assign ***@***.***>
Subject: Re: [spotbugs/spotbugs] Add CWE taxonomy to SARIF report (PR #2410)
@ThrawnCA commented on this pull request.
________________________________
In spotbugs/src/main/java/edu/umd/cs/findbugs/cwe/WeaknessCatalog.java<#2410 (comment)>:
/**
* The weakness catalog contains a number of weaknesses
*
* @author Jeremias Eppler
* @see Weakness
*/
public class WeaknessCatalog {
+ private static final Logger LOG = LoggerFactory.getLogger(AbstractBugReporter.class);
The linked page doesn't seem to give any actual basis for treating static final references differently from constants.
—
Reply to this email directly, view it on GitHub<#2410 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHODIZIGLHKWW7IZZQATWTXDQ7VVANCNFSM6AAAAAAXIBPPDM>.
You are receiving this because you were assigned.Message ID: ***@***.***>
|
|
Every reference you provided says the same thing about logger. Make sure to look at what a constant really is in the examples when determining to use upper case. That is the important part because it's not a constant and thus remains lower cased. Can be easy to be overlooked...
Sent from my Verizon, Samsung Galaxy smartphone
Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: Jeremy Landis ***@***.***>
Sent: Friday, April 28, 2023 9:13:30 PM
To: spotbugs/spotbugs ***@***.***>; spotbugs/spotbugs ***@***.***>
Cc: Assign ***@***.***>
Subject: Re: [spotbugs/spotbugs] Add CWE taxonomy to SARIF report (PR #2410)
2009 same statement. https://stackoverflow.com/questions/1417190/should-a-static-final-logger-be-declared-in-upper-case
Look at follow up 2020. Look at fact it's defined in jave style docs.
Then look again at Google. They in fact have it lower cased.
https://google.github.io/styleguide/javaguide.html#s5.2-specific-identifier-names
Look specifically at what they call constants. Logger is not.
Sent from my Verizon, Samsung Galaxy smartphone
Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: ThrawnCA ***@***.***>
Sent: Friday, April 28, 2023 6:25:30 PM
To: spotbugs/spotbugs ***@***.***>
Cc: Jeremy Landis ***@***.***>; Assign ***@***.***>
Subject: Re: [spotbugs/spotbugs] Add CWE taxonomy to SARIF report (PR #2410)
@ThrawnCA commented on this pull request.
________________________________
In spotbugs/src/main/java/edu/umd/cs/findbugs/cwe/WeaknessCatalog.java<#2410 (comment)>:
/**
* The weakness catalog contains a number of weaknesses
*
* @author Jeremias Eppler
* @see Weakness
*/
public class WeaknessCatalog {
+ private static final Logger LOG = LoggerFactory.getLogger(AbstractBugReporter.class);
The linked page doesn't seem to give any actual basis for treating static final references differently from constants.
—
Reply to this email directly, view it on GitHub<#2410 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHODIZIGLHKWW7IZZQATWTXDQ7VVANCNFSM6AAAAAAXIBPPDM>.
You are receiving this because you were assigned.Message ID: ***@***.***>
|
ThrawnCA
dismissed
their stale review
Style disagreement but not worth digging heels in, so abstaining
|
I'm good with this.
Sent from my Verizon, Samsung Galaxy smartphone
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Jeeppler ***@***.***>
Sent: Tuesday, May 2, 2023 3:55:28 AM
To: spotbugs/spotbugs ***@***.***>
Cc: Jeremy Landis ***@***.***>; Mention ***@***.***>
Subject: Re: [spotbugs/spotbugs] Add CWE taxonomy to SARIF report (PR #2410)
@hazendaz<https://github.com/hazendaz> and @ThrawnCA<https://github.com/ThrawnCA> is there anything outstanding which I have to change from your perspective? Is there any outstanding issue to get this merged?
—
Reply to this email directly, view it on GitHub<#2410 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHODI47XTRCXUNWCRJZVQDXEC4XBANCNFSM6AAAAAAXIBPPDM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@Jeeppler Merged, sorry for delays |
|
@hazendaz thanks for merging it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add CWE taxonomy to SARIF report.
closes: #2321
Make sure these boxes are checked before submitting your PR -- thank you!
CHANGELOG.mdif you have changed SpotBugs code