From 5c11a874bd24a581f534d283186e209bbccd8113 Mon Sep 17 00:00:00 2001 From: Thomas Mortagne Date: Fri, 6 Dec 2024 18:28:07 +0100 Subject: [PATCH] XWIKI-22718, XWIKI-22691: Improve query validation --- .../hibernate/query/HqlQueryExecutor.java | 36 +++++++++++++------ .../hibernate/query/HqlQueryUtilsTest.java | 2 ++ .../hibernate/query/HqlQueryExecutorTest.java | 11 +++--- .../search/AbstractDatabaseSearchSource.java | 1 + 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/hibernate/query/HqlQueryExecutor.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/hibernate/query/HqlQueryExecutor.java index b769c0522ea5..e1a9a7a40e97 100644 --- a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/hibernate/query/HqlQueryExecutor.java +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/store/hibernate/query/HqlQueryExecutor.java @@ -131,12 +131,23 @@ private Set 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 List 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 org.hibernate.query.Query createNamedHibernateQuery(Session session, Query query) diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/internal/store/hibernate/query/HqlQueryUtilsTest.java b/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/internal/store/hibernate/query/HqlQueryUtilsTest.java index 750ddb1ed321..0e7c5228e009 100644 --- a/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/internal/store/hibernate/query/HqlQueryUtilsTest.java +++ b/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 diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/store/hibernate/query/HqlQueryExecutorTest.java b/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/store/hibernate/query/HqlQueryExecutorTest.java index 088c67b47f2e..e7ab62b32e8c 100644 --- a/xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/store/hibernate/query/HqlQueryExecutorTest.java +++ b/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()); } } } diff --git a/xwiki-platform-core/xwiki-platform-rest/xwiki-platform-rest-server/src/main/java/org/xwiki/rest/internal/resources/search/AbstractDatabaseSearchSource.java b/xwiki-platform-core/xwiki-platform-rest/xwiki-platform-rest-server/src/main/java/org/xwiki/rest/internal/resources/search/AbstractDatabaseSearchSource.java index bad59c5b5c88..447c9dfc6fbf 100644 --- a/xwiki-platform-core/xwiki-platform-rest/xwiki-platform-rest-server/src/main/java/org/xwiki/rest/internal/resources/search/AbstractDatabaseSearchSource.java +++ b/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 xcontextProvider; @Inject + @Named("secure") protected QueryManager queryManager; @Inject