Skip to content

ESB-1154 refactored error handling, updated tests #346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MEM2677 wants to merge 6 commits into
base: release/7.5
Choose a base branch
from
Open

ESB-1154 refactored error handling, updated tests #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8e968a0
ESB-1154 refacotred error handling, updated tests
MEM2677 Jul 2, 2026
c14d0a9
ESB-1154 fix tests
MEM2677 Jul 2, 2026
6d3e8dc
ESB-1154 trimmed log
MEM2677 Jul 7, 2026
dd4a380
ESB-1154 fix jakarta servlet error attributes
MEM2677 Jul 7, 2026
3a1348e
ESB-1154 quality gate
MEM2677 Jul 7, 2026
bd295dc
ESB-1154 quality gate
MEM2677 Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions admin-console/src/main/webapp/error.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
%>
<!DOCTYPE html>
<html lang="en">
Expand Down
27 changes: 26 additions & 1 deletion ...l-ui/src/main/java/com/agiletec/aps/system/services/controller/control/Authenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
106 changes: 103 additions & 3 deletions ...in/java/org/entando/entando/plugins/jpsolr/aps/system/content/AdvContentFacetManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,23 @@
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;
import org.entando.entando.aps.system.exception.RestServerError;
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;
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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<String> getAllowedGroups(UserDetails currentUser) {
List<String> groupCodes = new ArrayList<>();
if (null != currentUser) {
Expand Down
41 changes: 27 additions & 14 deletions ...-plugin/src/main/java/org/entando/entando/plugins/jpsolr/aps/system/solr/SearcherDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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)) {
Expand Down
Loading
Loading