Merge pull request #52 from xingyutangyuan/master

Escape new lines for SafeQuery quoted strings; disallow new lines for SafeQuery identifiers
google · Dec 2, 2023 · 33ee99a · 33ee99a
2 parents adf8943 + b30a5dd
commit 33ee99a
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 22 deletions.
diff --git a/mug-guava/src/main/java/com/google/mu/safesql/SafeQuery.java b/mug-guava/src/main/java/com/google/mu/safesql/SafeQuery.java
@@ -23,9 +23,10 @@
 import com.google.mu.util.Substring;
 
 /**
- * Facade class to generate queries based on a string template in the syntax of {@link StringFormat},
- * with compile-time guard rails and runtime protection against SQL injection and programmer mistakes.
- * Special characters of string expressions are automatically escaped to prevent SQL injection errors.
+ * Facade class to generate queries based on a string template in the syntax of {@link
+ * StringFormat}, with compile-time guard rails and runtime protection against SQL injection and
+ * programmer mistakes. Special characters of string expressions are automatically escaped to
+ * prevent SQL injection errors.
  *
  * <p>A SafeQuery encapsulates the query string. You can use {@link #toString} to access the query
  * string.
@@ -39,9 +40,10 @@
  */
 @Immutable
 public final class SafeQuery {
-  private static final String TRUSTED_SQL_TYPE_NAME = firstNonNull(
-      System.getProperty("com.google.mu.safesql.SafeQuery.trusted_sql_type"),
-      "com.google.storage.googlesql.safesql.TrustedSqlString");
+  private static final String TRUSTED_SQL_TYPE_NAME =
+      firstNonNull(
+          System.getProperty("com.google.mu.safesql.SafeQuery.trusted_sql_type"),
+          "com.google.storage.googlesql.safesql.TrustedSqlString");
 
   private final String query;
 
@@ -119,9 +121,12 @@ public static SafeQuery of(@CompileTimeConstant String query) {
    * </ul>
    */
   public static StringFormat.To<SafeQuery> template(@CompileTimeConstant String formatString) {
-    return template(formatString, value -> {
-      throw new IllegalArgumentException("Unsupported argument type: " + value.getClass().getName());
-    });
+    return template(
+        formatString,
+        value -> {
+          throw new IllegalArgumentException(
+              "Unsupported argument type: " + value.getClass().getName());
+        });
   }
 
   static StringFormat.To<SafeQuery> template(
@@ -213,7 +218,8 @@ private static String unquoted(
         "Symbols should be wrapped inside %s;\n"
             + "subqueries must be wrapped in another SafeQuery object;\n"
             + "and string literals must be quoted like '%s'",
-            TRUSTED_SQL_TYPE_NAME, placeholder);
+        TRUSTED_SQL_TYPE_NAME,
+        placeholder);
     return byDefault.apply(value);
   }
 
@@ -226,9 +232,17 @@ private static String quotedBy(char quoteChar, Substring.Match placeholder, Obje
         quoteChar,
         placeholder,
         quoteChar);
-    return first(CharPredicate.is('\\').or(quoteChar))
+    return first(CharPredicate.is('\\').or(quoteChar).or('\n').or('\r'))
         .repeatedly()
-        .replaceAllFrom(value.toString(), c -> "\\" + c);
+        .replaceAllFrom(
+            value.toString(),
+            c -> {
+              switch (c.charAt(0)) {
+                case '\r': return "\\r";
+                case '\n': return "\\n";
+                default: return "\\" + c;
+              }
+            });
   }
 
   private static String backquoted(Substring.Match placeholder, Object value) {
@@ -238,7 +252,7 @@ private static String backquoted(Substring.Match placeholder, Object value) {
     String name = removeQuotes('`', value.toString(), '`'); // ok if already backquoted
     // Make sure the backquoted string doesn't contain some special chars that may cause trouble.
     checkArgument(
-        CharMatcher.anyOf("'\"`()[]{}\\~!@$^*,/?;").matchesNoneOf(name),
+        CharMatcher.anyOf("'\"`()[]{}\\~!@$^*,/?;\r\n").matchesNoneOf(name),
         "placeholder value for `%s` (%s) contains illegal character",
         placeholder,
         name);

diff --git a/mug-guava/src/test/java/com/google/mu/safesql/SafeQueryTest.java b/mug-guava/src/test/java/com/google/mu/safesql/SafeQueryTest.java
@@ -65,6 +65,48 @@ public void backslashEscapedWithinDoubleQuote() {
         .isEqualTo(SafeQuery.of("SELECT * FROM tbl WHERE id = \"\\\\1\""));
   }
 
+  @Test
+  public void newLineEscapedWithinSingleQuote() {
+    assertThat(template("SELECT * FROM tbl WHERE id = '{id}'").with("\n"))
+        .isEqualTo(SafeQuery.of("SELECT * FROM tbl WHERE id = '\\n'"));
+  }
+
+  @Test
+  public void newLineEscapedWithinDoubleQuote() {
+    assertThat(template("SELECT * FROM tbl WHERE id = \"{id}\"").with("\n"))
+        .isEqualTo(SafeQuery.of("SELECT * FROM tbl WHERE id = \"\\n\""));
+  }
+
+  @Test
+  public void newLineDisallowedWithinBackticks() {
+    StringFormat.To<SafeQuery> template = template("SELECT * FROM `{tbl}`");
+    assertThrows(IllegalArgumentException.class, () -> template.with(/* tbl */ "a\nb"));
+  }
+
+  @Test
+  public void carriageReturnEscapedWithinSingleQuote() {
+    assertThat(template("SELECT * FROM tbl WHERE id = '{id}'").with("\r"))
+        .isEqualTo(SafeQuery.of("SELECT * FROM tbl WHERE id = '\\r'"));
+  }
+
+  @Test
+  public void carriageReturnEscapedWithinDoubleQuote() {
+    assertThat(template("SELECT * FROM tbl WHERE id = \"{id}\"").with("\r"))
+        .isEqualTo(SafeQuery.of("SELECT * FROM tbl WHERE id = \"\\r\""));
+  }
+
+  @Test
+  public void carriageReturnDisallowedWithinBackticks() {
+    StringFormat.To<SafeQuery> template = template("SELECT * FROM `{tbl}`");
+    assertThrows(IllegalArgumentException.class, () -> template.with(/* tbl */ "a\rb"));
+  }
+
+  @Test
+  public void carriageReturnAndLineFeedEscapedWithinDoubleQuote() {
+    assertThat(template("SELECT * FROM tbl WHERE id = \"{id}\"").with("a\r\nb"))
+        .isEqualTo(SafeQuery.of("SELECT * FROM tbl WHERE id = \"a\\r\\nb\""));
+  }
+
   @Test
   public void singleQuoteNotEscapedWithinDoubleQuote() {
     assertThat(template("SELECT * FROM tbl WHERE id = \"{id}\"").with("'v'"))
@@ -97,7 +139,7 @@ public void booleanPlaceholder() {
   }
 
   @Test
-  @SuppressWarnings("SafeSqlArgsCheck")
+  @SuppressWarnings("SafeQueryArgsCheck")
   public void charPlaceholder_notAllowed() {
     IllegalArgumentException thrown =
         assertThrows(
@@ -307,7 +349,7 @@ public void stringWithSemicolonInside_cannotBeBackquoted() {
   }
 
   @Test
-  @SuppressWarnings("SafeSqlArgsCheck")
+  @SuppressWarnings("SafeQueryArgsCheck")
   public void safeQueryShouldNotBeSingleQuoted() {
     IllegalArgumentException thrown =
         assertThrows(
@@ -317,7 +359,7 @@ public void safeQueryShouldNotBeSingleQuoted() {
   }
 
   @Test
-  @SuppressWarnings("SafeSqlArgsCheck")
+  @SuppressWarnings("SafeQueryArgsCheck")
   public void safeQueryShouldNotBeDoubleQuoted() {
     IllegalArgumentException thrown =
         assertThrows(
@@ -339,7 +381,7 @@ public void safeQueryBackquoted() {
   }
 
   @Test
-  @SuppressWarnings("SafeSqlArgsCheck")
+  @SuppressWarnings("SafeQueryArgsCheck")
   public void subQueryDisallowed() {
     IllegalArgumentException thrown =
         assertThrows(
@@ -348,7 +390,7 @@ public void subQueryDisallowed() {
   }
 
   @Test
-  @SuppressWarnings("SafeSqlArgsCheck")
+  @SuppressWarnings("SafeQueryArgsCheck")
   public void stringLiteralDisallowed() {
     IllegalArgumentException thrown =
         assertThrows(
@@ -357,7 +399,7 @@ public void stringLiteralDisallowed() {
   }
 
   @Test
-  @SuppressWarnings("SafeSqlArgsCheck")
+  @SuppressWarnings("SafeQueryArgsCheck")
   public void backquoteAndSingleQuoteMixed() {
     IllegalArgumentException thrown =
         assertThrows(
@@ -366,7 +408,7 @@ public void backquoteAndSingleQuoteMixed() {
   }
 
   @Test
-  @SuppressWarnings("SafeSqlArgsCheck")
+  @SuppressWarnings("SafeQueryArgsCheck")
   public void singleQuoteAndBackquoteMixed() {
     IllegalArgumentException thrown =
         assertThrows(
@@ -466,7 +508,7 @@ public void trustedSqlStringBackquoted() {
   }
 
   @Test
-  @SuppressWarnings("SafeSqlArgsCheck")
+  @SuppressWarnings("SafeQueryArgsCheck")
   public void trustedSqlStringShouldNotBeSingleQuoted() {
     IllegalArgumentException thrown =
         assertThrows(
@@ -480,7 +522,7 @@ public void trustedSqlStringShouldNotBeSingleQuoted() {
   }
 
   @Test
-  @SuppressWarnings("SafeSqlArgsCheck")
+  @SuppressWarnings("SafeQueryArgsCheck")
   public void trustedSqlStringShouldNotBeDoubleQuoted() {
     IllegalArgumentException thrown =
         assertThrows(