diff --git a/core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java index c02b9f8ef..4069d9064 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java @@ -7,11 +7,14 @@ import io.codemodder.providers.sonar.RuleIssue; import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger; import io.codemodder.remediation.GenericRemediationMetadata; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.WithoutScopePositionMatcher; import io.codemodder.remediation.xxe.XXERemediator; import io.codemodder.sonar.model.Issue; import io.codemodder.sonar.model.SonarFinding; import java.util.List; import java.util.Objects; +import java.util.Optional; import javax.inject.Inject; @Codemod( @@ -21,14 +24,14 @@ executionPriority = CodemodExecutionPriority.HIGH) public final class SonarXXECodemod extends SonarRemediatingJavaParserChanger { - private final XXERemediator remediationStrategy; + private final Remediator remediationStrategy; private final RuleIssue issues; @Inject public SonarXXECodemod(@ProvidedSonarScan(ruleId = "java:S2755") final RuleIssue issues) { super(GenericRemediationMetadata.XXE.reporter(), issues); this.issues = Objects.requireNonNull(issues); - this.remediationStrategy = XXERemediator.DEFAULT; + this.remediationStrategy = new XXERemediator<>(new WithoutScopePositionMatcher()); } @Override @@ -49,7 +52,14 @@ public CodemodFileScanningResult visit( detectorRule(), issuesForFile, SonarFinding::getKey, - f -> f.getTextRange() != null ? f.getTextRange().getStartLine() : f.getLine(), - f -> f.getTextRange().getStartOffset()); + i -> i.getTextRange() != null ? i.getTextRange().getStartLine() : i.getLine(), + i -> + i.getTextRange() != null + ? Optional.of(i.getTextRange().getEndLine()) + : Optional.empty(), + i -> + i.getTextRange() != null + ? Optional.of(i.getTextRange().getStartOffset() + 1) + : Optional.empty()); } } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java index 3320fa687..7ac1357c3 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java @@ -1,11 +1,14 @@ package io.codemodder.codemods.codeql; +import com.contrastsecurity.sarif.Result; import com.github.javaparser.ast.CompilationUnit; import io.codemodder.*; import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan; import io.codemodder.remediation.GenericRemediationMetadata; +import io.codemodder.remediation.Remediator; import io.codemodder.remediation.xxe.XXEIntermediateXMLStreamReaderRemediator; +import java.util.Optional; import javax.inject.Inject; /** A codemod for automatically fixing SQL injection from CodeQL. */ @@ -16,12 +19,12 @@ executionPriority = CodemodExecutionPriority.HIGH) public final class CodeQLXXECodemod extends CodeQLRemediationCodemod { - private final XXEIntermediateXMLStreamReaderRemediator remediator; + private final Remediator remediator; @Inject public CodeQLXXECodemod(@ProvidedCodeQLScan(ruleId = "java/xxe") final RuleSarif sarif) { super(GenericRemediationMetadata.XXE.reporter(), sarif); - this.remediator = XXEIntermediateXMLStreamReaderRemediator.DEFAULT; + this.remediator = new XXEIntermediateXMLStreamReaderRemediator<>(); } @Override @@ -42,7 +45,11 @@ public CodemodFileScanningResult visit( ruleSarif.getResultsByLocationPath(context.path()), SarifFindingKeyUtil::buildFindingId, r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(), - r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine(), - r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()); + r -> + Optional.ofNullable( + r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()), + r -> + Optional.ofNullable( + r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn())); } } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepJavaDeserializationCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepJavaDeserializationCodemod.java index 6446d968c..dd7c83d84 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepJavaDeserializationCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepJavaDeserializationCodemod.java @@ -2,6 +2,8 @@ import com.contrastsecurity.sarif.Result; import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.expr.VariableDeclarationExpr; import io.codemodder.Codemod; import io.codemodder.CodemodExecutionPriority; import io.codemodder.CodemodFileScanningResult; @@ -12,9 +14,11 @@ import io.codemodder.SarifFindingKeyUtil; import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan; +import io.codemodder.remediation.FixCandidateSearcher; import io.codemodder.remediation.GenericRemediationMetadata; import io.codemodder.remediation.Remediator; -import io.codemodder.remediation.javadeserialization.JavaDeserializationRemediator; +import io.codemodder.remediation.SearcherStrategyRemediator; +import io.codemodder.remediation.javadeserialization.JavaDeserializationFixStrategy; import java.util.Optional; import javax.inject.Inject; @@ -37,7 +41,32 @@ public SemgrepJavaDeserializationCodemod( ruleId = "java.lang.security.audit.object-deserialization.object-deserialization") final RuleSarif sarif) { super(GenericRemediationMetadata.DESERIALIZATION.reporter(), sarif); - this.remediator = new JavaDeserializationRemediator<>(); + this.remediator = + new SearcherStrategyRemediator.Builder() + .withSearcherStrategyPair( + // matches declarations + new FixCandidateSearcher.Builder() + .withMatcher( + n -> + Optional.empty() + .or( + () -> + Optional.of(n) + .map( + m -> + m instanceof VariableDeclarationExpr vde + ? vde + : null) + .filter(JavaDeserializationFixStrategy::match)) + .or( + () -> + Optional.of(n) + .map(m -> m instanceof MethodCallExpr mce ? mce : null) + .filter(JavaDeserializationFixStrategy::match)) + .isPresent()) + .build(), + new JavaDeserializationFixStrategy()) + .build(); } @Override diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepServletResponseWriterXSSCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepServletResponseWriterXSSCodemod.java index c9d574b9c..39e3db10a 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepServletResponseWriterXSSCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepServletResponseWriterXSSCodemod.java @@ -1,5 +1,6 @@ package io.codemodder.codemods.semgrep; +import com.contrastsecurity.sarif.Result; import com.github.javaparser.ast.CompilationUnit; import io.codemodder.Codemod; import io.codemodder.CodemodExecutionPriority; @@ -12,7 +13,9 @@ import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan; import io.codemodder.remediation.GenericRemediationMetadata; +import io.codemodder.remediation.Remediator; import io.codemodder.remediation.xss.XSSRemediator; +import java.util.Optional; import javax.inject.Inject; /** @@ -26,7 +29,7 @@ importance = Importance.MEDIUM) public final class SemgrepServletResponseWriterXSSCodemod extends SemgrepJavaParserChanger { - private final XSSRemediator remediator; + private final Remediator remediator; @Inject public SemgrepServletResponseWriterXSSCodemod( @@ -34,7 +37,7 @@ public SemgrepServletResponseWriterXSSCodemod( ruleId = "java.lang.security.servletresponse-writer-xss.servletresponse-writer-xss") final RuleSarif sarif) { super(GenericRemediationMetadata.XSS.reporter(), sarif); - this.remediator = XSSRemediator.DEFAULT; + this.remediator = new XSSRemediator<>(); } @Override @@ -48,13 +51,16 @@ public DetectorRule detectorRule() { @Override public CodemodFileScanningResult visit( final CodemodInvocationContext context, final CompilationUnit cu) { - return remediator.remediateJava( + return remediator.remediateAll( cu, context.path().toString(), detectorRule(), ruleSarif.getResultsByLocationPath(context.path()), SarifFindingKeyUtil::buildFindingId, r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(), - r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()); + r -> Optional.of(r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()), + r -> + Optional.of( + r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn())); } } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepWeakRandomCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepWeakRandomCodemod.java index 5b42288c3..cb4ef4101 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepWeakRandomCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepWeakRandomCodemod.java @@ -1,5 +1,6 @@ package io.codemodder.codemods.semgrep; +import com.contrastsecurity.sarif.Result; import com.github.javaparser.ast.CompilationUnit; import io.codemodder.Codemod; import io.codemodder.CodemodExecutionPriority; @@ -11,7 +12,9 @@ import io.codemodder.SarifFindingKeyUtil; import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan; +import io.codemodder.remediation.Remediator; import io.codemodder.remediation.weakrandom.WeakRandomRemediator; +import java.util.Optional; import javax.inject.Inject; /** @@ -25,14 +28,14 @@ importance = Importance.MEDIUM) public final class SemgrepWeakRandomCodemod extends SemgrepJavaParserChanger { - private final WeakRandomRemediator remediator; + private final Remediator remediator; @Inject public SemgrepWeakRandomCodemod( @ProvidedSemgrepScan(ruleId = "java.lang.security.audit.crypto.weak-random.weak-random") final RuleSarif sarif) { super(io.codemodder.remediation.GenericRemediationMetadata.WEAK_RANDOM.reporter(), sarif); - this.remediator = WeakRandomRemediator.DEFAULT; + this.remediator = new WeakRandomRemediator<>(); } @Override @@ -53,6 +56,9 @@ public CodemodFileScanningResult visit( ruleSarif.getResultsByLocationPath(context.path()), SarifFindingKeyUtil::buildFindingId, r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(), - r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()); + r -> Optional.of(r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()), + r -> + Optional.of( + r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn())); } } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepXXECodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepXXECodemod.java index e04526583..d46986d1e 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepXXECodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepXXECodemod.java @@ -1,5 +1,6 @@ package io.codemodder.codemods.semgrep; +import com.contrastsecurity.sarif.Result; import com.github.javaparser.ast.CompilationUnit; import io.codemodder.Codemod; import io.codemodder.CodemodExecutionPriority; @@ -13,7 +14,10 @@ import io.codemodder.codetf.DetectorRule; import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan; import io.codemodder.remediation.GenericRemediationMetadata; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.WithoutScopePositionMatcher; import io.codemodder.remediation.xxe.XXERemediator; +import java.util.Optional; import javax.inject.Inject; /** Fixes some Semgrep XXE issues. */ @@ -35,7 +39,7 @@ public SemgrepXXECodemod( } public static class SemgrepXXEDocumentBuilderFactoryCodemod extends SemgrepJavaParserChanger { - private final XXERemediator remediator; + private final Remediator remediator; @Inject public SemgrepXXEDocumentBuilderFactoryCodemod( @@ -44,7 +48,7 @@ public SemgrepXXEDocumentBuilderFactoryCodemod( "java.lang.security.audit.xxe.documentbuilderfactory-disallow-doctype-decl-missing.documentbuilderfactory-disallow-doctype-decl-missing") final RuleSarif sarif) { super(GenericRemediationMetadata.WEAK_RANDOM.reporter(), sarif); - this.remediator = XXERemediator.DEFAULT; + this.remediator = new XXERemediator<>(new WithoutScopePositionMatcher()); } @Override @@ -65,15 +69,14 @@ public CodemodFileScanningResult visit( ruleSarif.getResultsByLocationPath(context.path()), SarifFindingKeyUtil::buildFindingId, r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(), - // we don't pass the column because it's deceiving as the column points to beginning of - // statement, not call - r -> null); + r -> Optional.of(r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()), + r -> Optional.empty()); } } public static class SemgrepXXESaxParserFactoryCodemod extends SemgrepJavaParserChanger { - private final XXERemediator remediator; + private final Remediator remediator; @Inject public SemgrepXXESaxParserFactoryCodemod( @@ -82,7 +85,7 @@ public SemgrepXXESaxParserFactoryCodemod( "java.lang.security.audit.xxe.saxparserfactory-disallow-doctype-decl-missing.saxparserfactory-disallow-doctype-decl-missing") final RuleSarif sarif) { super(GenericRemediationMetadata.WEAK_RANDOM.reporter(), sarif); - this.remediator = XXERemediator.DEFAULT; + this.remediator = new XXERemediator<>(); } @Override @@ -103,9 +106,8 @@ public CodemodFileScanningResult visit( ruleSarif.getResultsByLocationPath(context.path()), SarifFindingKeyUtil::buildFindingId, r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(), - // we don't pass the column because it's deceiving as the column points to beginning of - // statement, not call - r -> null); + r -> Optional.of(r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()), + r -> Optional.empty()); } } } diff --git a/core-codemods/src/test/resources/semgrep-missing-secure-flag/JWTVotesEndpoint.java.after b/core-codemods/src/test/resources/semgrep-missing-secure-flag/JWTVotesEndpoint.java.after index e1aa9fa1c..d05c4386b 100644 --- a/core-codemods/src/test/resources/semgrep-missing-secure-flag/JWTVotesEndpoint.java.after +++ b/core-codemods/src/test/resources/semgrep-missing-secure-flag/JWTVotesEndpoint.java.after @@ -134,6 +134,7 @@ public class JWTVotesEndpoint extends AssignmentEndpoint { response.setContentType(MediaType.APPLICATION_JSON_VALUE); } else { Cookie cookie = new Cookie("access_token", ""); + cookie.setSecure(true); response.addCookie(cookie); response.setStatus(HttpStatus.UNAUTHORIZED.value()); response.setContentType(MediaType.APPLICATION_JSON_VALUE); diff --git a/framework/codemodder-base/src/main/java/io/codemodder/ast/ASTs.java b/framework/codemodder-base/src/main/java/io/codemodder/ast/ASTs.java index f12bfef41..40cd8d471 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/ast/ASTs.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/ast/ASTs.java @@ -883,6 +883,33 @@ public boolean hasNext() { } } + /** + * Checks if a node is a MethodCallExpr that is the initialization of a declaration with one of + * the types in assignedToTypes. + * + * @param node + * @param methodName + * @param assignedToTypes + * @return + */ + public static Optional isInitializedToType( + final Node node, final String methodName, final List assignedToTypes) { + return Optional.of(node) + .map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null) + .filter(m -> methodName.equals(m.getNameAsString())) + .filter( + m -> { + Optional newFactoryVariableRef = + expect(m).toBeMethodCallExpression().initializingVariable().result(); + if (newFactoryVariableRef.isEmpty()) { + return false; + } + String type = newFactoryVariableRef.get().getTypeAsString(); + return assignedToTypes.contains(type) + || assignedToTypes.stream().anyMatch(type::endsWith); + }); + } + /** * This finds all methods that match the given location, with the given name, and is assigned to a * variable of one of the given types. diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultFixCandidateSearcher.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultFixCandidateSearcher.java index b46a44b36..5d89da027 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultFixCandidateSearcher.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultFixCandidateSearcher.java @@ -1,8 +1,8 @@ package io.codemodder.remediation; -import com.github.javaparser.Position; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.nodeTypes.NodeWithRange; import io.codemodder.codetf.DetectorRule; import io.codemodder.codetf.UnfixedFinding; import java.util.*; @@ -19,8 +19,17 @@ final class DefaultFixCandidateSearcher implements FixCandidateSearcher { private final List> matchers; + private final NodePositionMatcher nodePositionMatcher; + DefaultFixCandidateSearcher(final List> matchers) { this.matchers = matchers; + this.nodePositionMatcher = new DefaultNodePositionMatcher(); + } + + DefaultFixCandidateSearcher( + final List> matchers, final NodePositionMatcher nodePositionMatcher) { + this.matchers = matchers; + this.nodePositionMatcher = nodePositionMatcher; } @Override @@ -42,23 +51,17 @@ public FixCandidateSearchResults search( .filter(n -> matchers.stream().allMatch(m -> m.test(n))) .toList(); - Map> fixCandidateToIssueMapping = new HashMap<>(); + Map> fixCandidateToIssueMapping = new IdentityHashMap<>(); + Set matchedIssues = new HashSet<>(); for (T issue : issuesForFile) { String findingId = getKey.apply(issue); int issueStartLine = getStartLine.apply(issue); - Optional maybeEndLine = getEndLine.apply(issue); + int issueEndLine = getEndLine.apply(issue).orElse(issueStartLine); Optional maybeColumn = getColumn.apply(issue); List nodesForIssue = nodes.stream() - .filter( - n -> { - int nodeStartLine = n.getRange().orElseThrow().begin.line; - // if issue end line info is present, match the node line with the issue range - return maybeEndLine - .map(issueEndLine -> matches(issueStartLine, nodeStartLine, issueEndLine)) - .orElse(matches(issueStartLine, nodeStartLine)); - }) + .filter(NodeWithRange::hasRange) // if column info is present, check if the node starts after the issue start // coordinates .filter( @@ -66,19 +69,15 @@ public FixCandidateSearchResults search( maybeColumn .map( column -> - n.getRange() - .orElseThrow() - .begin - .compareTo(new Position(issueStartLine, column)) - >= 0) - .orElse(true)) + nodePositionMatcher.match( + n, issueStartLine, issueEndLine, column)) + .orElse(nodePositionMatcher.match(n, issueStartLine, issueEndLine))) .toList(); if (nodesForIssue.isEmpty()) { - unfixedFindings.add( - new UnfixedFinding( - findingId, rule, path, issueStartLine, RemediationMessages.noNodesAtThatLocation)); continue; - } else if (nodesForIssue.size() > 1) { + } + matchedIssues.add(issue); + if (nodesForIssue.size() > 1) { unfixedFindings.add( new UnfixedFinding( findingId, rule, path, issueStartLine, RemediationMessages.multipleNodesFound)); @@ -93,12 +92,18 @@ public FixCandidateSearchResults search( .map(entry -> new FixCandidate<>(entry.getKey(), entry.getValue())) .toList(); + // remove any issue that had a match return new FixCandidateSearchResults() { @Override public List unfixableFindings() { return unfixedFindings; } + @Override + public List unmatchedIssues() { + return issuesForFile.stream().filter(i -> !matchedIssues.contains(i)).toList(); + } + @Override public List> fixCandidates() { return fixCandidates; diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultLegacyFixCandidateSearcher.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultLegacyFixCandidateSearcher.java deleted file mode 100644 index e8e3929d6..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultLegacyFixCandidateSearcher.java +++ /dev/null @@ -1,124 +0,0 @@ -package io.codemodder.remediation; - -import com.github.javaparser.Position; -import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.Node; -import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.expr.ObjectCreationExpr; -import io.codemodder.codetf.DetectorRule; -import io.codemodder.codetf.UnfixedFinding; -import java.util.*; -import java.util.function.Function; -import java.util.function.Predicate; -import org.jetbrains.annotations.VisibleForTesting; - -@Deprecated -final class DefaultLegacyFixCandidateSearcher implements LegacyFixCandidateSearcher { - - private final List> matchers; - private final String methodName; - - DefaultLegacyFixCandidateSearcher( - final String methodName, final List> matchers) { - this.methodName = methodName; - this.matchers = matchers; - } - - @Override - public LegacyFixCandidateSearchResults search( - final CompilationUnit cu, - final String path, - final DetectorRule rule, - final List issuesForFile, - final Function getKey, - final Function getStartLine, - final Function getEndLine, - final Function getColumn) { - - List unfixedFindings = new ArrayList<>(); - - List calls = - cu.findAll(Node.class).stream() - // limit to just the interesting nodes for us - .filter(n -> n instanceof MethodCallExpr || n instanceof ObjectCreationExpr) - // turn them into our convenience type - .map(MethodOrConstructor::new) - // don't find calls we may have added -- you can pick these out by them not having a - // range yet - .filter(MethodOrConstructor::hasRange) - // filter by method name if one is provided in the search - .filter(mce -> methodName == null || mce.isMethodCallWithName(methodName)) - // filter by matchers - .filter(mce -> matchers.stream().allMatch(m -> m.test(mce))) - .toList(); - - Map> fixCandidateToIssueMapping = new HashMap<>(); - - for (T issue : issuesForFile) { - String findingId = getKey.apply(issue); - int issueStartLine = getStartLine.apply(issue); - Integer issueEndLine = getEndLine.apply(issue); - Integer column = getColumn.apply(issue); - List callsForIssue = - calls.stream() - .filter(MethodOrConstructor::hasRange) - .filter( - mce -> { - int callStartLine = mce.getRange().begin.line; - return matches(issueStartLine, issueEndLine, callStartLine); - }) - .toList(); - if (callsForIssue.size() > 1 && column != null) { - callsForIssue = - callsForIssue.stream() - .filter(mce -> mce.getRange().contains(new Position(issueStartLine, column))) - .toList(); - } - if (callsForIssue.isEmpty()) { - unfixedFindings.add( - new UnfixedFinding( - findingId, rule, path, issueStartLine, RemediationMessages.noNodesAtThatLocation)); - continue; - } else if (callsForIssue.size() > 1) { - unfixedFindings.add( - new UnfixedFinding( - findingId, rule, path, issueStartLine, RemediationMessages.multipleNodesFound)); - continue; - } - MethodOrConstructor call = callsForIssue.get(0); - fixCandidateToIssueMapping.computeIfAbsent(call, k -> new ArrayList<>()).add(issue); - } - - List> legacyFixCandidates = - fixCandidateToIssueMapping.entrySet().stream() - .map(entry -> new LegacyFixCandidate<>(entry.getKey(), entry.getValue())) - .toList(); - - return new LegacyFixCandidateSearchResults() { - @Override - public List unfixableFindings() { - return unfixedFindings; - } - - @Override - public List> fixCandidates() { - return legacyFixCandidates; - } - }; - } - - @VisibleForTesting - static boolean matches( - final int issueStartLine, final Integer issueEndLine, final int startCallLine) { - // if the issue spans multiple lines, the call must be within that range - if (issueEndLine != null) { - return isInBetween(startCallLine, issueStartLine, issueEndLine); - } - // if the issue is on a single line, the call must be on that line - return startCallLine == issueStartLine; - } - - private static boolean isInBetween(int number, int lowerBound, int upperBound) { - return number >= lowerBound && number <= upperBound; - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultNodePositionMatcher.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultNodePositionMatcher.java new file mode 100644 index 000000000..f55f2926d --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/DefaultNodePositionMatcher.java @@ -0,0 +1,40 @@ +package io.codemodder.remediation; + +import com.github.javaparser.Position; +import com.github.javaparser.Range; +import com.github.javaparser.ast.Node; + +class DefaultNodePositionMatcher implements NodePositionMatcher { + + @Override + public boolean match(final Node node, final int line) { + return getRange(node).begin.line == line; + } + + @Override + public boolean match(final Node node, int startLine, int endLine) { + return inInterval(getRange(node).begin.line, startLine, endLine); + } + + @Override + public boolean match(final Node node, int startLine, int endLine, int startColumn) { + return match(node, startLine, endLine) + && getRange(node).begin.compareTo(new Position(startLine, startColumn)) <= 0; + } + + @Override + public boolean match( + final Node node, int startLine, int endLine, int startColumn, int endColumn) { + return getRange(node) + .strictlyContains( + new Range(new Position(startLine, startColumn), new Position(endLine, endColumn))); + } + + private boolean inInterval(int number, int upper, int lower) { + return number >= upper && number <= lower; + } + + protected Range getRange(final Node node) { + return node.getRange().orElseThrow(); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidateSearchResults.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidateSearchResults.java index e7c9aaad7..d8b4096a6 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidateSearchResults.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidateSearchResults.java @@ -5,9 +5,17 @@ /** The results of a fix candidate search. */ public interface FixCandidateSearchResults { + /** The findings that for which we could not find potential fix locations. */ List unfixableFindings(); + /** + * Issues that were not matched by this searcher + * + * @return + */ + List unmatchedIssues(); + /** The potential fix locations. */ List> fixCandidates(); } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidateSearcher.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidateSearcher.java index dd2654258..df5cbfa7c 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidateSearcher.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidateSearcher.java @@ -11,17 +11,19 @@ public interface FixCandidateSearcher { /** - * Searches the AST for nodes associated with the given issues. + * Searches the AST for nodes associated with the given issues. Issues are sorted into + * FixCandidates, UnfixedFindings and unmatched. * * @param cu * @param path * @param rule - * @param issuesForFile + * @param issuesForFile A list of issues. * @param getKey A function that extracts the key for T. * @param getStartLine A function that extracts start line information from T. Always required. * @param getEndLine A function that extracts end line information from T. May not be available * @param getColumn A function that extracts column information from T. May not be available - * @return + * @return A FixSearchResults object that sorts the issues into FixCandidates, unfixed and + * unmatched. */ FixCandidateSearchResults search( CompilationUnit cu, @@ -36,9 +38,11 @@ FixCandidateSearchResults search( /** Builder for {@link FixCandidateSearcher}. */ final class Builder { private final List> matchers; + private NodePositionMatcher nodePositionMatcher; public Builder() { this.matchers = new ArrayList<>(); + this.nodePositionMatcher = new DefaultNodePositionMatcher(); } public Builder withMatcher(final Predicate matcher) { @@ -46,8 +50,13 @@ public Builder withMatcher(final Predicate matcher) { return this; } + public Builder withNodePositionMatcher(final NodePositionMatcher nodePositionMatcher) { + this.nodePositionMatcher = nodePositionMatcher; + return this; + } + public FixCandidateSearcher build() { - return new DefaultFixCandidateSearcher<>(List.copyOf(matchers)); + return new DefaultFixCandidateSearcher<>(List.copyOf(matchers), nodePositionMatcher); } } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/LegacyFixCandidate.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/LegacyFixCandidate.java deleted file mode 100644 index c3378a483..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/LegacyFixCandidate.java +++ /dev/null @@ -1,17 +0,0 @@ -package io.codemodder.remediation; - -import java.util.List; -import java.util.Objects; - -/** The potential fix location. */ -@Deprecated -public record LegacyFixCandidate(MethodOrConstructor call, List issues) { - - public LegacyFixCandidate { - Objects.requireNonNull(call); - Objects.requireNonNull(issues); - if (issues.isEmpty()) { - throw new IllegalArgumentException("issues cannot be empty"); - } - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/LegacyFixCandidateSearchResults.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/LegacyFixCandidateSearchResults.java deleted file mode 100644 index 684c1ff20..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/LegacyFixCandidateSearchResults.java +++ /dev/null @@ -1,14 +0,0 @@ -package io.codemodder.remediation; - -import io.codemodder.codetf.UnfixedFinding; -import java.util.List; - -/** The results of a fix candidate search. */ -@Deprecated -public interface LegacyFixCandidateSearchResults { - /** The findings that for which we could not find potential fix locations. */ - List unfixableFindings(); - - /** The potential fix locations. */ - List> fixCandidates(); -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/LegacyFixCandidateSearcher.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/LegacyFixCandidateSearcher.java deleted file mode 100644 index 378494d90..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/LegacyFixCandidateSearcher.java +++ /dev/null @@ -1,49 +0,0 @@ -package io.codemodder.remediation; - -import com.github.javaparser.ast.CompilationUnit; -import io.codemodder.codetf.DetectorRule; -import java.util.ArrayList; -import java.util.List; -import java.util.Objects; -import java.util.function.Function; -import java.util.function.Predicate; - -/** Searches for potential fix locations in the source code. */ -@Deprecated -public interface LegacyFixCandidateSearcher { - - /** Searches for potential fix locations in the source code. */ - LegacyFixCandidateSearchResults search( - CompilationUnit cu, - String path, - DetectorRule rule, - List issuesForFile, - Function getKey, - Function getStartLine, - Function getEndLine, - Function getColumn); - - /** Builder for {@link LegacyFixCandidateSearcher}. */ - final class Builder { - private String methodName; - private final List> methodMatchers; - - public Builder() { - this.methodMatchers = new ArrayList<>(); - } - - public Builder withMethodName(final String methodName) { - this.methodName = Objects.requireNonNull(methodName); - return this; - } - - public Builder withMatcher(final Predicate methodMatcher) { - this.methodMatchers.add(Objects.requireNonNull(methodMatcher)); - return this; - } - - public LegacyFixCandidateSearcher build() { - return new DefaultLegacyFixCandidateSearcher<>(methodName, List.copyOf(methodMatchers)); - } - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/MatchAndFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/MatchAndFixStrategy.java new file mode 100644 index 000000000..bf1bb777c --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/MatchAndFixStrategy.java @@ -0,0 +1,15 @@ +package io.codemodder.remediation; + +import com.github.javaparser.ast.Node; + +/** Provides matching logic as well as a fix strategy */ +public abstract class MatchAndFixStrategy implements RemediationStrategy { + + /** + * Matches a node against an expected pattern for a fix. + * + * @param node + * @return + */ + public abstract boolean match(final Node node); +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/NodePositionMatcher.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/NodePositionMatcher.java new file mode 100644 index 000000000..4c5c400b8 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/NodePositionMatcher.java @@ -0,0 +1,51 @@ +package io.codemodder.remediation; + +import com.github.javaparser.ast.Node; + +/** Provides methods for matching nodes to given coordinates. */ +public interface NodePositionMatcher { + + static final NodePositionMatcher DEFAULT = new DefaultNodePositionMatcher(); + + /** + * Matches a node with a range against a line + * + * @param node + * @param line + * @return + */ + boolean match(Node node, int line); + + /** + * Matches a node with a range against a line range + * + * @param node + * @param startLine + * @param endLine + * @return + */ + boolean match(Node node, int startLine, int endLine); + + /** + * Matches a node with a range against a line range and column + * + * @param node + * @param startLine + * @param endLine + * @param StartColumn + * @return + */ + boolean match(Node node, int startLine, int endLine, int StartColumn); + + /** + * Matches a node with a range against another range + * + * @param node + * @param startLine + * @param endLine + * @param StartColumn + * @param EndColumn + * @return + */ + boolean match(Node node, int startLine, int endLine, int StartColumn, int EndColumn); +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/SearcherStrategyRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/SearcherStrategyRemediator.java index 5075a5991..fdaace55d 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/SearcherStrategyRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/SearcherStrategyRemediator.java @@ -11,7 +11,7 @@ import java.util.function.BiFunction; import java.util.function.Function; import java.util.function.Predicate; -import org.javatuples.Pair; +import org.javatuples.Triplet; /** * Remediates issues with pairs of searchers and strategies. Searchers will associate an issue with @@ -35,6 +35,25 @@ public Builder withSearcherStrategyPair( return this; } + public Builder withMatchAndFixStrategy(final MatchAndFixStrategy maf) { + Objects.requireNonNull(maf); + this.searcherRemediatorMap.put( + new FixCandidateSearcher.Builder().withMatcher(maf::match).build(), maf); + return this; + } + + public Builder withMatchAndFixStrategyAndNodeMatcher( + final MatchAndFixStrategy maf, final NodePositionMatcher nodeMatcher) { + Objects.requireNonNull(maf); + this.searcherRemediatorMap.put( + new FixCandidateSearcher.Builder() + .withMatcher(maf::match) + .withNodePositionMatcher(nodeMatcher) + .build(), + maf); + return this; + } + public Builder withFunctions( final Predicate searcherMatcher, final BiFunction fixer) { @@ -59,11 +78,11 @@ protected SearcherStrategyRemediator( } /** Remediate with a chosen searcher-strategy pair. */ - private Pair, List> remediateWithStrategy( + private Triplet, List, List> remediateWithStrategy( final CompilationUnit cu, final String path, final DetectorRule detectorRule, - final Collection findingsForPath, + final List findingsForPath, final Function findingIdExtractor, final Function findingStartLineExtractor, final Function> findingEndLineExtractor, @@ -71,21 +90,20 @@ private Pair, List> remediateWithStrategy( final FixCandidateSearcher searcher, final RemediationStrategy strategy) { + if (findingsForPath.isEmpty()) { + return Triplet.with(List.of(), List.of(), findingsForPath); + } FixCandidateSearchResults results = searcher.search( cu, path, detectorRule, - new ArrayList<>(findingsForPath), + findingsForPath, findingIdExtractor, findingStartLineExtractor, findingEndLineExtractor, findingColumnExtractor); - if (findingsForPath.isEmpty()) { - return Pair.with(List.of(), List.of()); - } - final List unfixedFindings = new ArrayList<>(results.unfixableFindings()); final List changes = new ArrayList<>(); @@ -112,7 +130,6 @@ private Pair, List> remediateWithStrategy( .map(findingIdExtractor) .map(findingId -> new FixedFinding(findingId, detectorRule)) .toList(); - changes.add(CodemodChange.from(line, maybeDeps.getDependencies(), fixedFindings)); } else { issues.forEach( @@ -124,7 +141,7 @@ private Pair, List> remediateWithStrategy( }); } } - return Pair.with(changes, unfixedFindings); + return Triplet.with(changes, unfixedFindings, results.unmatchedIssues()); } public CodemodFileScanningResult remediateAll( @@ -136,26 +153,39 @@ public CodemodFileScanningResult remediateAll( final Function findingStartLineExtractor, final Function> findingEndLineExtractor, final Function> findingStartColumnExtractor) { - + List findings = new ArrayList<>(findingsForPath); List allChanges = new ArrayList<>(); List allUnfixed = new ArrayList<>(); for (var searcherAndStrategy : searcherRemediatorMap.entrySet()) { - var pairResult = + var tripletResults = remediateWithStrategy( cu, path, detectorRule, - findingsForPath, + findings, findingIdExtractor, findingStartLineExtractor, findingEndLineExtractor, findingStartColumnExtractor, searcherAndStrategy.getKey(), searcherAndStrategy.getValue()); - allChanges.addAll(pairResult.getValue0()); - allUnfixed.addAll(pairResult.getValue1()); + allChanges.addAll(tripletResults.getValue0()); + allUnfixed.addAll(tripletResults.getValue1()); + findings = tripletResults.getValue2(); } + // Any remaining, unmatched, findings are treated as unfixed + allUnfixed.addAll( + findings.stream() + .map( + f -> + new UnfixedFinding( + findingIdExtractor.apply(f), + detectorRule, + path, + findingStartLineExtractor.apply(f), + RemediationMessages.noNodesAtThatLocation)) + .toList()); return CodemodFileScanningResult.from(allChanges, allUnfixed); } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/WithoutScopePositionMatcher.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/WithoutScopePositionMatcher.java new file mode 100644 index 000000000..fbed39b72 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/WithoutScopePositionMatcher.java @@ -0,0 +1,22 @@ +package io.codemodder.remediation; + +import com.github.javaparser.Position; +import com.github.javaparser.Range; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.nodeTypes.NodeWithTraversableScope; + +/** Removes the range of the node's scope before matching it against a position. */ +public final class WithoutScopePositionMatcher extends DefaultNodePositionMatcher { + + @Override + protected Range getRange(final Node node) { + var originalRange = node.getRange().orElseThrow(); + if (node.hasScope()) { + var scope = ((NodeWithTraversableScope) node).traverseScope().get(); + var scopeRange = scope.getRange().orElseThrow(); + return new Range( + new Position(scopeRange.end.line, scopeRange.end.column + 1), originalRange.end); + } + return originalRange; + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/JavaDeserializationFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/JavaDeserializationFixStrategy.java index 87f992ae5..00fd5cb81 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/JavaDeserializationFixStrategy.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/JavaDeserializationFixStrategy.java @@ -5,10 +5,7 @@ import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.Node; import com.github.javaparser.ast.body.VariableDeclarator; -import com.github.javaparser.ast.expr.Expression; -import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.expr.NameExpr; -import com.github.javaparser.ast.expr.ObjectCreationExpr; +import com.github.javaparser.ast.expr.*; import io.codemodder.DependencyGAV; import io.codemodder.Either; import io.codemodder.ast.ASTs; @@ -60,18 +57,22 @@ private Either findConstructor(final MethodCallExpr @Override public SuccessOrReason fix(final CompilationUnit cu, final Node node) { - + // We know that the target must exist and be an OCE from the match + final Node consideredNode = + node instanceof VariableDeclarationExpr vd + ? vd.getVariable(0).getInitializer().get().asObjectCreationExpr() + : node; Optional> maybeCallOrConstructor = Optional.>empty() .or( () -> - node instanceof MethodCallExpr - ? Optional.of(Either.left((MethodCallExpr) node)) + consideredNode instanceof MethodCallExpr + ? Optional.of(Either.left((MethodCallExpr) consideredNode)) : Optional.empty()) .or( () -> - node instanceof ObjectCreationExpr - ? Optional.of(Either.right((ObjectCreationExpr) node)) + consideredNode instanceof ObjectCreationExpr + ? Optional.of(Either.right((ObjectCreationExpr) consideredNode)) : Optional.empty()); if (maybeCallOrConstructor.isEmpty()) { return SuccessOrReason.reason("Not a call or constructor"); @@ -85,7 +86,7 @@ public SuccessOrReason fix(final CompilationUnit cu, final Node node) { // declaration of the ObjectInputStream .ifLeftOrElseGet(this::findConstructor, Either::left); - // afailed to find the construction + // failed to find the construction if (maybeConstructor.isRight()) { return SuccessOrReason.reason(maybeConstructor.getRight()); } @@ -100,4 +101,61 @@ private void fixObjectInputStreamCreation(final ObjectCreationExpr objCreation) .withStaticImport() .withSameArguments(); } + + /** + * Match code shape for AssignExpr case + * + * @param node + * @return + */ + public static boolean match(final VariableDeclarationExpr node) { + return Optional.of(node) + .flatMap(vde -> vde.getVariables().getFirst()) + .flatMap(VariableDeclarator::getInitializer) + .map(e -> e.isObjectCreationExpr() ? e.asObjectCreationExpr() : null) + .filter(JavaDeserializationFixStrategy::match) + .isPresent(); + } + + /** + * Match code shape for ObjectCreationExpr case + * + * @param node + * @return + */ + public static boolean match(final ObjectCreationExpr node) { + return Optional.of(node) + .map(n -> n instanceof ObjectCreationExpr ? (ObjectCreationExpr) n : null) + .filter(oce -> "ObjectInputStream".equals(oce.getTypeAsString())) + .isPresent(); + } + + /** + * Match code shape for MethodCallExpr case + * + * @param node + * @return + */ + public static boolean match(final MethodCallExpr node) { + return Optional.of(node) + .map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null) + .filter(mce -> mce.getNameAsString().equals("readObject")) + .filter(mce -> mce.getArguments().isEmpty()) + .isPresent(); + } + + /** + * Default matching + * + * @param node + * @return + */ + public static boolean match(final Node node) { + if (node instanceof MethodCallExpr mce) { + return match(mce); + } else if (node instanceof ObjectCreationExpr oce) { + return match(oce); + } + return false; + } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/JavaDeserializationRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/JavaDeserializationRemediator.java index ce7320f99..041d25785 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/JavaDeserializationRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/JavaDeserializationRemediator.java @@ -1,9 +1,6 @@ package io.codemodder.remediation.javadeserialization; import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.Node; -import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.expr.ObjectCreationExpr; import io.codemodder.CodemodFileScanningResult; import io.codemodder.codetf.DetectorRule; import io.codemodder.remediation.*; @@ -25,34 +22,7 @@ public JavaDeserializationRemediator() { new SearcherStrategyRemediator.Builder() .withSearcherStrategyPair( new FixCandidateSearcher.Builder() - .withMatcher( - node -> - Optional.empty() - .or( - () -> - Optional.of(node) - .map( - n -> - n instanceof MethodCallExpr - ? (MethodCallExpr) n - : null) - .filter(Node::hasScope) - .filter( - mce -> mce.getNameAsString().equals("readObject")) - .filter(mce -> mce.getArguments().isEmpty())) - .or( - () -> - Optional.of(node) - .map( - n -> - n instanceof ObjectCreationExpr - ? (ObjectCreationExpr) n - : null) - .filter( - oce -> - "ObjectInputStream" - .equals(oce.getTypeAsString()))) - .isPresent()) + .withMatcher(JavaDeserializationFixStrategy::match) .build(), new JavaDeserializationFixStrategy()) .build(); diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/predictableseed/PredictableSeedRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/predictableseed/PredictableSeedRemediator.java index 78c1cbfee..27813c893 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/predictableseed/PredictableSeedRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/predictableseed/PredictableSeedRemediator.java @@ -1,80 +1,75 @@ package io.codemodder.remediation.predictableseed; import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; import com.github.javaparser.ast.NodeList; import com.github.javaparser.ast.expr.Expression; import com.github.javaparser.ast.expr.LiteralExpr; import com.github.javaparser.ast.expr.MethodCallExpr; import com.github.javaparser.ast.expr.NameExpr; -import io.codemodder.CodemodChange; import io.codemodder.CodemodFileScanningResult; import io.codemodder.codetf.DetectorRule; -import io.codemodder.codetf.FixedFinding; import io.codemodder.remediation.*; -import java.util.ArrayList; import java.util.Collection; -import java.util.List; import java.util.Optional; import java.util.function.Function; /** Remediator for predictable seed weaknesses. */ public final class PredictableSeedRemediator implements Remediator { - @Override - public CodemodFileScanningResult remediateAll( - final CompilationUnit cu, - final String path, - final DetectorRule detectorRule, - final Collection findingsForPath, - final Function findingIdExtractor, - final Function findingStartLineExtractor, - final Function> findingEndLineExtractor, - final Function> findingStartColumnExtractor) { + private final SearcherStrategyRemediator searchStrategyRemediator; - FixCandidateSearcher searcher = - new FixCandidateSearcher.Builder() - .withMatcher( - n -> - Optional.of(n) - .map(MethodOrConstructor::new) - .filter(mce -> mce.isMethodCallWithName("setSeed")) - .filter(mce -> mce.asNode().hasScope()) - .filter(mce -> mce.getArguments().size() == 1) - // technically, we don't need this, just to prevent a silly tool from - // reporting on hardcoded data - .filter(mce -> !(mce.getArguments().get(0) instanceof LiteralExpr)) - .isPresent()) + public PredictableSeedRemediator() { + this.searchStrategyRemediator = + new SearcherStrategyRemediator.Builder() + .withSearcherStrategyPair( + new FixCandidateSearcher.Builder() + .withMatcher( + n -> + Optional.of(n) + .map(MethodOrConstructor::new) + .filter(mce -> mce.isMethodCallWithName("setSeed")) + .filter(mce -> mce.asNode().hasScope()) + .filter(mce -> mce.getArguments().size() == 1) + // technically, we don't need this, just to prevent a silly tool + // from + // reporting on hardcoded data + .filter(mce -> !(mce.getArguments().get(0) instanceof LiteralExpr)) + .isPresent()) + .build(), + new RemediationStrategy() { + @Override + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + MethodCallExpr setSeedCall = (MethodCallExpr) node; + MethodCallExpr safeExpression = + new MethodCallExpr( + new NameExpr(System.class.getSimpleName()), "currentTimeMillis"); + NodeList arguments = setSeedCall.getArguments(); + arguments.set(0, safeExpression); + return SuccessOrReason.success(); + } + }) .build(); + } - FixCandidateSearchResults candidateSearchResults = - searcher.search( - cu, - path, - detectorRule, - List.copyOf(findingsForPath), - findingIdExtractor, - findingStartLineExtractor, - findingEndLineExtractor, - findingStartColumnExtractor); - - List changes = new ArrayList<>(); - for (FixCandidate fixCandidate : candidateSearchResults.fixCandidates()) { - MethodCallExpr setSeedCall = (MethodCallExpr) fixCandidate.node(); - MethodCallExpr safeExpression = - new MethodCallExpr(new NameExpr(System.class.getSimpleName()), "currentTimeMillis"); - NodeList arguments = setSeedCall.getArguments(); - arguments.set(0, safeExpression); - - // should only ever be one, but just in case - List fixedFindings = - fixCandidate.issues().stream() - .map(issue -> new FixedFinding(findingIdExtractor.apply(issue), detectorRule)) - .toList(); - - int reportedLine = setSeedCall.getRange().get().begin.line; - changes.add(CodemodChange.from(reportedLine, List.of(), fixedFindings)); - } - - return CodemodFileScanningResult.from(changes, candidateSearchResults.unfixableFindings()); + @Override + public CodemodFileScanningResult remediateAll( + CompilationUnit cu, + String path, + DetectorRule detectorRule, + Collection findingsForPath, + Function findingIdExtractor, + Function findingStartLineExtractor, + Function> findingEndLineExtractor, + Function> findingColumnExtractor) { + return searchStrategyRemediator.remediateAll( + cu, + path, + detectorRule, + findingsForPath, + findingIdExtractor, + findingStartLineExtractor, + findingEndLineExtractor, + findingColumnExtractor); } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/DefaultWeakRandomRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/DefaultWeakRandomRemediator.java deleted file mode 100644 index d746e7b8c..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/DefaultWeakRandomRemediator.java +++ /dev/null @@ -1,78 +0,0 @@ -package io.codemodder.remediation.weakrandom; - -import static io.codemodder.ast.ASTTransforms.addImportIfMissing; - -import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.expr.ObjectCreationExpr; -import io.codemodder.CodemodChange; -import io.codemodder.CodemodFileScanningResult; -import io.codemodder.codetf.DetectorRule; -import io.codemodder.codetf.FixedFinding; -import io.codemodder.codetf.UnfixedFinding; -import io.codemodder.remediation.RemediationMessages; -import java.security.SecureRandom; -import java.util.ArrayList; -import java.util.List; -import java.util.function.Function; - -final class DefaultWeakRandomRemediator implements WeakRandomRemediator { - - @Override - public CodemodFileScanningResult remediateAll( - final CompilationUnit cu, - final String path, - final DetectorRule detectorRule, - final List issuesForFile, - final Function getKey, - final Function getLine, - final Function getColumn) { - - List unfixedFindings = new ArrayList<>(); - List changes = new ArrayList<>(); - - for (T issue : issuesForFile) { - - List unsafeRandoms = - cu.findAll(ObjectCreationExpr.class).stream() - .filter(oc -> oc.getType().asString().equals("Random")) - .filter(oc -> getLine.apply(issue) == oc.getRange().get().begin.line) - .filter( - oc -> { - Integer column = getColumn.apply(issue); - return column == null || column == oc.getRange().get().begin.column; - }) - .toList(); - - if (unsafeRandoms.size() > 1) { - unfixedFindings.add( - new UnfixedFinding( - getKey.apply(issue), - detectorRule, - path, - getLine.apply(issue), - RemediationMessages.multipleNodesFound)); - continue; - } else if (unsafeRandoms.isEmpty()) { - unfixedFindings.add( - new UnfixedFinding( - getKey.apply(issue), - detectorRule, - path, - getLine.apply(issue), - RemediationMessages.noNodesAtThatLocation)); - continue; - } - - ObjectCreationExpr unsafeRandom = unsafeRandoms.get(0); - unsafeRandom.setType("SecureRandom"); - addImportIfMissing(cu, SecureRandom.class.getName()); - changes.add( - CodemodChange.from( - getLine.apply(issue), - List.of(), - List.of(new FixedFinding(getKey.apply(issue), detectorRule)))); - } - - return CodemodFileScanningResult.from(changes, unfixedFindings); - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomFixStrategy.java new file mode 100644 index 000000000..86c8d3358 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomFixStrategy.java @@ -0,0 +1,30 @@ +package io.codemodder.remediation.weakrandom; + +import static io.codemodder.ast.ASTTransforms.addImportIfMissing; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.ObjectCreationExpr; +import io.codemodder.DependencyGAV; +import io.codemodder.remediation.RemediationStrategy; +import io.codemodder.remediation.SuccessOrReason; +import java.security.SecureRandom; +import java.util.List; +import java.util.Optional; + +/** Fixes weak random vulnerabilities */ +final class WeakRandomFixStrategy implements RemediationStrategy { + + @Override + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + var maybeOCE = + Optional.of(node).map(n -> n instanceof ObjectCreationExpr ? (ObjectCreationExpr) n : null); + if (maybeOCE.isEmpty()) { + return SuccessOrReason.reason("Not an object creation expression"); + } + ObjectCreationExpr unsafeRandom = maybeOCE.get(); + unsafeRandom.setType("SecureRandom"); + addImportIfMissing(cu, SecureRandom.class.getName()); + return SuccessOrReason.success(List.of(DependencyGAV.JAVA_SECURITY_TOOLKIT)); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomRemediator.java index e301c0866..a8d79b78e 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomRemediator.java @@ -1,24 +1,56 @@ package io.codemodder.remediation.weakrandom; import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.*; import io.codemodder.CodemodFileScanningResult; import io.codemodder.codetf.DetectorRule; -import java.util.List; +import io.codemodder.remediation.*; +import java.util.Collection; +import java.util.Optional; import java.util.function.Function; -/** Fixes weak randomness. */ -public interface WeakRandomRemediator { +public final class WeakRandomRemediator implements Remediator { - /** A default implementation for callers. */ - WeakRandomRemediator DEFAULT = new DefaultWeakRandomRemediator(); + private final SearcherStrategyRemediator searchStrategyRemediator; - /** Remediate all weak random vulnerabilities in the given compilation unit. */ - CodemodFileScanningResult remediateAll( + public WeakRandomRemediator() { + this.searchStrategyRemediator = + new SearcherStrategyRemediator.Builder() + .withSearcherStrategyPair( + new FixCandidateSearcher.Builder() + .withMatcher( + n -> + Optional.of(n) + .map( + m -> + m instanceof ObjectCreationExpr + ? (ObjectCreationExpr) m + : null) + .filter(oc -> oc.getType().asString().equals("Random")) + .isPresent()) + .build(), + new WeakRandomFixStrategy()) + .build(); + } + + @Override + public CodemodFileScanningResult remediateAll( CompilationUnit cu, String path, DetectorRule detectorRule, - List issuesForFile, - Function getKey, - Function getStartLine, - Function getStartColumn); + Collection findingsForPath, + Function findingIdExtractor, + Function findingStartLineExtractor, + Function> findingEndLineExtractor, + Function> findingColumnExtractor) { + return searchStrategyRemediator.remediateAll( + cu, + path, + detectorRule, + findingsForPath, + findingIdExtractor, + findingStartLineExtractor, + findingEndLineExtractor, + findingColumnExtractor); + } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/DefaultXSSRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/DefaultXSSRemediator.java deleted file mode 100644 index e863b7870..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/DefaultXSSRemediator.java +++ /dev/null @@ -1,124 +0,0 @@ -package io.codemodder.remediation.xss; - -import static java.util.stream.Collectors.groupingBy; - -import com.github.javaparser.ast.CompilationUnit; -import io.codemodder.CodemodChange; -import io.codemodder.CodemodFileScanningResult; -import io.codemodder.DependencyGAV; -import io.codemodder.codetf.DetectorRule; -import io.codemodder.codetf.FixedFinding; -import io.codemodder.codetf.UnfixedFinding; -import java.nio.file.Path; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import java.util.function.Function; - -final class DefaultXSSRemediator implements XSSRemediator { - - private final List javaFixers; - - DefaultXSSRemediator() { - this.javaFixers = List.of(new PrintingMethodFixer(), new NakedVariableReturnFixer()); - } - - @Override - public CodemodFileScanningResult remediateJava( - final CompilationUnit cu, - final String path, - final DetectorRule detectorRule, - final List issuesForFile, - final Function getKey, - final Function getLine, - final Function getColumn) { - - List> fixGroups = createFixGroups(issuesForFile, getLine, getColumn); - - List unfixedFindings = new ArrayList<>(); - List changes = new ArrayList<>(); - - for (XSSFixGroup fixGroup : fixGroups) { - boolean foundResponsibleFixer = false; - for (XSSCodeShapeFixer javaFixer : javaFixers) { - XSSCodeShapeFixResult fixResult = - javaFixer.fixCodeShape( - cu, path, detectorRule, fixGroup.issues(), getKey, getLine, getColumn); - if (fixResult.isResponsibleFixer()) { - foundResponsibleFixer = true; - if (fixResult.isFixed()) { - List fixes = - fixGroup.issues().stream() - .map(fix -> new FixedFinding(getKey.apply(fix), detectorRule)) - .toList(); - changes.add( - CodemodChange.from( - fixResult.line(), List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER), fixes)); - } else { - List unfixed = - fixGroup.issues().stream() - .map( - fix -> - new UnfixedFinding( - getKey.apply(fix), - detectorRule, - path, - fixResult.line(), - fixResult.reasonNotFixed())) - .toList(); - unfixedFindings.addAll(unfixed); - } - // if this was the responsible fixer, no need to continue - break; - } - } - if (!foundResponsibleFixer) { - unfixedFindings.addAll( - fixGroup.issues().stream() - .map( - fix -> - new UnfixedFinding( - getKey.apply(fix), - detectorRule, - path, - getLine.apply(fix), - "Couldn't fix that shape of code")) - .toList()); - } - } - return CodemodFileScanningResult.from(changes, unfixedFindings); - } - - /** Split the issues into fix groups that all have the same location. */ - private List> createFixGroups( - final List issuesForFile, - final Function getLine, - final Function getColumn) { - List> fixGroups = new ArrayList<>(); - - Map> fixesPerLine = issuesForFile.stream().collect(groupingBy(getLine)); - - // now further separate the fixes by column if it's available - for (Map.Entry> entry : fixesPerLine.entrySet()) { - Map> fixesPerColumn = - entry.getValue().stream().collect(groupingBy(getColumn)); - for (List columnFixes : fixesPerColumn.values()) { - fixGroups.add(new XSSFixGroup<>(columnFixes)); - } - } - - return List.copyOf(fixGroups); - } - - @Override - public CodemodFileScanningResult remediateJSP( - final Path filePath, - final String path, - final DetectorRule detectorRule, - final List issuesForFile, - final Function getKey, - final Function getLine, - final Function getColumn) { - return CodemodFileScanningResult.none(); - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/NakedVariableReturnFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/NakedVariableReturnFixStrategy.java new file mode 100644 index 000000000..d80f12a48 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/NakedVariableReturnFixStrategy.java @@ -0,0 +1,36 @@ +package io.codemodder.remediation.xss; + +import static io.codemodder.javaparser.JavaParserTransformer.wrap; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.stmt.ReturnStmt; +import io.codemodder.DependencyGAV; +import io.codemodder.remediation.RemediationStrategy; +import io.codemodder.remediation.SuccessOrReason; +import java.util.List; +import java.util.Optional; +import org.jetbrains.annotations.VisibleForTesting; + +final class NakedVariableReturnFixStrategy implements RemediationStrategy { + @Override + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + var maybeReturn = Optional.of(node).map(n -> n instanceof ReturnStmt ? (ReturnStmt) n : null); + if (maybeReturn.isEmpty()) { + return SuccessOrReason.reason("Not a return statement"); + } + ReturnStmt nakedReturn = maybeReturn.get(); + wrap(nakedReturn.getExpression().get()) + .withStaticMethod("org.owasp.encoder.Encode", "forHtml", false); + return SuccessOrReason.success(List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER)); + } + + @VisibleForTesting + public static boolean match(final Node node) { + return Optional.of(node) + .map(n -> n instanceof ReturnStmt ? (ReturnStmt) n : null) + .filter(rs -> rs.getExpression().isPresent()) + .filter(rs -> rs.getExpression().get().isNameExpr()) + .isPresent(); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/NakedVariableReturnFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/NakedVariableReturnFixer.java deleted file mode 100644 index 5d66e89fd..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/NakedVariableReturnFixer.java +++ /dev/null @@ -1,54 +0,0 @@ -package io.codemodder.remediation.xss; - -import static io.codemodder.javaparser.JavaParserTransformer.wrap; - -import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.stmt.ReturnStmt; -import io.codemodder.codetf.DetectorRule; -import io.codemodder.remediation.RemediationMessages; -import java.util.List; -import java.util.function.Function; - -/** - * Fixes a return statement that returns a {@link String} expression. - * - * 
{@code
- * return foo; // should become return Encode.forHtml(foo)
- * }
- */ -final class NakedVariableReturnFixer implements XSSCodeShapeFixer { - - @Override - public XSSCodeShapeFixResult fixCodeShape( - final CompilationUnit cu, - final String path, - final DetectorRule detectorRule, - final List issues, - final Function getKey, - final Function getLine, - final Function getColumn) { - int line = getLine.apply(issues.get(0)); - Integer column = getColumn.apply(issues.get(0)); - - List matchingStatements = - cu.findAll(ReturnStmt.class).stream() - .filter(rs -> rs.getRange().isPresent()) - .filter(rs -> rs.getRange().get().begin.line == line) - .filter(rs -> column == null || rs.getRange().get().begin.column == column) - .filter(rs -> rs.getExpression().isPresent()) - .filter(rs -> rs.getExpression().get().isNameExpr()) - .toList(); - - if (matchingStatements.isEmpty()) { - return new XSSCodeShapeFixResult(false, false, null, line); - } else if (matchingStatements.size() > 1) { - return new XSSCodeShapeFixResult(true, false, RemediationMessages.multipleNodesFound, line); - } - - ReturnStmt nakedReturn = matchingStatements.get(0); - wrap(nakedReturn.getExpression().get()) - .withStaticMethod("org.owasp.encoder.Encode", "forHtml", false); - - return new XSSCodeShapeFixResult(true, true, null, line); - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/PrintingMethodFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/PrintingMethodFixStrategy.java new file mode 100644 index 000000000..65c798403 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/PrintingMethodFixStrategy.java @@ -0,0 +1,40 @@ +package io.codemodder.remediation.xss; + +import static io.codemodder.javaparser.JavaParserTransformer.wrap; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.MethodCallExpr; +import io.codemodder.DependencyGAV; +import io.codemodder.remediation.RemediationStrategy; +import io.codemodder.remediation.SuccessOrReason; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import org.jetbrains.annotations.VisibleForTesting; + +final class PrintingMethodFixStrategy implements RemediationStrategy { + + @Override + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + var maybeCall = + Optional.of(node).map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null); + if (maybeCall.isEmpty()) { + return SuccessOrReason.reason("Not a method call."); + } + MethodCallExpr call = maybeCall.get(); + wrap(call.getArgument(0)).withStaticMethod("org.owasp.encoder.Encode", "forHtml", false); + return SuccessOrReason.success(List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER)); + } + + private static final Set writingMethodNames = Set.of("print", "println", "write"); + + @VisibleForTesting + public static boolean match(final Node node) { + return Optional.of(node) + .map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null) + .filter(mce -> writingMethodNames.contains(mce.getNameAsString())) + .filter(mce -> mce.getArguments().size() == 1) + .isPresent(); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/PrintingMethodFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/PrintingMethodFixer.java deleted file mode 100644 index 7612c0f4d..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/PrintingMethodFixer.java +++ /dev/null @@ -1,76 +0,0 @@ -package io.codemodder.remediation.xss; - -import static io.codemodder.javaparser.JavaParserTransformer.wrap; - -import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.Node; -import com.github.javaparser.ast.expr.Expression; -import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.expr.NameExpr; -import com.github.javaparser.ast.nodeTypes.NodeWithType; -import io.codemodder.ast.ASTs; -import io.codemodder.codetf.DetectorRule; -import io.codemodder.remediation.RemediationMessages; -import java.util.List; -import java.util.Optional; -import java.util.Set; -import java.util.function.Function; - -/** - * Fixes a method invocation that prints {@link String} expression. - * - * 
{@code
- * out.println(foo); // should become return out.println(Encode.forHtml(foo));
- * }
- */ -final class PrintingMethodFixer implements XSSCodeShapeFixer { - - private static final Set writingMethodNames = Set.of("print", "println", "write"); - - @Override - public XSSCodeShapeFixResult fixCodeShape( - final CompilationUnit cu, - final String path, - final DetectorRule detectorRule, - final List issues, - final Function getKey, - final Function getLine, - final Function getColumn) { - int line = getLine.apply(issues.get(0)); - Integer column = getColumn.apply(issues.get(0)); - - List writingMethodCalls = - cu.findAll(MethodCallExpr.class).stream() - .filter(mce -> writingMethodNames.contains(mce.getNameAsString())) - .filter(mce -> mce.getArguments().size() == 1) - .filter(mce -> mce.getRange().isPresent()) - .filter(mce -> mce.getRange().get().begin.line == line) - .filter(mce -> column == null || mce.getRange().get().begin.column == column) - .filter( - mce -> { - Expression arg = mce.getArgument(0); - if (arg instanceof NameExpr nameExpr) { - Optional source = - ASTs.findNonCallableSimpleNameSource(nameExpr.getName()); - if (source.isPresent() - && source.get() instanceof NodeWithType typedNode) { - String typeAsString = typedNode.getTypeAsString(); - return "String".equals(typeAsString) - || "java.lang.String".equals(typeAsString); - } - } - return false; - }) - .toList(); - - if (writingMethodCalls.isEmpty()) { - return new XSSCodeShapeFixResult(false, false, null, line); - } else if (writingMethodCalls.size() > 1) { - return new XSSCodeShapeFixResult(true, false, RemediationMessages.multipleNodesFound, line); - } - - MethodCallExpr call = writingMethodCalls.get(0); - wrap(call.getArgument(0)).withStaticMethod("org.owasp.encoder.Encode", "forHtml", false); - return new XSSCodeShapeFixResult(true, true, null, line); - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSCodeShapeFixResult.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSCodeShapeFixResult.java deleted file mode 100644 index c37624043..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSCodeShapeFixResult.java +++ /dev/null @@ -1,20 +0,0 @@ -package io.codemodder.remediation.xss; - -/** The results of a fix attempt. */ -record XSSCodeShapeFixResult( - boolean isResponsibleFixer, boolean isFixed, String reasonNotFixed, int line) { - XSSCodeShapeFixResult { - if (!isResponsibleFixer && isFixed) { - throw new IllegalArgumentException("Must be responsible fixer if fixed"); - } - if (isFixed && reasonNotFixed != null) { - throw new IllegalArgumentException("Cannot be fixed and have a reason not fixed"); - } - if (isResponsibleFixer && !isFixed && reasonNotFixed == null) { - throw new IllegalArgumentException("Must have a reason not fixed if not fixed"); - } - if (isFixed && line < 0) { - throw new IllegalArgumentException("Line must be positive"); - } - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSCodeShapeFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSCodeShapeFixer.java deleted file mode 100644 index 96811b83e..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSCodeShapeFixer.java +++ /dev/null @@ -1,31 +0,0 @@ -package io.codemodder.remediation.xss; - -import com.github.javaparser.ast.CompilationUnit; -import io.codemodder.codetf.DetectorRule; -import java.util.List; -import java.util.function.Function; - -/** A function that can fix XSS that has a particular code shape. */ -interface XSSCodeShapeFixer { - - /** - * Find the shape of code at this location, and fix it if it's the expected shape. - * - * @param cu the compilation unit to fix - * @param path the path of the file being fixed - * @param detectorRule the detector rule that found the issue - * @param fixGroup the group of issues to fix - * @param getKey a function to get the key of an issue - * @param getLine a function to get the line of an issue - * @param getColumn a function to get the column of an issue - * @return the result of the fix - */ - XSSCodeShapeFixResult fixCodeShape( - CompilationUnit cu, - String path, - DetectorRule detectorRule, - List fixGroup, - Function getKey, - Function getLine, - Function getColumn); -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSFixGroup.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSFixGroup.java deleted file mode 100644 index 9053b8476..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSFixGroup.java +++ /dev/null @@ -1,14 +0,0 @@ -package io.codemodder.remediation.xss; - -import java.util.List; -import java.util.Objects; - -/** A collection of issues that are all fixable at the same line (and column, if available). */ -record XSSFixGroup(List issues) { - XSSFixGroup { - Objects.requireNonNull(issues, "issues"); - if (issues.isEmpty()) { - throw new IllegalArgumentException("Must have at least one issue"); - } - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSRemediator.java index dd365f8a4..7cbe26dc8 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xss/XSSRemediator.java @@ -3,32 +3,51 @@ import com.github.javaparser.ast.CompilationUnit; import io.codemodder.CodemodFileScanningResult; import io.codemodder.codetf.DetectorRule; -import java.nio.file.Path; -import java.util.List; +import io.codemodder.remediation.FixCandidateSearcher; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.SearcherStrategyRemediator; +import java.util.Collection; +import java.util.Optional; import java.util.function.Function; -/** Remediates header injection vulnerabilities. */ -public interface XSSRemediator { +public class XSSRemediator implements Remediator { - /** Remediate all header injection vulnerabilities in the given compilation unit. */ - CodemodFileScanningResult remediateJava( + private final SearcherStrategyRemediator searchStrategyRemediator; + + public XSSRemediator() { + this.searchStrategyRemediator = + new SearcherStrategyRemediator.Builder() + .withSearcherStrategyPair( + new FixCandidateSearcher.Builder() + .withMatcher(NakedVariableReturnFixStrategy::match) + .build(), + new NakedVariableReturnFixStrategy()) + .withSearcherStrategyPair( + new FixCandidateSearcher.Builder() + .withMatcher(PrintingMethodFixStrategy::match) + .build(), + new PrintingMethodFixStrategy()) + .build(); + } + + @Override + public CodemodFileScanningResult remediateAll( CompilationUnit cu, String path, DetectorRule detectorRule, - List issuesForFile, - Function getKey, - Function getLine, - Function getColumn); - - CodemodFileScanningResult remediateJSP( - Path filePath, - String relativePath, - DetectorRule detectorRule, - List issuesForFile, - Function getKey, - Function getLine, - Function getColumn); - - /** The default header injection remediation strategy. */ - XSSRemediator DEFAULT = new DefaultXSSRemediator(); + Collection findingsForPath, + Function findingIdExtractor, + Function findingStartLineExtractor, + Function> findingEndLineExtractor, + Function> findingColumnExtractor) { + return searchStrategyRemediator.remediateAll( + cu, + path, + detectorRule, + findingsForPath, + findingIdExtractor, + findingStartLineExtractor, + findingEndLineExtractor, + findingColumnExtractor); + } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediator.java deleted file mode 100644 index 3ac6edfb6..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediator.java +++ /dev/null @@ -1,108 +0,0 @@ -package io.codemodder.remediation.xxe; - -import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.expr.Expression; -import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.expr.NameExpr; -import com.github.javaparser.ast.stmt.Statement; -import io.codemodder.CodemodChange; -import io.codemodder.CodemodFileScanningResult; -import io.codemodder.codetf.DetectorRule; -import io.codemodder.codetf.FixedFinding; -import io.codemodder.codetf.UnfixedFinding; -import io.codemodder.remediation.LegacyFixCandidate; -import io.codemodder.remediation.LegacyFixCandidateSearchResults; -import io.codemodder.remediation.LegacyFixCandidateSearcher; -import java.util.ArrayList; -import java.util.List; -import java.util.Optional; -import java.util.function.Function; - -final class DefaultXXEIntermediateXMLStreamReaderRemediator - implements XXEIntermediateXMLStreamReaderRemediator { - - @Override - public CodemodFileScanningResult remediateAll( - final CompilationUnit cu, - final String path, - final DetectorRule detectorRule, - final List issuesForFile, - final Function getKey, - final Function getStartLine, - final Function getEndLine, - final Function getStartColumn) { - LegacyFixCandidateSearcher searcher = - new LegacyFixCandidateSearcher.Builder() - .withMethodName("createXMLStreamReader") - .withMatcher(mce -> mce.asNode().hasScope()) - .withMatcher(mce -> mce.getArguments().isNonEmpty()) - .build(); - - LegacyFixCandidateSearchResults results = - searcher.search( - cu, - path, - detectorRule, - issuesForFile, - getKey, - getStartLine, - getEndLine, - getStartColumn); - - List changes = new ArrayList<>(); - List unfixedFindings = new ArrayList<>(); - - for (LegacyFixCandidate legacyFixCandidate : results.fixCandidates()) { - List issues = legacyFixCandidate.issues(); - - // get the xmlstreamreader scope variable - MethodCallExpr createXMLStreamReaderCall = legacyFixCandidate.call().asMethodCall(); - Expression xmlStreamReaderScope = createXMLStreamReaderCall.getScope().get(); - - // make sure it's a variable - if (!xmlStreamReaderScope.isNameExpr()) { - issues.stream() - .map( - issue -> - new UnfixedFinding( - getKey.apply(issue), - detectorRule, - path, - getStartLine.apply(issue), - "Could not find the XMLStreamReader variable")) - .forEach(unfixedFindings::add); - continue; - } - // get the variable - NameExpr xmlStreamReaderVariable = xmlStreamReaderScope.asNameExpr(); - // get the JavaParser statement that contains the create call - Optional ancestorStatement = - createXMLStreamReaderCall.findAncestor(Statement.class); - if (ancestorStatement.isEmpty()) { - issues.stream() - .map( - issue -> - new UnfixedFinding( - getKey.apply(issue), - detectorRule, - path, - getStartLine.apply(issue), - "Could not find the statement containing the XMLStreamReader creation")) - .forEach(unfixedFindings::add); - continue; - } - Statement stmt = ancestorStatement.get(); - XMLFeatures.addXMLInputFactoryDisablingStatement(xmlStreamReaderVariable, stmt, true); - issues.stream() - .map( - issue -> - CodemodChange.from( - getStartLine.apply(issue), - new FixedFinding(getKey.apply(issue), detectorRule))) - .forEach(changes::add); - } - - unfixedFindings.addAll(results.unfixableFindings()); - return CodemodFileScanningResult.from(changes, unfixedFindings); - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DefaultXXERemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DefaultXXERemediator.java deleted file mode 100644 index 6181cabf2..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DefaultXXERemediator.java +++ /dev/null @@ -1,65 +0,0 @@ -package io.codemodder.remediation.xxe; - -import com.github.javaparser.ast.CompilationUnit; -import io.codemodder.CodemodChange; -import io.codemodder.CodemodFileScanningResult; -import io.codemodder.codetf.DetectorRule; -import io.codemodder.codetf.FixedFinding; -import io.codemodder.codetf.UnfixedFinding; -import java.util.ArrayList; -import java.util.List; -import java.util.function.Function; - -final class DefaultXXERemediator implements XXERemediator { - - private final List fixers; - - DefaultXXERemediator() { - this.fixers = - List.of( - new DocumentBuilderFactoryAndSAXParserAtCreationFixer(), - new DocumentBuilderFactoryAtNewDBFixer(), - new SAXParserAtNewSPFixer(), - new DocumentBuilderFactoryAtParseFixer(), - new TransformerFactoryAtCreationFixer(), - new XMLReaderAtParseFixer()); - } - - @Override - public CodemodFileScanningResult remediateAll( - final CompilationUnit cu, - final String path, - final DetectorRule detectorRule, - final List issuesForFile, - final Function getKey, - final Function getStartLine, - final Function getStartColumn) { - - List unfixedFindings = new ArrayList<>(); - List changes = new ArrayList<>(); - - for (T issue : issuesForFile) { - - String findingId = getKey.apply(issue); - int line = getStartLine.apply(issue); - Integer column = getStartColumn.apply(issue); - for (XXEFixer fixer : fixers) { - XXEFixAttempt fixAttempt = fixer.tryFix(line, column, cu); - if (!fixAttempt.isResponsibleFixer()) { - continue; - } - if (fixAttempt.isFixed()) { - CodemodChange change = - CodemodChange.from(line, new FixedFinding(findingId, detectorRule)); - changes.add(change); - } else { - UnfixedFinding unfixedFinding = - new UnfixedFinding(findingId, detectorRule, path, line, fixAttempt.reasonNotFixed()); - unfixedFindings.add(unfixedFinding); - } - } - } - - return CodemodFileScanningResult.from(changes, unfixedFindings); - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAndSAXParserAtCreationFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAndSAXParserAtCreationFixStrategy.java new file mode 100644 index 000000000..101f1135f --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAndSAXParserAtCreationFixStrategy.java @@ -0,0 +1,58 @@ +package io.codemodder.remediation.xxe; + +import static io.codemodder.javaparser.ASTExpectations.expect; +import static io.codemodder.remediation.xxe.XMLFixBuilder.addFeatureDisablingStatements; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.body.VariableDeclarator; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.ast.ASTs; +import io.codemodder.remediation.MatchAndFixStrategy; +import io.codemodder.remediation.SuccessOrReason; +import java.util.List; +import java.util.Optional; + +/** + * Fix strategy for XXE vulnerabilities anchored to the parser's newInstance() calls. Finds the + * parser's declaration and add statements disabling external entities and features. + */ +final class DocumentBuilderFactoryAndSAXParserAtCreationFixStrategy extends MatchAndFixStrategy { + + @Override + public SuccessOrReason fix(CompilationUnit cu, Node node) { + var maybeCall = + Optional.of(node).map(m -> m instanceof MethodCallExpr ? (MethodCallExpr) node : null); + if (maybeCall.isEmpty()) { + return SuccessOrReason.reason("Not a method call"); + } + MethodCallExpr newFactoryInstanceCall = maybeCall.get(); + Optional newFactoryVariableRef = + expect(newFactoryInstanceCall).toBeMethodCallExpression().initializingVariable().result(); + VariableDeclarator newFactoryVariable = newFactoryVariableRef.get(); + Optional variableDeclarationStmtRef = + newFactoryVariable.findAncestor(Statement.class); + + if (variableDeclarationStmtRef.isEmpty()) { + SuccessOrReason.reason("Not assigned as part of statement"); + } + + Statement statement = variableDeclarationStmtRef.get(); + return addFeatureDisablingStatements( + newFactoryVariable.getNameAsExpression(), statement, false); + } + + /** + * Matches [DocumentBuilderFactory, SAXParserFactory] y = (x.)newInstance(), where the node is the + * newInstance call. + * + * @param node + * @return + */ + public boolean match(final Node node) { + return ASTs.isInitializedToType( + node, "newInstance", List.of("DocumentBuilderFactory", "SAXParserFactory")) + .isPresent(); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAndSAXParserAtCreationFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAndSAXParserAtCreationFixer.java deleted file mode 100644 index 12f5c1a85..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAndSAXParserAtCreationFixer.java +++ /dev/null @@ -1,49 +0,0 @@ -package io.codemodder.remediation.xxe; - -import static io.codemodder.javaparser.ASTExpectations.expect; -import static io.codemodder.remediation.RemediationMessages.multipleNodesFound; -import static io.codemodder.remediation.RemediationMessages.noNodesAtThatLocation; -import static io.codemodder.remediation.xxe.XMLFeatures.addFeatureDisablingStatements; - -import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.body.VariableDeclarator; -import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.stmt.Statement; -import io.codemodder.ast.ASTs; -import java.util.List; -import java.util.Optional; - -/** - * Fix XXEs that are reported at the (DocumentBuilderFactory/SAXParserFactory).newInstance() call - * locations. - */ -final class DocumentBuilderFactoryAndSAXParserAtCreationFixer implements XXEFixer { - - @Override - public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUnit cu) { - List candidateMethods = - ASTs.findMethodCallsWhichAreAssignedToType( - cu, line, column, "newInstance", List.of("DocumentBuilderFactory", "SAXParserFactory")); - - if (candidateMethods.isEmpty()) { - return new XXEFixAttempt(false, false, noNodesAtThatLocation); - } else if (candidateMethods.size() > 1) { - return new XXEFixAttempt(false, false, multipleNodesFound); - } - - MethodCallExpr newFactoryInstanceCall = candidateMethods.get(0); - Optional newFactoryVariableRef = - expect(newFactoryInstanceCall).toBeMethodCallExpression().initializingVariable().result(); - VariableDeclarator newFactoryVariable = newFactoryVariableRef.get(); - Optional variableDeclarationStmtRef = - newFactoryVariable.findAncestor(Statement.class); - - if (variableDeclarationStmtRef.isEmpty()) { - return new XXEFixAttempt(true, false, "Not assigned as part of statement"); - } - - Statement statement = variableDeclarationStmtRef.get(); - return addFeatureDisablingStatements( - newFactoryVariable.getNameAsExpression(), statement, false); - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixStrategy.java new file mode 100644 index 000000000..8d1ba2c1d --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixStrategy.java @@ -0,0 +1,62 @@ +package io.codemodder.remediation.xxe; + +import static io.codemodder.remediation.xxe.XMLFixBuilder.addFeatureDisablingStatements; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.Expression; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.ast.ASTs; +import io.codemodder.remediation.MatchAndFixStrategy; +import io.codemodder.remediation.SuccessOrReason; +import java.util.List; +import java.util.Optional; + +/** + * Fix strategy for XXE vulnerabilities anchored to the parser's newDocumentBuilder() calls. Finds + * the parser's declaration and add statements disabling external entities and features. + */ +final class DocumentBuilderFactoryAtNewDBFixStrategy extends MatchAndFixStrategy { + + @Override + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + + var maybeCall = + Optional.of(node).map(m -> m instanceof MethodCallExpr ? (MethodCallExpr) node : null); + if (maybeCall.isEmpty()) { + return SuccessOrReason.reason("Not a method call"); + } + + MethodCallExpr newDocumentBuilderCall = maybeCall.get(); + + // the scope must be the DocumentBuilderFactory + Optional newDocumentBuilderCallScope = newDocumentBuilderCall.getScope(); + if (newDocumentBuilderCallScope.isEmpty()) { + SuccessOrReason.reason("No scope found"); + } + + Expression scope = newDocumentBuilderCallScope.get(); + if (!scope.isNameExpr()) { + return SuccessOrReason.reason("Scope is not a name"); + } + + Optional statement = scope.findAncestor(Statement.class); + if (statement.isEmpty()) { + return SuccessOrReason.reason("No statement found"); + } + return addFeatureDisablingStatements(scope.asNameExpr(), statement.get(), true); + } + + /** + * Matches DocumentBuilder y = (x.)newDocumentBuilder(), where the node is the newDocumentBuilder + * call. + * + * @param node + * @return + */ + public boolean match(final Node node) { + return ASTs.isInitializedToType(node, "newDocumentBuilder", List.of("DocumentBuilder")) + .isPresent(); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixer.java deleted file mode 100644 index f600e71b6..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixer.java +++ /dev/null @@ -1,49 +0,0 @@ -package io.codemodder.remediation.xxe; - -import static io.codemodder.remediation.RemediationMessages.multipleNodesFound; -import static io.codemodder.remediation.RemediationMessages.noNodesAtThatLocation; -import static io.codemodder.remediation.xxe.XMLFeatures.addFeatureDisablingStatements; - -import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.expr.Expression; -import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.stmt.Statement; -import io.codemodder.ast.ASTs; -import java.util.List; -import java.util.Optional; - -/** Fix XXEs that are reported at the DocumentBuilderFactory.newDocumentBuilder() call locations. */ -final class DocumentBuilderFactoryAtNewDBFixer implements XXEFixer { - - @Override - public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUnit cu) { - List candidateMethods = - ASTs.findMethodCallsWhichAreAssignedToType( - cu, line, column, "newDocumentBuilder", List.of("DocumentBuilder")); - - if (candidateMethods.isEmpty()) { - return new XXEFixAttempt(false, false, noNodesAtThatLocation); - } else if (candidateMethods.size() > 1) { - return new XXEFixAttempt(false, false, multipleNodesFound); - } - - MethodCallExpr newDocumentBuilderCall = candidateMethods.get(0); - - // the scope must be the DocumentBuilderFactory - Optional newDocumentBuilderCallScope = newDocumentBuilderCall.getScope(); - if (newDocumentBuilderCallScope.isEmpty()) { - return new XXEFixAttempt(true, false, "No scope found"); - } - - Expression scope = newDocumentBuilderCallScope.get(); - if (!scope.isNameExpr()) { - return new XXEFixAttempt(true, false, "Scope is not a name"); - } - - Optional statement = scope.findAncestor(Statement.class); - if (statement.isEmpty()) { - return new XXEFixAttempt(true, false, "No statement found"); - } - return addFeatureDisablingStatements(scope.asNameExpr(), statement.get(), true); - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixStrategy.java similarity index 50% rename from framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixer.java rename to framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixStrategy.java index d7daadae2..2313d1370 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixer.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixStrategy.java @@ -1,9 +1,5 @@ package io.codemodder.remediation.xxe; -import static io.codemodder.remediation.RemediationMessages.multipleNodesFound; -import static io.codemodder.remediation.RemediationMessages.noNodesAtThatLocation; - -import com.github.javaparser.Position; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.ast.Node; import com.github.javaparser.ast.body.MethodDeclaration; @@ -15,38 +11,27 @@ import com.github.javaparser.ast.nodeTypes.NodeWithType; import com.github.javaparser.ast.stmt.Statement; import io.codemodder.ast.ASTs; -import java.util.List; +import io.codemodder.remediation.MatchAndFixStrategy; +import io.codemodder.remediation.SuccessOrReason; import java.util.Optional; import java.util.Set; -/** Fix XXEs that are reported at the DocumentBuilder.parse() call locations. */ -final class DocumentBuilderFactoryAtParseFixer implements XXEFixer { +/** + * Fix strategy for XXE vulnerabilities anchored to the parser's parse() calls. Finds the parser's + * declaration and add statements disabling external entities and features. + */ +final class DocumentBuilderFactoryAtParseFixStrategy extends MatchAndFixStrategy { @Override - public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUnit cu) { - List candidateMethods = - cu.findAll(MethodCallExpr.class).stream() - .filter(m -> "parse".equals(m.getNameAsString())) - .filter(m -> m.getScope().isPresent()) - .filter(m -> m.getScope().get().isNameExpr()) - .filter(m -> m.getRange().isPresent() && m.getRange().get().begin.line == line) - .toList(); - - if (candidateMethods.size() > 1 && column != null) { - candidateMethods = - candidateMethods.stream() - .filter(m -> m.getRange().get().contains(new Position(line, column))) - .toList(); - } + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { - if (candidateMethods.isEmpty()) { - return new XXEFixAttempt(false, false, noNodesAtThatLocation); - } else if (candidateMethods.size() > 1) { - return new XXEFixAttempt(false, false, multipleNodesFound); + var maybeCall = + Optional.of(node).map(m -> m instanceof MethodCallExpr ? (MethodCallExpr) node : null); + if (maybeCall.isEmpty()) { + return SuccessOrReason.reason("Not a method call"); } - // find the variable that we're calling parse() on - MethodCallExpr parseCall = candidateMethods.get(0); + MethodCallExpr parseCall = maybeCall.get(); // we know from the filter that it's scope is a name expression NameExpr documentBuilder = parseCall.getScope().get().asNameExpr(); @@ -54,61 +39,68 @@ public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUni // so, we need to see if we can see the DocumentBuilderFactory from whence this came Optional methodBody = ASTs.findMethodBodyFrom(documentBuilder); if (methodBody.isEmpty()) { - return new XXEFixAttempt(false, false, "No method body found for the call"); + return SuccessOrReason.reason("No method body found for the call"); } Optional documentBuilderAssignment = ASTs.findNonCallableSimpleNameSource(documentBuilder.getName()); if (documentBuilderAssignment.isEmpty()) { - return new XXEFixAttempt(false, false, "No assignment found for the DocumentBuilder"); + return SuccessOrReason.reason("No assignment found for the DocumentBuilder"); } Node parserAssignmentNode = documentBuilderAssignment.get(); if (!(parserAssignmentNode instanceof NodeWithType)) { - return new XXEFixAttempt(false, false, "Unknown DocumentBuilder assignment"); + return SuccessOrReason.reason("Unknown DocumentBuilder assignment"); } String parserType = ((NodeWithType) parserAssignmentNode).getTypeAsString(); if (!Set.of("DocumentBuilder", "javax.xml.parsers.DocumentBuilder").contains(parserType)) { - return new XXEFixAttempt(false, false, "Parsing method is not a DocumentBuilder"); + return SuccessOrReason.reason("Parsing method is not a DocumentBuilder"); } if (parserAssignmentNode instanceof VariableDeclarator dbVar) { Optional initializer = dbVar.getInitializer(); if (initializer.isEmpty()) { - return new XXEFixAttempt( - false, false, "DocumentBuilder was not initialized in an expected way"); + return SuccessOrReason.reason("DocumentBuilder was not initialized in an expected way"); } else if (!(initializer.get() instanceof MethodCallExpr)) { - return new XXEFixAttempt( - false, false, "DocumentBuilder was not initialized with a factory call"); + return SuccessOrReason.reason("DocumentBuilder was not initialized with a factory call"); } MethodCallExpr potentialFactoryCall = (MethodCallExpr) initializer.get(); if (!"newDocumentBuilder".equals(potentialFactoryCall.getNameAsString())) { - return new XXEFixAttempt( - false, false, "DocumentBuilder was initialized with newDocumentBuilder"); + return SuccessOrReason.reason("DocumentBuilder was initialized with newDocumentBuilder"); } if (potentialFactoryCall.getScope().isEmpty()) { - return new XXEFixAttempt( - true, false, "DocumentBuilder was initialized with a factory call without a scope"); + return SuccessOrReason.reason( + "DocumentBuilder was initialized with a factory call without a scope"); } if (!(potentialFactoryCall.getScope().get() instanceof NameExpr)) { - return new XXEFixAttempt( - false, - false, + return SuccessOrReason.reason( "DocumentBuilder was initialized with a factory call with a non-name scope"); } NameExpr factoryNameExpr = (NameExpr) potentialFactoryCall.getScope().get(); Optional newDocumentBuilderStatement = ASTs.findParentStatementFrom(dbVar); if (newDocumentBuilderStatement.isEmpty()) { - return new XXEFixAttempt( - false, - false, + return SuccessOrReason.reason( "DocumentBuilder was initialized with a factory call without a statement"); } - return XMLFeatures.addFeatureDisablingStatements( + return XMLFixBuilder.addFeatureDisablingStatements( factoryNameExpr, newDocumentBuilderStatement.get(), true); } else if (parserAssignmentNode instanceof Parameter) { - return new XXEFixAttempt(true, false, "DocumentBuilder came from outside the method scope"); + return SuccessOrReason.reason("DocumentBuilder came from outside the method scope"); } - return new XXEFixAttempt(true, false, "DocumentBuilder was not initialized in an expected way"); + return SuccessOrReason.reason("DocumentBuilder was not initialized in an expected way"); + } + + /** + * Matches against x.parse() calls + * + * @param node + * @return + */ + public boolean match(final Node node) { + return Optional.of(node) + .map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null) + .filter(m -> "parse".equals(m.getNameAsString())) + .filter(m -> m.getScope().filter(Expression::isNameExpr).isPresent()) + .isPresent(); } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixStrategy.java new file mode 100644 index 000000000..8be57139a --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixStrategy.java @@ -0,0 +1,60 @@ +package io.codemodder.remediation.xxe; + +import static io.codemodder.remediation.xxe.XMLFixBuilder.addFeatureDisablingStatements; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.Expression; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.ast.ASTs; +import io.codemodder.remediation.MatchAndFixStrategy; +import io.codemodder.remediation.SuccessOrReason; +import java.util.List; +import java.util.Optional; + +/** + * Fix strategy for XXE vulnerabilities anchored to the SAXParser newSaxParser() calls. Finds the + * parser's declaration and add statements disabling external entities and features. + */ +final class SAXParserAtNewSPFixStrategy extends MatchAndFixStrategy { + + /** + * Matches SaxParser y = (x.)newSaxParser(), where the node is the newSaxParser call. + * + * @param node + * @return + */ + public boolean match(final Node node) { + return ASTs.isInitializedToType(node, "newSAXParser", List.of("SAXParser")).isPresent(); + } + + @Override + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + var maybeCall = + Optional.of(node).map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null); + + if (maybeCall.isEmpty()) { + return SuccessOrReason.reason("Not a method call."); + } + + MethodCallExpr newSaxParserCall = maybeCall.get(); + + // the scope must be the SAXParserFactory + Optional newSaxParserCallScope = newSaxParserCall.getScope(); + if (newSaxParserCallScope.isEmpty()) { + SuccessOrReason.reason("No scope found"); + } + + Expression scope = newSaxParserCallScope.get(); + if (!scope.isNameExpr()) { + SuccessOrReason.reason("Scope is not a name"); + } + + Optional statement = scope.findAncestor(Statement.class); + if (statement.isEmpty()) { + return SuccessOrReason.reason("No statement found"); + } + return addFeatureDisablingStatements(scope.asNameExpr(), statement.get(), true); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixer.java deleted file mode 100644 index c988b0159..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixer.java +++ /dev/null @@ -1,49 +0,0 @@ -package io.codemodder.remediation.xxe; - -import static io.codemodder.remediation.RemediationMessages.multipleNodesFound; -import static io.codemodder.remediation.RemediationMessages.noNodesAtThatLocation; -import static io.codemodder.remediation.xxe.XMLFeatures.addFeatureDisablingStatements; - -import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.expr.Expression; -import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.stmt.Statement; -import io.codemodder.ast.ASTs; -import java.util.List; -import java.util.Optional; - -/** Fix XXEs that are reported at the SAXParserFactory.newSAXParser() call locations. */ -final class SAXParserAtNewSPFixer implements XXEFixer { - - @Override - public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUnit cu) { - List candidateMethods = - ASTs.findMethodCallsWhichAreAssignedToType( - cu, line, column, "newSAXParser", List.of("SAXParser")); - - if (candidateMethods.isEmpty()) { - return new XXEFixAttempt(false, false, noNodesAtThatLocation); - } else if (candidateMethods.size() > 1) { - return new XXEFixAttempt(false, false, multipleNodesFound); - } - - MethodCallExpr newSaxParserCall = candidateMethods.get(0); - - // the scope must be the SAXParserFactory - Optional newSaxParserCallScope = newSaxParserCall.getScope(); - if (newSaxParserCallScope.isEmpty()) { - return new XXEFixAttempt(true, false, "No scope found"); - } - - Expression scope = newSaxParserCallScope.get(); - if (!scope.isNameExpr()) { - return new XXEFixAttempt(true, false, "Scope is not a name"); - } - - Optional statement = scope.findAncestor(Statement.class); - if (statement.isEmpty()) { - return new XXEFixAttempt(true, false, "No statement found"); - } - return addFeatureDisablingStatements(scope.asNameExpr(), statement.get(), true); - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixStrategy.java similarity index 63% rename from framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixer.java rename to framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixStrategy.java index f9c5bc59c..b6bd73c3d 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixer.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixStrategy.java @@ -2,10 +2,9 @@ import static io.codemodder.ast.ASTTransforms.addImportIfMissing; import static io.codemodder.javaparser.ASTExpectations.expect; -import static io.codemodder.remediation.RemediationMessages.*; -import static io.codemodder.remediation.RemediationMessages.multipleNodesFound; import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; import com.github.javaparser.ast.NodeList; import com.github.javaparser.ast.body.VariableDeclarator; import com.github.javaparser.ast.expr.FieldAccessExpr; @@ -16,23 +15,26 @@ import com.github.javaparser.ast.stmt.ExpressionStmt; import com.github.javaparser.ast.stmt.Statement; import io.codemodder.ast.ASTs; +import io.codemodder.remediation.MatchAndFixStrategy; +import io.codemodder.remediation.SuccessOrReason; import java.util.List; import java.util.Optional; -/** Fix XXEs reported at the TransformerFactory.newInstance() call locations. */ -final class TransformerFactoryAtCreationFixer implements XXEFixer { +/** + * Fix strategy for XXE vulnerabilities anchored to the TransformerParser newInstance() calls. Finds + * the parser's declaration and add statements disabling external entities and features. + */ +final class TransformerFactoryAtCreationFixStrategy extends MatchAndFixStrategy { @Override - public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUnit cu) { - List candidateMethods = - ASTs.findMethodCallsWhichAreAssignedToType( - cu, line, column, "newInstance", List.of("TransformerFactory")); - if (candidateMethods.isEmpty()) { - return new XXEFixAttempt(false, false, noNodesAtThatLocation); - } else if (candidateMethods.size() > 1) { - return new XXEFixAttempt(false, false, multipleNodesFound); + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + var maybeCall = + Optional.of(node).map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null); + if (maybeCall.isEmpty()) { + return SuccessOrReason.reason("Not a method call."); } - MethodCallExpr newFactoryInstanceCall = candidateMethods.get(0); + + MethodCallExpr newFactoryInstanceCall = maybeCall.get(); Optional newFactoryVariableRef = expect(newFactoryInstanceCall).toBeMethodCallExpression().initializingVariable().result(); VariableDeclarator newFactoryVariable = newFactoryVariableRef.get(); @@ -40,13 +42,13 @@ public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUni newFactoryVariable.findAncestor(Statement.class); if (variableDeclarationStmtRef.isEmpty()) { - return new XXEFixAttempt(true, false, "Not assigned as part of statement"); + return SuccessOrReason.reason("Not assigned as part of statement"); } Statement statement = variableDeclarationStmtRef.get(); Optional block = ASTs.findBlockStatementFrom(statement); if (block.isEmpty()) { - return new XXEFixAttempt(true, false, "No block statement found for newFactory() call"); + return SuccessOrReason.reason("No block statement found for newFactory() call"); } BlockStmt blockStmt = block.get(); @@ -64,6 +66,16 @@ public XXEFixAttempt tryFix(final int line, final Integer column, CompilationUni NodeList existingStatements = blockStmt.getStatements(); int index = existingStatements.indexOf(statement); existingStatements.add(index + 1, fixStatement); - return new XXEFixAttempt(true, true, null); + return SuccessOrReason.success(); + } + + /** + * Matches against TransformerFactory.newInstance() calls + * + * @param node + * @return + */ + public boolean match(final Node node) { + return ASTs.isInitializedToType(node, "newInstance", List.of("TransformerFactory")).isPresent(); } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLFeatures.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLFixBuilder.java similarity index 87% rename from framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLFeatures.java rename to framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLFixBuilder.java index c5898d6a1..4b30fef5a 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLFeatures.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLFixBuilder.java @@ -10,13 +10,12 @@ import com.github.javaparser.ast.stmt.ExpressionStmt; import com.github.javaparser.ast.stmt.Statement; import io.codemodder.ast.ASTs; +import io.codemodder.remediation.SuccessOrReason; import java.util.List; import java.util.Optional; -/** Holds shared APIs for working with shared XML feature setting. */ -final class XMLFeatures { - - private XMLFeatures() {} +/** Holds shared APIs for building nodes shared between XML fixers */ +public class XMLFixBuilder { /** Creates a call that disables parameter entities. */ static MethodCallExpr createParameterEntityDisabledCall(final NameExpr nameExpr) { @@ -42,7 +41,7 @@ static MethodCallExpr createGeneralEntityDisablingCall(final NameExpr nameExpr) * Creates a call for {@link javax.xml.stream.XMLInputFactory#setProperty(java.lang.String, * java.lang.Object)} for disabling. */ - static XXEFixAttempt addXMLInputFactoryDisablingStatement( + static SuccessOrReason addXMLInputFactoryDisablingStatement( final NameExpr variable, final Statement statementToInjectAround, final boolean before) { MethodCallExpr call = new MethodCallExpr( @@ -54,7 +53,7 @@ static XXEFixAttempt addXMLInputFactoryDisablingStatement( Optional block = ASTs.findBlockStatementFrom(statementToInjectAround); if (block.isEmpty()) { - return new XXEFixAttempt(true, false, "No block statement found for newFactory() call"); + return SuccessOrReason.reason("No block statement found for newFactory() call"); } BlockStmt blockStmt = block.get(); @@ -66,14 +65,14 @@ static XXEFixAttempt addXMLInputFactoryDisablingStatement( Statement fixStatement = new ExpressionStmt(call); existingStatements.add(index, fixStatement); - return new XXEFixAttempt(true, true, null); + return SuccessOrReason.success(); } - static XXEFixAttempt addFeatureDisablingStatements( + static SuccessOrReason addFeatureDisablingStatements( final NameExpr variable, final Statement statementToInjectAround, final boolean before) { Optional block = ASTs.findBlockStatementFrom(statementToInjectAround); if (block.isEmpty()) { - return new XXEFixAttempt(true, false, "No block statement found for newFactory() call"); + return SuccessOrReason.reason("No block statement found for newFactory() call"); } BlockStmt blockStmt = block.get(); @@ -106,6 +105,6 @@ static XXEFixAttempt addFeatureDisablingStatements( .map(Optional::get) .forEach(Node::remove); - return new XXEFixAttempt(true, true, null); + return SuccessOrReason.success(); } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixStrategy.java new file mode 100644 index 000000000..fe450be6e --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixStrategy.java @@ -0,0 +1,75 @@ +package io.codemodder.remediation.xxe; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.Expression; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.expr.NameExpr; +import com.github.javaparser.ast.nodeTypes.NodeWithType; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.ast.ASTs; +import io.codemodder.remediation.MatchAndFixStrategy; +import io.codemodder.remediation.SuccessOrReason; +import java.util.Optional; +import java.util.Set; + +/** + * Fix strategy for XXE vulnerabilities anchored to the XMLReader parse() calls. Finds the parser's + * declaration and add statements disabling external entities and features. + */ +final class XMLReaderAtParseFixStrategy extends MatchAndFixStrategy { + + @Override + public SuccessOrReason fix(CompilationUnit cu, Node node) { + var maybeCall = + Optional.of(node).map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null); + if (maybeCall.isEmpty()) { + return SuccessOrReason.reason("Not a method call."); + } + MethodCallExpr parseCall = maybeCall.get(); + + Optional parserRef = parseCall.getScope(); + if (parserRef.isEmpty()) { + return SuccessOrReason.reason("No scope found for parse() call"); + } else if (!parserRef.get().isNameExpr()) { + return SuccessOrReason.reason("Scope is not a name expression"); + } + NameExpr parser = parserRef.get().asNameExpr(); + + Optional parseStatement = parseCall.findAncestor(Statement.class); + if (parseStatement.isEmpty()) { + return SuccessOrReason.reason("No statement found for parse() call"); + } + return XMLFixBuilder.addFeatureDisablingStatements( + parser.asNameExpr(), parseStatement.get(), true); + } + + /** + * Matches against XMLReader.parse calls + * + * @param node + * @return + */ + public boolean match(final Node node) { + return Optional.of(node) + .map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null) + .filter(m -> "parse".equals(m.getNameAsString())) + .filter(m -> m.getScope().isPresent()) + .filter(m -> m.getScope().get().isNameExpr()) + .filter( + m -> { + Optional sourceRef = + ASTs.findNonCallableSimpleNameSource(m.getScope().get().asNameExpr().getName()); + if (sourceRef.isEmpty()) { + return false; + } + Node source = sourceRef.get(); + if (source instanceof NodeWithType) { + return Set.of("XMLReader", "org.xml.sax.XMLReader") + .contains(((NodeWithType) source).getTypeAsString()); + } + return false; + }) + .isPresent(); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixer.java deleted file mode 100644 index bfc5b69cc..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixer.java +++ /dev/null @@ -1,77 +0,0 @@ -package io.codemodder.remediation.xxe; - -import static io.codemodder.remediation.RemediationMessages.multipleNodesFound; -import static io.codemodder.remediation.RemediationMessages.noNodesAtThatLocation; - -import com.github.javaparser.Position; -import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.Node; -import com.github.javaparser.ast.expr.Expression; -import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.expr.NameExpr; -import com.github.javaparser.ast.nodeTypes.NodeWithType; -import com.github.javaparser.ast.stmt.Statement; -import io.codemodder.ast.ASTs; -import java.util.List; -import java.util.Optional; -import java.util.Set; - -/** Fixes XXEs that are reported at the XMLReader#parse() call. */ -final class XMLReaderAtParseFixer implements XXEFixer { - - @Override - public XXEFixAttempt tryFix(final int line, final Integer column, final CompilationUnit cu) { - List candidateMethods = - cu.findAll(MethodCallExpr.class).stream() - .filter(m -> "parse".equals(m.getNameAsString())) - .filter(m -> m.getScope().isPresent()) - .filter(m -> m.getScope().get().isNameExpr()) - .filter( - m -> { - Optional sourceRef = - ASTs.findNonCallableSimpleNameSource( - m.getScope().get().asNameExpr().getName()); - if (sourceRef.isEmpty()) { - return false; - } - Node source = sourceRef.get(); - if (source instanceof NodeWithType) { - return Set.of("XMLReader", "org.xml.sax.XMLReader") - .contains(((NodeWithType) source).getTypeAsString()); - } - return false; - }) - .filter(m -> m.getRange().isPresent()) - .filter(m -> m.getRange().get().begin.line == line) - .toList(); - - if (column != null) { - Position reportedPosition = new Position(line, column); - candidateMethods = - candidateMethods.stream() - .filter(m -> m.getRange().get().contains(reportedPosition)) - .toList(); - } - - if (candidateMethods.isEmpty()) { - return new XXEFixAttempt(false, false, noNodesAtThatLocation); - } else if (candidateMethods.size() > 1) { - return new XXEFixAttempt(false, false, multipleNodesFound); - } - - MethodCallExpr parseCall = candidateMethods.get(0); - Optional parserRef = parseCall.getScope(); - if (parserRef.isEmpty()) { - return new XXEFixAttempt(false, false, "No scope found for parse() call"); - } else if (!parserRef.get().isNameExpr()) { - return new XXEFixAttempt(false, false, "Scope is not a name expression"); - } - NameExpr parser = parserRef.get().asNameExpr(); - Optional parseStatement = parseCall.findAncestor(Statement.class); - if (parseStatement.isEmpty()) { - return new XXEFixAttempt(true, false, "No statement found for parse() call"); - } - return XMLFeatures.addFeatureDisablingStatements( - parser.asNameExpr(), parseStatement.get(), true); - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixAttempt.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixAttempt.java deleted file mode 100644 index be4b2c75a..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixAttempt.java +++ /dev/null @@ -1,14 +0,0 @@ -package io.codemodder.remediation.xxe; - -/** Represents an attempt to fix an XXE vulnerability. */ -record XXEFixAttempt(boolean isResponsibleFixer, boolean isFixed, String reasonNotFixed) { - - XXEFixAttempt { - if (!isResponsibleFixer && isFixed) { - throw new IllegalStateException("Cannot be fixed by a non-responsible fixer"); - } - if (!isFixed && reasonNotFixed == null) { - throw new IllegalStateException("Reason must be provided if not fixed"); - } - } -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixer.java deleted file mode 100644 index 59a6029d3..000000000 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEFixer.java +++ /dev/null @@ -1,10 +0,0 @@ -package io.codemodder.remediation.xxe; - -import com.github.javaparser.ast.CompilationUnit; - -/** Interface for fixing XXEs. */ -interface XXEFixer { - - /** A provider (for a given XML API) attempts to fix the given issue. */ - XXEFixAttempt tryFix(int line, Integer column, CompilationUnit cu); -} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderFixStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderFixStrategy.java new file mode 100644 index 000000000..e0691d5b9 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderFixStrategy.java @@ -0,0 +1,47 @@ +package io.codemodder.remediation.xxe; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.expr.Expression; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.expr.NameExpr; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.remediation.RemediationStrategy; +import io.codemodder.remediation.SuccessOrReason; +import java.util.Optional; + +/** + * Fix strategy for XXE vulnerabilities anchored to the XMLStreamReader calls. Finds the parser's + * declaration and add statements disabling external entities and features. + */ +final class XXEIntermediateXMLStreamReaderFixStrategy implements RemediationStrategy { + + @Override + public SuccessOrReason fix(final CompilationUnit cu, final Node node) { + + var maybeCall = + Optional.of(node).map(m -> m instanceof MethodCallExpr ? (MethodCallExpr) node : null); + if (maybeCall.isEmpty()) { + return SuccessOrReason.reason("Not a method call"); + } + // get the xmlstreamreader scope variable + MethodCallExpr createXMLStreamReaderCall = maybeCall.get(); + Expression xmlStreamReaderScope = createXMLStreamReaderCall.getScope().get(); + + // make sure it's a variable + if (!xmlStreamReaderScope.isNameExpr()) { + return SuccessOrReason.reason("Could not find the XMLStreamReader variable"); + } + // get the variable + NameExpr xmlStreamReaderVariable = xmlStreamReaderScope.asNameExpr(); + // get the JavaParser statement that contains the create call + Optional ancestorStatement = createXMLStreamReaderCall.findAncestor(Statement.class); + + if (ancestorStatement.isEmpty()) { + return SuccessOrReason.reason( + "Could not find the statement containing the " + "XMLStreamReader " + "creation"); + } + Statement stmt = ancestorStatement.get(); + return XMLFixBuilder.addXMLInputFactoryDisablingStatement(xmlStreamReaderVariable, stmt, true); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderRemediator.java index 68682e80a..2555df025 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderRemediator.java @@ -1,29 +1,55 @@ package io.codemodder.remediation.xxe; import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.MethodCallExpr; import io.codemodder.CodemodFileScanningResult; import io.codemodder.codetf.DetectorRule; -import java.util.List; +import io.codemodder.remediation.FixCandidateSearcher; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.SearcherStrategyRemediator; +import java.util.Collection; +import java.util.Optional; import java.util.function.Function; -/** - * Strategy for remediating XXE vulnerabilities at an intermediate step in {@link - * javax.xml.stream.XMLInputFactory#createXMLStreamReader}. - */ -public interface XXEIntermediateXMLStreamReaderRemediator { +public class XXEIntermediateXMLStreamReaderRemediator implements Remediator { - /** A default implementation for callers. */ - XXEIntermediateXMLStreamReaderRemediator DEFAULT = - new DefaultXXEIntermediateXMLStreamReaderRemediator(); + private final SearcherStrategyRemediator searchStrategyRemediator; - /** Remediate all XXE vulnerabilities in the given compilation unit. */ - CodemodFileScanningResult remediateAll( + public XXEIntermediateXMLStreamReaderRemediator() { + this.searchStrategyRemediator = + new SearcherStrategyRemediator.Builder() + .withSearcherStrategyPair( + new FixCandidateSearcher.Builder() + .withMatcher( + node -> + Optional.of(node) + .map(n -> n instanceof MethodCallExpr mce ? mce : null) + .filter(mce -> mce.hasScope()) + .filter(mce -> mce.getArguments().isNonEmpty()) + .isPresent()) + .build(), + new XXEIntermediateXMLStreamReaderFixStrategy()) + .build(); + } + + @Override + public CodemodFileScanningResult remediateAll( CompilationUnit cu, String path, DetectorRule detectorRule, - List issuesForFile, - Function getKey, - Function getEndLine, - Function getStartColumn, - Function getEndColumn); + Collection findingsForPath, + Function findingIdExtractor, + Function findingStartLineExtractor, + Function> findingEndLineExtractor, + Function> findingColumnExtractor) { + return searchStrategyRemediator.remediateAll( + cu, + path, + detectorRule, + findingsForPath, + findingIdExtractor, + findingStartLineExtractor, + findingEndLineExtractor, + findingColumnExtractor); + } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXERemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXERemediator.java index 8f4fdfb6c..fac108846 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXERemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXERemediator.java @@ -3,22 +3,53 @@ import com.github.javaparser.ast.CompilationUnit; import io.codemodder.CodemodFileScanningResult; import io.codemodder.codetf.DetectorRule; -import java.util.List; +import io.codemodder.remediation.*; +import java.util.Collection; +import java.util.Optional; import java.util.function.Function; -/** Strategy for remediating XXE vulnerabilities at the sink for multiple parser APIs. */ -public interface XXERemediator { +public class XXERemediator implements Remediator { - /** A default implementation for callers. */ - XXERemediator DEFAULT = new DefaultXXERemediator(); + private final SearcherStrategyRemediator searchStrategyRemediator; - /** Remediate all XXE vulnerabilities in the given compilation unit. */ - CodemodFileScanningResult remediateAll( + public XXERemediator() { + this(NodePositionMatcher.DEFAULT); + } + + public XXERemediator(final NodePositionMatcher matcher) { + this.searchStrategyRemediator = + new SearcherStrategyRemediator.Builder() + .withMatchAndFixStrategyAndNodeMatcher( + new DocumentBuilderFactoryAndSAXParserAtCreationFixStrategy(), matcher) + .withMatchAndFixStrategyAndNodeMatcher( + new DocumentBuilderFactoryAtNewDBFixStrategy(), matcher) + .withMatchAndFixStrategyAndNodeMatcher(new SAXParserAtNewSPFixStrategy(), matcher) + .withMatchAndFixStrategyAndNodeMatcher( + new DocumentBuilderFactoryAtParseFixStrategy(), matcher) + .withMatchAndFixStrategyAndNodeMatcher( + new TransformerFactoryAtCreationFixStrategy(), matcher) + .withMatchAndFixStrategyAndNodeMatcher(new XMLReaderAtParseFixStrategy(), matcher) + .build(); + } + + @Override + public CodemodFileScanningResult remediateAll( CompilationUnit cu, String path, DetectorRule detectorRule, - List issuesForFile, - Function getKey, - Function getStartLine, - Function getColumn); + Collection findingsForPath, + Function findingIdExtractor, + Function findingStartLineExtractor, + Function> findingEndLineExtractor, + Function> findingColumnExtractor) { + return searchStrategyRemediator.remediateAll( + cu, + path, + detectorRule, + findingsForPath, + findingIdExtractor, + findingStartLineExtractor, + findingEndLineExtractor, + findingColumnExtractor); + } } diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/DefaultFixCandidateSearcherTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/DefaultFixCandidateSearcherTest.java new file mode 100644 index 000000000..a4df0f66b --- /dev/null +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/DefaultFixCandidateSearcherTest.java @@ -0,0 +1,169 @@ +package io.codemodder.remediation; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.github.javaparser.StaticJavaParser; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.expr.MethodCallExpr; +import io.codemodder.codetf.DetectorRule; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +final class DefaultFixCandidateSearcherTest { + + private DefaultFixCandidateSearcher searcher; + private DetectorRule rule1; + + private record Issue(String key, int line) {} + + @BeforeEach + void setup() { + searcher = + new DefaultFixCandidateSearcher<>( + List.of( + n -> + Optional.of(n) + .map(m -> m instanceof MethodCallExpr mce ? mce : null) + .filter(mce -> "println".equals(mce.getNameAsString())) + .isPresent())); + rule1 = new DetectorRule("key1", "rule 1", null); + } + + @Test + void it_groups_overlapping_issues_together() { + + String javaCode = + """ + class A { + void has_issues_1_and_2() { + System.out.println("Foo"); // multiple issues point to this line + } + + void has_issue_3() { + System.out.println("Bar"); + } + } + """; + + Issue issue1 = new Issue("key1", 3); + Issue issue2 = new Issue("key2", 3); + Issue issue3 = new Issue("key3", 7); + Issue issue4 = new Issue("key4", 505); // doesnt exist + + List allIssues = new ArrayList<>(List.of(issue1, issue2, issue3, issue4)); + + CompilationUnit cu = StaticJavaParser.parse(javaCode); + + FixCandidateSearchResults fixCandidateSearchResults = + searcher.search( + cu, + "path", + rule1, + allIssues, + Issue::key, + Issue::line, + i -> Optional.empty(), + i -> Optional.empty()); + List> legacyFixCandidates = fixCandidateSearchResults.fixCandidates(); + assertThat(legacyFixCandidates).hasSize(2); + + // There is no guarantee about issue ordering + // Find if an issue with argument Foo exists + Optional> maybeFooFixCandidateIssue = + legacyFixCandidates.stream() + .filter( + fc -> + Optional.of(fc.node()) + .map(n -> n instanceof MethodCallExpr mce ? mce : null) + .filter( + mce -> + mce.getArguments() + .getFirst() + .filter(arg -> "\"Foo\"".equals(arg.toString())) + .isPresent()) + .isPresent()) + .findAny(); + // Find if an issue with argument Bar exists + Optional> maybeBarFixCandidateIssue = + legacyFixCandidates.stream() + .filter( + fc -> + Optional.of(fc.node()) + .map(n -> n instanceof MethodCallExpr mce ? mce : null) + .filter( + mce -> + mce.getArguments() + .getFirst() + .filter(arg -> "\"Bar\"".equals(arg.toString())) + .isPresent()) + .isPresent()) + .findAny(); + + // one issue should match 2 issues and the first call which prints "Foo" + assertThat(maybeFooFixCandidateIssue.isPresent()).isTrue(); + assertThat(maybeFooFixCandidateIssue.get().issues()).containsExactly(issue1, issue2); + + // another issue should match 1 issue and the second call which prints "Bar" + assertThat(maybeBarFixCandidateIssue.isPresent()).isTrue(); + assertThat(maybeBarFixCandidateIssue.get().issues()).containsExactly(issue3); + } + + @Test + void it_finds_calls_that_span_multiple_lines() { + + String javaCode = + """ + class A { + void has_issues_1_and_2() { + int a = // both on line 3 + System.out.println("Foo"); // and on line 4 + } + } + """; + + CompilationUnit cu = StaticJavaParser.parse(javaCode); + + Issue issueForLine3 = new Issue("key1", 3); + Issue issueForLine4 = new Issue("key2", 4); + List issues = new ArrayList<>(List.of(issueForLine3, issueForLine4)); + + // try without specifying an end line first. + // the issue that points to startline=3 alone should not match the actual call location (4) so + // it should not match + // the issue that points to startline=4 alone should match the actual call location + FixCandidateSearchResults results = + searcher.search( + cu, + "path", + rule1, + issues, + Issue::key, + Issue::line, + i -> Optional.empty(), + i -> Optional.empty()); + + assertThat(results.fixCandidates().get(0).issues()).hasSize(1); + assertThat(results.fixCandidates().get(0).issues()).containsExactly(issueForLine4); + + // now with the endline=4 + // the issue that points to startline=3 will match + // the issue that points to startline=4 should still match + results = + searcher.search( + cu, + "path", + rule1, + issues, + Issue::key, + Issue::line, + i -> Optional.of(4), + i -> Optional.empty()); + + assertThat(results.fixCandidates().get(0).issues()).hasSize(2); + assertThat(results.fixCandidates().get(0).issues()) + .containsExactly(issueForLine3, issueForLine4); + } +} diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/DefaultLegacyLegacyFixCandidateSearcherTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/DefaultLegacyLegacyFixCandidateSearcherTest.java deleted file mode 100644 index c83b73ac5..000000000 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/DefaultLegacyLegacyFixCandidateSearcherTest.java +++ /dev/null @@ -1,116 +0,0 @@ -package io.codemodder.remediation; - -import static org.assertj.core.api.Assertions.assertThat; - -import com.github.javaparser.StaticJavaParser; -import com.github.javaparser.ast.CompilationUnit; -import io.codemodder.codetf.DetectorRule; -import io.codemodder.codetf.UnfixedFinding; -import java.util.List; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -final class DefaultLegacyLegacyFixCandidateSearcherTest { - - private DefaultLegacyFixCandidateSearcher searcher; - private DetectorRule rule1; - - private record Issue(String key, int line) {} - - @BeforeEach - void setup() { - searcher = new DefaultLegacyFixCandidateSearcher<>("println", List.of()); - rule1 = new DetectorRule("key1", "rule 1", null); - } - - @Test - void it_groups_overlapping_issues_together() { - - String javaCode = - """ - class A { - void has_issues_1_and_2() { - System.out.println("Foo"); // multiple issues point to this line - } - - void has_issue_3() { - System.out.println("Bar"); - } - } - """; - - Issue issue1 = new Issue("key1", 3); - Issue issue2 = new Issue("key2", 3); - Issue issue3 = new Issue("key3", 7); - Issue issue4 = new Issue("key4", 505); // doesnt exist - - List allIssues = List.of(issue1, issue2, issue3, issue4); - - CompilationUnit cu = StaticJavaParser.parse(javaCode); - - LegacyFixCandidateSearchResults legacyFixCandidateSearchResults = - searcher.search( - cu, "path", rule1, allIssues, Issue::key, Issue::line, i -> null, i -> null); - List> legacyFixCandidates = - legacyFixCandidateSearchResults.fixCandidates(); - assertThat(legacyFixCandidates).hasSize(2); - - // the first issue should match 2 issues and the first call which prints "Foo" - LegacyFixCandidate legacyFixCandidate1 = legacyFixCandidates.get(0); - assertThat(legacyFixCandidate1.call().asMethodCall().getArgument(0).toString()) - .hasToString("\"Foo\""); - assertThat(legacyFixCandidate1.issues()).containsExactly(issue1, issue2); - - // the second issue should match 1 issue and the second call which prints "Bar" - LegacyFixCandidate legacyFixCandidate2 = legacyFixCandidates.get(1); - assertThat(legacyFixCandidate2.call().asMethodCall().getArgument(0).toString()) - .hasToString("\"Bar\""); - assertThat(legacyFixCandidate2.issues()).containsExactly(issue3); - - // confirm that the unfixed finding is the one that doesn't exist - assertThat(legacyFixCandidateSearchResults.unfixableFindings()) - .containsExactly( - new UnfixedFinding( - "key4", rule1, "path", 505, RemediationMessages.noNodesAtThatLocation)); - } - - @Test - void it_finds_calls_that_span_multiple_lines() { - - String javaCode = - """ - class A { - void has_issues_1_and_2() { - int a = // both on line 3 - System.out.println("Foo"); // and on line 4 - } - } - """; - - CompilationUnit cu = StaticJavaParser.parse(javaCode); - - Issue issueForLine3 = new Issue("key1", 3); - Issue issueForLine4 = new Issue("key2", 4); - List issues = List.of(issueForLine3, issueForLine4); - - // try without specifying an end line first. - // the issue that points to startline=3 alone should not match the actual call location (4) so - // it should not match - // the issue that points to startline=4 alone should match the actual call location - LegacyFixCandidateSearchResults results = - searcher.search(cu, "path", rule1, issues, Issue::key, Issue::line, i -> null, i -> null); - - assertThat(results.fixCandidates().get(0).issues()).hasSize(1); - assertThat(results.fixCandidates().get(0).issues()).containsExactly(issueForLine4); - - // now with the endline=4 - // the issue that points to startline=3 will match - // the issue that points to startline=4 should still match - results = - searcher.search(cu, "path", rule1, issues, Issue::key, Issue::line, i -> 4, i -> null); - - assertThat(results.fixCandidates().get(0).issues()).hasSize(2); - assertThat(results.fixCandidates().get(0).issues()) - .containsExactly(issueForLine3, issueForLine4); - } -} diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/NakedVariableReturnFixerTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/NakedVariableReturnFixerTest.java index 064bfb92f..65b3103ff 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/NakedVariableReturnFixerTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/NakedVariableReturnFixerTest.java @@ -6,8 +6,11 @@ import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; import io.codemodder.codetf.DetectorRule; +import io.codemodder.remediation.FixCandidateSearcher; import io.codemodder.remediation.RemediationMessages; +import io.codemodder.remediation.SearcherStrategyRemediator; import java.util.List; +import java.util.Optional; import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -32,7 +35,7 @@ void should_not_be_fixed_method_call() { } """, 5, - null), + RemediationMessages.noNodesAtThatLocation), Arguments.of( """ class Samples { @@ -56,7 +59,7 @@ void should_not_be_fixed_multiple_returns() { } """, 5, - null)); + RemediationMessages.noNodesAtThatLocation)); } @BeforeEach @@ -71,25 +74,30 @@ void it_does_not_fix_unexpected_shapes( CompilationUnit cu = StaticJavaParser.parse(code); LexicalPreservingPrinter.setup(cu); - NakedVariableReturnFixer fixer = new NakedVariableReturnFixer(); + var fixer = new NakedVariableReturnFixStrategy(); List findings = List.of(new XSSFinding("id", line, null)); - XSSCodeShapeFixResult result = - fixer.fixCodeShape( + var remediator = + new SearcherStrategyRemediator.Builder() + .withSearcherStrategyPair( + new FixCandidateSearcher.Builder() + .withMatcher(NakedVariableReturnFixStrategy::match) + .build(), + fixer) + .build(); + var result = + remediator.remediateAll( cu, "path", detectorRule, findings, XSSFinding::key, XSSFinding::line, - XSSFinding::column); - assertThat(result.isFixed()).isFalse(); - if (result.isResponsibleFixer()) { - assertThat(result.reasonNotFixed()).isEqualTo(reasonForFailure); - } else { - assertThat(reasonForFailure).isNull(); - } + x -> Optional.empty(), + x -> Optional.ofNullable(x.column())); + assertThat(result.unfixedFindings().isEmpty()).isFalse(); + assertThat(result.unfixedFindings().get(0).getReason()).isEqualTo(reasonForFailure); } @Test @@ -107,23 +115,31 @@ void should_be_fixed_simple_name() { CompilationUnit cu = StaticJavaParser.parse(code); LexicalPreservingPrinter.setup(cu); - NakedVariableReturnFixer fixer = new NakedVariableReturnFixer(); + var fixer = new NakedVariableReturnFixStrategy(); // when we try to find both, we should error - fix groups should only have one location List findings = List.of(new XSSFinding("should_be_fixed_simple_name", 5, null)); - XSSCodeShapeFixResult result = - fixer.fixCodeShape( + var remediator = + new SearcherStrategyRemediator.Builder() + .withSearcherStrategyPair( + new FixCandidateSearcher.Builder() + .withMatcher(NakedVariableReturnFixStrategy::match) + .build(), + fixer) + .build(); + var result = + remediator.remediateAll( cu, "path", detectorRule, findings, XSSFinding::key, XSSFinding::line, - XSSFinding::column); - assertThat(result.isFixed()).isTrue(); - assertThat(result.isResponsibleFixer()).isTrue(); - assertThat(result.line()).isEqualTo(5); + x -> Optional.empty(), + x -> Optional.ofNullable(x.column())); + assertThat(result.changes().isEmpty()).isFalse(); + assertThat(result.changes().get(0).lineNumber()).isEqualTo(5); String afterCode = LexicalPreservingPrinter.print(cu); assertThat(afterCode) diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/PrintingMethodFixerTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/PrintingMethodFixerTest.java index cf5d38d26..f1d29005e 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/PrintingMethodFixerTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xss/PrintingMethodFixerTest.java @@ -6,7 +6,10 @@ import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; import io.codemodder.codetf.DetectorRule; +import io.codemodder.remediation.FixCandidateSearcher; +import io.codemodder.remediation.SearcherStrategyRemediator; import java.util.List; +import java.util.Optional; import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.params.ParameterizedTest; @@ -15,12 +18,12 @@ final class PrintingMethodFixerTest { - private PrintingMethodFixer fixer; + private PrintingMethodFixStrategy fixer; private DetectorRule rule; @BeforeEach void setup() { - this.fixer = new PrintingMethodFixer(); + this.fixer = new PrintingMethodFixStrategy(); this.rule = new DetectorRule("xss", "XSS", null); } @@ -83,17 +86,25 @@ void it_fixes_obvious_response_write_methods(final String beforeCode, final Stri LexicalPreservingPrinter.setup(cu); XSSFinding finding = new XSSFinding("should_be_fixed", 3, null); - XSSCodeShapeFixResult result = - fixer.fixCodeShape( + var remediator = + new SearcherStrategyRemediator.Builder() + .withSearcherStrategyPair( + new FixCandidateSearcher.Builder() + .withMatcher(PrintingMethodFixStrategy::match) + .build(), + fixer) + .build(); + var result = + remediator.remediateAll( cu, "path", rule, List.of(finding), XSSFinding::key, XSSFinding::line, - XSSFinding::column); - assertThat(result.isResponsibleFixer()).isTrue(); - assertThat(result.isFixed()).isTrue(); + x -> Optional.empty(), + x -> Optional.ofNullable(x.column())); + assertThat(result.changes().isEmpty()).isFalse(); String actualCode = LexicalPreservingPrinter.print(cu); assertThat(actualCode).isEqualToIgnoringWhitespace(afterCode); } diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediatorTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediatorTest.java index f0975d74a..a66d4935d 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediatorTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXEIntermediateXMLStreamReaderRemediatorTest.java @@ -10,17 +10,18 @@ import io.codemodder.codetf.DetectorRule; import io.codemodder.codetf.FixedFinding; import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; final class DefaultXXEIntermediateXMLStreamReaderRemediatorTest { - private DefaultXXEIntermediateXMLStreamReaderRemediator remediator; + private XXEIntermediateXMLStreamReaderRemediator remediator; private DetectorRule rule; @BeforeEach void setup() { - remediator = new DefaultXXEIntermediateXMLStreamReaderRemediator(); + remediator = new XXEIntermediateXMLStreamReaderRemediator<>(); rule = new DetectorRule("xxe", "XXE Fixed At XMLStreamReader", null); } @@ -48,7 +49,14 @@ Message read(String xml) { CodemodFileScanningResult result = remediator.remediateAll( - cu, "foo", rule, List.of(new Object()), f -> "my-id-1", f -> 9, f -> null, f -> null); + cu, + "foo", + rule, + List.of(new Object()), + f -> "my-id-1", + f -> 9, + f -> Optional.empty(), + f -> Optional.empty()); // confirm code is what's expected String actualCode = LexicalPreservingPrinter.print(cu); diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXERemediatorTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXERemediatorTest.java index a16e41db2..a9ef1bcc4 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXERemediatorTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DefaultXXERemediatorTest.java @@ -9,17 +9,18 @@ import io.codemodder.CodemodFileScanningResult; import io.codemodder.codetf.DetectorRule; import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; final class DefaultXXERemediatorTest { - private DefaultXXERemediator remediator; + private XXERemediator remediator; private DetectorRule rule; @BeforeEach void setup() { - this.remediator = new DefaultXXERemediator(); + this.remediator = new XXERemediator<>(); this.rule = new DetectorRule("xxe", "XXE", null); } @@ -52,7 +53,14 @@ public void foo() { LexicalPreservingPrinter.setup(cu); CodemodFileScanningResult result = remediator.remediateAll( - cu, "foo", rule, findings, XXEFinding::key, XXEFinding::line, XXEFinding::column); + cu, + "foo", + rule, + findings, + XXEFinding::key, + XXEFinding::line, + x -> Optional.empty(), + x -> Optional.ofNullable(x.column())); assertThat(result.unfixedFindings()).isEmpty(); assertThat(result.changes()).hasSize(1); CodemodChange change = result.changes().get(0); @@ -118,8 +126,14 @@ public void foo() { LexicalPreservingPrinter.setup(cu); CodemodFileScanningResult result = remediator.remediateAll( - cu, "foo", rule, findings, XXEFinding::key, XXEFinding::line, XXEFinding::column); + cu, + "foo", + rule, + findings, + XXEFinding::key, + XXEFinding::line, + x -> Optional.empty(), + x -> Optional.ofNullable(x.column())); assertThat(result.changes()).isEmpty(); - assertThat(result.unfixedFindings()).isEmpty(); } } diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixerTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixerTest.java index 3a224c275..fda1ff441 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixerTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixerTest.java @@ -5,16 +5,24 @@ import com.github.javaparser.StaticJavaParser; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.SearcherStrategyRemediator; +import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; final class DocumentBuilderFactoryAtNewDBFixerTest { - private DocumentBuilderFactoryAtNewDBFixer fixer; + private Remediator fixer; @BeforeEach void setup() { - this.fixer = new DocumentBuilderFactoryAtNewDBFixer(); + fixer = + new SearcherStrategyRemediator.Builder<>() + .withMatchAndFixStrategy(new DocumentBuilderFactoryAtNewDBFixStrategy()) + .build(); } @Test @@ -43,9 +51,17 @@ public void foo() { CompilationUnit cu = StaticJavaParser.parse(vulnerableCode); LexicalPreservingPrinter.setup(cu); - XXEFixAttempt attempt = fixer.tryFix(10, null, cu); - assertThat(attempt.isFixed()).isTrue(); - assertThat(attempt.isResponsibleFixer()).isTrue(); + var result = + fixer.remediateAll( + cu, + "path", + new DetectorRule("", "", ""), + List.of(new Object()), + o -> "id", + o -> 10, + o -> Optional.empty(), + o -> Optional.empty()); + assertThat(result.changes().isEmpty()).isFalse(); String fixedCode = """ diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixerTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixerTest.java index aa585a046..60393738a 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixerTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixerTest.java @@ -5,7 +5,10 @@ import com.github.javaparser.StaticJavaParser; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; -import io.codemodder.remediation.RemediationMessages; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.remediation.*; +import java.util.List; +import java.util.Optional; import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -15,7 +18,7 @@ final class DocumentBuilderFactoryAtParseFixerTest { - private DocumentBuilderFactoryAtParseFixer fixer; + private Remediator fixer; private static final String unfixableBecauseDocumentBuilderCameFromOutside = """ @@ -70,7 +73,10 @@ private static Stream unfixableSamples() { @BeforeEach void setup() { - this.fixer = new DocumentBuilderFactoryAtParseFixer(); + fixer = + new SearcherStrategyRemediator.Builder<>() + .withMatchAndFixStrategy(new DocumentBuilderFactoryAtParseFixStrategy()) + .build(); } @ParameterizedTest @@ -78,9 +84,18 @@ void setup() { void it_doesnt_fix(final String code, final int line, final Integer column, final String reason) { CompilationUnit cu = StaticJavaParser.parse(code); LexicalPreservingPrinter.setup(cu); - XXEFixAttempt attempt = fixer.tryFix(line, column, cu); - assertThat(attempt.isFixed()).isFalse(); - assertThat(attempt.reasonNotFixed()).isEqualTo(reason); + var result = + fixer.remediateAll( + cu, + "path", + new DetectorRule("", "", ""), + List.of(new Object()), + o -> "id", + o -> line, + o -> Optional.empty(), + o -> Optional.ofNullable(column)); + assertThat(result.unfixedFindings().isEmpty()).isFalse(); + assertThat(result.unfixedFindings().get(0).getReason()).isEqualTo(reason); } @Test @@ -109,9 +124,17 @@ public void foo() { CompilationUnit cu = StaticJavaParser.parse(vulnerableCode); LexicalPreservingPrinter.setup(cu); - XXEFixAttempt attempt = fixer.tryFix(11, 16, cu); - assertThat(attempt.isFixed()).isTrue(); - assertThat(attempt.isResponsibleFixer()).isTrue(); + var result = + fixer.remediateAll( + cu, + "path", + new DetectorRule("", "", ""), + List.of(new Object()), + o -> "id", + o -> 11, + o -> Optional.empty(), + o -> Optional.ofNullable(16)); + assertThat(result.changes().isEmpty()).isFalse(); String fixedCode = """ diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixerTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixerTest.java index 4aa3096a4..6af3f1776 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixerTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixerTest.java @@ -5,16 +5,24 @@ import com.github.javaparser.StaticJavaParser; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.SearcherStrategyRemediator; +import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; final class SAXParserAtNewSPFixerTest { - private SAXParserAtNewSPFixer fixer; + private Remediator fixer; @BeforeEach void setup() { - this.fixer = new SAXParserAtNewSPFixer(); + fixer = + new SearcherStrategyRemediator.Builder<>() + .withMatchAndFixStrategy(new SAXParserAtNewSPFixStrategy()) + .build(); } @Test @@ -39,9 +47,17 @@ public void foo() { CompilationUnit cu = StaticJavaParser.parse(vulnerableCode); LexicalPreservingPrinter.setup(cu); - XXEFixAttempt attempt = fixer.tryFix(6, null, cu); - assertThat(attempt.isFixed()).isTrue(); - assertThat(attempt.isResponsibleFixer()).isTrue(); + var result = + fixer.remediateAll( + cu, + "path", + new DetectorRule("", "", ""), + List.of(new Object()), + o -> "id", + o -> 6, + o -> Optional.empty(), + o -> Optional.empty()); + assertThat(result.changes().isEmpty()).isFalse(); String fixedCode = """ diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixerTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixerTest.java index 17098666e..4da9234d4 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixerTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/TransformerFactoryAtCreationFixerTest.java @@ -5,16 +5,24 @@ import com.github.javaparser.StaticJavaParser; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.remediation.Remediator; +import io.codemodder.remediation.SearcherStrategyRemediator; +import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; final class TransformerFactoryAtCreationFixerTest { - private TransformerFactoryAtCreationFixer fixer; + private Remediator fixer; @BeforeEach void setup() { - this.fixer = new TransformerFactoryAtCreationFixer(); + fixer = + new SearcherStrategyRemediator.Builder<>() + .withMatchAndFixStrategy(new TransformerFactoryAtCreationFixStrategy()) + .build(); } @Test @@ -30,10 +38,17 @@ public void foo() { """; CompilationUnit cu = StaticJavaParser.parse(vulnerableCode); LexicalPreservingPrinter.setup(cu); - XXEFixAttempt fixAttempt = fixer.tryFix(3, 52, cu); - assertThat(fixAttempt.isFixed()).isTrue(); - assertThat(fixAttempt.isResponsibleFixer()).isTrue(); - assertThat(fixAttempt.reasonNotFixed()).isNullOrEmpty(); + var result = + fixer.remediateAll( + cu, + "path", + new DetectorRule("", "", ""), + List.of(new Object()), + o -> "id", + o -> 3, + o -> Optional.empty(), + o -> Optional.ofNullable(52)); + assertThat(result.changes().isEmpty()).isFalse(); String fixedCode = """ diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixerTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixerTest.java index 5532ef91d..9b108b54f 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixerTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixerTest.java @@ -5,6 +5,10 @@ import com.github.javaparser.StaticJavaParser; import com.github.javaparser.ast.CompilationUnit; import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.remediation.SearcherStrategyRemediator; +import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.Test; final class XMLReaderAtParseFixerTest { @@ -39,11 +43,22 @@ public void foo() { CompilationUnit cu = StaticJavaParser.parse(vulnerableCode); LexicalPreservingPrinter.setup(cu); - XXEFixAttempt fixAttempt = new XMLReaderAtParseFixer().tryFix(14, 19, cu); + var searcherRemediator = + new SearcherStrategyRemediator.Builder<>() + .withMatchAndFixStrategy(new XMLReaderAtParseFixStrategy()) + .build(); + var result = + searcherRemediator.remediateAll( + cu, + "path", + new DetectorRule("", "", ""), + List.of(new Object()), + o -> "id", + o -> 14, + o -> Optional.empty(), + o -> Optional.of(19)); - assertThat(fixAttempt.isFixed()).isTrue(); - assertThat(fixAttempt.isResponsibleFixer()).isTrue(); - assertThat(fixAttempt.reasonNotFixed()).isNullOrEmpty(); + assertThat(result.changes().isEmpty()).isFalse(); String fixedCode = """ public class MyCode {