Populate detectionTool metadata for Sonar codemods #346

carlosu7 · 2024-04-11T14:51:29Z

add finding to sonar codemods and add assertion to check if FixOnlyCodeChanger has findings

Issue #337

…anger has findings

nahsra · 2024-04-12T01:13:03Z

framework/codemodder-base/src/main/java/io/codemodder/SarifPluginJavaParserChanger.java

@@ -133,7 +133,8 @@ public CodemodFileScanningResult visit(
            if (regionNodeMatcher.matches(region, range)) {
              ChangesResult changeSuccessful = onResultFound(context, cu, (T) node, result);
              if (changeSuccessful.areChangesApplied()) {
-                final Optional<FixedFinding> optionalFixedFinding = getFixedFinding();
+                final Optional<FixedFinding> optionalFixedFinding =


It's not the rule ID we want, but the finding ID (or "issue" ID in Sonar's parlance. Maybe this isn't clear, I would check the JavaDoc for this method and make sure it's good.

the sonar issue ID is set correctly at SonarPluginJavaParserChanger.java line 85

this is for the sarif plugin

right now SemgrepSarifJavaParserChanger extends from SarifPluginFixOnlyCodeChanger that extends from SarifPluginJavaParserChanger

when runing the only rule Semgrep codemod: SemgrepOverlyPermissiveFilePermissionsCodemod, the semgrep provided rule id is: "java.lang.security.audit.overly-permissive-file-permission.overly-permissive-file-permission" which is the same as result.RuleId

We don' want the rule ID. We want the finding ID.

@nahsra

taking the DefectDojo codemod as reference:

the finding ID is taken from the RuleFindings findings argument of the injected constructor

in the case of the semgrep codemod, i see that the argument type of the injected constructor is RuleSarif sarif. I'm not sure if the Result::guid would be the desired value.

RuleSarif sarif has a method called getResultsByLocationPath which returns a List, and the javadoc for this method says: "Get all the SARIF results with the matching path for the first location field"

And Result class has the guid field that says: "A stable, unique identifer for the result in the form of a GUID."

So what I'm trying to understand is if this Result::guid for the Sarif codemods is similar to the Finding::id for the defect dojo codemods given the fact that the Finding class javadoc says: "Represents a finding in DefectDojo."

Yes, it's usually either guid or correlationGuid. However, the DefectDojo ID, not the underlying Semgrep ID, is the one that's valuable, because that's where we need to link the user to see the result.

This logic being here is wrong because SarifPluginJavaParserChanger isn't necessarily a FixOnlyChanger, so it has no idea where to find the finding ID, and thus, maybe it doesn't know how to make a CodemodChange for all subtypes. This inheritance isn't working, so we should favor composition. The first thought to me is to create a new interface CodemodChangeFactory, and have have the subclasses inject one into the constructor if they want a different behavior.

@nahsra should core-codemods/src/test/resources/semgrep-overly-permissive-file-permission/out.sarif file have the tool vendor's ID property ?

OOF -- it doesn't have one. I did not see that coming. I don't even know what to do in that scenario.

I think we have two options:

make our own "composite ID", maybe rule ID + location + line

match fingerprint ID

I feel like either is fine, because both are temporary. I'd prefer match fingerprint ID.

framework/codemodder-testutils/src/main/java/io/codemodder/testutils/CodemodTestMixin.java

framework/codemodder-base/src/main/java/io/codemodder/CompositeJavaParserChanger.java

nahsra · 2024-04-15T13:56:01Z

framework/codemodder-base/src/main/java/io/codemodder/SarifPluginJavaParserChanger.java

@@ -133,7 +133,8 @@ public CodemodFileScanningResult visit(
            if (regionNodeMatcher.matches(region, range)) {
              ChangesResult changeSuccessful = onResultFound(context, cu, (T) node, result);
              if (changeSuccessful.areChangesApplied()) {
-                final Optional<FixedFinding> optionalFixedFinding = getFixedFinding();
+                final Optional<FixedFinding> optionalFixedFinding =


Yes, it's usually either guid or correlationGuid. However, the DefectDojo ID, not the underlying Semgrep ID, is the one that's valuable, because that's where we need to link the user to see the result.

This logic being here is wrong because SarifPluginJavaParserChanger isn't necessarily a FixOnlyChanger, so it has no idea where to find the finding ID, and thus, maybe it doesn't know how to make a CodemodChange for all subtypes. This inheritance isn't working, so we should favor composition. The first thought to me is to create a new interface CodemodChangeFactory, and have have the subclasses inject one into the constructor if they want a different behavior.

…etadata

carlosu7 · 2024-04-16T07:39:58Z

framework/codemodder-base/src/main/java/io/codemodder/SarifPluginJavaParserChanger.java

+                          // TODO
+                          fixOnlyCodeChangerInformtionOptional
+                              .get()
+                              .buildFixedFinding(result.getFingerprints().toString())));


@nahsra would this be OK since fingerprints is basically a map of strings or do you prefer the other way you suggested: make our own "composite ID", maybe rule ID + location + line ?

Yuck, I hate both. Let's use the fingerprint, but what you've got here doesn't look like it will be the string fingerprint value, it looks like it is a toString() of the array.

@nahsra fingerprint is basically a Map<String, String> which represent a set of strings each of which individually defines a stable, unique identity for the result, do you want the string value of that map?

I'd rather just get the first value in the map (not the key, the value.)

nahsra

Needs a lot more design love. Don't rush to get another revision. Take your time, draw it out.

nahsra · 2024-04-16T13:13:45Z

framework/codemodder-base/src/main/java/io/codemodder/FixOnlyCodeChangerInformation.java

+
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.codetf.FixedFinding;
+


framework/codemodder-base/src/main/java/io/codemodder/VendorName.java

nahsra · 2024-04-16T14:21:13Z

framework/codemodder-base/src/main/java/io/codemodder/FixOnlyCodeChangerInformation.java

+
+  DetectorRule detectorRule();
+
+  default FixedFinding buildFixedFinding(String id) {


The scope of this type is very strange. It's primarily a model, but it also looks like a Factory type -- except the Factory looks like it's just a utility that won't be very different.

I think you've now got a much better idea of what information needs to be where, but these patterns are not working for me.

nahsra · 2024-04-16T14:22:49Z

framework/codemodder-base/src/main/java/io/codemodder/SarifPluginJavaParserChanger.java

+                          // TODO
+                          fixOnlyCodeChangerInformtionOptional
+                              .get()
+                              .buildFixedFinding(result.getFingerprints().toString())));


Yuck, I hate both. Let's use the fingerprint, but what you've got here doesn't look like it will be the string fingerprint value, it looks like it is a toString() of the array.

…etadata

nahsra

Minor nitpick -- please fix then we're good to go.

framework/codemodder-base/src/main/java/io/codemodder/SarifFindingKeyUtil.java

sonarqubecloud · 2024-04-18T01:13:05Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

add finding to sonar codemods and add assertion to check if fixonlych…

f564cc6

…anger has findings

carlosu7 requested a review from nahsra April 11, 2024 14:51

nahsra requested changes Apr 12, 2024

View reviewed changes

remove get changers, rename builFixedFinding

cf03790

nahsra requested changes Apr 15, 2024

View reviewed changes

carlosu7 added 4 commits April 15, 2024 13:47

delete SarifPluginFixOnlyCodeChanger file

e36e899

refactor fix only code changer

4f59bd9

Merge remote-tracking branch 'origin/main' into sonar-detectiontool-m…

db94024

…etadata

refactor to record

3e4192f

carlosu7 commented Apr 16, 2024

View reviewed changes

carlosu7 requested a review from nahsra April 16, 2024 07:41

nahsra requested changes Apr 16, 2024

View reviewed changes

carlosu7 added 10 commits April 16, 2024 12:37

remove vendor name

20f701c

update refactor design

44d0e1b

Merge remote-tracking branch 'origin/main' into sonar-detectiontool-m…

9075f15

…etadata

fix compilation error

3425894

fix tests

3f3f29e

remove default fix only code changer

21f6d28

remove finding factory

297281d

javadoc and rename

dadea1e

update key

3466b58

key changes

21eb4f5

carlosu7 requested a review from nahsra April 17, 2024 20:00

nahsra approved these changes Apr 17, 2024

View reviewed changes

framework/codemodder-base/src/main/java/io/codemodder/SarifFindingKeyUtil.java Show resolved Hide resolved

framework/codemodder-base/src/main/java/io/codemodder/SarifFindingKeyUtil.java Outdated Show resolved Hide resolved

final feedback

3bf99cf

carlosu7 merged commit 380a8a9 into main Apr 18, 2024
5 checks passed

carlosu7 deleted the sonar-detectiontool-metadata branch April 18, 2024 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Populate detectionTool metadata for Sonar codemods #346

Populate detectionTool metadata for Sonar codemods #346

carlosu7 commented Apr 11, 2024

nahsra Apr 12, 2024

carlosu7 Apr 12, 2024

nahsra Apr 12, 2024

carlosu7 Apr 15, 2024 •

edited

Loading

nahsra Apr 15, 2024

carlosu7 Apr 15, 2024

nahsra Apr 15, 2024

nahsra Apr 15, 2024

carlosu7 Apr 16, 2024 •

edited

Loading

nahsra Apr 16, 2024

carlosu7 Apr 16, 2024

nahsra Apr 16, 2024

nahsra left a comment

nahsra Apr 16, 2024

nahsra Apr 16, 2024

nahsra Apr 16, 2024

nahsra left a comment

sonarqubecloud bot commented Apr 18, 2024


		import io.codemodder.codetf.DetectorRule;
		import io.codemodder.codetf.FixedFinding;


		DetectorRule detectorRule();

		default FixedFinding buildFixedFinding(String id) {

Populate detectionTool metadata for Sonar codemods #346

Populate detectionTool metadata for Sonar codemods #346

Conversation

carlosu7 commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosu7 Apr 15, 2024 • edited Loading

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

carlosu7 Apr 16, 2024 • edited Loading

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

nahsra 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

nahsra left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 18, 2024

Quality Gate passed

carlosu7 Apr 15, 2024 •

edited

Loading

carlosu7 Apr 16, 2024 •

edited

Loading