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.

Sign up for GitHub

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

Populate detectionTool metadata for Sonar codemods #346

Merged
merged 17 commits into from
Apr 18, 2024
Merged

Populate detectionTool metadata for Sonar codemods #346

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f564cc6
add finding to sonar codemods and add assertion to check if fixonlych…
carlosu7 Apr 11, 2024
cf03790
remove get changers, rename builFixedFinding
carlosu7 Apr 15, 2024
e36e899
delete SarifPluginFixOnlyCodeChanger file
carlosu7 Apr 15, 2024
4f59bd9
refactor fix only code changer
carlosu7 Apr 16, 2024
db94024
Merge remote-tracking branch 'origin/main' into sonar-detectiontool-m…
carlosu7 Apr 16, 2024
3e4192f
refactor to record
carlosu7 Apr 16, 2024
20f701c
remove vendor name
carlosu7 Apr 16, 2024
44d0e1b
update refactor design
carlosu7 Apr 17, 2024
9075f15
Merge remote-tracking branch 'origin/main' into sonar-detectiontool-m…
carlosu7 Apr 17, 2024
3425894
fix compilation error
carlosu7 Apr 17, 2024
3f3f29e
fix tests
carlosu7 Apr 17, 2024
21f6d28
remove default fix only code changer
carlosu7 Apr 17, 2024
297281d
remove finding factory
carlosu7 Apr 17, 2024
dadea1e
javadoc and rename
carlosu7 Apr 17, 2024
3466b58
update key
carlosu7 Apr 17, 2024
21eb4f5
key changes
carlosu7 Apr 17, 2024
3bf99cf
final feedback
carlosu7 Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions framework/codemodder-base/src/main/java/io/codemodder/CompositeJavaParserChanger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,8 @@ public CodemodFileScanningResult visit(
public boolean shouldRun() {
return changers.stream().anyMatch(CodeChanger::shouldRun);
}
carlosu7 marked this conversation as resolved.
Show resolved Hide resolved

public List<JavaParserChanger> getChangers() {
return changers;
}
}
5 changes: 3 additions & 2 deletions framework/codemodder-base/src/main/java/io/codemodder/SarifPluginJavaParserChanger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@carlosu7 carlosu7 Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

getFixedFinding(result.getRuleId());
if (optionalFixedFinding.isPresent()) {
codemodChanges.add(
CodemodChange.from(
Expand All @@ -155,7 +156,7 @@ public CodemodFileScanningResult visit(
return CodemodFileScanningResult.withOnlyChanges(codemodChanges);
}

protected Optional<FixedFinding> getFixedFinding() {
protected Optional<FixedFinding> getFixedFinding(final String id) {
return Optional.empty();
}

Expand Down
17 changes: 16 additions & 1 deletion framework/codemodder-testutils/src/main/java/io/codemodder/testutils/CodemodTestMixin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,22 @@ private void verifyCodemod(
List<CodeTFChange> changes =
changeset.stream().map(CodeTFChangesetEntry::getChanges).flatMap(List::stream).toList();

if (codemod.getChanger() instanceof CompositeJavaParserChanger compositeJavaParserChanger) {
compositeJavaParserChanger
.getChangers()
.forEach(
changer -> {
if (changer instanceof FixOnlyCodeChanger) {
assertThat(
changes.stream().anyMatch(c -> !c.getFixedFindings().isEmpty()), is(true));
}
});
}

if (codemod.getChanger() instanceof FixOnlyCodeChanger) {
assertThat(changes.stream().anyMatch(c -> !c.getFixedFindings().isEmpty()), is(true));
}

for (int expectedFixLine : expectedFixLines) {
assertThat(changes.stream().anyMatch(c -> c.getLineNumber() == expectedFixLine), is(true));
}
Expand Down Expand Up @@ -266,7 +282,6 @@ default void verifyTransformedCode(final Path before, final Path expected, final
throws IOException {
String expectedCode = Files.readString(expected);
String transformedJavaCode = Files.readString(after);
carlosu7 marked this conversation as resolved.
Show resolved Hide resolved
System.out.println(transformedJavaCode);
assertThat(transformedJavaCode, equalTo(expectedCode));
}

Expand Down
8 changes: 6 additions & 2 deletions ...lugin-sonar/src/main/java/io/codemodder/providers/sonar/SonarPluginJavaParserChanger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,13 @@ public CodemodFileScanningResult visit(
if (regionNodeMatcher.matches(region, range)) {
ChangesResult changeSuccessful = onIssueFound(context, cu, (T) node, issue);
if (changeSuccessful.areChangesApplied()) {
final Optional<FixedFinding> optionalFixedFinding =
getFixedFinding(issue.getRule());
codemodChanges.add(
CodemodChange.from(
region.start().line(), changeSuccessful.getDependenciesRequired()));
region.start().line(),
changeSuccessful.getDependenciesRequired(),
optionalFixedFinding.get()));
}
}
}
Expand Down Expand Up @@ -116,7 +120,7 @@ public abstract ChangesResult onIssueFound(
CodemodInvocationContext context, CompilationUnit cu, T node, Issue issue);

@Override
public Optional<FixedFinding> getFixedFinding(String id) {
public Optional<FixedFinding> getFixedFinding(final String id) {
return Optional.of(new FixedFinding(id, getDetectorRule()));
}
}
Loading