Skip to content

Commit eb91667

Browse files
andrecsilvaandrecsilva
committed
Adjusted tests/codemods for bug in node matching fixed previously
1 parent 7f588c5 commit eb91667

File tree

11 files changed

+121
-98
lines changed

11 files changed

+121
-98
lines changed

‎core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public CodemodFileScanningResult visit(
5959
: Optional.empty(),
6060
i ->
6161
i.getTextRange() != null
62-
? Optional.of(i.getTextRange().getStartOffset() + 1)
62+
? Optional.of(i.getTextRange().getStartOffset())
6363
: Optional.empty());
6464
}
6565
}

‎core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLErrorMessageExposureCodemod.java‎

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,8 @@
88
import io.codemodder.remediation.GenericRemediationMetadata;
99
import io.codemodder.remediation.Remediator;
1010
import io.codemodder.remediation.errorexposure.ErrorMessageExposureRemediator;
11-
import io.codemodder.remediation.xxe.XXEIntermediateXMLStreamReaderRemediator;
12-
13-
import javax.inject.Inject;
1411
import java.util.Optional;
12+
import javax.inject.Inject;
1513

1614
/** A codemod for automatically fixing SQL injection from CodeQL. */
1715
@Codemod(
@@ -24,7 +22,8 @@ public final class CodeQLErrorMessageExposureCodemod extends CodeQLRemediationCo
2422
private final Remediator<Result> remediator;
2523

2624
@Inject
27-
public CodeQLErrorMessageExposureCodemod(@ProvidedCodeQLScan(ruleId = "java/error-message-exposure") final RuleSarif sarif) {
25+
public CodeQLErrorMessageExposureCodemod(
26+
@ProvidedCodeQLScan(ruleId = "java/error-message-exposure") final RuleSarif sarif) {
2827
super(GenericRemediationMetadata.ERROR_MESSAGE_EXPOSURE.reporter(), sarif);
2928
this.remediator = new ErrorMessageExposureRemediator<>();
3029
}
@@ -40,18 +39,18 @@ public DetectorRule detectorRule() {
4039
@Override
4140
public CodemodFileScanningResult visit(
4241
final CodemodInvocationContext context, final CompilationUnit cu) {
43-
return remediator.remediateAll(
44-
cu,
45-
context.path().toString(),
46-
detectorRule(),
47-
ruleSarif.getResultsByLocationPath(context.path()),
48-
SarifFindingKeyUtil::buildFindingId,
49-
r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
50-
r ->
51-
Optional.ofNullable(
52-
r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
53-
r ->
54-
Optional.ofNullable(
55-
r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()));
42+
return remediator.remediateAll(
43+
cu,
44+
context.path().toString(),
45+
detectorRule(),
46+
ruleSarif.getResultsByLocationPath(context.path()),
47+
SarifFindingKeyUtil::buildFindingId,
48+
r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
49+
r ->
50+
Optional.ofNullable(
51+
r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
52+
r ->
53+
Optional.ofNullable(
54+
r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()));
5655
}
5756
}

‎framework/codemodder-base/src/main/java/io/codemodder/remediation/errorexposure/ErrorMessageExposureFixStrategy.java‎

Lines changed: 58 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -2,69 +2,76 @@
22

33
import com.github.javaparser.ast.CompilationUnit;
44
import com.github.javaparser.ast.Node;
5-
import com.github.javaparser.ast.NodeList;
65
import com.github.javaparser.ast.expr.Expression;
76
import com.github.javaparser.ast.expr.MethodCallExpr;
8-
import com.github.javaparser.ast.expr.StringLiteralExpr;
97
import com.github.javaparser.ast.stmt.ExpressionStmt;
108
import com.github.javaparser.ast.stmt.Statement;
11-
import io.codemodder.CodemodFileScanningResult;
129
import io.codemodder.ast.ASTs;
13-
import io.codemodder.codetf.DetectorRule;
14-
import io.codemodder.javaparser.ChangesResult;
1510
import io.codemodder.remediation.*;
16-
import io.codemodder.remediation.headerinjection.HeaderInjectionFixStrategy;
17-
import javassist.expr.MethodCall;
18-
19-
import java.lang.reflect.Method;
20-
import java.util.Collection;
2111
import java.util.List;
2212
import java.util.Optional;
23-
import java.util.Set;
24-
import java.util.function.Function;
2513

26-
/**
27-
* Removes exposure from error messages.
28-
*/
14+
/** Removes exposure from error messages. */
2915
public final class ErrorMessageExposureFixStrategy extends MatchAndFixStrategy {
3016

31-
private static List<String> printErrorMethods = List.of("printStackTrace");
32-
private static List<String> printMethods = List.of("println", "print", "sendError");
17+
private static List<String> printErrorMethods = List.of("printStackTrace");
18+
private static List<String> printMethods = List.of("println", "print", "sendError");
3319

34-
/**
35-
* Test if the node is an expression that is the argument of a method call
36-
* @param node
37-
* @return
38-
*/
39-
@Override
40-
public boolean match(final Node node) {
41-
return Optional.empty()
42-
// is an argument of a call e.g. (response.sendError(418,<expression>))
43-
.or(() -> Optional.of(node).map(n -> n instanceof Expression? (Expression) n : null).flatMap(ASTs::isArgumentOfMethodCall).filter(mce -> printMethods.contains(mce.getNameAsString())))
44-
// is itself a method call that send errors: e.g. err.printStackTrace()
45-
.or(() -> Optional.of(node).map(n -> n instanceof Expression? (Expression) n : null).flatMap(e -> e.toMethodCallExpr()).filter(mce -> printErrorMethods.contains(mce.getNameAsString())))
46-
.isPresent()
47-
;
48-
}
20+
/**
21+
* Test if the node is an expression that is the argument of a method call
22+
*
23+
* @param node
24+
* @return
25+
*/
26+
@Override
27+
public boolean match(final Node node) {
28+
return Optional.empty()
29+
// is an argument of a call e.g. (response.sendError(418,<expression>))
30+
.or(
31+
() ->
32+
Optional.of(node)
33+
.map(n -> n instanceof Expression ? (Expression) n : null)
34+
.flatMap(ASTs::isArgumentOfMethodCall)
35+
.filter(mce -> printMethods.contains(mce.getNameAsString())))
36+
// is itself a method call that send errors: e.g. err.printStackTrace()
37+
.or(
38+
() ->
39+
Optional.of(node)
40+
.map(n -> n instanceof Expression ? (Expression) n : null)
41+
.flatMap(e -> e.toMethodCallExpr())
42+
.filter(mce -> printErrorMethods.contains(mce.getNameAsString())))
43+
.isPresent();
44+
}
4945

50-
@Override
51-
public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
52-
// we know from the match that this is true
53-
Expression expr = (Expression) node;
54-
// find encompassing statement
55-
Optional<Statement> maybeStmt = Optional.<MethodCallExpr>empty()
56-
// grab the relevant method call from the two cases
57-
.or(() -> Optional.of(node).map(n -> n instanceof Expression? (Expression) n : null).flatMap(ASTs::isArgumentOfMethodCall).filter(mce -> printMethods.contains(mce.getNameAsString())))
58-
// is itself a method call that send errors: e.g. err.printStackTrace()
59-
.or(() -> Optional.of(node).map(n -> n instanceof Expression? (Expression) n : null).flatMap(e -> e.toMethodCallExpr()).filter(mce -> printErrorMethods.contains(mce.getNameAsString())))
60-
// check if the method call is in a statement by itself
61-
.flatMap(mce -> mce.getParentNode())
62-
.map(p -> p instanceof ExpressionStmt? (ExpressionStmt) p : null);
63-
// Remove it
64-
if (maybeStmt.isPresent()){
65-
maybeStmt.get().remove();
66-
return SuccessOrReason.success();
67-
}
68-
return SuccessOrReason.reason("The call is not a statement");
46+
@Override
47+
public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
48+
// we know from the match that this is true
49+
Expression expr = (Expression) node;
50+
// find encompassing statement
51+
Optional<Statement> maybeStmt =
52+
Optional.<MethodCallExpr>empty()
53+
// grab the relevant method call from the two cases
54+
.or(
55+
() ->
56+
Optional.of(node)
57+
.map(n -> n instanceof Expression ? (Expression) n : null)
58+
.flatMap(ASTs::isArgumentOfMethodCall)
59+
.filter(mce -> printMethods.contains(mce.getNameAsString())))
60+
// is itself a method call that send errors: e.g. err.printStackTrace()
61+
.or(
62+
() ->
63+
Optional.of(node)
64+
.map(n -> n instanceof Expression ? (Expression) n : null)
65+
.flatMap(e -> e.toMethodCallExpr())
66+
.filter(mce -> printErrorMethods.contains(mce.getNameAsString())))
67+
// check if the method call is in a statement by itself
68+
.flatMap(mce -> mce.getParentNode())
69+
.map(p -> p instanceof ExpressionStmt ? (ExpressionStmt) p : null);
70+
// Remove it
71+
if (maybeStmt.isPresent()) {
72+
maybeStmt.get().remove();
73+
return SuccessOrReason.success();
6974
}
75+
return SuccessOrReason.reason("The call is not a statement");
76+
}
7077
}

‎framework/codemodder-base/src/main/java/io/codemodder/remediation/errorexposure/ErrorMessageExposureRemediator.java‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,12 @@
11
package io.codemodder.remediation.errorexposure;
22

33
import com.github.javaparser.ast.CompilationUnit;
4-
import com.github.javaparser.ast.Node;
5-
import com.github.javaparser.ast.expr.MethodCallExpr;
6-
import com.github.javaparser.ast.expr.StringLiteralExpr;
74
import io.codemodder.CodemodFileScanningResult;
85
import io.codemodder.codetf.DetectorRule;
9-
import io.codemodder.remediation.FixCandidateSearcher;
106
import io.codemodder.remediation.Remediator;
117
import io.codemodder.remediation.SearcherStrategyRemediator;
12-
import io.codemodder.remediation.headerinjection.HeaderInjectionFixStrategy;
13-
148
import java.util.Collection;
159
import java.util.Optional;
16-
import java.util.Set;
1710
import java.util.function.Function;
1811

1912
/**
@@ -28,7 +21,7 @@ public final class ErrorMessageExposureRemediator<T> implements Remediator<T> {
2821
public ErrorMessageExposureRemediator() {
2922
this.searchStrategyRemediator =
3023
new SearcherStrategyRemediator.Builder<T>()
31-
.withMatchAndFixStrategy(new ErrorMessageExposureFixStrategy())
24+
.withMatchAndFixStrategy(new ErrorMessageExposureFixStrategy())
3225
.build();
3326
}
3427

‎framework/codemodder-base/src/main/java/io/codemodder/remediation/regexinjection/RegexInjectionRemediator.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import com.github.javaparser.ast.expr.Expression;
88
import com.github.javaparser.ast.expr.MethodCallExpr;
99
import io.codemodder.CodemodFileScanningResult;
10+
import io.codemodder.ast.ASTs;
1011
import io.codemodder.codetf.DetectorRule;
1112
import io.codemodder.remediation.*;
1213
import java.util.Collection;
@@ -27,7 +28,8 @@ public RegexInjectionRemediator() {
2728
.withMatcher(
2829
node ->
2930
Optional.of(node)
30-
.map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null)
31+
.map(n -> n instanceof Expression e ? e : null)
32+
.flatMap(ASTs::isArgumentOfMethodCall)
3133
.filter(RegexInjectionRemediator::isCompileCall)
3234
.isPresent())
3335
.build(),
@@ -37,7 +39,12 @@ public RegexInjectionRemediator() {
3739
.withMatcher(
3840
node ->
3941
Optional.of(node)
40-
.map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null)
42+
.map(n -> n instanceof Expression e ? e : null)
43+
// Must be the first argument
44+
.flatMap(
45+
e ->
46+
ASTs.isArgumentOfMethodCall(e)
47+
.filter(mce -> mce.getArgument(0) == e))
4148
.filter(RegexInjectionRemediator::isReplaceFirstCall)
4249
.isPresent())
4350
.build(),
@@ -49,7 +56,7 @@ public RegexInjectionRemediator() {
4956
private static class FixPatternCompileStrategy implements RemediationStrategy {
5057
@Override
5158
public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
52-
final MethodCallExpr compileCall = (MethodCallExpr) node;
59+
final MethodCallExpr compileCall = ASTs.isArgumentOfMethodCall((Expression) node).get();
5360
Expression argument = compileCall.getArgument(0);
5461
wrap(argument).withStaticMethod(Pattern.class.getName(), "quote", false);
5562
return SuccessOrReason.success();
@@ -75,7 +82,7 @@ private static boolean isReplaceFirstCall(final MethodCallExpr methodCallExpr) {
7582
private static class FixStringReplaceFirstStrategy implements RemediationStrategy {
7683
@Override
7784
public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
78-
final MethodCallExpr replaceFirstCall = (MethodCallExpr) node;
85+
final MethodCallExpr replaceFirstCall = ASTs.isArgumentOfMethodCall((Expression) node).get();
7986
Expression argument = replaceFirstCall.getArgument(0);
8087
wrap(argument).withStaticMethod(Pattern.class.getName(), "quote", false);
8188
return SuccessOrReason.success();

‎framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderFixStrategy.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,21 +6,24 @@
66
import com.github.javaparser.ast.expr.MethodCallExpr;
77
import com.github.javaparser.ast.expr.NameExpr;
88
import com.github.javaparser.ast.stmt.Statement;
9+
import io.codemodder.ast.ASTs;
910
import io.codemodder.remediation.RemediationStrategy;
1011
import io.codemodder.remediation.SuccessOrReason;
1112
import java.util.Optional;
1213

1314
/**
14-
* Fix strategy for XXE vulnerabilities anchored to the XMLStreamReader calls. Finds the parser's
15-
* declaration and add statements disabling external entities and features.
15+
* Fix strategy for XXE vulnerabilities anchored to arguments of a XMLStreamReader calls. Finds the
16+
* parser's declaration and add statements disabling external entities and features.
1617
*/
1718
final class XXEIntermediateXMLStreamReaderFixStrategy implements RemediationStrategy {
1819

1920
@Override
2021
public SuccessOrReason fix(final CompilationUnit cu, final Node node) {
2122

2223
var maybeCall =
23-
Optional.of(node).map(m -> m instanceof MethodCallExpr ? (MethodCallExpr) node : null);
24+
Optional.of(node)
25+
.map(m -> m instanceof Expression e ? e : null)
26+
.flatMap(ASTs::isArgumentOfMethodCall);
2427
if (maybeCall.isEmpty()) {
2528
return SuccessOrReason.reason("Not a method call");
2629
}

‎framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderRemediator.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
package io.codemodder.remediation.xxe;
22

33
import com.github.javaparser.ast.CompilationUnit;
4-
import com.github.javaparser.ast.expr.MethodCallExpr;
4+
import com.github.javaparser.ast.Node;
5+
import com.github.javaparser.ast.expr.Expression;
56
import io.codemodder.CodemodFileScanningResult;
7+
import io.codemodder.ast.ASTs;
68
import io.codemodder.codetf.DetectorRule;
79
import io.codemodder.remediation.FixCandidateSearcher;
810
import io.codemodder.remediation.Remediator;
@@ -23,9 +25,9 @@ public XXEIntermediateXMLStreamReaderRemediator() {
2325
.withMatcher(
2426
node ->
2527
Optional.of(node)
26-
.map(n -> n instanceof MethodCallExpr mce ? mce : null)
27-
.filter(mce -> mce.hasScope())
28-
.filter(mce -> mce.getArguments().isNonEmpty())
28+
.map(n -> n instanceof Expression e ? e : null)
29+
.flatMap(ASTs::isArgumentOfMethodCall)
30+
.filter(Node::hasScope)
2931
.isPresent())
3032
.build(),
3133
new XXEIntermediateXMLStreamReaderFixStrategy())

‎framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXERemediatorTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import io.codemodder.CodemodChange;
99
import io.codemodder.CodemodFileScanningResult;
1010
import io.codemodder.codetf.DetectorRule;
11+
import io.codemodder.remediation.WithoutScopePositionMatcher;
1112
import java.util.List;
1213
import java.util.Optional;
1314
import org.junit.jupiter.api.BeforeEach;
@@ -20,7 +21,7 @@ final class DefaultXXERemediatorTest {
2021

2122
@BeforeEach
2223
void setup() {
23-
this.remediator = new XXERemediator<>();
24+
this.remediator = new XXERemediator<>(new WithoutScopePositionMatcher());
2425
this.rule = new DetectorRule("xxe", "XXE", null);
2526
}
2627

@@ -48,7 +49,7 @@ public void foo() {
4849
}
4950
""";
5051

51-
List<XXEFinding> findings = List.of(new XXEFinding("foo", 11, 16));
52+
List<XXEFinding> findings = List.of(new XXEFinding("foo", 11, 15));
5253
CompilationUnit cu = StaticJavaParser.parse(vulnerableCode);
5354
LexicalPreservingPrinter.setup(cu);
5455
CodemodFileScanningResult result =

‎framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixerTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,8 @@ private static Stream<Arguments> unfixableSamples() {
7575
void setup() {
7676
fixer =
7777
new SearcherStrategyRemediator.Builder<>()
78-
.withMatchAndFixStrategy(new DocumentBuilderFactoryAtParseFixStrategy())
78+
.withMatchAndFixStrategyAndNodeMatcher(
79+
new DocumentBuilderFactoryAtParseFixStrategy(), new WithoutScopePositionMatcher())
7980
.build();
8081
}
8182

@@ -133,7 +134,7 @@ public void foo() {
133134
o -> "id",
134135
o -> 11,
135136
o -> Optional.empty(),
136-
o -> Optional.ofNullable(16));
137+
o -> Optional.ofNullable(15));
137138
assertThat(result.changes().isEmpty()).isFalse();
138139

139140
String fixedCode =

‎framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixerTest.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import io.codemodder.codetf.DetectorRule;
99
import io.codemodder.remediation.Remediator;
1010
import io.codemodder.remediation.SearcherStrategyRemediator;
11+
import io.codemodder.remediation.WithoutScopePositionMatcher;
1112
import java.util.List;
1213
import java.util.Optional;
1314
import org.junit.jupiter.api.BeforeEach;
@@ -21,7 +22,8 @@ final class TransformerFactoryAtCreationFixerTest {
2122
void setup() {
2223
fixer =
2324
new SearcherStrategyRemediator.Builder<>()
24-
.withMatchAndFixStrategy(new TransformerFactoryAtCreationFixStrategy())
25+
.withMatchAndFixStrategyAndNodeMatcher(
26+
new TransformerFactoryAtCreationFixStrategy(), new WithoutScopePositionMatcher())
2527
.build();
2628
}
2729

0 commit comments

Comments
 (0)