From f0a3d74473a3cd5f45bdc55a40d57ce2f168cd70 Mon Sep 17 00:00:00 2001 From: Vitaliy Baschlykoff Date: Thu, 23 Nov 2023 07:45:33 +1100 Subject: [PATCH] Escape column names and paths in order by clause with backticks to prevent potential hql injection --- .../test/JpaOperationsSortTest.java | 46 +++++++++++++++++-- .../java/io/quarkus/panache/common/Sort.java | 15 ++++++ .../common/runtime/PanacheJpaUtil.java | 32 ++++++++++++- .../io/quarkus/it/panache/TestEndpoint.java | 28 +++++++++++ .../it/panache/PanacheFunctionalityTest.java | 5 ++ 5 files changed, 120 insertions(+), 6 deletions(-) diff --git a/extensions/panache/hibernate-orm-panache/deployment/src/test/java/io/quarkus/hibernate/orm/panache/deployment/test/JpaOperationsSortTest.java b/extensions/panache/hibernate-orm-panache/deployment/src/test/java/io/quarkus/hibernate/orm/panache/deployment/test/JpaOperationsSortTest.java index 30e13c7a45c5f..ff9e1f9f751ba 100644 --- a/extensions/panache/hibernate-orm-panache/deployment/src/test/java/io/quarkus/hibernate/orm/panache/deployment/test/JpaOperationsSortTest.java +++ b/extensions/panache/hibernate-orm-panache/deployment/src/test/java/io/quarkus/hibernate/orm/panache/deployment/test/JpaOperationsSortTest.java @@ -2,9 +2,11 @@ import static org.junit.jupiter.api.Assertions.assertEquals; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import io.quarkus.panache.common.Sort; +import io.quarkus.panache.common.exception.PanacheQueryException; import io.quarkus.panache.hibernate.common.runtime.PanacheJpaUtil; public class JpaOperationsSortTest { @@ -18,7 +20,7 @@ public void testEmptySortByYieldsEmptyString() { @Test public void testSortBy() { Sort sort = Sort.by("foo", "bar"); - assertEquals(" ORDER BY foo , bar", PanacheJpaUtil.toOrderBy(sort)); + assertEquals(" ORDER BY `foo` , `bar`", PanacheJpaUtil.toOrderBy(sort)); } @Test @@ -29,14 +31,48 @@ public void testEmptySortEmptyYieldsEmptyString() { @Test public void testSortByNullsFirst() { - Sort emptySort = Sort.by("foo", Sort.Direction.Ascending, Sort.NullPrecedence.NULLS_FIRST); - assertEquals(" ORDER BY foo NULLS FIRST", PanacheJpaUtil.toOrderBy(emptySort)); + Sort sort = Sort.by("foo", Sort.Direction.Ascending, Sort.NullPrecedence.NULLS_FIRST); + assertEquals(" ORDER BY `foo` NULLS FIRST", PanacheJpaUtil.toOrderBy(sort)); } @Test public void testSortByNullsLast() { - Sort emptySort = Sort.by("foo", Sort.Direction.Descending, Sort.NullPrecedence.NULLS_LAST); - assertEquals(" ORDER BY foo DESC NULLS LAST", PanacheJpaUtil.toOrderBy(emptySort)); + Sort sort = Sort.by("foo", Sort.Direction.Descending, Sort.NullPrecedence.NULLS_LAST); + assertEquals(" ORDER BY `foo` DESC NULLS LAST", PanacheJpaUtil.toOrderBy(sort)); } + @Test + public void testSortByColumnWithBacktick() { + Sort sort = Sort.by("jeanne", "d`arc"); + Assertions.assertThrowsExactly(PanacheQueryException.class, () -> PanacheJpaUtil.toOrderBy(sort), + "Sort column name cannot have backticks"); + } + + @Test + public void testSortByQuotedColumn() { + Sort sort = Sort.by("`foo`", "bar"); + assertEquals(" ORDER BY `foo` , `bar`", PanacheJpaUtil.toOrderBy(sort)); + } + + @Test + public void testSortByEmbeddedColumn() { + Sort sort = Sort.by("foo.bar"); + assertEquals(" ORDER BY `foo`.`bar`", PanacheJpaUtil.toOrderBy(sort)); + } + + @Test + public void testSortByQuotedEmbeddedColumn() { + Sort sort1 = Sort.by("foo.`bar`"); + assertEquals(" ORDER BY `foo`.`bar`", PanacheJpaUtil.toOrderBy(sort1)); + Sort sort2 = Sort.by("`foo`.bar"); + assertEquals(" ORDER BY `foo`.`bar`", PanacheJpaUtil.toOrderBy(sort2)); + Sort sort3 = Sort.by("`foo`.`bar`"); + assertEquals(" ORDER BY `foo`.`bar`", PanacheJpaUtil.toOrderBy(sort3)); + } + + @Test + public void testSortByDisabledEscaping() { + Sort sort1 = Sort.by("foo.`bar`").disableEscaping(); + assertEquals(" ORDER BY foo.`bar`", PanacheJpaUtil.toOrderBy(sort1)); + } } diff --git a/extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java b/extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java index d19ca3a7904d2..a503da211661c 100644 --- a/extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java +++ b/extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java @@ -100,6 +100,7 @@ public void setNullPrecedence(NullPrecedence nullPrecedence) { } private List columns = new ArrayList<>(); + private boolean escapingEnabled = true; private Sort() { } @@ -293,6 +294,16 @@ public Sort and(String name, Direction direction, NullPrecedence nullPrecedence) return this; } + /** + * Disables escaping of column names with a backticks during HQL Order By clause generation + * + * @return this instance, modified. + */ + public Sort disableEscaping() { + escapingEnabled = false; + return this; + } + /** * Get the sort columns * @@ -311,4 +322,8 @@ public List getColumns() { public static Sort empty() { return by(); } + + public boolean isEscapingEnabled() { + return escapingEnabled; + } } diff --git a/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java b/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java index 3029a33398242..5c265e0f892d4 100644 --- a/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java +++ b/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java @@ -175,7 +175,11 @@ public static String toOrderBy(Sort sort) { Sort.Column column = sort.getColumns().get(i); if (i > 0) sb.append(" , "); - sb.append(column.getName()); + if (sort.isEscapingEnabled()) { + sb.append(escapeColumnName(column.getName())); + } else { + sb.append(column.getName()); + } if (column.getDirection() != Sort.Direction.Ascending) { sb.append(" DESC"); } @@ -191,4 +195,30 @@ public static String toOrderBy(Sort sort) { } return sb.toString(); } + + private static StringBuilder escapeColumnName(String columnName) { + StringBuilder sb = new StringBuilder(); + String[] path = columnName.split("\\."); + for (int j = 0; j < path.length; j++) { + if (j > 0) + sb.append('.'); + sb.append('`').append(unquoteColumnName(path[j])).append('`'); + } + return sb; + } + + private static String unquoteColumnName(String columnName) { + String unquotedColumnName; + //Note HQL uses backticks to escape/quote special words that are used as identifiers + if (columnName.charAt(0) == '`' && columnName.charAt(columnName.length() - 1) == '`') { + unquotedColumnName = columnName.substring(1, columnName.length() - 1); + } else { + unquotedColumnName = columnName; + } + // Note we're not dealing with columns but with entity attributes so no backticks expected in unquoted column name + if (unquotedColumnName.indexOf('`') >= 0) { + throw new PanacheQueryException("Sort column name cannot have backticks"); + } + return unquotedColumnName; + } } diff --git a/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java b/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java index 26e58e1a32492..cc367d8d9e4fa 100644 --- a/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java +++ b/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java @@ -1741,6 +1741,34 @@ public String testSortByNullPrecedence() { return "OK"; } + @GET + @Path("testSortByEmbedded") + @Transactional + public String testSortByEmbedded() { + Person.deleteAll(); + + Person stefPerson = new Person(); + stefPerson.name = "Stef"; + stefPerson.description = new PersonDescription(); + stefPerson.description.size = 0; + stefPerson.persist(); + + Person josePerson = new Person(); + josePerson.name = "Jose"; + josePerson.description = new PersonDescription(); + josePerson.description.size = 100; + josePerson.persist(); + + List persons = Person.findAll(Sort.by("description.size", Sort.Direction.Descending)).list(); + assertEquals(josePerson.id, persons.get(0).id); + persons = Person.findAll(Sort.by("description.size", Sort.Direction.Ascending)).list(); + assertEquals(josePerson.id, persons.get(persons.size() - 1).id); + + Person.deleteAll(); + + return "OK"; + } + @GET @Path("testEnhancement27184DeleteDetached") // NOT @Transactional diff --git a/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java b/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java index 8ed3e24918c05..3625fce017ac3 100644 --- a/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java +++ b/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java @@ -90,6 +90,11 @@ public void testSortByNullPrecedence() { RestAssured.when().get("/test/testSortByNullPrecedence").then().body(is("OK")); } + @Test + public void testSortByEmbedded() { + RestAssured.when().get("/test/testSortByEmbedded").then().body(is("OK")); + } + @Test public void testJaxbAnnotationTransfer() { RestAssured.when()