diff --git a/core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java index eeb083b4a..ed11b6da9 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java @@ -59,6 +59,7 @@ public CodemodFileScanningResult visit( detectorRule(), findingsForThisPath, finding -> String.valueOf(finding.getId()), - Finding::getLine); + Finding::getLine, + f -> null); } } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java index d093c5776..3ae760eab 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java @@ -50,6 +50,7 @@ public CodemodFileScanningResult visit( detectorRule(), hotspotsForFile, SonarFinding::getKey, - SonarFinding::getLine); + i -> i.getTextRange() != null ? i.getTextRange().getStartLine() : i.getLine(), + i -> i.getTextRange() != null ? i.getTextRange().getEndLine() : null); } } diff --git a/core-codemods/src/main/java/io/codemodder/codemods/SonarUnsafeReflectionRemediationCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/SonarUnsafeReflectionRemediationCodemod.java index fe2f9e8b6..29ef707ed 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/SonarUnsafeReflectionRemediationCodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/SonarUnsafeReflectionRemediationCodemod.java @@ -49,7 +49,8 @@ public CodemodFileScanningResult visit( detectorRule(), issues.getResultsByPath(context.path()), Issue::getKey, - Issue::getLine, + i -> i.getTextRange() != null ? i.getTextRange().getStartLine() : i.getLine(), + i -> i.getTextRange() != null ? i.getTextRange().getEndLine() : null, i -> i.getTextRange().getStartOffset()); } } 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 e7ae0148b..c02b9f8ef 100644 --- a/core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java +++ b/core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java @@ -49,7 +49,7 @@ public CodemodFileScanningResult visit( detectorRule(), issuesForFile, SonarFinding::getKey, - SonarFinding::getLine, + f -> f.getTextRange() != null ? f.getTextRange().getStartLine() : f.getLine(), f -> f.getTextRange().getStartOffset()); } } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/javaparser/ChangesResult.java b/framework/codemodder-base/src/main/java/io/codemodder/javaparser/ChangesResult.java index 8b3be97e7..f51700fc4 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/javaparser/ChangesResult.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/javaparser/ChangesResult.java @@ -5,6 +5,8 @@ /** Represents the result of changes made during parsing. */ public interface ChangesResult { + + /** Returns true if changes were applied. */ boolean areChangesApplied(); List getDependenciesRequired(); 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 85f6c073d..da2e082ae 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 @@ -2,23 +2,23 @@ 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.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.function.Function; import java.util.function.Predicate; +import org.jetbrains.annotations.VisibleForTesting; final class DefaultFixCandidateSearcher implements FixCandidateSearcher { - private final List> matchers; + private final List> matchers; private final String methodName; DefaultFixCandidateSearcher( - final String methodName, final List> matchers) { + final String methodName, final List> matchers) { this.methodName = methodName; this.matchers = matchers; } @@ -30,50 +30,61 @@ public FixCandidateSearchResults search( final DetectorRule rule, final List issuesForFile, final Function getKey, - final Function getLine, + final Function getStartLine, + final Function getEndLine, final Function getColumn) { List unfixedFindings = new ArrayList<>(); - List calls = - cu.findAll(MethodCallExpr.class).stream() - .filter( - mce -> - mce.getRange() - .isPresent()) // don't find calls we may have added -- you can pick these - // out by them not having a range yet - .filter(mce -> methodName == null || methodName.equals(mce.getNameAsString())) + + 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<>(); + Map> fixCandidateToIssueMapping = new HashMap<>(); for (T issue : issuesForFile) { String findingId = getKey.apply(issue); - int line = getLine.apply(issue); + int issueStartLine = getStartLine.apply(issue); + Integer issueEndLine = getEndLine.apply(issue); Integer column = getColumn.apply(issue); - List callsForIssue = + List callsForIssue = calls.stream() - .filter(mce -> mce.getRange().isPresent()) - .filter(mce -> mce.getRange().get().begin.line == line) + .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().get().contains(new Position(line, column))) + .filter(mce -> mce.getRange().contains(new Position(issueStartLine, column))) .toList(); } if (callsForIssue.isEmpty()) { unfixedFindings.add( new UnfixedFinding( - findingId, rule, path, line, RemediationMessages.noCallsAtThatLocation)); + findingId, rule, path, issueStartLine, RemediationMessages.noCallsAtThatLocation)); continue; } else if (callsForIssue.size() > 1) { unfixedFindings.add( new UnfixedFinding( - findingId, rule, path, line, RemediationMessages.multipleCallsFound)); + findingId, rule, path, issueStartLine, RemediationMessages.multipleCallsFound)); continue; } - MethodCallExpr call = callsForIssue.get(0); + MethodOrConstructor call = callsForIssue.get(0); fixCandidateToIssueMapping.computeIfAbsent(call, k -> new ArrayList<>()).add(issue); } @@ -94,4 +105,19 @@ public List> fixCandidates() { } }; } + + @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/FixCandidate.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidate.java index b7deb0331..5fcccf8ac 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidate.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidate.java @@ -1,14 +1,13 @@ package io.codemodder.remediation; -import com.github.javaparser.ast.expr.MethodCallExpr; import java.util.List; import java.util.Objects; /** The potential fix location. */ -public record FixCandidate(MethodCallExpr methodCall, List issues) { +public record FixCandidate(MethodOrConstructor call, List issues) { public FixCandidate { - Objects.requireNonNull(methodCall); + 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/FixCandidateSearcher.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/FixCandidateSearcher.java index 0cbc3d991..f3c45f9f1 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 @@ -1,7 +1,6 @@ package io.codemodder.remediation; 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; @@ -19,13 +18,14 @@ FixCandidateSearchResults search( DetectorRule rule, List issuesForFile, Function getKey, - Function getLine, + Function getStartLine, + Function getEndLine, Function getColumn); /** Builder for {@link FixCandidateSearcher}. */ final class Builder { private String methodName; - private final List> methodMatchers; + private final List> methodMatchers; public Builder() { this.methodMatchers = new ArrayList<>(); @@ -36,7 +36,7 @@ public Builder withMethodName(final String methodName) { return this; } - public Builder withMatcher(final Predicate methodMatcher) { + public Builder withMatcher(final Predicate methodMatcher) { this.methodMatchers.add(Objects.requireNonNull(methodMatcher)); return this; } diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java index c88e3674e..72b773030 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java @@ -11,6 +11,7 @@ public enum GenericRemediationMetadata { HEADER_INJECTION("header-injection"), REFLECTION_INJECTION("reflection-injection"), DESERIALIZATION("java-deserialization"), + MISSING_SECURE_FLAG("missing-secure-flag"), SQL_INJECTION("sql-injection"); private final CodemodReporterStrategy reporter; diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/MethodOrConstructor.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/MethodOrConstructor.java new file mode 100644 index 000000000..e26d8a78a --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/MethodOrConstructor.java @@ -0,0 +1,81 @@ +package io.codemodder.remediation; + +import com.github.javaparser.Range; +import com.github.javaparser.ast.Node; +import com.github.javaparser.ast.NodeList; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.expr.ObjectCreationExpr; +import com.github.javaparser.ast.nodeTypes.NodeWithArguments; +import java.util.Collection; +import java.util.Objects; + +/** Convenience type to represent a method or constructor call in our APIs that work with both. */ +public record MethodOrConstructor(Node node) { + + public MethodOrConstructor { + Objects.requireNonNull(node); + } + + /** Return the wrapped node as a {@link Node}. */ + public Node asNode() { + return this.node; + } + + /** This assumes a range is present, and explodes if it doesn't. */ + public Range getRange() { + return this.asNode().getRange().orElseThrow(); + } + + /** Return the wrapped node as a {@link com.github.javaparser.ast.nodeTypes.NodeWithArguments}. */ + public NodeWithArguments asNodeWithArguments() { + return (NodeWithArguments) this.node; + } + + /** Get the arguments for the call. */ + public NodeList getArguments() { + return this.asNodeWithArguments().getArguments(); + } + + /** Return true if this is a constructor call. */ + public boolean isConstructor() { + return this.node instanceof ObjectCreationExpr; + } + + /** Return true if this is a method call. */ + public boolean isMethodCall() { + return this.node instanceof MethodCallExpr; + } + + /** Return true if this is a method call with a scope. */ + public boolean isMethodCallWithScope() { + return this.node instanceof MethodCallExpr mce && mce.getScope().isPresent(); + } + + /** Return true if this is a constructor call for the given type. */ + public boolean isConstructorForType(final String type) { + return this.node instanceof ObjectCreationExpr oce && oce.getTypeAsString().equals(type); + } + + /** Return true if the node has a range, meaning it was not added by us. */ + public boolean hasRange() { + return this.asNode().getRange().isPresent(); + } + + /** Return true if this is a method call and it has the given name. */ + public boolean isMethodCallWithName(final String name) { + return this.isMethodCall() && ((MethodCallExpr) this.node).getNameAsString().equals(name); + } + + /** Return true if this is a method call and it has one of the given names. */ + public boolean isMethodCallWithNameIn(final Collection names) { + return this.isMethodCall() && names.contains(((MethodCallExpr) this.node).getNameAsString()); + } + + /** Return the wrapped node as a {@link MethodCallExpr} or blow up. */ + public MethodCallExpr asMethodCall() { + if (this.isMethodCall()) { + return (MethodCallExpr) this.node; + } + throw new IllegalStateException("Not a method call"); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/headerinjection/DefaultHeaderInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/headerinjection/DefaultHeaderInjectionRemediator.java index fdf29c1de..3c840fc03 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/headerinjection/DefaultHeaderInjectionRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/headerinjection/DefaultHeaderInjectionRemediator.java @@ -17,6 +17,7 @@ import io.codemodder.remediation.FixCandidate; import io.codemodder.remediation.FixCandidateSearchResults; import io.codemodder.remediation.FixCandidateSearcher; +import io.codemodder.remediation.MethodOrConstructor; import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -45,25 +46,34 @@ public CodemodFileScanningResult remediateAll( final DetectorRule detectorRule, final List issuesForFile, final Function getKey, - final Function getLine, - final Function getColumn) { + final Function getStartLine, + final Function getEndLine, + final Function getStartColumn) { FixCandidateSearcher searcher = new FixCandidateSearcher.Builder() - .withMatcher(mce -> setHeaderNames.contains(mce.getNameAsString())) - .withMatcher(mce -> mce.getScope().isPresent()) + .withMatcher(mce -> mce.isMethodCallWithNameIn(setHeaderNames)) + .withMatcher(MethodOrConstructor::isMethodCallWithScope) .withMatcher(mce -> mce.getArguments().size() == 2) - .withMatcher(mce -> !mce.getArgument(1).isStringLiteralExpr()) + .withMatcher(mce -> !(mce.getArguments().get(1) instanceof StringLiteralExpr)) .build(); FixCandidateSearchResults results = - searcher.search(cu, path, detectorRule, issuesForFile, getKey, getLine, getColumn); + searcher.search( + cu, + path, + detectorRule, + issuesForFile, + getKey, + getStartLine, + getEndLine, + getStartColumn); List changes = new ArrayList<>(); for (FixCandidate fixCandidate : results.fixCandidates()) { List issues = fixCandidate.issues(); - MethodCallExpr setHeaderCall = fixCandidate.methodCall(); + MethodCallExpr setHeaderCall = fixCandidate.call().asMethodCall(); Expression headerValueArgument = setHeaderCall.getArgument(1); wrap(headerValueArgument).withScopelessMethod(validatorMethodName); @@ -97,7 +107,7 @@ public CodemodFileScanningResult remediateAll( } // all the line numbers should be the same, so we just grab the first one - int line = getLine.apply(fixCandidate.issues().get(0)); + int line = getStartLine.apply(fixCandidate.issues().get(0)); List fixedFindings = issues.stream() .map(issue -> new FixedFinding(getKey.apply(issue), detectorRule)) diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/headerinjection/HeaderInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/headerinjection/HeaderInjectionRemediator.java index cdbb18c1b..b3e28c398 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/headerinjection/HeaderInjectionRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/headerinjection/HeaderInjectionRemediator.java @@ -16,7 +16,8 @@ CodemodFileScanningResult remediateAll( DetectorRule detectorRule, List issuesForFile, Function getKey, - Function getLine, + Function getStartLine, + Function getEndLine, Function getColumn); /** The default header injection remediation strategy. */ diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/DefaultJavaDeserializationRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/DefaultJavaDeserializationRemediator.java index 50d350189..f5fe148f0 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/DefaultJavaDeserializationRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/javadeserialization/DefaultJavaDeserializationRemediator.java @@ -19,11 +19,13 @@ import io.codemodder.remediation.FixCandidate; import io.codemodder.remediation.FixCandidateSearchResults; import io.codemodder.remediation.FixCandidateSearcher; +import io.codemodder.remediation.MethodOrConstructor; import io.github.pixee.security.ObjectInputFilters; import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.function.Function; +import org.jetbrains.annotations.NotNull; final class DefaultJavaDeserializationRemediator implements JavaDeserializationRemediator { @@ -34,32 +36,79 @@ public CodemodFileScanningResult remediateAll( final DetectorRule detectorRule, final List issuesForFile, final Function getKey, - final Function getLine, - final Function getColumn) { + final Function getStartLine, + final Function getEndLine, + final Function getStartColumn) { + FixCandidateSearcher searcher = new FixCandidateSearcher.Builder() .withMethodName("readObject") - .withMatcher(mce -> mce.getScope().isPresent()) + .withMatcher(MethodOrConstructor::isMethodCallWithScope) .withMatcher(mce -> mce.getArguments().isEmpty()) .build(); + // search for readObject() calls on those lines, assuming the tool points there FixCandidateSearchResults results = - searcher.search(cu, path, detectorRule, issuesForFile, getKey, getLine, getColumn); + searcher.search( + cu, + path, + detectorRule, + issuesForFile, + getKey, + getStartLine, + getEndLine, + getStartColumn); + + if (results.fixCandidates().isEmpty()) { + // try searching for matching ObjectInputStream creation objects, maybe that's where they're + // pointing + searcher = + new FixCandidateSearcher.Builder() + .withMatcher(mc -> mc.isConstructorForType("ObjectInputStream")) + .build(); + + results = + searcher.search( + cu, + path, + detectorRule, + issuesForFile, + getKey, + getStartLine, + getEndLine, + getStartColumn); + } List changes = new ArrayList<>(); List unfixedFindings = new ArrayList<>(); + for (FixCandidate fixCandidate : results.fixCandidates()) { List issues = fixCandidate.issues(); - MethodCallExpr call = fixCandidate.methodCall(); - // get the declaration of the ObjectInputStream - Expression callScope = call.getScope().get(); + MethodOrConstructor call = fixCandidate.call(); + + if (call.isConstructor()) { + // we're pointing to the readObject(), fix and move on + fixObjectInputStreamCreation((ObjectCreationExpr) call.asNode()); + CodemodChange change = buildFixChange(detectorRule, getKey, getStartLine, issues); + changes.add(change); + continue; + } + + // we're pointing to the readObject(), so we must work backwards to find the declaration of + // the ObjectInputStream + MethodCallExpr mce = (MethodCallExpr) call.asNode(); + Expression callScope = mce.getScope().get(); if (!callScope.isNameExpr()) { // can't fix these issues.stream() .map( i -> new UnfixedFinding( - getKey.apply(i), detectorRule, path, getLine.apply(i), "Unexpected shape")) + getKey.apply(i), + detectorRule, + path, + getStartLine.apply(i), + "Unexpected shape")) .forEach(unfixedFindings::add); continue; } @@ -74,7 +123,7 @@ public CodemodFileScanningResult remediateAll( getKey.apply(i), detectorRule, path, - getLine.apply(i), + getStartLine.apply(i), "No declaration found")) .forEach(unfixedFindings::add); continue; @@ -92,7 +141,7 @@ public CodemodFileScanningResult remediateAll( getKey.apply(i), detectorRule, path, - getLine.apply(i), + getStartLine.apply(i), "No initializer found")) .forEach(unfixedFindings::add); continue; @@ -101,13 +150,7 @@ public CodemodFileScanningResult remediateAll( Expression expression = initializer.get(); if (expression instanceof ObjectCreationExpr objCreation) { fixObjectInputStreamCreation(objCreation); - CodemodChange change = - CodemodChange.from( - getLine.apply(issues.get(0)), - List.of(DependencyGAV.JAVA_SECURITY_TOOLKIT), - issues.stream() - .map(i -> new FixedFinding(getKey.apply(i), detectorRule)) - .toList()); + CodemodChange change = buildFixChange(detectorRule, getKey, getStartLine, issues); changes.add(change); } } else { @@ -118,7 +161,7 @@ public CodemodFileScanningResult remediateAll( getKey.apply(i), detectorRule, path, - getLine.apply(i), + getStartLine.apply(i), "Unexpected declaration type")) .forEach(unfixedFindings::add); } @@ -128,6 +171,20 @@ public CodemodFileScanningResult remediateAll( return CodemodFileScanningResult.from(changes, unfixedFindings); } + /** + * Build a {@link io.codemodder.CodemodChange} for this code change that fixes the given issues. + */ + private static @NotNull CodemodChange buildFixChange( + final DetectorRule detectorRule, + final Function getKey, + final Function getLine, + final List issues) { + return CodemodChange.from( + getLine.apply(issues.get(0)), + List.of(DependencyGAV.JAVA_SECURITY_TOOLKIT), + issues.stream().map(i -> new FixedFinding(getKey.apply(i), detectorRule)).toList()); + } + private void fixObjectInputStreamCreation(final ObjectCreationExpr objCreation) { replace(objCreation) .withStaticMethod(ObjectInputFilters.class.getName(), "createSafeObjectInputStream") 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 1040e37a2..216c76c5e 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 @@ -16,7 +16,8 @@ CodemodFileScanningResult remediateAll( DetectorRule detectorRule, List issuesForFile, Function getKey, - Function getLine, + Function getStartLine, + Function getEndLine, Function getColumn); /** The default header injection remediation strategy. */ diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/DefaultJNDIInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/DefaultJNDIInjectionRemediator.java index 121cf988b..7f962b370 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/DefaultJNDIInjectionRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/DefaultJNDIInjectionRemediator.java @@ -42,29 +42,38 @@ public CodemodFileScanningResult remediateAll( final DetectorRule detectorRule, final List issuesForFile, final Function getKey, - final Function getLine, - final Function getColumn) { + final Function getStartLine, + final Function getEndLine, + final Function getStartColumn) { FixCandidateSearcher searcher = new FixCandidateSearcher.Builder() .withMethodName("lookup") - .withMatcher(mce -> mce.getScope().isPresent()) + .withMatcher(mce -> mce.asNode().hasScope()) .withMatcher(mce -> mce.getArguments().size() == 1) - .withMatcher(mce -> mce.getArgument(0).isNameExpr()) + .withMatcher(mce -> mce.getArguments().get(0) instanceof NameExpr) .build(); // find all the potential lookup() calls FixCandidateSearchResults results = - searcher.search(cu, path, detectorRule, issuesForFile, getKey, getLine, getColumn); + searcher.search( + cu, + path, + detectorRule, + issuesForFile, + getKey, + getStartLine, + getEndLine, + getStartColumn); List unfixedFindings = new ArrayList<>(); List changes = new ArrayList<>(); for (FixCandidate fixCandidate : results.fixCandidates()) { List issues = fixCandidate.issues(); - int line = getLine.apply(issues.get(0)); + int line = getStartLine.apply(issues.get(0)); - MethodCallExpr lookupCall = fixCandidate.methodCall(); + MethodCallExpr lookupCall = fixCandidate.call().asMethodCall(); // get the parent method of the lookup() call Optional parentMethod = lookupCall.findAncestor(MethodDeclaration.class); if (parentMethod.isEmpty()) { diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/JNDIInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/JNDIInjectionRemediator.java index e95cea775..361a701c6 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/JNDIInjectionRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/JNDIInjectionRemediator.java @@ -22,8 +22,9 @@ CodemodFileScanningResult remediateAll( DetectorRule detectorRule, List issuesForFile, Function getKey, - Function getLine, - Function getColumn); + Function getStartLine, + Function getEndLine, + Function getStartColumn); /** The default JNDI injection remediation strategy. */ JNDIInjectionRemediator DEFAULT = new DefaultJNDIInjectionRemediator(); diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/missingsecureflag/DefaultMissingSecureFlagRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/missingsecureflag/DefaultMissingSecureFlagRemediator.java new file mode 100644 index 000000000..50185b824 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/missingsecureflag/DefaultMissingSecureFlagRemediator.java @@ -0,0 +1,126 @@ +package io.codemodder.remediation.missingsecureflag; + +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.ast.NodeList; +import com.github.javaparser.ast.expr.BooleanLiteralExpr; +import com.github.javaparser.ast.expr.Expression; +import com.github.javaparser.ast.expr.MethodCallExpr; +import com.github.javaparser.ast.stmt.ExpressionStmt; +import com.github.javaparser.ast.stmt.Statement; +import io.codemodder.CodemodChange; +import io.codemodder.CodemodFileScanningResult; +import io.codemodder.ast.ASTTransforms; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.codetf.FixedFinding; +import io.codemodder.codetf.UnfixedFinding; +import io.codemodder.remediation.FixCandidate; +import io.codemodder.remediation.FixCandidateSearchResults; +import io.codemodder.remediation.FixCandidateSearcher; +import io.codemodder.remediation.RemediationMessages; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.function.Function; +import org.jetbrains.annotations.NotNull; + +final class DefaultMissingSecureFlagRemediator implements MissingSecureFlagRemediator { + + @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) { + + // this remediator assumes we're pointing to a response.addCookie() call + FixCandidateSearcher searcher = + new FixCandidateSearcher.Builder() + .withMethodName("addCookie") + .withMatcher(mce -> mce.getArguments().size() == 1) + .build(); + + FixCandidateSearchResults results = + searcher.search( + cu, + path, + detectorRule, + issuesForFile, + getKey, + getStartLine, + getEndLine, + getStartColumn); + + List changes = new ArrayList<>(); + List unfixedFindings = new ArrayList<>(); + + for (FixCandidate result : results.fixCandidates()) { + MethodCallExpr methodCallExpr = result.call().asMethodCall(); + List issues = result.issues(); + + if (methodCallExpr.getScope().isPresent()) { + + // This assumption is a bit strong, but covers the most common cases while avoiding weird + // ones + Optional maybeStmt = + methodCallExpr + .getParentNode() + .map(p -> p instanceof Statement ? (Statement) p : null) + .filter(Statement::isExpressionStmt); + + // We want to avoid things like: response.addCookie(new Cookie(...)), so we restrict + // ourselves + Optional maybeCookieExpression = + Optional.of(methodCallExpr.getArgument(0)) + .filter(expr -> expr.isNameExpr() || expr.isFieldAccessExpr()); + + if (maybeStmt.isPresent() && maybeCookieExpression.isPresent()) { + final var newStatement = + new ExpressionStmt( + new MethodCallExpr( + maybeCookieExpression.get(), + "setSecure", + new NodeList<>(new BooleanLiteralExpr(true)))); + + ASTTransforms.addStatementBeforeStatement(maybeStmt.get(), newStatement); + + FixedFinding fixedFinding = new FixedFinding(getKey.apply(issues.get(0)), detectorRule); + CodemodChange change = + CodemodChange.from( + result.call().getRange().begin.line, List.of(), List.of(fixedFinding)); + changes.add(change); + } else { + List unfixedFindingsToAdd = + createUnfixedFindingList(path, detectorRule, getKey, getStartLine, issues); + unfixedFindings.addAll(unfixedFindingsToAdd); + } + } else { + List unfixedFindingsToAdd = + createUnfixedFindingList(path, detectorRule, getKey, getStartLine, issues); + unfixedFindings.addAll(unfixedFindingsToAdd); + } + } + return CodemodFileScanningResult.from(changes, unfixedFindings); + } + + private static @NotNull List createUnfixedFindingList( + final String path, + final DetectorRule detectorRule, + final Function getKey, + final Function getStartLine, + final List issues) { + return issues.stream() + .map( + issue -> + new UnfixedFinding( + getKey.apply(issue), + detectorRule, + path, + getStartLine.apply(issue), + RemediationMessages.ambiguousCodeShape)) + .toList(); + } +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/missingsecureflag/MissingSecureFlagRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/missingsecureflag/MissingSecureFlagRemediator.java new file mode 100644 index 000000000..3cc14c77b --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/missingsecureflag/MissingSecureFlagRemediator.java @@ -0,0 +1,24 @@ +package io.codemodder.remediation.missingsecureflag; + +import com.github.javaparser.ast.CompilationUnit; +import io.codemodder.CodemodFileScanningResult; +import io.codemodder.codetf.DetectorRule; +import java.util.List; +import java.util.function.Function; + +/** Strategy interface for remediating missing secure flag vulnerabilities. */ +public interface MissingSecureFlagRemediator { + + /** Remediate all missing secure flag vulnerabilities in the given compilation unit. */ + CodemodFileScanningResult remediateAll( + CompilationUnit cu, + String path, + DetectorRule detectorRule, + List issuesForFile, + Function getKey, + Function getStartLine, + Function getEndLine, + Function getStartColumn); + + MissingSecureFlagRemediator DEFAULT = new DefaultMissingSecureFlagRemediator(); +} diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/reflectioninjection/DefaultReflectionInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/reflectioninjection/DefaultReflectionInjectionRemediator.java index f46e66901..047e499f3 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/reflectioninjection/DefaultReflectionInjectionRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/reflectioninjection/DefaultReflectionInjectionRemediator.java @@ -13,6 +13,7 @@ import io.codemodder.remediation.FixCandidate; import io.codemodder.remediation.FixCandidateSearchResults; import io.codemodder.remediation.FixCandidateSearcher; +import io.codemodder.remediation.MethodOrConstructor; import io.github.pixee.security.Reflection; import java.util.ArrayList; import java.util.List; @@ -27,8 +28,9 @@ public CodemodFileScanningResult remediateAll( final DetectorRule detectorRule, final List issuesForFile, final Function getKey, - final Function getLine, - final Function getColumn) { + final Function getStartLine, + final Function getEndLine, + final Function getStartColumn) { FixCandidateSearcher searcher = new FixCandidateSearcher.Builder() @@ -36,14 +38,23 @@ public CodemodFileScanningResult remediateAll( .build(); FixCandidateSearchResults results = - searcher.search(cu, path, detectorRule, issuesForFile, getKey, getLine, getColumn); + searcher.search( + cu, + path, + detectorRule, + issuesForFile, + getKey, + getStartLine, + getEndLine, + getStartColumn); + List changes = new ArrayList<>(); for (FixCandidate fixCandidate : results.fixCandidates()) { List issues = fixCandidate.issues(); - int line = getLine.apply(issues.get(0)); + int line = getStartLine.apply(issues.get(0)); - MethodCallExpr methodCallExpr = fixCandidate.methodCall(); + MethodCallExpr methodCallExpr = fixCandidate.call().asMethodCall(); replaceMethodCallExpression(cu, methodCallExpr); issues.stream() @@ -85,11 +96,15 @@ private static void replaceMethodCallExpression( * the method expression. * * @param cu CompilationUnit for checking static imports - * @param methodCallExpr the method call expression to check + * @param methodOrConstructor the node to check * @return true if the method call expression is a call to {@code Class.forName(String)} */ private static boolean isClassForNameCall( - final CompilationUnit cu, final MethodCallExpr methodCallExpr) { + final CompilationUnit cu, final MethodOrConstructor methodOrConstructor) { + if (!methodOrConstructor.isMethodCall()) { + return false; + } + MethodCallExpr methodCallExpr = methodOrConstructor.asMethodCall(); final boolean scopeMatches = methodCallExpr .getScope() diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/reflectioninjection/ReflectionInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/reflectioninjection/ReflectionInjectionRemediator.java index c52b412d0..b9012611f 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/reflectioninjection/ReflectionInjectionRemediator.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/reflectioninjection/ReflectionInjectionRemediator.java @@ -16,7 +16,8 @@ CodemodFileScanningResult remediateAll( DetectorRule detectorRule, List issuesForFile, Function getKey, - Function getLine, + Function getStartLine, + Function getEndLine, Function getColumn); ReflectionInjectionRemediator DEFAULT = new DefaultReflectionInjectionRemediator(); diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java index eeec9542a..c5f07ef73 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java @@ -1,7 +1,6 @@ package io.codemodder.remediation.sqlinjection; import com.github.javaparser.ast.CompilationUnit; -import com.github.javaparser.ast.expr.MethodCallExpr; import io.codemodder.CodemodChange; import io.codemodder.CodemodFileScanningResult; import io.codemodder.codetf.DetectorRule; @@ -10,6 +9,7 @@ import io.codemodder.remediation.FixCandidate; import io.codemodder.remediation.FixCandidateSearchResults; import io.codemodder.remediation.FixCandidateSearcher; +import io.codemodder.remediation.MethodOrConstructor; import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -25,7 +25,8 @@ final class DefaultJavaParserSQLInjectionRemediatorStrategy implements JavaParserSQLInjectionRemediatorStrategy { - private final Map, Predicate> remediationStrategies; + private final Map, Predicate> + remediationStrategies; /** * Builds a strategy from a matcher-fixer pair. A matcher is a predicate that matches the call, @@ -33,13 +34,13 @@ final class DefaultJavaParserSQLInjectionRemediatorStrategy * as a side-effect and reports if successful with a boolean. */ DefaultJavaParserSQLInjectionRemediatorStrategy( - final Predicate matcher, final Predicate fixer) { + final Predicate matcher, final Predicate fixer) { this.remediationStrategies = Map.of(matcher, fixer); } /** Builds a grand strategy as a combination of several strategies. */ DefaultJavaParserSQLInjectionRemediatorStrategy( - final Map, Predicate> strategies) { + final Map, Predicate> strategies) { this.remediationStrategies = strategies; } @@ -50,9 +51,10 @@ private Pair, List> remediateWithStrateg final DetectorRule detectorRule, final Collection findingsForPath, final Function findingIdExtractor, - final Function findingLineExtractor, - final Predicate matcher, - final Predicate fixer) { + final Function findingStartLineExtractor, + final Function findingEndLineExtractor, + final Predicate matcher, + final Predicate fixer) { FixCandidateSearcher searcher = new FixCandidateSearcher.Builder().withMatcher(matcher).build(); @@ -64,7 +66,8 @@ private Pair, List> remediateWithStrateg detectorRule, new ArrayList<>(findingsForPath), findingIdExtractor, - findingLineExtractor, + findingStartLineExtractor, + findingEndLineExtractor, f -> null); if (findingsForPath.isEmpty()) { @@ -76,7 +79,7 @@ private Pair, List> remediateWithStrateg for (FixCandidate fixCandidate : results.fixCandidates()) { List issues = fixCandidate.issues(); - Integer line = findingLineExtractor.apply(issues.get(0)); + Integer line = findingStartLineExtractor.apply(issues.get(0)); if (line == null) { issues.forEach( @@ -89,8 +92,7 @@ private Pair, List> remediateWithStrateg continue; } - final MethodCallExpr methodCallExpr = fixCandidate.methodCall(); - if (fixer.test(methodCallExpr)) { + if (fixer.test(fixCandidate.call())) { issues.forEach( issue -> { final String id = findingIdExtractor.apply(issue); @@ -122,7 +124,7 @@ private Pair, List> remediateWithStrateg * @param detectorRule the detector rule that generated the findings * @param findingsForPath a collection of findings to be processed * @param findingIdExtractor a function to extract the ID from a finding - * @param findingLineExtractor a function to extract the line number from a finding + * @param findingStartLineExtractor a function to extract the line number from a finding * @param the type of the findings * @return a result object containing the changes and unfixed findings */ @@ -133,7 +135,9 @@ public CodemodFileScanningResult remediateAll( final DetectorRule detectorRule, final Collection findingsForPath, final Function findingIdExtractor, - final Function findingLineExtractor) { + final Function findingStartLineExtractor, + final Function findingEndLineExtractor) { + List allChanges = new ArrayList<>(); List allUnfixed = new ArrayList<>(); @@ -146,7 +150,8 @@ public CodemodFileScanningResult remediateAll( detectorRule, findingsForPath, findingIdExtractor, - findingLineExtractor, + findingStartLineExtractor, + findingEndLineExtractor, matcher, fixer); allChanges.addAll(pairResult.getValue0()); diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java index c0b7e45c8..4ff64b6c6 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java @@ -20,7 +20,7 @@ public interface JavaParserSQLInjectionRemediatorStrategy { * @param cu the compilation unit to be scanned * @param pathFindings a collection of findings to be processed * @param findingIdExtractor a function to extract the ID from a finding - * @param findingLineExtractor a function to extract the line number from a finding + * @param findingStartLineExtractor a function to extract the line number from a finding * @param the type of the findings * @return a result object containing the changes and unfixed findings */ @@ -30,7 +30,8 @@ CodemodFileScanningResult remediateAll( final DetectorRule rule, final Collection pathFindings, final Function findingIdExtractor, - final Function findingLineExtractor); + final Function findingStartLineExtractor, + final Function findingEndLineExtractor); /** A default implementation that should be used in all non-test scenarios. */ JavaParserSQLInjectionRemediatorStrategy DEFAULT = diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionFixComposer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionFixComposer.java index 4fc5fbb71..2a7ff650f 100644 --- a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionFixComposer.java +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionFixComposer.java @@ -1,6 +1,7 @@ package io.codemodder.remediation.sqlinjection; import com.github.javaparser.ast.expr.MethodCallExpr; +import io.codemodder.remediation.MethodOrConstructor; /** Composes several transformations related to SQL injections. */ public final class SQLInjectionFixComposer { @@ -12,7 +13,14 @@ private SQLInjectionFixComposer() {} * prepareStatement(), executeQuery(), etc.), parameterize data injections or add a validation * step for structural injections. */ - public static boolean checkAndFix(final MethodCallExpr methodCallExpr) { + public static boolean checkAndFix(final MethodOrConstructor m) { + + if (!m.isMethodCall()) { + return false; + } + + MethodCallExpr methodCallExpr = m.asMethodCall(); + // First, check if any data injection fixes apply var maybeFixed = new SQLParameterizer(methodCallExpr).checkAndFix(); if (maybeFixed.isPresent()) { @@ -30,7 +38,11 @@ public static boolean checkAndFix(final MethodCallExpr methodCallExpr) { * Check if the {@link MethodCallExpr} is a JDBC API query method that is a target of a SQL * injection transformation. */ - public static boolean match(final MethodCallExpr methodCallExpr) { + public static boolean match(final MethodOrConstructor methodOrConstructor) { + if (!methodOrConstructor.isMethodCall()) { + return false; + } + MethodCallExpr methodCallExpr = methodOrConstructor.asMethodCall(); return SQLParameterizer.isSupportedJdbcMethodCall(methodCallExpr) || SQLTableInjectionFilterTransform.matchCall(methodCallExpr); } 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 index 6efb228ec..a6192b5ed 100644 --- 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 @@ -28,17 +28,26 @@ public CodemodFileScanningResult remediateAll( final DetectorRule detectorRule, final List issuesForFile, final Function getKey, - final Function getLine, - final Function getColumn) { + final Function getStartLine, + final Function getEndLine, + final Function getStartColumn) { FixCandidateSearcher searcher = new FixCandidateSearcher.Builder() .withMethodName("createXMLStreamReader") - .withMatcher(mce -> mce.getScope().isPresent()) + .withMatcher(mce -> mce.asNode().hasScope()) .withMatcher(mce -> mce.getArguments().isNonEmpty()) .build(); FixCandidateSearchResults results = - searcher.search(cu, path, detectorRule, issuesForFile, getKey, getLine, getColumn); + searcher.search( + cu, + path, + detectorRule, + issuesForFile, + getKey, + getStartLine, + getEndLine, + getStartColumn); List changes = new ArrayList<>(); List unfixedFindings = new ArrayList<>(); @@ -47,9 +56,10 @@ public CodemodFileScanningResult remediateAll( List issues = fixCandidate.issues(); // get the xmlstreamreader scope variable - MethodCallExpr createXMLStreamReaderCall = fixCandidate.methodCall(); + MethodCallExpr createXMLStreamReaderCall = fixCandidate.call().asMethodCall(); Expression xmlStreamReaderScope = createXMLStreamReaderCall.getScope().get(); - // make sure its a variable + + // make sure it's a variable if (!xmlStreamReaderScope.isNameExpr()) { issues.stream() .map( @@ -58,7 +68,7 @@ public CodemodFileScanningResult remediateAll( getKey.apply(issue), detectorRule, path, - getLine.apply(issue), + getStartLine.apply(issue), "Could not find the XMLStreamReader variable")) .forEach(unfixedFindings::add); continue; @@ -76,7 +86,7 @@ public CodemodFileScanningResult remediateAll( getKey.apply(issue), detectorRule, path, - getLine.apply(issue), + getStartLine.apply(issue), "Could not find the statement containing the XMLStreamReader creation")) .forEach(unfixedFindings::add); continue; @@ -87,7 +97,8 @@ public CodemodFileScanningResult remediateAll( .map( issue -> CodemodChange.from( - getLine.apply(issue), new FixedFinding(getKey.apply(issue), detectorRule))) + getStartLine.apply(issue), + new FixedFinding(getKey.apply(issue), detectorRule))) .forEach(changes::add); } 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 index 8c4780e59..6a09449dd 100644 --- 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 @@ -18,6 +18,8 @@ final class DefaultXXERemediator implements XXERemediator { this.fixers = List.of( new DocumentBuilderFactoryAndSAXParserAtCreationFixer(), + new DocumentBuilderFactoryAtNewDBFixer(), + new SAXParserAtNewSPFixer(), new DocumentBuilderFactoryAtParseFixer(), new TransformerFactoryAtCreationFixer(), new XMLReaderAtParseFixer()); @@ -30,7 +32,7 @@ public CodemodFileScanningResult remediateAll( final DetectorRule detectorRule, final List issuesForFile, final Function getKey, - final Function getLine, + final Function getStartLine, final Function getColumn) { List unfixedFindings = new ArrayList<>(); @@ -39,7 +41,7 @@ public CodemodFileScanningResult remediateAll( for (T issue : issuesForFile) { String findingId = getKey.apply(issue); - int line = getLine.apply(issue); + int line = getStartLine.apply(issue); Integer column = getColumn.apply(issue); for (XXEFixer fixer : fixers) { XXEFixAttempt fixAttempt = fixer.tryFix(line, column, cu); 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 new file mode 100644 index 000000000..051d91cb2 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixer.java @@ -0,0 +1,49 @@ +package io.codemodder.remediation.xxe; + +import static io.codemodder.remediation.RemediationMessages.multipleCallsFound; +import static io.codemodder.remediation.RemediationMessages.noCallsAtThatLocation; +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, noCallsAtThatLocation); + } else if (candidateMethods.size() > 1) { + return new XXEFixAttempt(false, false, multipleCallsFound); + } + + 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/SAXParserAtNewSPFixer.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixer.java new file mode 100644 index 000000000..614d15476 --- /dev/null +++ b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixer.java @@ -0,0 +1,49 @@ +package io.codemodder.remediation.xxe; + +import static io.codemodder.remediation.RemediationMessages.multipleCallsFound; +import static io.codemodder.remediation.RemediationMessages.noCallsAtThatLocation; +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, noCallsAtThatLocation); + } else if (candidateMethods.size() > 1) { + return new XXEFixAttempt(false, false, multipleCallsFound); + } + + 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/XXEIntermediateXMLStreamReaderRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/xxe/XXEIntermediateXMLStreamReaderRemediator.java index f972bcd96..68682e80a 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 @@ -23,6 +23,7 @@ CodemodFileScanningResult remediateAll( DetectorRule detectorRule, List issuesForFile, Function getKey, - Function getLine, - Function getColumn); + Function getEndLine, + Function getStartColumn, + Function getEndColumn); } 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 6f162e3cf..8f4fdfb6c 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 @@ -19,6 +19,6 @@ CodemodFileScanningResult remediateAll( DetectorRule detectorRule, List issuesForFile, Function getKey, - Function getLine, + Function getStartLine, Function getColumn); } diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/missing-secure-flag/description.md b/framework/codemodder-base/src/main/resources/generic-remediation-reports/missing-secure-flag/description.md new file mode 100644 index 000000000..cb5a0e691 --- /dev/null +++ b/framework/codemodder-base/src/main/resources/generic-remediation-reports/missing-secure-flag/description.md @@ -0,0 +1,11 @@ +This change marks new cookies sent in the HTTP with the ["secure" flag](https://owasp.org/www-community/controls/SecureCookieAttribute). This flag, despite its ambitious name, only provides one type of protection: confidentiality. Cookies with this flag are guaranteed by the browser never to be sent over a cleartext channel ("http://") and only sent over secure channels ("https://"). + +Our change introduces this flag with a simple 1-line statement: + +```diff + Cookie cookie = new Cookie("my_cookie", userCookieValue); ++ cookie.setSecure(true); + response.addCookie(cookie); +``` + +Note: this code change **may cause issues** with the application if any of the places this code runs (in CI, pre-production or in production) are running in non-HTTPS protocol. diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/missing-secure-flag/report.json b/framework/codemodder-base/src/main/resources/generic-remediation-reports/missing-secure-flag/report.json new file mode 100644 index 000000000..18274be66 --- /dev/null +++ b/framework/codemodder-base/src/main/resources/generic-remediation-reports/missing-secure-flag/report.json @@ -0,0 +1,10 @@ +{ + "summary" : "Added secure flag to HTTP cookies", + "change": "Added a call to `setSecure()` to make sure the cookie is only transferred over HTTPS traffic", + "reviewGuidanceIJustification" : "This code change may cause issues with the application if any of the places this code runs (in CI, pre-production or in production) are running over plaintext HTTP.", + "references" : [ + "https://owasp.org/www-community/controls/SecureCookieAttribute", + "https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies", + "https://cwe.mitre.org/data/definitions/614.html" + ] +} 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 index 859190fa6..3187173ca 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/DefaultFixCandidateSearcherTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/DefaultFixCandidateSearcherTest.java @@ -7,27 +7,37 @@ 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 DefaultFixCandidateSearcherTest { + private DefaultFixCandidateSearcher searcher; + private DetectorRule rule1; + private record Issue(String key, int line) {} + @BeforeEach + void setup() { + searcher = new DefaultFixCandidateSearcher<>("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 - } + 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"); - } - } - """; + void has_issue_3() { + System.out.println("Bar"); + } + } + """; Issue issue1 = new Issue("key1", 3); Issue issue2 = new Issue("key2", 3); @@ -37,23 +47,23 @@ void has_issue_3() { List allIssues = List.of(issue1, issue2, issue3, issue4); CompilationUnit cu = StaticJavaParser.parse(javaCode); - DefaultFixCandidateSearcher searcher = - new DefaultFixCandidateSearcher<>("println", List.of()); - DetectorRule rule1 = new DetectorRule("key1", "rule 1", null); FixCandidateSearchResults fixCandidateSearchResults = - searcher.search(cu, "path", rule1, allIssues, Issue::key, Issue::line, i -> null); + searcher.search( + cu, "path", rule1, allIssues, Issue::key, Issue::line, i -> null, i -> null); List> fixCandidates = fixCandidateSearchResults.fixCandidates(); assertThat(fixCandidates).hasSize(2); // the first issue should match 2 issues and the first call which prints "Foo" FixCandidate fixCandidate1 = fixCandidates.get(0); - assertThat(fixCandidate1.methodCall().getArgument(0).toString()).hasToString("\"Foo\""); + assertThat(fixCandidate1.call().asMethodCall().getArgument(0).toString()) + .hasToString("\"Foo\""); assertThat(fixCandidate1.issues()).containsExactly(issue1, issue2); // the second issue should match 1 issue and the second call which prints "Bar" FixCandidate fixCandidate2 = fixCandidates.get(1); - assertThat(fixCandidate2.methodCall().getArgument(0).toString()).hasToString("\"Bar\""); + assertThat(fixCandidate2.call().asMethodCall().getArgument(0).toString()) + .hasToString("\"Bar\""); assertThat(fixCandidate2.issues()).containsExactly(issue3); // confirm that the unfixed finding is the one that doesn't exist @@ -62,4 +72,44 @@ void has_issue_3() { new UnfixedFinding( "key4", rule1, "path", 505, RemediationMessages.noCallsAtThatLocation)); } + + @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 + FixCandidateSearchResults 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/headerinjection/DefaultHeaderInjectionRemediatorTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/headerinjection/DefaultHeaderInjectionRemediatorTest.java index 82f5acd56..ca2c5fb88 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/headerinjection/DefaultHeaderInjectionRemediatorTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/headerinjection/DefaultHeaderInjectionRemediatorTest.java @@ -46,7 +46,14 @@ void it_doesnt_fix_unfixable(final String unfixableCode, final int line, final S new HeaderInjectionFinding("header-injection", "SearchController.java", line); CodemodFileScanningResult result = remediator.remediateAll( - cu, "SearchController.java", rule, List.of(finding), f -> f.id, f -> line, f -> null); + cu, + "SearchController.java", + rule, + List.of(finding), + f -> f.id, + f -> line, + f -> null, + f -> null); assertThat(result.changes()).isEmpty(); assertThat(result.unfixedFindings()).hasSize(1); UnfixedFinding unfixedFinding = result.unfixedFindings().get(0); @@ -173,7 +180,14 @@ void it_fixes_header_injection( new HeaderInjectionFinding("header-injection", "SearchController.java", line); CodemodFileScanningResult result = remediator.remediateAll( - cu, "SearchController.java", rule, List.of(finding), f -> f.id, f -> f.line, f -> null); + cu, + "SearchController.java", + rule, + List.of(finding), + f -> f.id, + f -> f.line, + f -> null, + f -> null); assertThat(result.changes()).hasSize(1); CodemodChange change = result.changes().get(0); assertThat(change.lineNumber()).isEqualTo(line); diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/javadeserialization/DefaultJavaDeserializationRemediatorTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/javadeserialization/DefaultJavaDeserializationRemediatorTest.java index c5ee9b9ec..2d9a6e1c9 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/javadeserialization/DefaultJavaDeserializationRemediatorTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/javadeserialization/DefaultJavaDeserializationRemediatorTest.java @@ -14,10 +14,10 @@ import java.util.List; import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; final class DefaultJavaDeserializationRemediatorTest { @@ -64,7 +64,7 @@ void it_doesnt_handle_unfixables(final String badCode, final int line, final Str CodemodFileScanningResult result = remediator.remediateAll( - cu, "path", rule, List.of(new Object()), o -> "id", o -> line, o -> null); + cu, "path", rule, List.of(new Object()), o -> "id", o -> line, o -> null, o -> null); assertThat(result.unfixedFindings()).hasSize(1); assertThat(result.changes()).isEmpty(); UnfixedFinding unfixedFinding = result.unfixedFindings().get(0); @@ -74,35 +74,40 @@ void it_doesnt_handle_unfixables(final String badCode, final int line, final Str assertThat(unfixedFinding.getPath()).isEqualTo("path"); } - @Test - void it_fixes_java_deserialization() { - - String fixableCode = - """ - package com.acme; - import java.io.ObjectInputStream; - import java.io.InputStream; - - class Foo { - Acme readAcme(InputStream is) { - ObjectInputStream ois = new ObjectInputStream(is); - // read the obj - Acme acme = (Acme) ois.readObject(); - return acme; + private static final String fixableCode = + """ + package com.acme; + import java.io.ObjectInputStream; + import java.io.InputStream; + + class Foo { + Acme readAcme(InputStream is) { + ObjectInputStream ois = new ObjectInputStream(is); + // read the obj + Acme acme = (Acme) ois.readObject(); + return acme; + } } - } - """; + """; + + /** + * Confirm that whether the tool points to the readObject() or the constructor, they both fix + * well. + */ + @ParameterizedTest + @ValueSource(ints = {7, 9}) + void it_fixes_java_deserialization(final int line) { CompilationUnit cu = StaticJavaParser.parse(fixableCode); LexicalPreservingPrinter.setup(cu); CodemodFileScanningResult result = remediator.remediateAll( - cu, "path", rule, List.of(new Object()), o -> "id", o -> 9, o -> null); + cu, "path", rule, List.of(new Object()), o -> "id", o -> line, o -> null, o -> null); assertThat(result.unfixedFindings()).isEmpty(); assertThat(result.changes()).hasSize(1); CodemodChange change = result.changes().get(0); - assertThat(change.lineNumber()).isEqualTo(9); + assertThat(change.lineNumber()).isEqualTo(line); List fixedFindings = change.getFixedFindings(); assertThat(fixedFindings).hasSize(1); assertThat(change.getDependenciesNeeded()).containsExactly(DependencyGAV.JAVA_SECURITY_TOOLKIT); diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/jndiinjection/DefaultJNDIInjectionRemediatorTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/jndiinjection/DefaultJNDIInjectionRemediatorTest.java index c1fe7ca01..008f162ef 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/jndiinjection/DefaultJNDIInjectionRemediatorTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/jndiinjection/DefaultJNDIInjectionRemediatorTest.java @@ -47,6 +47,7 @@ void it_doesnt_fix_unfixable( List.of(issue), JNDIInjectionIssue::key, JNDIInjectionIssue::line, + i -> null, JNDIInjectionIssue::column); assertThat(result.changes()).isEmpty(); @@ -172,6 +173,7 @@ public static void lookup(HttpServletRequest request) { List.of(issue), JNDIInjectionIssue::key, JNDIInjectionIssue::line, + i -> null, JNDIInjectionIssue::column); assertThat(result.changes()).hasSize(1); @@ -236,6 +238,7 @@ private static void validateResourceName(final String name) { List.of(issue), JNDIInjectionIssue::key, JNDIInjectionIssue::line, + i -> null, JNDIInjectionIssue::column); assertThat(result.changes()).hasSize(1); diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/missingsecureflag/DefaultMissingSecureFlagRemediatorTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/missingsecureflag/DefaultMissingSecureFlagRemediatorTest.java new file mode 100644 index 000000000..c22613249 --- /dev/null +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/missingsecureflag/DefaultMissingSecureFlagRemediatorTest.java @@ -0,0 +1,145 @@ +package io.codemodder.remediation.missingsecureflag; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.github.javaparser.StaticJavaParser; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; +import io.codemodder.CodemodChange; +import io.codemodder.CodemodFileScanningResult; +import io.codemodder.codetf.DetectorRule; +import io.codemodder.codetf.FixedFinding; +import java.util.List; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +final class DefaultMissingSecureFlagRemediatorTest { + + private DefaultMissingSecureFlagRemediator remediator; + + @BeforeEach + void setup() { + remediator = new DefaultMissingSecureFlagRemediator(); + } + + @ParameterizedTest + @ValueSource( + strings = { + """ + package com.acme.web; + class MyServlet extends HttpServlet { + void doGet(HttpServletRequest request, HttpServletResponse response) { + Cookie cookie = new Cookie("name", "value"); + addCookie(cookie); // no scope + } + } + """, + """ + package com.acme.web; + class MyServlet extends HttpServlet { + void doGet(HttpServletRequest request, HttpServletResponse response) { + Cookie cookie = new Cookie("name", "value"); + response.addCookie(); // no args + } + } + """, + """ + package com.acme.web; + class MyServlet extends HttpServlet { + void doGet(HttpServletRequest request, HttpServletResponse response) { + Cookie cookie = new Cookie("name", "value"); + int a = 1; + response.addCookie(cookie); // wrong line + } + } + """ + }) + void it_does_not_add_secure_flag(final String javaCode) { + CompilationUnit cu = StaticJavaParser.parse(javaCode); + LexicalPreservingPrinter.setup(cu); + DetectorRule rule = new DetectorRule("insecure-cookie", "Add secure flag", null); + + CodemodFileScanningResult result = + remediator.remediateAll( + cu, + "MyServlet.java", + rule, + List.of(new Object()), + r -> "id-1", + r -> 5, + r -> null, + r -> null); + + assertThat(result.changes()).isEmpty(); + result + .unfixedFindings() + .forEach(unfixedFinding -> assertThat(unfixedFinding.getReason()).isNotEmpty()); + } + + /** + * This test should be able to pass with "5" as well, ideally. Unfortunately, this can cause + * confusion and result in a "double fix". + */ + @ParameterizedTest + @ValueSource(ints = {6}) + void it_adds_secure_flag(final int line) { + String javaCode = + """ + package com.acme.web; + + class MyServlet extends HttpServlet { + void doGet(HttpServletRequest request, HttpServletResponse response) { + Cookie cookie = new Cookie("name", "value"); + response.addCookie(cookie); + } + } + """; + + CompilationUnit cu = StaticJavaParser.parse(javaCode); + LexicalPreservingPrinter.setup(cu); + DetectorRule rule = new DetectorRule("insecure-cookie", "Add secure flag", null); + + CodemodFileScanningResult result = + remediator.remediateAll( + cu, + "MyServlet.java", + rule, + List.of(new Object()), + r -> "id-1", + r -> line, + r -> null, + r -> null); + + assertThat(result.unfixedFindings()).isEmpty(); + List changes = result.changes(); + assertThat(changes).hasSize(1); + CodemodChange change = changes.get(0); + + assertThat(change.getDependenciesNeeded()).isEmpty(); + assertThat(change.lineNumber()).isEqualTo(line); + List fixedFindings = change.getFixedFindings(); + assertThat(fixedFindings).hasSize(1); + + FixedFinding fixedFinding = fixedFindings.get(0); + assertThat(fixedFinding.getId()).isEqualTo("id-1"); + assertThat(fixedFinding.getRule()).isSameAs(rule); + + String actual = LexicalPreservingPrinter.print(cu); + + String fixedCode = + """ + package com.acme.web; + + class MyServlet extends HttpServlet { + void doGet(HttpServletRequest request, HttpServletResponse response) { + Cookie cookie = new Cookie("name", "value"); + cookie.setSecure(true); + response.addCookie(cookie); + } + } + """; + + assertThat(actual).isEqualToIgnoringWhitespace(fixedCode); + } +} diff --git a/framework/codemodder-base/src/test/java/io/codemodder/remediation/reflectioninjection/DefaultReflectionInjectionRemediatorTest.java b/framework/codemodder-base/src/test/java/io/codemodder/remediation/reflectioninjection/DefaultReflectionInjectionRemediatorTest.java index 977cc490f..db13038fe 100644 --- a/framework/codemodder-base/src/test/java/io/codemodder/remediation/reflectioninjection/DefaultReflectionInjectionRemediatorTest.java +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/reflectioninjection/DefaultReflectionInjectionRemediatorTest.java @@ -201,6 +201,7 @@ private void verifyFixable( List.of(finding), ReflectionInjectionFinding::key, ReflectionInjectionFinding::line, + f -> null, ReflectionInjectionFinding::column); assertThat(result.unfixedFindings()).isEmpty(); assertThat(result.changes()).hasSize(1); @@ -269,6 +270,7 @@ private void verifyUnfixable( List.of(finding), ReflectionInjectionFinding::key, ReflectionInjectionFinding::line, + f -> null, ReflectionInjectionFinding::column); assertThat(result.changes()).isEmpty(); assertThat(result.unfixedFindings()).hasSize(1); 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 cb91076cf..f0975d74a 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 @@ -48,7 +48,7 @@ Message read(String xml) { CodemodFileScanningResult result = remediator.remediateAll( - cu, "foo", rule, List.of(new Object()), f -> "my-id-1", f -> 9, f -> null); + cu, "foo", rule, List.of(new Object()), f -> "my-id-1", f -> 9, f -> null, f -> null); // confirm code is what's expected String actualCode = LexicalPreservingPrinter.print(cu); 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 new file mode 100644 index 000000000..3a224c275 --- /dev/null +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtNewDBFixerTest.java @@ -0,0 +1,77 @@ +package io.codemodder.remediation.xxe; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.github.javaparser.StaticJavaParser; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +final class DocumentBuilderFactoryAtNewDBFixerTest { + + private DocumentBuilderFactoryAtNewDBFixer fixer; + + @BeforeEach + void setup() { + this.fixer = new DocumentBuilderFactoryAtNewDBFixer(); + } + + @Test + void it_fixes_dbf_at_new_db_call() { + String vulnerableCode = + """ + public class MyCode { + public void foo() { + XMLReader parser = null; + DocumentBuilderFactory dbf = null; + StringReader sr = null; + boolean success; + try + { + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + DocumentBuilder db = dbf.newDocumentBuilder(); + db.parse(new InputSource(sr)); + success = true; + } catch (FileNotFoundException e){ + success = false; + logError(e); + } + } + } + """; + + CompilationUnit cu = StaticJavaParser.parse(vulnerableCode); + LexicalPreservingPrinter.setup(cu); + XXEFixAttempt attempt = fixer.tryFix(10, null, cu); + assertThat(attempt.isFixed()).isTrue(); + assertThat(attempt.isResponsibleFixer()).isTrue(); + + String fixedCode = + """ + public class MyCode { + public void foo() { + XMLReader parser = null; + DocumentBuilderFactory dbf = null; + StringReader sr = null; + boolean success; + try + { + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + dbf.setFeature("http://xml.org/sax/features/external-general-entities", false); + dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + DocumentBuilder db = dbf.newDocumentBuilder(); + db.parse(new InputSource(sr)); + success = true; + } catch (FileNotFoundException e){ + success = false; + logError(e); + } + } + } + """; + + String actualCode = LexicalPreservingPrinter.print(cu); + assertThat(actualCode).isEqualToIgnoringCase(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 new file mode 100644 index 000000000..4aa3096a4 --- /dev/null +++ b/framework/codemodder-base/src/test/java/io/codemodder/remediation/xxe/SAXParserAtNewSPFixerTest.java @@ -0,0 +1,69 @@ +package io.codemodder.remediation.xxe; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.github.javaparser.StaticJavaParser; +import com.github.javaparser.ast.CompilationUnit; +import com.github.javaparser.printer.lexicalpreservation.LexicalPreservingPrinter; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +final class SAXParserAtNewSPFixerTest { + + private SAXParserAtNewSPFixer fixer; + + @BeforeEach + void setup() { + this.fixer = new SAXParserAtNewSPFixer(); + } + + @Test + void it_fixes_sax_parser_at_new_sp_call() { + String vulnerableCode = + """ + public class MyCode { + public void foo() { + try + { + SAXParserFactory factory = getFactory(); + SAXParser parser = factory.newSAXParser(); + parser.parse(new InputSource(sr)); + success = true; + } catch (FileNotFoundException e){ + success = false; + logError(e); + } + } + } + """; + + CompilationUnit cu = StaticJavaParser.parse(vulnerableCode); + LexicalPreservingPrinter.setup(cu); + XXEFixAttempt attempt = fixer.tryFix(6, null, cu); + assertThat(attempt.isFixed()).isTrue(); + assertThat(attempt.isResponsibleFixer()).isTrue(); + + String fixedCode = + """ + public class MyCode { + public void foo() { + try + { + SAXParserFactory factory = getFactory(); + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + SAXParser parser = factory.newSAXParser(); + parser.parse(new InputSource(sr)); + success = true; + } catch (FileNotFoundException e){ + success = false; + logError(e); + } + } + } + """; + + String actualCode = LexicalPreservingPrinter.print(cu); + assertThat(actualCode).isEqualToIgnoringCase(fixedCode); + } +}