-
Notifications
You must be signed in to change notification settings - Fork 65
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
da92ae4
commit a5b0650
Showing
4 changed files
with
343 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
98 changes: 98 additions & 0 deletions
98
mug-errorprone/src/main/java/com/google/mu/errorprone/SafeSqlArgsCheck.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| package com.google.mu.errorprone; | ||
|
|
||
|
|
||
| import static com.google.common.collect.ImmutableList.toImmutableList; | ||
| import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; | ||
| import static com.google.errorprone.matchers.Matchers.anyMethod; | ||
|
|
||
| import com.google.auto.service.AutoService; | ||
| import com.google.common.collect.ImmutableList; | ||
| import com.google.common.collect.ImmutableSet; | ||
| import com.google.mu.util.Substring; | ||
| import com.google.errorprone.BugPattern; | ||
| import com.google.errorprone.BugPattern.LinkType; | ||
| import com.google.errorprone.VisitorState; | ||
| import com.google.errorprone.bugpatterns.BugChecker; | ||
| import com.google.errorprone.matchers.method.MethodMatchers.MethodClassMatcher; | ||
| import com.google.errorprone.util.ASTHelpers; | ||
| import com.sun.source.tree.ExpressionTree; | ||
| import com.sun.source.tree.MethodInvocationTree; | ||
| import com.sun.tools.javac.code.Symbol.MethodSymbol; | ||
| import com.sun.tools.javac.code.Type; | ||
|
|
||
| /** | ||
| * Warns against potential SQL injection risks caused by unquoted string placeholders. This class | ||
| * mainly checks that all string placeholder values must correspond to a quoted placeholder such as | ||
| * {@code '{foo}'}; and unquoted placeholders should only accept trusted types (enums, numbers, | ||
| * booleans, TrustedSqlString etc.) | ||
| */ | ||
| @BugPattern( | ||
| summary = "Checks that string placeholders in SQL template strings are quoted.", | ||
| link = "go/java-tips/024#preventing-sql-injection", | ||
| linkType = LinkType.CUSTOM, | ||
| severity = WARNING) | ||
| @AutoService(BugChecker.class) | ||
| public final class SafeSqlArgsCheck extends AbstractBugChecker | ||
| implements AbstractBugChecker.MethodInvocationCheck { | ||
| private static final MethodClassMatcher MATCHER = | ||
| anyMethod().onDescendantOf("com.google.mu.util.StringFormat.To"); | ||
| private static final ImmutableSet<TypeName> ARG_TYPES_THAT_SHOULD_NOT_BE_QUOTED = | ||
| ImmutableSet.of( | ||
| new TypeName("com.google.storage.googlesql.safesql.TrustedSqlString"), | ||
| new TypeName("com.google.mu.safesql.SafeQuery"), | ||
| new TypeName("com.google.protobuf.Timestamp")); | ||
| private static final ImmutableSet<TypeName> ARG_TYPES_THAT_MUST_BE_QUOTED = | ||
| ImmutableSet.of( | ||
| TypeName.of(String.class), TypeName.of(Character.class), TypeName.of(char.class)); | ||
|
|
||
| @Override | ||
| public void checkMethodInvocation(MethodInvocationTree tree, VisitorState state) | ||
| throws ErrorReport { | ||
| if (!MATCHER.matches(tree, state)) { | ||
| return; | ||
| } | ||
| MethodSymbol symbol = ASTHelpers.getSymbol(tree); | ||
| if (!symbol.isVarArgs() || symbol.getParameters().size() != 1) { | ||
| return; | ||
| } | ||
| ExpressionTree formatter = ASTHelpers.getReceiver(tree); | ||
| String formatString = FormatStringUtils.findFormatString(formatter, state).orElse(null); | ||
| if (formatString == null || !FormatStringUtils.looksLikeSql(formatString)) { | ||
| return; | ||
| } | ||
| ImmutableList<Substring.Match> placeholders = | ||
| FormatStringUtils | ||
| .PLACEHOLDER_PATTERN | ||
| .repeatedly() | ||
| .match(formatString) | ||
| .collect(toImmutableList()); | ||
| if (placeholders.size() != tree.getArguments().size()) { | ||
| // Shouldn't happen. Will leave it to the other checks to report. | ||
| return; | ||
| } | ||
| for (int i = 0; i < placeholders.size(); i++) { | ||
| Substring.Match placeholder = placeholders.get(i); | ||
| ExpressionTree arg = tree.getArguments().get(i); | ||
| Type type = ASTHelpers.getType(arg); | ||
| if (placeholder.isImmediatelyBetween("'", "'") | ||
| || placeholder.isImmediatelyBetween("\"", "\"")) { | ||
| // It's a quoted string literal. Do not use sql query types | ||
| checkingOn(arg) | ||
| .require( | ||
| ARG_TYPES_THAT_SHOULD_NOT_BE_QUOTED.stream() | ||
| .noneMatch(t -> t.isSameType(type, state)), | ||
| "argument of type %s should not be quoted: '%s'", | ||
| type, | ||
| placeholder); | ||
| } else if (!placeholder.isImmediatelyBetween("`", "`")) { | ||
| // Disallow arbitrary string literals or characters unless backquoted. | ||
| checkingOn(arg) | ||
| .require( | ||
| ARG_TYPES_THAT_MUST_BE_QUOTED.stream().noneMatch(t -> t.isSameType(type, state)), | ||
| "argument of type %s must be quoted (for example '%s')", | ||
| type, | ||
| placeholder); | ||
| } | ||
| } | ||
| } | ||
| } |
220 changes: 220 additions & 0 deletions
220
mug-errorprone/src/test/java/com/google/mu/errorprone/SafeSqlArgsCheckTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,220 @@ | ||
| package com.google.mu.errorprone; | ||
|
|
||
| import com.google.errorprone.CompilationTestHelper; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.junit.runners.JUnit4; | ||
|
|
||
| @RunWith(JUnit4.class) | ||
| public final class SafeSqlArgsCheckTest { | ||
| private final CompilationTestHelper helper = | ||
| CompilationTestHelper.newInstance(SafeSqlArgsCheck.class, getClass()); | ||
|
|
||
| @Test | ||
| public void stringArgCanBeSingleQuoted() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "class Sql {", | ||
| " private static final StringFormat.To<Sql> SELECT =", | ||
| " StringFormat.to(Sql::new, \"SELECT '{v}' FROM tbl\");", | ||
| " Sql(String query) {} ", | ||
| " Sql test() {", | ||
| " return SELECT.with(\"value\");", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void stringArgCanBeDoubleQuoted() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "class Sql {", | ||
| " private static final StringFormat.To<Sql> SELECT =", | ||
| " StringFormat.to(Sql::new, \"SELECT \\\"{v}\\\" FROM tbl\");", | ||
| " Sql(String query) {} ", | ||
| " Sql test() {", | ||
| " return SELECT.with(\"value\");", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void charArgCanBeSingleQuoted() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "class Sql {", | ||
| " private static final StringFormat.To<Sql> SELECT =", | ||
| " StringFormat.to(Sql::new, \"SELECT '{v}' FROM tbl\");", | ||
| " Sql(String query) {} ", | ||
| " Sql test() {", | ||
| " return SELECT.with('v');", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void charArgCanBeDoubleQuoted() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "class Sql {", | ||
| " private static final StringFormat.To<Sql> SELECT =", | ||
| " StringFormat.to(Sql::new, \"SELECT \\\"{v}\\\" FROM tbl\");", | ||
| " Sql(String query) {} ", | ||
| " Sql test() {", | ||
| " return SELECT.with('v');", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void stringArgMustBeQuoted() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "class Sql {", | ||
| " private static final StringFormat.To<Sql> SELECT =", | ||
| " StringFormat.to(Sql::new, \"SELECT {c} FROM tbl\");", | ||
| " Sql(String query) {} ", | ||
| " Sql test() {", | ||
| " // BUG: Diagnostic contains: java.lang.String must be quoted", | ||
| " return SELECT.with(\"column\");", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void stringArgCanBeBackquoted() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "class Sql {", | ||
| " private static final StringFormat.To<Sql> SELECT =", | ||
| " StringFormat.to(Sql::new, \"SELECT `{c}` FROM tbl\");", | ||
| " Sql(String query) {} ", | ||
| " Sql test() {", | ||
| " return SELECT.with(\"column\");", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void charArgMustBeQuoted() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "class Sql {", | ||
| " private static final StringFormat.To<Sql> SELECT =", | ||
| " StringFormat.to(Sql::new, \"SELECT {c} FROM tbl\");", | ||
| " Sql(String query) {} ", | ||
| " Sql test() {", | ||
| " // BUG: Diagnostic contains: char must be quoted (for example '{c}')", | ||
| " return SELECT.with('x');", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void safeQueryCannotBeSingleQuoted() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "import com.google.mu.safesql.SafeQuery;", | ||
| "class Sql {", | ||
| " private static final StringFormat.To<Sql> UPDATE =", | ||
| " StringFormat.to(Sql::new, \"UPDATE '{c}' FROM tbl\");", | ||
| " Sql(String query) {} ", | ||
| " Sql test() {", | ||
| " // BUG: Diagnostic contains: SafeQuery should not be quoted: '{c}'", | ||
| " return UPDATE.with(SafeQuery.of(\"foo\"));", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void safeQueryNotQuoted() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "import com.google.mu.safesql.SafeQuery;", | ||
| "class Sql {", | ||
| " private static final StringFormat.To<Sql> SELECT =", | ||
| " StringFormat.to(Sql::new, \"SELECT {c} FROM tbl\");", | ||
| " Sql(String query) {} ", | ||
| " Sql test() {", | ||
| " return SELECT.with(SafeQuery.of(\"column\"));", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void safeQueryCannotBeDoubleQuoted() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "import com.google.mu.safesql.SafeQuery;", | ||
| "class Sql {", | ||
| " private static final StringFormat.To<Sql> UPDATE =", | ||
| " StringFormat.to(Sql::new, \"UPDATE \\\"{c}\\\" FROM tbl\");", | ||
| " Sql(String query) {} ", | ||
| " Sql test() {", | ||
| " // BUG: Diagnostic contains: SafeQuery should not be quoted: '{c}'", | ||
| " return UPDATE.with(SafeQuery.of(\"foo\"));", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void formatStringNotFound() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "class Sql {", | ||
| " Sql test(StringFormat.To<Sql> select) {", | ||
| " return select.with('x');", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void nonSql_notChecked() { | ||
| helper | ||
| .addSourceLines( | ||
| "Test.java", | ||
| "import com.google.mu.util.StringFormat;", | ||
| "class NotSql {", | ||
| " private static final StringFormat.To<NotSql> UPDATE =", | ||
| " StringFormat.to(NotSql::new, \"update {c}\");", | ||
| " NotSql(String query) {} ", | ||
| " NotSql test() {", | ||
| " return UPDATE.with('x');", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters