Add opaque rule id for Sarif log #1434

yongyan-gh · 2021-02-18T01:12:04Z

Related to issue #1427

Assigned opaque rule Id values to BugPattern metadata
Changed Sarif Result.ruleId/Rule.id value to opaque rule id defined in metadata.
Added name property to Sarif Rule, value is original rule "id" (BugPattern.type)

Attached Sarif log generated before/after this change. Please review.
newSarifLog.zip

KengoTODA

I understand the motivation to set ruleId explicitly in the XML, however, we need to consider how to support plugins.
Do we need to enforce them to add ruleId attribute? If so, we need to ask each plugin owner to update.

for reference: SpotBugs has a plugin mechanism, and we have several famous plugins like find-sec-bugs and fb-contrib. Ideally it's better to keep API backward compatible.

spotbugs/src/main/java/edu/umd/cs/findbugs/BugPattern.java

KengoTODA · 2021-02-18T04:23:34Z

spotbugs/src/main/java/edu/umd/cs/findbugs/BugPattern.java

            String longDescription, String detailText, String bugsUrl, int cweid) {

+        this.ruleId = ruleId;


If we ask plugins to provide ruleId attribute certainly, it's better to check null-ness of given value and throw a NullPointerException with a user-friendly message. Also it's better to annotate the parameter with @NonNull (even though other existing parameters aren't annotated so :<).

@KengoTODA thank you for pointing out the plugin mechanism, you are right we should keep it backward compatible.
in case plugin doesn't pass rule id we can use the BugPattern type as rule id, which is currently used as rule id. and provide new API interface so plugin owners can add the opaque rule id in future.

@michaelcfanning @eddynaka what do you think?

Thanks.

If we need to follow the definition of 'stable value' and cannot make this attribute optional, it is also possible to release a major version to break compatibility. Of course we need to help plugin authors to migrate, though.

Ideally, we would break plug-in authors on update so that the opportunity to provide a valid opaque id is very obvious, but also provide a very easy way to fall back to the legacy rule id if they can't stop to make a fix. @yongyan-gh, please go read the SARIF spec on the deprecatedIds property for SARIF reportingDescriptor objects. If we are emitting the rules table to Spotbugs SARIF, this data can be populated. If we do not emit this data, obviously, we can't persist the deprecated ids. Doing this is not a high priority, as I doubt there are SARIF readers out there who consume this information, though. :) People worried about compatibility will just consult the readable name, which maps to the old rule id value.

@KengoTODA in latest change we added an overload constructor to wrap the new constructor and marked with deprecated annotation.
In this way it wont break existing plugins. Plugins call deprecated constructor will get a warning to use new constructor and provide opaque rule id.
Please let me know what do you think?

KengoTODA · 2021-02-18T05:19:01Z

The broken build has been fixed in the master branch, please merge the latest master to your topic branch or simply rebase on it.

…lugins

KengoTODA · 2021-02-19T05:04:28Z

spotbugs/src/main/java/edu/umd/cs/findbugs/BugPattern.java

@@ -36,6 +37,8 @@
 * @see BugInstance
 */
 public class BugPattern implements Comparable<BugPattern> {
+    final private String ruleId;


Consider to add a documentation comment, especially a @see tag with an anchor tag to http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html#def_rule_id

KengoTODA · 2021-02-19T05:20:35Z

spotbugs/src/main/java/edu/umd/cs/findbugs/PluginLoader.java

@@ -928,6 +928,7 @@ private void loadPluginComponents()
        // Create BugPatterns
        List<Node> bugPatternNodeList = XMLUtil.selectNodes(pluginDescriptor, "/FindbugsPlugin/BugPattern");
        for (Node bugPatternNode : bugPatternNodeList) {
+            String ruleId = bugPatternNode.valueOf("@ruleId");


This ruleId can be an empty string, right? Then I think here we need a consideration for (old) plugins.

Consider to test your implementation with a plugin. You can create distributed files by ./gradlew assemble. Unpack the generated .tar or .zip and run it in your local.

I suggest you to use find-sec-bugs to run with, but if you need a small project for faster build, you may try findbugs-slf4j instead. It's also possible to create a plugin project by your own.

I see the plugins don't init BugPattern class by themselves. Is there anyway to not break current plugins but can remind them to use opaque rule id?

I am able to compile and run SpotBugs to anaylze .jar file. But I am not sure how to include other plugins into SpotBugs binaries.
With latest change if ruleId is not provided it will use type as rule id to avoid break plugins.
I tested it locally by removing ruleId of some BugPatterns in FindBugs.xml. In the output Sarif log rule id is set to type as expected.

Fix format violation.

KengoTODA · 2021-03-03T01:03:54Z

@yongyan-gh sorry but please resolve conflicts later.

yongyan-gh · 2021-03-03T01:37:59Z

@KengoTODA no problem, merged changes from remote and resolved the conflicts.

KengoTODA · 2021-07-01T23:06:34Z

Sorry but again we have a conflict, please merge the latest origin/master to your topic branch and resolve it.
You also need to update CHANGELOG.md to move your changes under ## Unreleased. 🙇

yongyan-gh

Sorry but again we have a conflict, please merge the latest origin/master to your topic branch and resolve it.
You also need to update CHANGELOG.md to move your changes under ## Unreleased. 🙇

Resolved & merged, updated the change log.

1 thing we can also improve for the opaque rule id: we may divide the ids into couple segments based on bug pattern/rule category etc. so that each segment has spare ids can be assigned to new bug patterns/rules in the future, and don't need to change existing id.

yongyan-gh · 2021-02-19T03:02:44Z

spotbugs/etc/findbugs.xml

-          <BugPattern abbrev="TLW" type="TLW_TWO_LOCK_WAIT" category="MT_CORRECTNESS"/>
-          <BugPattern abbrev="TLW" type="TLW_TWO_LOCK_NOTIFY" category="MT_CORRECTNESS"
-                    deprecated="true" experimental="true"/>
+          <BugPattern ruleId="SPOT1001" abbrev="JUA" type="JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS" category="BAD_PRACTICE" experimental="true" />


@KengoTODA can you pls advise a proper opaque rule id here since you are one of tool owner?
We have seen the some rule id naming conversions that starts with language name plus numbers:
e.g. C6001, VB7001, CS8001 etc.
or tool name abbv plus numbers:
e.g. FF1001 (Flawfinder)

@KengoTODA any thought about the rule id format?

yongyan-gh · 2021-02-19T10:10:25Z

spotbugs/src/main/java/edu/umd/cs/findbugs/PluginLoader.java

@@ -928,6 +928,7 @@ private void loadPluginComponents()
        // Create BugPatterns
        List<Node> bugPatternNodeList = XMLUtil.selectNodes(pluginDescriptor, "/FindbugsPlugin/BugPattern");
        for (Node bugPatternNode : bugPatternNodeList) {
+            String ruleId = bugPatternNode.valueOf("@ruleId");


I see the plugins don't init BugPattern class by themselves. Is there anyway to not break current plugins but can remind them to use opaque rule id?

yongyan-gh · 2021-02-22T06:59:25Z

spotbugs/src/main/java/edu/umd/cs/findbugs/BugPattern.java

@@ -36,6 +37,8 @@
 * @see BugInstance
 */
 public class BugPattern implements Comparable<BugPattern> {
+    final private String ruleId;


yongyan-gh · 2021-02-22T07:06:31Z

spotbugs/src/main/java/edu/umd/cs/findbugs/PluginLoader.java

@@ -928,6 +928,7 @@ private void loadPluginComponents()
        // Create BugPatterns
        List<Node> bugPatternNodeList = XMLUtil.selectNodes(pluginDescriptor, "/FindbugsPlugin/BugPattern");
        for (Node bugPatternNode : bugPatternNodeList) {
+            String ruleId = bugPatternNode.valueOf("@ruleId");


I am able to compile and run SpotBugs to anaylze .jar file. But I am not sure how to include other plugins into SpotBugs binaries.
With latest change if ruleId is not provided it will use type as rule id to avoid break plugins.
I tested it locally by removing ruleId of some BugPatterns in FindBugs.xml. In the output Sarif log rule id is set to type as expected.

KengoTODA

Hi @yongyan-gh sorry I did not notice that this PR got updated.

I've checked DIFF and changes are LGTM. However, it surely brings a new requirement to plugin, then we need to be accountable and provide enough guides for users. For example, I will list what I can imagine to provide:

Explain why type is not enough stable (so we need a new attribute, ruleId)
Guide how to design ruleId to make it stable and avoid conflict with another ruleId associated by other analysis tools
System to confirm that a specific ruleId is not conflict with others

2 and 3 aren't limited to SpotBugs, so it's OK to have document in SARIF web site or other places.

And one more request, add a unit test which verifies that we have no conflict (duplicated value) ruleId in findbugs.xml. It will help our project maintenance in future.

To be honest, I'm not so positive for this change. Generally format is not the concern of the rules. When the context of format is visible for rules, it's usually a sign of bad architecture, I believe.

Add opaque rule id for Sarif log

f4610ff

KengoTODA reviewed Feb 18, 2021

View reviewed changes

yongyan-gh added 2 commits February 18, 2021 01:13

Merge branch 'master' into users/yongyan-gh/fixRuleIds

e14c332

Added overload constructor, marked old as deprecated to avoid break p…

fe0f3a9

…lugins

KengoTODA requested changes Feb 19, 2021

View reviewed changes

yongyan-gh added 2 commits February 19, 2021 12:26

Handle missing ruleId case in Plugin loader.

b31d009

Fix format violation.

Add comment to right place

7099b71

yongyan-gh mentioned this pull request Feb 28, 2021

Replace 'org.json:json' lib with 'com.google.code.gson' lib #1437

Merged

yongyan-gh marked this pull request as ready for review March 2, 2021 23:25

merge from master branch

3aac606

yongyan-gh added 2 commits July 1, 2021 17:27

Merge branch 'master' into users/yongyan-gh/fixRuleIds

705397f

move change log under ##Unreleased

d403b59

yongyan-gh commented Jul 2, 2021

View reviewed changes

KengoTODA requested changes Oct 7, 2021

View reviewed changes

github-actions bot added the Stale label Mar 6, 2024

github-actions bot closed this Apr 6, 2024

hazendaz reopened this Apr 12, 2024

github-actions bot removed the Stale label Apr 15, 2024

github-actions bot added the Stale label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add opaque rule id for Sarif log #1434

Add opaque rule id for Sarif log #1434

yongyan-gh commented Feb 18, 2021

KengoTODA left a comment •

edited

KengoTODA Feb 18, 2021 •

edited

yongyan-gh Feb 18, 2021

KengoTODA Feb 18, 2021

michaelcfanning Feb 18, 2021

yongyan-gh Feb 19, 2021

KengoTODA commented Feb 18, 2021

KengoTODA Feb 19, 2021

yongyan-gh Feb 22, 2021

KengoTODA Feb 19, 2021

yongyan-gh Feb 19, 2021

yongyan-gh Feb 22, 2021

KengoTODA commented Mar 3, 2021

yongyan-gh commented Mar 3, 2021

KengoTODA commented Jul 1, 2021

yongyan-gh left a comment

yongyan-gh Feb 19, 2021

yongyan-gh Feb 25, 2021

yongyan-gh Feb 19, 2021

yongyan-gh Feb 22, 2021

yongyan-gh Feb 22, 2021

KengoTODA left a comment

		String longDescription, String detailText, String bugsUrl, int cweid) {

		this.ruleId = ruleId;

Add opaque rule id for Sarif log #1434

Are you sure you want to change the base?

Add opaque rule id for Sarif log #1434

Conversation

yongyan-gh commented Feb 18, 2021

KengoTODA left a comment • edited

Choose a reason for hiding this comment

KengoTODA Feb 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KengoTODA commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KengoTODA commented Mar 3, 2021

yongyan-gh commented Mar 3, 2021

KengoTODA commented Jul 1, 2021

yongyan-gh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KengoTODA left a comment

Choose a reason for hiding this comment

KengoTODA left a comment •

edited

KengoTODA Feb 18, 2021 •

edited