From 0b26038f996d3c298661e4435257a94a5deb3b83 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 13 Dec 2024 13:53:52 +0100 Subject: [PATCH] XWIKI-22726: Allow customizing the validation of HQL queries through configuration (#3747) (#3763) * introduce an (internal) extensible validation system HQL queries * introduce a new validator based on configurable regex of safe/unsafe regex patterns (cherry picked from commit 620256c23459ca643ad15269d3979e15ccb9ca8a) --- .../store/hibernate/query/HqlQueryUtils.java | 48 +++++ .../hibernate/query/HqlQueryExecutor.java | 90 +++++----- ...igurableHQLCompleteStatementValidator.java | 107 +++++++++++ .../DefaultHQLStatementValidator.java | 96 ++++++++++ .../HQLCompleteStatementValidator.java | 50 ++++++ .../hql/internal/HQLStatementValidator.java | 46 +++++ ...StandardHQLCompleteStatementValidator.java | 50 ++++++ .../main/resources/META-INF/components.txt | 3 + .../hibernate/query/HqlQueryExecutorTest.java | 167 ++++++++++++------ .../java/org/xwiki/query/QueryException.java | 20 ++- .../src/main/resources/xwiki.properties.vm | 20 +++ 11 files changed, 591 insertions(+), 106 deletions(-) create mode 100644 xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/ConfigurableHQLCompleteStatementValidator.java create mode 100644 xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/DefaultHQLStatementValidator.java create mode 100644 xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/HQLCompleteStatementValidator.java create mode 100644 xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/HQLStatementValidator.java create mode 100644 xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/StandardHQLCompleteStatementValidator.java diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/internal/store/hibernate/query/HqlQueryUtils.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/internal/store/hibernate/query/HqlQueryUtils.java index c08a3622496d..7091687fb355 100644 --- a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/internal/store/hibernate/query/HqlQueryUtils.java +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/internal/store/hibernate/query/HqlQueryUtils.java @@ -32,6 +32,8 @@ import org.apache.commons.lang3.exception.ExceptionUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.xwiki.query.Query; +import org.xwiki.query.WrappingQuery; import net.sf.jsqlparser.JSQLParserException; import net.sf.jsqlparser.expression.Expression; @@ -343,4 +345,50 @@ public static String replaceLegacyQueryParameters(String queryString) convertedString = builder.toString(); } } + + /** + * @param statement a potentially short form statement to complete + * @return a complete version of the input {@link Query} (or the {@link Query} as is if it's already complete) + * @since 17.0.0RC1 + * @since 16.10.2 + * @since 15.10.16 + * @since 16.4.6 + */ + public static String toCompleteStatement(String statement) + { + String completeStatement = statement; + + if (StringUtils.isEmpty(statement) || isShortFormStatement(statement)) { + completeStatement = "select doc.fullName from XWikiDocument doc " + statement.trim(); + } + + return completeStatement; + } + + /** + * @param query a potentially short form query to complete + * @return a complete version of the input {@link Query} (or the {@link Query} as is if it's already complete) + * @since 17.0.0RC1 + * @since 16.10.2 + * @since 15.10.16 + * @since 16.4.6 + */ + public static Query toCompleteQuery(Query query) + { + Query completeQuery = query; + + String completeStatement = toCompleteStatement(query.getStatement()); + if (completeStatement != query.getStatement()) { + completeQuery = new WrappingQuery(query) + { + @Override + public String getStatement() + { + return completeStatement; + } + }; + } + + return completeQuery; + } } 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 e1a9a7a40e97..6da1a2d0635d 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 @@ -32,11 +32,13 @@ import javax.inject.Singleton; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.hibernate.Session; import org.hibernate.cfg.Configuration; import org.hibernate.engine.spi.NamedQueryDefinition; import org.hibernate.engine.spi.NamedSQLQueryDefinition; import org.hibernate.query.NativeQuery; +import org.slf4j.Logger; import org.xwiki.component.annotation.Component; import org.xwiki.component.manager.ComponentLookupException; import org.xwiki.component.manager.ComponentManager; @@ -50,6 +52,7 @@ import org.xwiki.query.QueryParameter; import org.xwiki.query.SecureQuery; import org.xwiki.query.WrappingQuery; +import org.xwiki.query.hql.internal.HQLStatementValidator; import org.xwiki.security.authorization.ContextualAuthorizationManager; import org.xwiki.security.authorization.Right; @@ -58,13 +61,15 @@ import com.xpn.xwiki.internal.store.hibernate.query.HqlQueryUtils; import com.xpn.xwiki.store.XWikiHibernateStore; import com.xpn.xwiki.util.Util; +import com.xpn.xwiki.web.Utils; /** - * QueryExecutor implementation for Hibernate Store. + * {@link QueryExecutor} implementation for Hibernate Store. * * @version $Id$ * @since 1.6M1 */ +@SuppressWarnings("checkstyle:ClassFanOutComplexity") @Component @Named("hql") @Singleton @@ -93,7 +98,13 @@ public class HqlQueryExecutor implements QueryExecutor, Initializable @Named("context") private Provider componentManagerProvider; - private volatile Set allowedNamedQueries; + @Inject + private HQLStatementValidator queryValidator; + + @Inject + private Logger logger; + + private volatile Set safeNamedQueries; @Override public void initialize() throws InitializationException @@ -103,44 +114,50 @@ public void initialize() throws InitializationException configuration.addInputStream(Util.getResourceAsStream(MAPPING_PATH)); } - private Set getAllowedNamedQueries() + private Set getSafeNamedQueries() { - if (this.allowedNamedQueries == null) { + if (this.safeNamedQueries == null) { synchronized (this) { - if (this.allowedNamedQueries == null) { - this.allowedNamedQueries = new HashSet<>(); + if (this.safeNamedQueries == null) { + Set safeQueries = new HashSet<>(); // Gather the list of allowed named queries Collection namedQueries = this.hibernate.getConfigurationMetadata().getNamedQueryDefinitions(); for (NamedQueryDefinition definition : namedQueries) { - if (HqlQueryUtils.isSafe(definition.getQuery())) { - this.allowedNamedQueries.add(definition.getName()); + try { + if (this.queryValidator.isSafe(definition.getQuery())) { + safeQueries.add(definition.getName()); + } + } catch (QueryException e) { + this.logger.warn("Failed to validate named query [{}] with statement [{}]: {}", + definition.getName(), definition.getQuery(), ExceptionUtils.getRootCauseMessage(e)); } } + + this.safeNamedQueries = safeQueries; } } } - return this.allowedNamedQueries; + return this.safeNamedQueries; } /** - * @param statementString the statement to evaluate + * @param statement the statement to evaluate * @return true if the select is allowed for user without PR + * @deprecated */ - protected static boolean isSafeSelect(String statementString) + @Deprecated(since = "17.0.0RC1, 16.10.2, 15.10.16, 16.4.6") + protected static boolean isSafeSelect(String statement) { - // An empty statement is safe - if (statementString.isEmpty()) { - return true; - } + HQLStatementValidator validator = Utils.getComponent(HQLStatementValidator.class); - // HqlQueryUtils#isSafe only works with complete statements - String completeStatement = toCompleteShortForm(statementString); - - // Parse and validate the statement - return HqlQueryUtils.isSafe(completeStatement); + try { + return validator.isSafe(statement); + } catch (QueryException e) { + return false; + } } protected void checkAllowed(final Query query) throws QueryException @@ -149,11 +166,11 @@ protected void checkAllowed(final Query query) throws QueryException 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())) { + if (query.isNamed() && !getSafeNamedQueries().contains(query.getStatement())) { throw new QueryException("Named queries requires programming right", query, null); } - if (!isSafeSelect(query.getStatement())) { + if (!this.queryValidator.isSafe(query.getStatement())) { throw new QueryException("The query requires programming right", query, null); } } @@ -200,17 +217,13 @@ public List execute(final Query query) throws QueryException protected Query filterQuery(Query query) { Query filteredQuery = query; + + // Named queries are not filtered if (!filteredQuery.isNamed()) { - // For non-named queries, convert the short form into long form before we apply the filters. - filteredQuery = new WrappingQuery(filteredQuery) - { - @Override - public String getStatement() - { - // handle short queries - return completeShortFormStatement(getWrappedQuery().getStatement()); - } - }; + // Make sure to work with the complete form of the query + filteredQuery = HqlQueryUtils.toCompleteQuery(filteredQuery); + + // Execute filters filteredQuery = filterQuery(filteredQuery, Query.HQL); } @@ -344,18 +357,7 @@ private boolean hasQueryParametersType(Query query) */ protected String completeShortFormStatement(String statement) { - 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 filteredStatement; + return HqlQueryUtils.toCompleteStatement(statement); } private org.hibernate.query.Query createNamedHibernateQuery(Session session, Query query) diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/ConfigurableHQLCompleteStatementValidator.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/ConfigurableHQLCompleteStatementValidator.java new file mode 100644 index 000000000000..81f9dc952466 --- /dev/null +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/ConfigurableHQLCompleteStatementValidator.java @@ -0,0 +1,107 @@ +/* + * See the NOTICE file distributed with this work for additional + * information regarding copyright ownership. + * + * This is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as + * published by the Free Software Foundation; either version 2.1 of + * the License, or (at your option) any later version. + * + * This software is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this software; if not, write to the Free + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA, or see the FSF site: http://www.fsf.org. + */ +package org.xwiki.query.hql.internal; + +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.regex.Pattern; + +import javax.annotation.Priority; +import javax.inject.Inject; +import javax.inject.Named; +import javax.inject.Singleton; + +import org.xwiki.component.annotation.Component; +import org.xwiki.component.descriptor.ComponentDescriptor; +import org.xwiki.component.phase.Initializable; +import org.xwiki.component.phase.InitializationException; +import org.xwiki.configuration.ConfigurationSource; + +/** + * A validator which can be configured. This validator is executer before the default one using component priorities + * + * @version $Id$ + * @since 17.0.0RC1 + * @since 16.10.2 + * @since 15.10.16 + * @since 16.4.6 + */ +@Component +@Singleton +@Named("configurable") +@Priority(ComponentDescriptor.DEFAULT_PRIORITY - 100) +public class ConfigurableHQLCompleteStatementValidator implements HQLCompleteStatementValidator, Initializable +{ + @Inject + @Named("xwikiproperties") + private ConfigurationSource configuration; + + private List unsafe; + + private List safe; + + @Override + public void initialize() throws InitializationException + { + this.unsafe = getPatterns("query.hql.unsafe"); + this.safe = getPatterns("query.hql.safe"); + } + + private List getPatterns(String key) throws InitializationException + { + List patternStrings = this.configuration.getProperty(key, List.class); + + List patterns; + if (patternStrings != null) { + patterns = new ArrayList<>(patternStrings.size()); + for (String patternString : patternStrings) { + try { + patterns.add(Pattern.compile(patternString)); + } catch (Exception e) { + throw new InitializationException( + String.format("Failed to parse pattern [%s] for configuration [%s]", patternString, key), e); + } + } + } else { + patterns = List.of(); + } + + return patterns; + } + + @Override + public Optional isSafe(String statement) + { + for (Pattern pattern : this.unsafe) { + if (pattern.matcher(statement).matches()) { + return Optional.of(Boolean.FALSE); + } + } + + for (Pattern pattern : this.safe) { + if (pattern.matcher(statement).matches()) { + return Optional.of(Boolean.TRUE); + } + } + + return Optional.empty(); + } +} diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/DefaultHQLStatementValidator.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/DefaultHQLStatementValidator.java new file mode 100644 index 000000000000..aac9db5581fd --- /dev/null +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/DefaultHQLStatementValidator.java @@ -0,0 +1,96 @@ +/* + * See the NOTICE file distributed with this work for additional + * information regarding copyright ownership. + * + * This is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as + * published by the Free Software Foundation; either version 2.1 of + * the License, or (at your option) any later version. + * + * This software is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this software; if not, write to the Free + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA, or see the FSF site: http://www.fsf.org. + */ +package org.xwiki.query.hql.internal; + +import java.util.Optional; + +import javax.inject.Inject; +import javax.inject.Named; +import javax.inject.Provider; +import javax.inject.Singleton; + +import org.apache.commons.lang3.StringUtils; +import org.xwiki.component.annotation.Component; +import org.xwiki.component.manager.ComponentLookupException; +import org.xwiki.component.manager.ComponentManager; +import org.xwiki.query.Query; +import org.xwiki.query.QueryException; +import org.xwiki.query.internal.DefaultQuery; + +import com.xpn.xwiki.internal.store.hibernate.query.HqlQueryUtils; + +/** + * The main entry point to validate a query. + * + * @version $Id$ + * @since 17.0.0RC1 + * @since 16.10.2 + * @since 15.10.16 + * @since 16.4.6 + */ +@Component +@Singleton +public class DefaultHQLStatementValidator implements HQLStatementValidator +{ + @Inject + @Named("context") + private Provider componentManagerProvider; + + @Inject + @Named("standard") + private HQLCompleteStatementValidator standardValidator; + + @Override + public boolean isSafe(String statement) throws QueryException + { + // An empty statement is safe + if (StringUtils.isEmpty(statement)) { + return true; + } + + // Validators are expected to work with complete statements statements + String completeStatement = HqlQueryUtils.toCompleteStatement(statement); + + // Parse and validate the statement + ComponentManager componentManager = this.componentManagerProvider.get(); + try { + for (HQLCompleteStatementValidator validator : componentManager + .getInstanceList(HQLCompleteStatementValidator.class)) { + Optional result = validator.isSafe(completeStatement); + if (result.isPresent()) { + return result.get(); + } + } + } catch (ComponentLookupException e) { + throw new QueryException("Failed to get query statement validators", + new DefaultQuery(completeStatement, Query.HQL, null), e); + } + + // Fallback on the standard validator (it's already supposed to be part of the found validators above, but + // something might cause it to be unregistered) + Optional result = this.standardValidator.isSafe(completeStatement); + if (result.isPresent()) { + return result.get(); + } + + // If we really could not find any way to validate the statement, consider it unsafe + return false; + } +} diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/HQLCompleteStatementValidator.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/HQLCompleteStatementValidator.java new file mode 100644 index 000000000000..363ad1fa0279 --- /dev/null +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/HQLCompleteStatementValidator.java @@ -0,0 +1,50 @@ +/* + * See the NOTICE file distributed with this work for additional + * information regarding copyright ownership. + * + * This is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as + * published by the Free Software Foundation; either version 2.1 of + * the License, or (at your option) any later version. + * + * This software is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this software; if not, write to the Free + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA, or see the FSF site: http://www.fsf.org. + */ +package org.xwiki.query.hql.internal; + +import java.util.Optional; + +import org.xwiki.component.annotation.Role; +import org.xwiki.query.QueryException; + +/** + * A component in charge of validating a passed HQL statement. + *

+ * Note that since the {@code standard} validator always gives an answer, any extra validator must have a priority value + * lower than the default one ({@link org.xwiki.component.descriptor.ComponentDescriptor#DEFAULT_PRIORITY} to have a + * chance to be called. + * + * @version $Id$ + * @since 17.0.0RC1 + * @since 16.10.2 + * @since 15.10.16 + * @since 16.4.6 + */ +@Role +public interface HQLCompleteStatementValidator +{ + /** + * @param statement the HQL statement to validate + * @return {@link Boolean#TRUE} if the passed query is safe, {@link Boolean#FALSE} if it's not and + * {@link Optional#empty()} if unknown. + * @throws QueryException when failing the validate the query + */ + Optional isSafe(String statement) throws QueryException; +} diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/HQLStatementValidator.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/HQLStatementValidator.java new file mode 100644 index 000000000000..29d1e19969ed --- /dev/null +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/HQLStatementValidator.java @@ -0,0 +1,46 @@ +/* + * See the NOTICE file distributed with this work for additional + * information regarding copyright ownership. + * + * This is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as + * published by the Free Software Foundation; either version 2.1 of + * the License, or (at your option) any later version. + * + * This software is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this software; if not, write to the Free + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA, or see the FSF site: http://www.fsf.org. + */ +package org.xwiki.query.hql.internal; + +import java.util.Optional; + +import org.xwiki.component.annotation.Role; +import org.xwiki.query.QueryException; + +/** + * A component in charge of validating a passed HQL statement. + * + * @version $Id$ + * @since 17.0.0RC1 + * @since 16.10.2 + * @since 15.10.16 + * @since 16.4.6 + */ +@Role +public interface HQLStatementValidator +{ + /** + * @param statement the HQL statement to validate + * @return {@link Boolean#TRUE} if the passed query is safe, {@link Boolean#FALSE} if it's not and + * {@link Optional#empty()} if unknown. + * @throws QueryException when failing the validate the query + */ + boolean isSafe(String statement) throws QueryException; +} diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/StandardHQLCompleteStatementValidator.java b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/StandardHQLCompleteStatementValidator.java new file mode 100644 index 000000000000..973bf6776406 --- /dev/null +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/org/xwiki/query/hql/internal/StandardHQLCompleteStatementValidator.java @@ -0,0 +1,50 @@ +/* + * See the NOTICE file distributed with this work for additional + * information regarding copyright ownership. + * + * This is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as + * published by the Free Software Foundation; either version 2.1 of + * the License, or (at your option) any later version. + * + * This software is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this software; if not, write to the Free + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA, or see the FSF site: http://www.fsf.org. + */ +package org.xwiki.query.hql.internal; + +import java.util.Optional; + +import javax.inject.Named; +import javax.inject.Singleton; + +import org.xwiki.component.annotation.Component; + +import com.xpn.xwiki.internal.store.hibernate.query.HqlQueryUtils; + +/** + * A HQL validator which relies on {@link HqlQueryUtils#isSafe(String)}. + * + * @version $Id$ + * @since 17.0.0RC1 + * @since 16.10.2 + * @since 15.10.16 + * @since 16.4.6 + */ +@Component +@Singleton +@Named("standard") +public class StandardHQLCompleteStatementValidator implements HQLCompleteStatementValidator +{ + @Override + public Optional isSafe(String statement) + { + return Optional.of(HqlQueryUtils.isSafe(statement)); + } +} diff --git a/xwiki-platform-core/xwiki-platform-oldcore/src/main/resources/META-INF/components.txt b/xwiki-platform-core/xwiki-platform-oldcore/src/main/resources/META-INF/components.txt index 6d15471359f3..9c58b84061e8 100644 --- a/xwiki-platform-core/xwiki-platform-oldcore/src/main/resources/META-INF/components.txt +++ b/xwiki-platform-core/xwiki-platform-oldcore/src/main/resources/META-INF/components.txt @@ -285,3 +285,6 @@ org.xwiki.internal.document.DocumentOverrideListener org.xwiki.internal.document.DocumentRequiredRightsReader org.xwiki.internal.document.RequiredRightClassMandatoryDocumentInitializer org.xwiki.internal.document.DefaultSimpleDocumentCache +org.xwiki.query.hql.internal.ConfigurableHQLCompleteStatementValidator +org.xwiki.query.hql.internal.DefaultHQLStatementValidator +org.xwiki.query.hql.internal.StandardHQLCompleteStatementValidator 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 e7ab62b32e8c..eea286d3689b 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 @@ -26,27 +26,37 @@ import java.util.List; import java.util.Map; +import javax.inject.Named; + import org.hibernate.Session; import org.hibernate.boot.Metadata; import org.hibernate.cfg.Configuration; import org.hibernate.engine.spi.NamedSQLQueryDefinition; import org.hibernate.query.NativeQuery; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; import org.xwiki.component.manager.ComponentManager; +import org.xwiki.configuration.ConfigurationSource; import org.xwiki.context.Execution; import org.xwiki.context.ExecutionContext; import org.xwiki.query.Query; import org.xwiki.query.QueryException; import org.xwiki.query.QueryFilter; import org.xwiki.query.WrappingQuery; +import org.xwiki.query.hql.internal.ConfigurableHQLCompleteStatementValidator; +import org.xwiki.query.hql.internal.DefaultHQLStatementValidator; +import org.xwiki.query.hql.internal.HQLCompleteStatementValidator; +import org.xwiki.query.hql.internal.StandardHQLCompleteStatementValidator; import org.xwiki.query.internal.DefaultQuery; import org.xwiki.security.authorization.ContextualAuthorizationManager; import org.xwiki.security.authorization.Right; -import org.xwiki.test.mockito.MockitoComponentMockingRule; +import org.xwiki.test.annotation.AfterComponent; +import org.xwiki.test.annotation.ComponentList; +import org.xwiki.test.junit5.mockito.ComponentTest; +import org.xwiki.test.junit5.mockito.InjectMockComponents; +import org.xwiki.test.junit5.mockito.MockComponent; import com.xpn.xwiki.XWikiContext; import com.xpn.xwiki.XWikiException; @@ -54,9 +64,9 @@ import com.xpn.xwiki.store.XWikiHibernateBaseStore; import com.xpn.xwiki.store.XWikiHibernateStore; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertSame; -import static org.junit.Assert.fail; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.AdditionalAnswers.returnsFirstArg; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; @@ -69,34 +79,57 @@ * * @version $Id$ */ -public class HqlQueryExecutorTest +@ComponentTest +@ComponentList({DefaultHQLStatementValidator.class, StandardHQLCompleteStatementValidator.class, + ConfigurableHQLCompleteStatementValidator.class}) +class HqlQueryExecutorTest { - @Rule - public MockitoComponentMockingRule mocker = - new MockitoComponentMockingRule<>(HqlQueryExecutor.class); + @InjectMockComponents + private HqlQueryExecutor executor; - private ContextualAuthorizationManager authorization; + @InjectMockComponents + private DefaultHQLStatementValidator defaultValidator; - private boolean hasProgrammingRight; + @InjectMockComponents + private ConfigurableHQLCompleteStatementValidator configurableValidator; - /** - * The component under test. - */ - private HqlQueryExecutor executor; + @MockComponent + private ContextualAuthorizationManager authorization; + @MockComponent private XWikiHibernateStore store; - @Before - public void before() throws Exception + @MockComponent + private HibernateStore hibernateStore; + + @MockComponent + private Execution execution; + + @MockComponent + @Named("xwikiproperties") + private ConfigurationSource xwikiproperties; + + @MockComponent + @Named("context") + private ComponentManager contextComponentMannager; + + private boolean hasProgrammingRight; + + @AfterComponent + public void afterComponent() { - HibernateStore hibernateStore = this.mocker.getInstance(HibernateStore.class); - when(hibernateStore.getConfiguration()).thenReturn(new Configuration()); - Metadata metadata = mock(Metadata.class); - when(hibernateStore.getConfigurationMetadata()).thenReturn(metadata); + when(this.hibernateStore.getConfiguration()).thenReturn(new Configuration()); + when(this.hibernateStore.getConfigurationMetadata()).thenReturn(mock(Metadata.class)); - this.executor = this.mocker.getComponentUnderTest(); - this.authorization = this.mocker.getInstance(ContextualAuthorizationManager.class); + when(this.xwikiproperties.getProperty("query.hql.safe", List.class)) + .thenReturn(List.of("select normallynotallowed from XWikiDocument as doc where 1=\\d+")); + when(this.xwikiproperties.getProperty("query.hql.unsafe", List.class)) + .thenReturn(List.of("select name from XWikiDocument as doc where 1=\\d+")); + } + @BeforeEach + public void beforeEach() throws Exception + { when(this.authorization.hasAccess(Right.PROGRAM)).then(new Answer() { @Override @@ -108,19 +141,21 @@ public Boolean answer(InvocationOnMock invocation) throws Throwable this.hasProgrammingRight = true; + when(this.contextComponentMannager + .getInstanceList(HQLCompleteStatementValidator.class)) + .thenReturn(List.of(configurableValidator)); + // Actual Hibernate query - Execution execution = this.mocker.getInstance(Execution.class); ExecutionContext executionContext = mock(ExecutionContext.class); - when(execution.getContext()).thenReturn(executionContext); + when(this.execution.getContext()).thenReturn(executionContext); XWikiContext xwikiContext = mock(XWikiContext.class); when(executionContext.getProperty(XWikiContext.EXECUTIONCONTEXT_KEY)).thenReturn(xwikiContext); when(xwikiContext.getWikiId()).thenReturn("currentwikid"); com.xpn.xwiki.XWiki xwiki = mock(com.xpn.xwiki.XWiki.class); when(xwikiContext.getWiki()).thenReturn(xwiki); - this.store = mock(XWikiHibernateStore.class); - when(xwiki.getHibernateStore()).thenReturn(store); + when(xwiki.getHibernateStore()).thenReturn(this.store); } private void execute(String statement, Boolean withProgrammingRights) throws QueryException @@ -150,20 +185,20 @@ private void executeNamed(String name, Boolean withProgrammingRights) throws Que // Tests @Test - public void completeShortStatementWhenEmpty() + void completeShortStatementWhenEmpty() { assertEquals("select doc.fullName from XWikiDocument doc ", this.executor.completeShortFormStatement("")); } @Test - public void completeShortStatementStartingWithWhere() + void completeShortStatementStartingWithWhere() { assertEquals("select doc.fullName from XWikiDocument doc where doc.author='XWiki.Admin'", this.executor.completeShortFormStatement("where doc.author='XWiki.Admin'")); } @Test - public void completeShortStatementStartingWithFrom() + void completeShortStatementStartingWithFrom() { assertEquals( "select doc.fullName from XWikiDocument doc , BaseObject obj where doc.fullName=obj.name " @@ -173,28 +208,28 @@ public void completeShortStatementStartingWithFrom() } @Test - public void completeShortStatementStartingWithOrderBy() + void completeShortStatementStartingWithOrderBy() { assertEquals("select doc.fullName from XWikiDocument doc order by doc.date desc", this.executor.completeShortFormStatement("order by doc.date desc")); } @Test - public void completeShortStatementPassingAnAlreadyCompleteQuery() + void completeShortStatementPassingAnAlreadyCompleteQuery() { assertEquals("select doc.fullName from XWikiDocument doc order by doc.date desc", this.executor .completeShortFormStatement("select doc.fullName from XWikiDocument doc order by doc.date desc")); } @Test - public void completeShortStatementPassingAQueryOnSomethingElseThanADocument() + void completeShortStatementPassingAQueryOnSomethingElseThanADocument() { assertEquals("select lock.docId from XWikiLock as lock ", this.executor.completeShortFormStatement("select lock.docId from XWikiLock as lock ")); } @Test - public void setNamedParameter() + void setNamedParameter() { org.hibernate.query.Query query = mock(org.hibernate.query.Query.class); String name = "abc"; @@ -205,7 +240,7 @@ public void setNamedParameter() } @Test - public void setNamedParameterList() + void setNamedParameterList() { org.hibernate.query.Query query = mock(org.hibernate.query.Query.class); String name = "foo"; @@ -216,7 +251,7 @@ public void setNamedParameterList() } @Test - public void setNamedParameterArray() + void setNamedParameterArray() { org.hibernate.query.Query query = mock(org.hibernate.query.Query.class); String name = "bar"; @@ -227,7 +262,7 @@ public void setNamedParameterArray() } @Test - public void populateParameters() + void populateParameters() { org.hibernate.query.Query hquery = mock(org.hibernate.query.Query.class); Query query = mock(Query.class); @@ -253,7 +288,7 @@ public void populateParameters() } @Test - public void executeWhenStoreException() throws Exception + void executeWhenStoreException() throws Exception { XWikiException exception = mock(XWikiException.class); when(exception.getMessage()).thenReturn("nestedmessage"); @@ -272,7 +307,7 @@ public void executeWhenStoreException() throws Exception } @Test - public void createNamedNativeHibernateQuery() throws Exception + void createNamedNativeHibernateQuery() throws Exception { DefaultQuery query = new DefaultQuery("queryName", this.executor); @@ -294,8 +329,7 @@ public void createNamedNativeHibernateQuery() throws Exception when(definition.getResultSetRef()).thenReturn("someResultSet"); when(definition.getName()).thenReturn(query.getStatement()); - HibernateStore hibernateStore = this.mocker.getInstance(HibernateStore.class); - when(hibernateStore.getConfigurationMetadata().getNamedNativeQueryDefinition(query.getStatement())) + when(this.hibernateStore.getConfigurationMetadata().getNamedNativeQueryDefinition(query.getStatement())) .thenReturn(definition); assertSame(finalQuery, this.executor.createHibernateQuery(session, query)); @@ -304,7 +338,7 @@ public void createNamedNativeHibernateQuery() throws Exception } @Test - public void createHibernateQueryWhenFilter() throws Exception + void createHibernateQueryWhenFilter() throws Exception { Session session = mock(Session.class); @@ -332,7 +366,7 @@ public String getStatement() } @Test - public void createHibernateQueryAutomaticallyAddEscapeLikeParametersFilterWhenQueryParameter() throws Exception + void createHibernateQueryAutomaticallyAddEscapeLikeParametersFilterWhenQueryParameter() throws Exception { Session session = mock(Session.class); DefaultQuery query = new DefaultQuery("where space like :space", Query.HQL, this.executor); @@ -342,8 +376,7 @@ public void createHibernateQueryAutomaticallyAddEscapeLikeParametersFilterWhenQu when(filter.filterStatement(anyString(), anyString())).then(returnsFirstArg()); when(filter.filterQuery(any(Query.class))).then(returnsFirstArg()); - ComponentManager cm = this.mocker.getInstance(ComponentManager.class, "context"); - when(cm.getInstance(QueryFilter.class, "escapeLikeParameters")).thenReturn(filter); + when(this.contextComponentMannager.getInstance(QueryFilter.class, "escapeLikeParameters")).thenReturn(filter); when(session.createQuery(anyString())).thenReturn(mock(org.hibernate.query.Query.class)); @@ -355,46 +388,51 @@ public void createHibernateQueryAutomaticallyAddEscapeLikeParametersFilterWhenQu } @Test - public void executeShortWhereHQLQueryWithProgrammingRights() throws QueryException + void executeShortWhereHQLQueryWithProgrammingRights() throws QueryException { execute("where doc.space='Main'", true); } @Test - public void executeShortFromHQLQueryWithProgrammingRights() throws QueryException + void executeShortFromHQLQueryWithProgrammingRights() throws QueryException { execute(", BaseObject as obj", true); } @Test - public void executeCompleteHQLQueryWithProgrammingRights() throws QueryException + void executeCompleteHQLQueryWithProgrammingRights() throws QueryException { execute("select u from XWikiDocument as doc", true); - } @Test - public void executeNamedQueryWithProgrammingRights() throws QueryException + void executeNamedQueryWithProgrammingRights() throws QueryException { executeNamed("somename", true); } @Test - public void executeShortWhereHQLQueryWithoutProgrammingRights() throws QueryException + void executeShortWhereHQLQueryWithoutProgrammingRights() throws QueryException { execute("where doc.space='Main'", false); } @Test - public void executeShortFromHQLQueryWithoutProgrammingRights() throws QueryException + void executeShortFromHQLQueryWithoutProgrammingRights() throws QueryException { execute(", BaseObject as obj", false); } + @Test + void executeWhenOverwrittenAllowedSelect() throws Exception + { + execute("select normallynotallowed from XWikiDocument as doc where 1=42", false); + } + // Not allowed @Test - public void executeWhenNotAllowedSelect() throws Exception + void executeWhenNotAllowedSelect() throws Exception { try { execute("select notallowed.name from NotAllowedTable notallowed", false); @@ -408,7 +446,7 @@ public void executeWhenNotAllowedSelect() throws Exception } @Test - public void executeDeleteWithoutProgrammingRight() throws Exception + void executeDeleteWithoutProgrammingRight() throws Exception { try { execute("delete from XWikiDocument as doc", false); @@ -420,7 +458,7 @@ public void executeDeleteWithoutProgrammingRight() throws Exception } @Test - public void executeNamedQueryWithoutProgrammingRight() throws Exception + void executeNamedQueryWithoutProgrammingRight() throws Exception { try { executeNamed("somename", false); @@ -432,7 +470,7 @@ public void executeNamedQueryWithoutProgrammingRight() throws Exception } @Test - public void executeUpdateWithoutProgrammingRight() throws Exception + void executeUpdateWithoutProgrammingRight() throws Exception { try { execute("update XWikiDocument set name='name'", false); @@ -443,4 +481,17 @@ public void executeUpdateWithoutProgrammingRight() throws Exception expected.getCause().getMessage()); } } + + @Test + void executeWhenOverwrittenUnsafeSelect() throws Exception + { + try { + execute("select name from XWikiDocument as doc where 1=42", false); + } catch (QueryException expected) { + assertEquals( + "The query requires programming right." + + " Query statement = [select name from XWikiDocument as doc where 1=42]", + expected.getCause().getMessage()); + } + } } diff --git a/xwiki-platform-core/xwiki-platform-query/xwiki-platform-query-manager/src/main/java/org/xwiki/query/QueryException.java b/xwiki-platform-core/xwiki-platform-query/xwiki-platform-query-manager/src/main/java/org/xwiki/query/QueryException.java index 881506b566e4..3ceee8f5abdb 100644 --- a/xwiki-platform-core/xwiki-platform-query/xwiki-platform-query-manager/src/main/java/org/xwiki/query/QueryException.java +++ b/xwiki-platform-core/xwiki-platform-query/xwiki-platform-query-manager/src/main/java/org/xwiki/query/QueryException.java @@ -32,6 +32,17 @@ public class QueryException extends Exception */ private final Query query; + /** + * @param message exception message + * @param query Query object + */ + public QueryException(String message, Query query) + { + super(message); + + this.query = query; + } + /** * @param message exception message * @param query Query object @@ -40,19 +51,20 @@ public class QueryException extends Exception public QueryException(String message, Query query, Throwable cause) { super(message, cause); + this.query = query; } @Override public String getMessage() { - if (query == null) { + if (this.query == null) { return super.getMessage(); } else { - if (query.isNamed()) { - return super.getMessage() + ". Named query = [" + query.getStatement() + "]"; + if (this.query.isNamed()) { + return super.getMessage() + ". Named query = [" + this.query.getStatement() + "]"; } else { - return super.getMessage() + ". Query statement = [" + query.getStatement() + "]"; + return super.getMessage() + ". Query statement = [" + this.query.getStatement() + "]"; } } } diff --git a/xwiki-platform-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm b/xwiki-platform-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm index 3a9c609979b9..7b9fb6ae1f3b 100644 --- a/xwiki-platform-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm +++ b/xwiki-platform-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm @@ -878,6 +878,26 @@ distribution.automaticStartOnWiki=$xwikiPropertiesAutomaticStartOnWiki #-# issues # security.requiredRights.protection=warning +#------------------------------------------------------------------------------------- +# Query +#------------------------------------------------------------------------------------- + +#-# [Since 17.0.0RC1] +#-# [Since 16.10.2] +#-# [Since 15.10.16] +#-# [Since 16.4.6] +#-# While HQL queries executed by users without programming right are already validated, it's sometimes necessary to +#-# customize the behavior of the standard validator because it might be too strict (or not strict enough). +#-# The following properties allow giving a list of (Java) regular expressions. +#-# In case both safe and unsafe patterns match the query, the priority goes to the unsafe pattern. +#-# +# query.hql.unsafe=.*some native syntax.* +#-# +#-# Note: be very careful to not use too large regular expression when adding a safe pattern as it could introduce a vulnerability. +#-# +# query.hql.safe=select prop1, prop2 from CustomTable +# query.hql.safe=select\\s+((prop1|prop2|prop3)\\s*,?\\s*)+\\s+from MyCustomTable + #------------------------------------------------------------------------------------- # URL #-------------------------------------------------------------------------------------