Skip to content

Commit 5c11a87

Browse files
tmortagnetmortagne
committed
XWIKI-22718, XWIKI-22691: Improve query validation
1 parent 06c503c commit 5c11a87

File tree

4 files changed

+35
-15
lines changed

4 files changed

+35
-15
lines changed

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

+26-10
Original file line numberDiff line numberDiff line change
@@ -131,12 +131,23 @@ private Set<String> getAllowedNamedQueries()
131131
*/
132132
protected static boolean isSafeSelect(String statementString)
133133
{
134-
return HqlQueryUtils.isShortFormStatement(statementString) || HqlQueryUtils.isSafe(statementString);
134+
// An empty statement is safe
135+
if (statementString.isEmpty()) {
136+
return true;
137+
}
138+
139+
// HqlQueryUtils#isSafe only works with complete statements
140+
String completeStatement = toCompleteShortForm(statementString);
141+
142+
// Parse and validate the statement
143+
return HqlQueryUtils.isSafe(completeStatement);
135144
}
136145

137146
protected void checkAllowed(final Query query) throws QueryException
138147
{
139-
if (query instanceof SecureQuery && ((SecureQuery) query).isCurrentAuthorChecked()) {
148+
// Check if the query needs to be validated according to the current author
149+
if (query instanceof SecureQuery secureQuery && secureQuery.isCurrentAuthorChecked()) {
150+
// Not need to check the details if current author has programming right
140151
if (!this.authorization.hasAccess(Right.PROGRAM)) {
141152
if (query.isNamed() && !getAllowedNamedQueries().contains(query.getStatement())) {
142153
throw new QueryException("Named queries requires programming right", query, null);
@@ -152,15 +163,15 @@ protected void checkAllowed(final Query query) throws QueryException
152163
@Override
153164
public <T> List<T> execute(final Query query) throws QueryException
154165
{
155-
// Make sure the query is allowed in the current context
156-
checkAllowed(query);
157-
158166
String oldDatabase = getContext().getWikiId();
159167
try {
160168
if (query.getWiki() != null) {
161169
getContext().setWikiId(query.getWiki());
162170
}
163171

172+
// Make sure the query is allowed. Make sure to do it in the target context.
173+
checkAllowed(query);
174+
164175
// Filter the query
165176
Query filteredQuery = filterQuery(query);
166177

@@ -333,13 +344,18 @@ private boolean hasQueryParametersType(Query query)
333344
*/
334345
protected String completeShortFormStatement(String statement)
335346
{
336-
String lcStatement = statement.toLowerCase().trim();
337-
if (lcStatement.isEmpty() || lcStatement.startsWith(",") || lcStatement.startsWith("where ")
338-
|| lcStatement.startsWith("order by ")) {
339-
return "select doc.fullName from XWikiDocument doc " + statement.trim();
347+
return toCompleteShortForm(statement);
348+
}
349+
350+
private static String toCompleteShortForm(String statement)
351+
{
352+
String filteredStatement = statement;
353+
354+
if (statement.isEmpty() || HqlQueryUtils.isShortFormStatement(statement)) {
355+
filteredStatement = "select doc.fullName from XWikiDocument doc " + statement.trim();
340356
}
341357

342-
return statement;
358+
return filteredStatement;
343359
}
344360

345361
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

+2
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@ public void isSafe()
6363
.isSafe("select doc.name, ot.field from XWikiDocument doc, XWikiSpace space, OtherTable as ot"));
6464
assertFalse(HqlQueryUtils.isSafe("select count(*) from OtherTable"));
6565
assertFalse(HqlQueryUtils.isSafe("select count(other.*) from OtherTable other"));
66+
assertFalse(HqlQueryUtils.isSafe("select doc.fullName from XWikiDocument doc union all select name from OtherTable"));
67+
assertFalse(HqlQueryUtils.isSafe("select doc.fullName from XWikiDocument doc where 1<>'1\\'' union select name from OtherTable #'"));
6668
}
6769

6870
@Test

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

+6-5
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ public void setNamedParameterArray()
220220
{
221221
org.hibernate.query.Query query = mock(org.hibernate.query.Query.class);
222222
String name = "bar";
223-
Integer[] value = new Integer[] { 1, 2, 3 };
223+
Integer[] value = new Integer[] {1, 2, 3};
224224
this.executor.setNamedParameter(query, name, value);
225225

226226
verify(query).setParameterList(name, value);
@@ -403,7 +403,7 @@ public void executeWhenNotAllowedSelect() throws Exception
403403
assertEquals(
404404
"The query requires programming right."
405405
+ " Query statement = [select notallowed.name from NotAllowedTable notallowed]",
406-
expected.getMessage());
406+
expected.getCause().getMessage());
407407
}
408408
}
409409

@@ -415,7 +415,7 @@ public void executeDeleteWithoutProgrammingRight() throws Exception
415415
fail("Should have thrown an exception here");
416416
} catch (QueryException expected) {
417417
assertEquals("The query requires programming right. Query statement = [delete from XWikiDocument as doc]",
418-
expected.getMessage());
418+
expected.getCause().getMessage());
419419
}
420420
}
421421

@@ -426,7 +426,8 @@ public void executeNamedQueryWithoutProgrammingRight() throws Exception
426426
executeNamed("somename", false);
427427
fail("Should have thrown an exception here");
428428
} catch (QueryException expected) {
429-
assertEquals("Named queries requires programming right. Named query = [somename]", expected.getMessage());
429+
assertEquals("Named queries requires programming right. Named query = [somename]",
430+
expected.getCause().getMessage());
430431
}
431432
}
432433

@@ -439,7 +440,7 @@ public void executeUpdateWithoutProgrammingRight() throws Exception
439440
} catch (QueryException expected) {
440441
assertEquals(
441442
"The query requires programming right. Query statement = [update XWikiDocument set name='name']",
442-
expected.getMessage());
443+
expected.getCause().getMessage());
443444
}
444445
}
445446
}

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

+1
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ public abstract class AbstractDatabaseSearchSource extends AbstractSearchSource
6363
protected Provider<XWikiContext> xcontextProvider;
6464

6565
@Inject
66+
@Named("secure")
6667
protected QueryManager queryManager;
6768

6869
@Inject

0 commit comments

Comments
 (0)