XWIKI-22718, XWIKI-22691: Improve query validation

Author: tmortagne

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/hibernate/query/HqlQueryExecutor.java

@@ -131,12 +131,23 @@ private Set<String> getAllowedNamedQueries()
      */
     protected static boolean isSafeSelect(String statementString)
     {
-        return HqlQueryUtils.isShortFormStatement(statementString) || HqlQueryUtils.isSafe(statementString);
+        // An empty statement is safe
+        if (statementString.isEmpty()) {
+            return true;
+        }
+
+        // HqlQueryUtils#isSafe only works with complete statements
+        String completeStatement = toCompleteShortForm(statementString);
+
+        // Parse and validate the statement
+        return HqlQueryUtils.isSafe(completeStatement);
     }
 
     protected void checkAllowed(final Query query) throws QueryException
     {
-        if (query instanceof SecureQuery && ((SecureQuery) query).isCurrentAuthorChecked()) {
+        // Check if the query needs to be validated according to the current author
+        if (query instanceof SecureQuery secureQuery && secureQuery.isCurrentAuthorChecked()) {
+            // Not need to check the details if current author has programming right
             if (!this.authorization.hasAccess(Right.PROGRAM)) {
                 if (query.isNamed() && !getAllowedNamedQueries().contains(query.getStatement())) {
                     throw new QueryException("Named queries requires programming right", query, null);
@@ -152,15 +163,15 @@ protected void checkAllowed(final Query query) throws QueryException
     @Override
     public <T> List<T> execute(final Query query) throws QueryException
     {
-        // Make sure the query is allowed in the current context
-        checkAllowed(query);
-
         String oldDatabase = getContext().getWikiId();
         try {
             if (query.getWiki() != null) {
                 getContext().setWikiId(query.getWiki());
             }
 
+            // Make sure the query is allowed. Make sure to do it in the target context.
+            checkAllowed(query);
+
             // Filter the query
             Query filteredQuery = filterQuery(query);
 
@@ -333,13 +344,18 @@ private boolean hasQueryParametersType(Query query)
      */
     protected String completeShortFormStatement(String statement)
     {
-        String lcStatement = statement.toLowerCase().trim();
-        if (lcStatement.isEmpty() || lcStatement.startsWith(",") || lcStatement.startsWith("where ")
-            || lcStatement.startsWith("order by ")) {
-            return "select doc.fullName from XWikiDocument doc " + statement.trim();
+        return toCompleteShortForm(statement);
+    }
+
+    private static String toCompleteShortForm(String statement)
+    {
+        String filteredStatement = statement;
+
+        if (statement.isEmpty() || HqlQueryUtils.isShortFormStatement(statement)) {
+            filteredStatement = "select doc.fullName from XWikiDocument doc " + statement.trim();
         }
 
-        return statement;
+        return filteredStatement;
     }
 
     private <T> org.hibernate.query.Query<T> createNamedHibernateQuery(Session session, Query query)

xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/internal/store/hibernate/query/HqlQueryUtilsTest.java

@@ -63,6 +63,8 @@ public void isSafe()
             .isSafe("select doc.name, ot.field from XWikiDocument doc, XWikiSpace space, OtherTable as ot"));
         assertFalse(HqlQueryUtils.isSafe("select count(*) from OtherTable"));
         assertFalse(HqlQueryUtils.isSafe("select count(other.*) from OtherTable other"));
+        assertFalse(HqlQueryUtils.isSafe("select doc.fullName from XWikiDocument doc union all select name from OtherTable"));
+        assertFalse(HqlQueryUtils.isSafe("select doc.fullName from XWikiDocument doc where 1<>'1\\'' union select name from OtherTable #'"));
     }
 
     @Test

xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/store/hibernate/query/HqlQueryExecutorTest.java

@@ -220,7 +220,7 @@ public void setNamedParameterArray()
     {
         org.hibernate.query.Query query = mock(org.hibernate.query.Query.class);
         String name = "bar";
-        Integer[] value = new Integer[] { 1, 2, 3 };
+        Integer[] value = new Integer[] {1, 2, 3};
         this.executor.setNamedParameter(query, name, value);
 
         verify(query).setParameterList(name, value);
@@ -403,7 +403,7 @@ public void executeWhenNotAllowedSelect() throws Exception
             assertEquals(
                 "The query requires programming right."
                     + " Query statement = [select notallowed.name from NotAllowedTable notallowed]",
-                expected.getMessage());
+                expected.getCause().getMessage());
         }
     }
 
@@ -415,7 +415,7 @@ public void executeDeleteWithoutProgrammingRight() throws Exception
             fail("Should have thrown an exception here");
         } catch (QueryException expected) {
             assertEquals("The query requires programming right. Query statement = [delete from XWikiDocument as doc]",
-                expected.getMessage());
+                expected.getCause().getMessage());
         }
     }
 
@@ -426,7 +426,8 @@ public void executeNamedQueryWithoutProgrammingRight() throws Exception
             executeNamed("somename", false);
             fail("Should have thrown an exception here");
         } catch (QueryException expected) {
-            assertEquals("Named queries requires programming right. Named query = [somename]", expected.getMessage());
+            assertEquals("Named queries requires programming right. Named query = [somename]",
+                expected.getCause().getMessage());
         }
     }
 
@@ -439,7 +440,7 @@ public void executeUpdateWithoutProgrammingRight() throws Exception
         } catch (QueryException expected) {
             assertEquals(
                 "The query requires programming right. Query statement = [update XWikiDocument set name='name']",
-                expected.getMessage());
+                expected.getCause().getMessage());
         }
     }
 }

xwiki-platform-core/xwiki-platform-rest/xwiki-platform-rest-server/src/main/java/org/xwiki/rest/internal/resources/search/AbstractDatabaseSearchSource.java

@@ -63,6 +63,7 @@ public abstract class AbstractDatabaseSearchSource extends AbstractSearchSource
     protected Provider<XWikiContext> xcontextProvider;
 
     @Inject
+    @Named("secure")
     protected QueryManager queryManager;
 
     @Inject