From c8a6995edc4b7d83f8e56124dbfde4f8875dbfba Mon Sep 17 00:00:00 2001 From: xingyutangyuan <147447743+xingyutangyuan@users.noreply.github.com> Date: Thu, 30 Nov 2023 19:00:33 -0800 Subject: [PATCH 1/6] fix bazel tests --- .../com/google/mu/safesql/SafeQueryTest.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) 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 index 015937b13f..7f51d41192 100644 --- a/mug-guava/src/test/java/com/google/mu/safesql/SafeQueryTest.java +++ b/mug-guava/src/test/java/com/google/mu/safesql/SafeQueryTest.java @@ -97,7 +97,7 @@ public void booleanPlaceholder() { } @Test - @SuppressWarnings("SafeSqlArgsCheck") + @SuppressWarnings("SafeQueryArgsCheck") public void charPlaceholder_notAllowed() { IllegalArgumentException thrown = assertThrows( @@ -307,7 +307,7 @@ public void stringWithSemicolonInside_cannotBeBackquoted() { } @Test - @SuppressWarnings("SafeSqlArgsCheck") + @SuppressWarnings("SafeQueryArgsCheck") public void safeQueryShouldNotBeSingleQuoted() { IllegalArgumentException thrown = assertThrows( @@ -317,7 +317,7 @@ public void safeQueryShouldNotBeSingleQuoted() { } @Test - @SuppressWarnings("SafeSqlArgsCheck") + @SuppressWarnings("SafeQueryArgsCheck") public void safeQueryShouldNotBeDoubleQuoted() { IllegalArgumentException thrown = assertThrows( @@ -339,7 +339,7 @@ public void safeQueryBackquoted() { } @Test - @SuppressWarnings("SafeSqlArgsCheck") + @SuppressWarnings("SafeQueryArgsCheck") public void subQueryDisallowed() { IllegalArgumentException thrown = assertThrows( @@ -348,7 +348,7 @@ public void subQueryDisallowed() { } @Test - @SuppressWarnings("SafeSqlArgsCheck") + @SuppressWarnings("SafeQueryArgsCheck") public void stringLiteralDisallowed() { IllegalArgumentException thrown = assertThrows( @@ -357,7 +357,7 @@ public void stringLiteralDisallowed() { } @Test - @SuppressWarnings("SafeSqlArgsCheck") + @SuppressWarnings("SafeQueryArgsCheck") public void backquoteAndSingleQuoteMixed() { IllegalArgumentException thrown = assertThrows( @@ -366,7 +366,7 @@ public void backquoteAndSingleQuoteMixed() { } @Test - @SuppressWarnings("SafeSqlArgsCheck") + @SuppressWarnings("SafeQueryArgsCheck") public void singleQuoteAndBackquoteMixed() { IllegalArgumentException thrown = assertThrows( @@ -466,7 +466,7 @@ public void trustedSqlStringBackquoted() { } @Test - @SuppressWarnings("SafeSqlArgsCheck") + @SuppressWarnings("SafeQueryArgsCheck") public void trustedSqlStringShouldNotBeSingleQuoted() { IllegalArgumentException thrown = assertThrows( @@ -480,7 +480,7 @@ public void trustedSqlStringShouldNotBeSingleQuoted() { } @Test - @SuppressWarnings("SafeSqlArgsCheck") + @SuppressWarnings("SafeQueryArgsCheck") public void trustedSqlStringShouldNotBeDoubleQuoted() { IllegalArgumentException thrown = assertThrows( From ff964886f70593ab1bf334e045e4972977de0873 Mon Sep 17 00:00:00 2001 From: xingyutangyuan <147447743+xingyutangyuan@users.noreply.github.com> Date: Thu, 30 Nov 2023 19:01:22 -0800 Subject: [PATCH 2/6] fix bazel tests --- .../src/test/java/com/google/mu/safesql/SafeQueryTest.java | 6 ++++++ 1 file changed, 6 insertions(+) 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 index 7f51d41192..9ab06600c4 100644 --- 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,12 @@ 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 singleQuoteNotEscapedWithinDoubleQuote() { assertThat(template("SELECT * FROM tbl WHERE id = \"{id}\"").with("'v'")) From c447bcc945220b6da97781e4d0cd8eae9c486cc1 Mon Sep 17 00:00:00 2001 From: xingyutangyuan <147447743+xingyutangyuan@users.noreply.github.com> Date: Thu, 30 Nov 2023 19:09:01 -0800 Subject: [PATCH 3/6] Escape newline for quoted strings in SafeQuery; Disallow newline for backtick-quoted identifiers. --- .../main/java/com/google/mu/safesql/SafeQuery.java | 5 +++-- .../java/com/google/mu/safesql/SafeQueryTest.java | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 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 index 34ccaf9c0f..fc8788c189 100644 --- a/mug-guava/src/main/java/com/google/mu/safesql/SafeQuery.java +++ b/mug-guava/src/main/java/com/google/mu/safesql/SafeQuery.java @@ -226,9 +226,10 @@ private static String quotedBy(char quoteChar, Substring.Match placeholder, Obje quoteChar, placeholder, quoteChar); - return first(CharPredicate.is('\\').or(quoteChar)) + String escaped = first(CharPredicate.is('\\').or(quoteChar)) .repeatedly() .replaceAllFrom(value.toString(), c -> "\\" + c); + return first('\n').repeatedly().replaceAllFrom(escaped, unused -> "\\n"); } private static String backquoted(Substring.Match placeholder, Object value) { @@ -238,7 +239,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("'\"`()[]{}\\~!@$^*,/?;\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 index 9ab06600c4..da7fa4fd05 100644 --- a/mug-guava/src/test/java/com/google/mu/safesql/SafeQueryTest.java +++ b/mug-guava/src/test/java/com/google/mu/safesql/SafeQueryTest.java @@ -71,6 +71,18 @@ public void newLineEscapedWithinSingleQuote() { .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 template = template("SELECT * FROM `{tbl}`"); + assertThrows(IllegalArgumentException.class, () -> template.with(/* tbl */ "a\nb")); + } + @Test public void singleQuoteNotEscapedWithinDoubleQuote() { assertThat(template("SELECT * FROM tbl WHERE id = \"{id}\"").with("'v'")) From b2e7fa8e4d96ad4d32ea4cb958e365bc11575b59 Mon Sep 17 00:00:00 2001 From: xingyutangyuan <147447743+xingyutangyuan@users.noreply.github.com> Date: Thu, 30 Nov 2023 19:43:54 -0800 Subject: [PATCH 4/6] escape new lines in one pass --- .../java/com/google/mu/safesql/SafeQuery.java | 40 +++++++++++++------ 1 file changed, 27 insertions(+), 13 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 index fc8788c189..e4ff644f17 100644 --- 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. * *

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) { * */ public static StringFormat.To 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 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,10 +232,18 @@ private static String quotedBy(char quoteChar, Substring.Match placeholder, Obje quoteChar, placeholder, quoteChar); - String escaped = first(CharPredicate.is('\\').or(quoteChar)) + return first(CharPredicate.is('\\').or(quoteChar).or('\n')) .repeatedly() - .replaceAllFrom(value.toString(), c -> "\\" + c); - return first('\n').repeatedly().replaceAllFrom(escaped, unused -> "\\n"); + .replaceAllFrom( + value.toString(), + c -> { + switch (c.charAt(0)) { + case '\n': + return "\\n"; + default: + return "\\" + c; + } + }); } private static String backquoted(Substring.Match placeholder, Object value) { From 529c74f3ec3c9cfc5fb1b6f7cfd8b1c326fdd2ab Mon Sep 17 00:00:00 2001 From: xingyutangyuan <147447743+xingyutangyuan@users.noreply.github.com> Date: Thu, 30 Nov 2023 19:44:44 -0800 Subject: [PATCH 5/6] disallow \r in identifiers --- mug-guava/src/main/java/com/google/mu/safesql/SafeQuery.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index e4ff644f17..73fbdde103 100644 --- a/mug-guava/src/main/java/com/google/mu/safesql/SafeQuery.java +++ b/mug-guava/src/main/java/com/google/mu/safesql/SafeQuery.java @@ -253,7 +253,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("'\"`()[]{}\\~!@$^*,/?;\n").matchesNoneOf(name), + CharMatcher.anyOf("'\"`()[]{}\\~!@$^*,/?;\r\n").matchesNoneOf(name), "placeholder value for `%s` (%s) contains illegal character", placeholder, name); From b30a5dd9a20308502e13916c25f487e643262c49 Mon Sep 17 00:00:00 2001 From: xingyutangyuan <147447743+xingyutangyuan@users.noreply.github.com> Date: Thu, 30 Nov 2023 21:23:00 -0800 Subject: [PATCH 6/6] Escape \r --- .../java/com/google/mu/safesql/SafeQuery.java | 9 ++++--- .../com/google/mu/safesql/SafeQueryTest.java | 24 +++++++++++++++++++ 2 files changed, 28 insertions(+), 5 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 index 73fbdde103..398501af13 100644 --- a/mug-guava/src/main/java/com/google/mu/safesql/SafeQuery.java +++ b/mug-guava/src/main/java/com/google/mu/safesql/SafeQuery.java @@ -232,16 +232,15 @@ private static String quotedBy(char quoteChar, Substring.Match placeholder, Obje quoteChar, placeholder, quoteChar); - return first(CharPredicate.is('\\').or(quoteChar).or('\n')) + return first(CharPredicate.is('\\').or(quoteChar).or('\n').or('\r')) .repeatedly() .replaceAllFrom( value.toString(), c -> { switch (c.charAt(0)) { - case '\n': - return "\\n"; - default: - return "\\" + c; + case '\r': return "\\r"; + case '\n': return "\\n"; + default: return "\\" + c; } }); } 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 index da7fa4fd05..4c08f8b96e 100644 --- a/mug-guava/src/test/java/com/google/mu/safesql/SafeQueryTest.java +++ b/mug-guava/src/test/java/com/google/mu/safesql/SafeQueryTest.java @@ -83,6 +83,30 @@ public void newLineDisallowedWithinBackticks() { 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 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'"))