Skip to content

Commit f99775e

Browse files
andrecsilvaandrecsilva
authored
Extra cleanup for SQLParameterizer (#329)
Adds: - A new transformation for replacing concatenations of string literals with a single literal. - The aforementioned transformation as a cleanup step for SQLParameterizer. - A new transformation that combines the SQLParamterizer with the extra cleanup steps. Closes #300 and #327.
1 parent f6a5cc2 commit f99775e

File tree

8 files changed

+202
-45
lines changed

8 files changed

+202
-45
lines changed

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

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

33
import com.github.javaparser.ast.CompilationUnit;
4-
import com.github.javaparser.ast.body.CallableDeclaration;
54
import com.github.javaparser.ast.expr.MethodCallExpr;
65
import io.codemodder.*;
7-
import io.codemodder.ast.ASTTransforms;
86
import io.codemodder.codetf.DetectionTool;
97
import io.codemodder.codetf.DetectorFinding;
108
import io.codemodder.codetf.DetectorRule;
@@ -91,16 +89,8 @@ public CodemodFileScanningResult visit(
9189
}
9290

9391
MethodCallExpr methodCallExpr = supportedSqlMethodCallsOnThatLine.get(0);
94-
SQLParameterizer parameterizer = new SQLParameterizer(methodCallExpr);
95-
96-
if (parameterizer.checkAndFix()) {
97-
final var maybeMethodDecl = methodCallExpr.findAncestor(CallableDeclaration.class);
98-
// Cleanup, removes empty string concatenations and unused variables
99-
maybeMethodDecl.ifPresent(cd -> ASTTransforms.removeEmptyStringConcatenation(cd));
100-
// TODO hits a bug with javaparser, where adding nodes won't result in the correct children
101-
// order. This causes the following to remove actually used variables
102-
// maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md));
10392

93+
if (SQLParameterizerWithCleanup.checkAndFix(methodCallExpr)) {
10494
DetectorFinding fixedFinding = new DetectorFinding(id, true, null);
10595
allFindings.add(fixedFinding);
10696
changes.add(CodemodChange.from(line, "Fixes issue " + id + " by parameterizing SQL"));

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

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
* Contains most of the logic for detecting and fixing parameterizable SQL statements for a given
2222
* {@link MethodCallExpr}.
2323
*/
24-
public final class SQLParameterizer {
24+
final class SQLParameterizer {
2525

2626
private static final String preparedStatementNamePrefix = "stmt";
2727
private static final String preparedStatementNamePrefixAlternative = "statement";
@@ -382,7 +382,7 @@ private Expression buildParameter(
382382
*
383383
* <p>(3) Change <stmtCreation>.execute*() to pstmt.execute().
384384
*/
385-
private void fix(
385+
private MethodCallExpr fix(
386386
final Either<MethodCallExpr, LocalVariableDeclaration> stmtCreation,
387387
final QueryParameterizer queryParameterizer,
388388
final MethodCallExpr executeCall) {
@@ -448,16 +448,16 @@ private void fix(
448448
executeCall.setScope(new NameExpr(stmtName));
449449
executeCall.setArguments(new NodeList<>());
450450

451+
MethodCallExpr pstmtCreation;
451452
// (2.a)
452453
if (stmtCreation.isLeft()) {
454+
pstmtCreation =
455+
new MethodCallExpr(stmtCreation.getLeft().getScope().get(), "prepareStatement", args);
453456
final var pstmtCreationStmt =
454457
new ExpressionStmt(
455458
new VariableDeclarationExpr(
456459
new VariableDeclarator(
457-
StaticJavaParser.parseType("PreparedStatement"),
458-
stmtName,
459-
new MethodCallExpr(
460-
stmtCreation.getLeft().getScope().get(), "prepareStatement", args))));
460+
StaticJavaParser.parseType("PreparedStatement"), stmtName, pstmtCreation)));
461461
ASTTransforms.addStatementBeforeStatement(topStatement, pstmtCreationStmt);
462462

463463
// (2.b)
@@ -476,7 +476,10 @@ private void fix(
476476
.getVariableDeclarator()
477477
.getInitializer()
478478
.ifPresent(expr -> expr.asMethodCallExpr().setArguments(args));
479+
pstmtCreation =
480+
stmtCreation.getRight().getVariableDeclarator().getInitializer().get().asMethodCallExpr();
479481
}
482+
return pstmtCreation;
480483
}
481484

482485
private boolean assignedOrDefinedInScope(NameExpr name, LocalVariableDeclaration lvd) {
@@ -500,13 +503,13 @@ private boolean assignedOrDefinedInScope(NameExpr name, LocalVariableDeclaration
500503

501504
/**
502505
* Checks if {@code methodCall} is a query call that needs to be fixed and fixes if that's the
503-
* case.
506+
* case. If the parameterization happened, returns the PreparedStatement creation.
504507
*/
505-
public boolean checkAndFix() {
508+
public Optional<MethodCallExpr> checkAndFix() {
506509
if (executeCall.findCompilationUnit().isPresent()) {
507510
this.compilationUnit = executeCall.findCompilationUnit().get();
508511
} else {
509-
return false;
512+
return Optional.empty();
510513
}
511514
// validate the call itself first
512515
if (isParameterizationCandidate(executeCall) && validateExecuteCall(executeCall).isPresent()) {
@@ -518,7 +521,7 @@ public boolean checkAndFix() {
518521
final QueryParameterizer queryp;
519522
// should not be emtpy
520523
if (executeCall.getArguments().isEmpty()) {
521-
return false;
524+
return Optional.empty();
522525
}
523526
queryp = new QueryParameterizer(executeCall.getArgument(0));
524527

@@ -546,13 +549,12 @@ public boolean checkAndFix() {
546549
.anyMatch(name -> assignedOrDefinedInScope(name, stmtLVD)));
547550

548551
if (queryp.getInjections().isEmpty() || resolvedInScope || nameInScope) {
549-
return false;
552+
return Optional.empty();
550553
}
551554

552-
fix(stmtObject.get(), queryp, executeCall);
553-
return true;
555+
return Optional.of(fix(stmtObject.get(), queryp, executeCall));
554556
}
555557
}
556-
return false;
558+
return Optional.empty();
557559
}
558560
}

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

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,8 @@
11
package io.codemodder.codemods;
22

33
import com.github.javaparser.ast.CompilationUnit;
4-
import com.github.javaparser.ast.body.CallableDeclaration;
54
import com.github.javaparser.ast.expr.MethodCallExpr;
65
import io.codemodder.*;
7-
import io.codemodder.ast.ASTTransforms;
86
import io.codemodder.javaparser.JavaParserChanger;
97
import java.util.List;
108
import java.util.Optional;
@@ -18,13 +16,7 @@
1816
public final class SQLParameterizerCodemod extends JavaParserChanger {
1917

2018
private Optional<CodemodChange> onNodeFound(final MethodCallExpr methodCallExpr) {
21-
if (new SQLParameterizer(methodCallExpr).checkAndFix()) {
22-
var maybeMethodDecl = methodCallExpr.findAncestor(CallableDeclaration.class);
23-
// Cleanup, removes empty string concatenations and unused variables
24-
maybeMethodDecl.ifPresent(cd -> ASTTransforms.removeEmptyStringConcatenation(cd));
25-
// TODO hits a bug with javaparser, where adding nodes won't result in the correct children
26-
// order. This causes the following to remove actually used variables
27-
// maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md));
19+
if (SQLParameterizerWithCleanup.checkAndFix(methodCallExpr)) {
2820
return Optional.of(CodemodChange.from(methodCallExpr.getBegin().get().line));
2921
} else {
3022
return Optional.empty();

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
package io.codemodder.codemods;
2+
3+
import com.github.javaparser.ast.body.CallableDeclaration;
4+
import com.github.javaparser.ast.expr.MethodCallExpr;
5+
import io.codemodder.ast.ASTTransforms;
6+
7+
public final class SQLParameterizerWithCleanup {
8+
9+
private SQLParameterizerWithCleanup() {}
10+
11+
public static boolean checkAndFix(final MethodCallExpr methodCallExpr) {
12+
var maybeFixed = new SQLParameterizer(methodCallExpr).checkAndFix();
13+
if (maybeFixed.isPresent()) {
14+
// Cleanup
15+
var maybeMethodDecl = methodCallExpr.findAncestor(CallableDeclaration.class);
16+
// Remove concatenation with empty strings e.g "first" + "" -> "first";
17+
maybeMethodDecl.ifPresent(ASTTransforms::removeEmptyStringConcatenation);
18+
// TODO hits a bug with javaparser, where adding nodes won't result in the correct children
19+
// order. This causes the following to remove actually used variables
20+
// maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md));
21+
22+
// Merge concatenated literals, e.g. "first" + " and second" -> "first and second"
23+
maybeFixed
24+
.flatMap(mce -> mce.getArguments().getFirst())
25+
.ifPresent(ASTTransforms::mergeConcatenatedLiterals);
26+
return true;
27+
}
28+
return false;
29+
}
30+
}

‎core-codemods/src/test/resources/defectdojo-sql-injection/SqlInjectionLesson8/SqlInjectionLesson8.java.after‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,7 @@ public class SqlInjectionLesson8 extends AssignmentEndpoint {
6464
protected AttackResult injectableQueryConfidentiality(String name, String auth_tan) {
6565
StringBuilder output = new StringBuilder();
6666
String query =
67-
"SELECT * FROM employees WHERE last_name = ?"
68-
+ " AND auth_tan = ?";
67+
"SELECT * FROM employees WHERE last_name = ? AND auth_tan = ?";
6968

7069
try (Connection connection = dataSource.getConnection()) {
7170
try {
@@ -151,7 +150,7 @@ query, ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_UPDAT
151150
String time = sdf.format(cal.getTime());
152151

153152
String logQuery =
154-
"INSERT INTO access_log (time, action) VALUES (?" + ", ?" + ")";
153+
"INSERT INTO access_log (time, action) VALUES (?, ?)";
155154

156155
try {
157156
PreparedStatement statement = connection.prepareStatement(logQuery, TYPE_SCROLL_SENSITIVE, CONCUR_UPDATABLE);

‎core-codemods/src/test/resources/sql-parameterizer/Test.java.after‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ public final class Test {
5555
}
5656

5757
public ResultSet stringAfterQuote(String input, String input2) throws SQLException {
58-
String sql = "SELECT * FROM USERS WHERE USER = ?" + " AND PHONE=?";
58+
String sql = "SELECT * FROM USERS WHERE USER = ? AND PHONE=?";
5959
PreparedStatement stmt = conn.prepareStatement(sql);
6060
stmt.setString(1, "user_" + input + "_name");
6161
stmt.setString(2, input2);

‎framework/codemodder-base/src/main/java/io/codemodder/ast/ASTTransforms.java‎

Lines changed: 79 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,10 @@
66
import com.github.javaparser.ast.NodeList;
77
import com.github.javaparser.ast.body.VariableDeclarator;
88
import com.github.javaparser.ast.expr.BinaryExpr;
9+
import com.github.javaparser.ast.expr.EnclosedExpr;
910
import com.github.javaparser.ast.expr.Expression;
1011
import com.github.javaparser.ast.expr.LambdaExpr;
12+
import com.github.javaparser.ast.expr.NameExpr;
1113
import com.github.javaparser.ast.expr.StringLiteralExpr;
1214
import com.github.javaparser.ast.expr.VariableDeclarationExpr;
1315
import com.github.javaparser.ast.nodeTypes.NodeWithBody;
@@ -19,6 +21,7 @@
1921
import com.github.javaparser.ast.stmt.Statement;
2022
import com.github.javaparser.ast.stmt.TryStmt;
2123
import com.github.javaparser.ast.type.ClassOrInterfaceType;
24+
import com.github.javaparser.resolution.types.ResolvedType;
2225
import java.util.Optional;
2326

2427
public final class ASTTransforms {
@@ -219,10 +222,7 @@ public static void removeImportIfUnused(final CompilationUnit cu, final String c
219222
private static boolean isEmptyString(final Expression expr) {
220223
// TODO declared as empty with one assignment
221224
var resolved = ASTs.resolveLocalExpression(expr);
222-
if (resolved.isStringLiteralExpr() && resolved.asStringLiteralExpr().getValue().equals("")) {
223-
return true;
224-
}
225-
return false;
225+
return resolved.isStringLiteralExpr() && resolved.asStringLiteralExpr().getValue().isEmpty();
226226
}
227227

228228
/**
@@ -253,7 +253,8 @@ public static Expression removeEmptyStringConcatenation(final BinaryExpr binexp)
253253

254254
/** Removes all concatenations with empty strings in the given subtree. */
255255
public static void removeEmptyStringConcatenation(Node subtree) {
256-
subtree.findAll(BinaryExpr.class, Node.TreeTraversal.POSTORDER).stream()
256+
subtree
257+
.findAll(BinaryExpr.class, Node.TreeTraversal.POSTORDER)
257258
.forEach(binexp -> binexp.replace(removeEmptyStringConcatenation(binexp)));
258259
}
259260

@@ -268,7 +269,7 @@ public static void removeUnusedLocalVariables(final Node subtree) {
268269
var lvd = maybelvd.get();
269270
var allReferences = ASTs.findAllReferences(lvd);
270271
// No references?
271-
if (allReferences.size() == 0) {
272+
if (allReferences.isEmpty()) {
272273
maybelvd.get().getStatement().remove();
273274
}
274275

@@ -279,7 +280,7 @@ public static void removeUnusedLocalVariables(final Node subtree) {
279280
if (allAssignments.size() == 1) {
280281
var aexprStmt =
281282
Optional.of(allAssignments.get(0))
282-
.flatMap(ae -> ae.getParentNode())
283+
.flatMap(Node::getParentNode)
283284
.map(p -> p instanceof ExpressionStmt ? (ExpressionStmt) p : null);
284285
if (aexprStmt.isPresent()) {
285286
aexprStmt.get().remove();
@@ -291,4 +292,75 @@ public static void removeUnusedLocalVariables(final Node subtree) {
291292
}
292293
}
293294
}
295+
296+
private static Optional<StringLiteralExpr> removeAndReturnRightmostExpression(
297+
final BinaryExpr binExpr) {
298+
if (binExpr.getRight().isStringLiteralExpr()) {
299+
var right = binExpr.asBinaryExpr().getRight().asStringLiteralExpr();
300+
binExpr.replace(binExpr.getLeft());
301+
return Optional.of(right);
302+
}
303+
if (binExpr.isStringLiteralExpr()) {
304+
return Optional.of(binExpr.asStringLiteralExpr());
305+
}
306+
return Optional.empty();
307+
}
308+
309+
/**
310+
* Given a string expression, merge any literals that are directly concatenated. This transform
311+
* will recurse over any Names referenced.
312+
*/
313+
public static void mergeConcatenatedLiterals(final Expression e) {
314+
// EnclosedExpr and BinaryExpr are considered as internal nodes, so we recurse
315+
if (e instanceof EnclosedExpr) {
316+
if (calculateResolvedType(e)
317+
.filter(rt -> rt.describe().equals("java.lang.String"))
318+
.isPresent()) {
319+
mergeConcatenatedLiterals(e.asEnclosedExpr().getInner());
320+
}
321+
}
322+
// Only BinaryExpr between strings should be considered
323+
else if (e instanceof BinaryExpr
324+
&& e.asBinaryExpr().getOperator().equals(BinaryExpr.Operator.PLUS)) {
325+
mergeConcatenatedLiterals(e.asBinaryExpr().getLeft());
326+
mergeConcatenatedLiterals(e.asBinaryExpr().getRight());
327+
var left = e.asBinaryExpr().getLeft();
328+
var right = e.asBinaryExpr().getRight();
329+
330+
if (right.isStringLiteralExpr()) {
331+
if (left.isStringLiteralExpr()) {
332+
e.replace(
333+
new StringLiteralExpr(
334+
left.asStringLiteralExpr().getValue() + right.asStringLiteralExpr().getValue()));
335+
}
336+
if (left.isBinaryExpr()) {
337+
var maybeLiteral = removeAndReturnRightmostExpression(left.asBinaryExpr());
338+
maybeLiteral.ifPresent(
339+
sl ->
340+
right.replace(
341+
new StringLiteralExpr(
342+
sl.getValue() + right.asStringLiteralExpr().getValue())));
343+
}
344+
}
345+
346+
}
347+
// NameExpr of String types should be recursively searched for more expressions.
348+
else if (e instanceof NameExpr
349+
&& calculateResolvedType(e)
350+
.filter(rt -> rt.describe().equals("java.lang.String"))
351+
.isPresent()) {
352+
final var resolved = ASTs.resolveLocalExpression(e);
353+
if (resolved != e) {
354+
mergeConcatenatedLiterals(resolved);
355+
}
356+
}
357+
}
358+
359+
private static Optional<ResolvedType> calculateResolvedType(final Expression e) {
360+
try {
361+
return Optional.of(e.calculateResolvedType());
362+
} catch (final RuntimeException exception) {
363+
return Optional.empty();
364+
}
365+
}
294366
}

0 commit comments

Comments
 (0)