Escape column names and paths in order by clause with backticks to pr…

…event potential hql injection
quarkusio · Feb 7, 2024 · f0a3d74 · f0a3d74
1 parent ddcb144
commit f0a3d74
Show file tree

Hide file tree

Showing 5 changed files with 120 additions and 6 deletions.
diff --git a/...src/test/java/io/quarkus/hibernate/orm/panache/deployment/test/JpaOperationsSortTest.java b/...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
@@ -100,6 +100,7 @@ public void setNullPrecedence(NullPrecedence nullPrecedence) {
     }
 
     private List<Column> 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<Column> getColumns() {
     public static Sort empty() {
         return by();
     }
+
+    public boolean isEscapingEnabled() {
+        return escapingEnabled;
+    }
 }
diff --git a/...mon/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java b/...mon/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/...gration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java b/...gration-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<Person> 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/...s/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java b/...s/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()