[ https://issues.apache.org/jira/browse/WW-5352?focusedWorklogId=897753&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-897753 ]
ASF GitHub Bot logged work on WW-5352: -------------------------------------- Author: ASF GitHub Bot Created on: 03/Jan/24 05:20 Start Date: 03/Jan/24 05:20 Worklog Time Spent: 10m Work Description: github-advanced-security[bot] commented on code in PR #829: URL: https://github.com/apache/struts/pull/829#discussion_r1440083331 ########## core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java: ########## @@ -0,0 +1,559 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor.parameter; + +import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.ActionInvocation; +import com.opensymphony.xwork2.TextProvider; +import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.interceptor.MethodFilterInterceptor; +import com.opensymphony.xwork2.interceptor.ValidationAware; +import com.opensymphony.xwork2.security.AcceptedPatternsChecker; +import com.opensymphony.xwork2.security.ExcludedPatternsChecker; +import com.opensymphony.xwork2.util.ClearableValueStack; +import com.opensymphony.xwork2.util.MemberAccessValueStack; +import com.opensymphony.xwork2.util.TextParseUtil; +import com.opensymphony.xwork2.util.ValueStack; +import com.opensymphony.xwork2.util.ValueStackFactory; +import com.opensymphony.xwork2.util.reflection.ReflectionContextState; +import org.apache.commons.lang3.BooleanUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.action.NoParameters; +import org.apache.struts2.dispatcher.HttpParameters; +import org.apache.struts2.dispatcher.Parameter; + +import java.util.Collection; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.regex.Pattern; + +import static org.apache.commons.lang3.StringUtils.normalizeSpace; + +/** + * This interceptor sets all parameters on the value stack. + */ +public class ParametersInterceptor extends MethodFilterInterceptor { + + private static final Logger LOG = LogManager.getLogger(ParametersInterceptor.class); + + protected static final int PARAM_NAME_MAX_LENGTH = 100; + + private static final Pattern DMI_IGNORED_PATTERN = Pattern.compile("^(action|method):.*", Pattern.CASE_INSENSITIVE); + + private int paramNameMaxLength = PARAM_NAME_MAX_LENGTH; + private boolean devMode = false; + private boolean dmiEnabled = false; + + protected boolean ordered = false; + + private ValueStackFactory valueStackFactory; + private ExcludedPatternsChecker excludedPatterns; + private AcceptedPatternsChecker acceptedPatterns; + private Set<Pattern> excludedValuePatterns = null; + private Set<Pattern> acceptedValuePatterns = null; + + @Inject + public void setValueStackFactory(ValueStackFactory valueStackFactory) { + this.valueStackFactory = valueStackFactory; + } + + @Inject(StrutsConstants.STRUTS_DEVMODE) + public void setDevMode(String mode) { + this.devMode = BooleanUtils.toBoolean(mode); + } + + @Inject + public void setExcludedPatterns(ExcludedPatternsChecker excludedPatterns) { + this.excludedPatterns = excludedPatterns; + } + + @Inject + public void setAcceptedPatterns(AcceptedPatternsChecker acceptedPatterns) { + this.acceptedPatterns = acceptedPatterns; + } + + @Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false) + protected void setDynamicMethodInvocation(String dmiEnabled) { + this.dmiEnabled = Boolean.parseBoolean(dmiEnabled); + } + + /** + * If the param name exceeds the configured maximum length it will not be + * accepted. + * + * @param paramNameMaxLength Maximum length of param names + */ + public void setParamNameMaxLength(int paramNameMaxLength) { + this.paramNameMaxLength = paramNameMaxLength; + } + + static private int countOGNLCharacters(String s) { + int count = 0; + for (int i = s.length() - 1; i >= 0; i--) { + char c = s.charAt(i); + if (c == '.' || c == '[') count++; + } + return count; + } + + /** + * Compares based on number of '.' and '[' characters (fewer is higher) + */ + static final Comparator<String> rbCollator = (s1, s2) -> { + int l1 = countOGNLCharacters(s1); + int l2 = countOGNLCharacters(s2); + return l1 < l2 ? -1 : (l2 < l1 ? 1 : s1.compareTo(s2)); + }; + + @Override + public String doIntercept(ActionInvocation invocation) throws Exception { + Object action = invocation.getAction(); + if (!(action instanceof NoParameters)) { + ActionContext ac = invocation.getInvocationContext(); + HttpParameters parameters = retrieveParameters(ac); + + if (LOG.isDebugEnabled()) { + LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters))); + } + + if (parameters != null) { + Map<String, Object> contextMap = ac.getContextMap(); + try { + ReflectionContextState.setCreatingNullObjects(contextMap, true); + ReflectionContextState.setDenyMethodExecution(contextMap, true); + ReflectionContextState.setReportingConversionErrors(contextMap, true); + + ValueStack stack = ac.getValueStack(); + setParameters(action, stack, parameters); + } finally { + ReflectionContextState.setCreatingNullObjects(contextMap, false); + ReflectionContextState.setDenyMethodExecution(contextMap, false); + ReflectionContextState.setReportingConversionErrors(contextMap, false); + } + } + } + return invocation.invoke(); + } + + /** + * Gets the parameter map to apply from wherever appropriate + * + * @param ac The action context + * @return The parameter map to apply + */ + protected HttpParameters retrieveParameters(ActionContext ac) { + return ac.getParameters(); + } + + + /** + * Adds the parameters into context's ParameterMap + * + * @param ac The action context + * @param newParams The parameter map to apply + * <p> + * In this class this is a no-op, since the parameters were fetched from the same location. + * In subclasses both retrieveParameters() and addParametersToContext() should be overridden. + * </p> + */ + protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams) { + } + + protected void setParameters(final Object action, ValueStack stack, HttpParameters parameters) { + HttpParameters params; + Map<String, Parameter> acceptableParameters; + if (ordered) { + params = HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build(); + acceptableParameters = new TreeMap<>(getOrderedComparator()); + } else { + params = HttpParameters.create().withParent(parameters).build(); + acceptableParameters = new TreeMap<>(); + } + + for (Map.Entry<String, Parameter> entry : params.entrySet()) { + String parameterName = entry.getKey(); + boolean isAcceptableParameter = isAcceptableParameter(parameterName, action); + isAcceptableParameter &= isAcceptableParameterValue(entry.getValue(), action); + + if (isAcceptableParameter) { + acceptableParameters.put(parameterName, entry.getValue()); + } + } + + ValueStack newStack = valueStackFactory.createValueStack(stack); + boolean clearableStack = newStack instanceof ClearableValueStack; + if (clearableStack) { + //if the stack's context can be cleared, do that to prevent OGNL + //from having access to objects in the stack, see XW-641 + ((ClearableValueStack) newStack).clearContextValues(); + Map<String, Object> context = newStack.getContext(); + ReflectionContextState.setCreatingNullObjects(context, true); + ReflectionContextState.setDenyMethodExecution(context, true); + ReflectionContextState.setReportingConversionErrors(context, true); + + //keep locale from original context + newStack.getActionContext().withLocale(stack.getActionContext().getLocale()).withValueStack(stack); + } + + boolean memberAccessStack = newStack instanceof MemberAccessValueStack; + if (memberAccessStack) { + //block or allow access to properties + //see WW-2761 for more details + MemberAccessValueStack accessValueStack = (MemberAccessValueStack) newStack; + accessValueStack.useAcceptProperties(acceptedPatterns.getAcceptedPatterns()); + accessValueStack.useExcludeProperties(excludedPatterns.getExcludedPatterns()); + } + + for (Map.Entry<String, Parameter> entry : acceptableParameters.entrySet()) { + String name = entry.getKey(); + Parameter value = entry.getValue(); + try { + newStack.setParameter(name, value.getObject()); + } catch (RuntimeException e) { + if (devMode) { + notifyDeveloperParameterException(action, name, e.getMessage()); + } + } + } + + if (clearableStack) { + stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors()); + } + + addParametersToContext(ActionContext.getContext(), acceptableParameters); + } + + protected void notifyDeveloperParameterException(Object action, String property, String message) { + String developerNotification = "Unexpected Exception caught setting '" + property + "' on '" + action.getClass() + ": " + message; + if (action instanceof TextProvider) { + TextProvider tp = (TextProvider) action; + developerNotification = tp.getText("devmode.notification", + "Developer Notification:\n{0}", + new String[]{developerNotification} + ); + } + + LOG.error(developerNotification); + + if (action instanceof ValidationAware) { + // see https://issues.apache.org/jira/browse/WW-4066 + Collection<String> messages = ((ValidationAware) action).getActionMessages(); + messages.add(message); + ((ValidationAware) action).setActionMessages(messages); + } + } + + /** + * Checks if name of parameter can be accepted or thrown away + * + * @param name parameter name + * @param action current action + * @return true if parameter is accepted + */ + protected boolean isAcceptableParameter(String name, Object action) { + ParameterNameAware parameterNameAware = (action instanceof ParameterNameAware) ? (ParameterNameAware) action : null; + return acceptableName(name) && (parameterNameAware == null || parameterNameAware.acceptableParameterName(name)); + } + + /** + * Checks if parameter value can be accepted or thrown away + * + * @param param the parameter + * @param action current action + * @return true if parameter is accepted + */ + protected boolean isAcceptableParameterValue(Parameter param, Object action) { + ParameterValueAware parameterValueAware = (action instanceof ParameterValueAware) ? (ParameterValueAware) action : null; + boolean acceptableParamValue = (parameterValueAware == null || parameterValueAware.acceptableParameterValue(param.getValue())); + if (hasParamValuesToExclude() || hasParamValuesToAccept()) { + // Additional validations to process + acceptableParamValue &= acceptableValue(param.getName(), param.getValue()); + } + return acceptableParamValue; + } + + /** + * Gets an instance of the comparator to use for the ordered sorting. Override this + * method to customize the ordering of the parameters as they are set to the + * action. + * + * @return A comparator to sort the parameters + */ + protected Comparator<String> getOrderedComparator() { + return rbCollator; + } + + protected String getParameterLogMap(HttpParameters parameters) { + if (parameters == null) { + return "NONE"; + } + + StringBuilder logEntry = new StringBuilder(); + for (Map.Entry<String, Parameter> entry : parameters.entrySet()) { + logEntry.append(entry.getKey()); + logEntry.append(" => "); + logEntry.append(entry.getValue().getValue()); + logEntry.append(" "); + } + + return logEntry.toString(); + } + + /** + * Validates the name passed is: + * * Within the max length of a parameter name + * * Is not excluded + * * Is accepted + * + * @param name - Name to check + * @return true if accepted + */ + protected boolean acceptableName(String name) { + if (isIgnoredDMI(name)) { + LOG.trace("DMI is enabled, ignoring DMI method: {}", name); + return false; + } + boolean accepted = isWithinLengthLimit(name) && !isExcluded(name) && isAccepted(name); + if (devMode && accepted) { // notify only when in devMode + LOG.debug("Parameter [{}] was accepted and will be appended to action!", name); + } + return accepted; + } + + private boolean isIgnoredDMI(String name) { + if (dmiEnabled) { + return DMI_IGNORED_PATTERN.matcher(name).matches(); + } else { + return false; + } + } + + /** + * Validates: + * * Value is null/blank + * * Value is not excluded + * * Value is accepted + * + * @param name - Param name (for logging) + * @param value - value to check + * @return true if accepted + */ + protected boolean acceptableValue(String name, String value) { + boolean accepted = (value == null || value.isEmpty() || (!isParamValueExcluded(value) && isParamValueAccepted(value))); + if (!accepted) { + String message = "Value [{}] of parameter [{}] was not accepted and will be dropped!"; + if (devMode) { + LOG.warn(message, normalizeSpace(value), normalizeSpace(name)); Review Comment: ## Logging should not be vulnerable to injection attacks <!--SONAR_ISSUE_KEY:AYzNdMMm8ZfvrnGxTt_i-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AYzNdMMm8ZfvrnGxTt_i&open=AYzNdMMm8ZfvrnGxTt_i&pullRequest=829">SonarCloud</a></p> [Show more details](https://github.com/apache/struts/security/code-scanning/393) ########## core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java: ########## @@ -0,0 +1,559 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor.parameter; + +import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.ActionInvocation; +import com.opensymphony.xwork2.TextProvider; +import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.interceptor.MethodFilterInterceptor; +import com.opensymphony.xwork2.interceptor.ValidationAware; +import com.opensymphony.xwork2.security.AcceptedPatternsChecker; +import com.opensymphony.xwork2.security.ExcludedPatternsChecker; +import com.opensymphony.xwork2.util.ClearableValueStack; +import com.opensymphony.xwork2.util.MemberAccessValueStack; +import com.opensymphony.xwork2.util.TextParseUtil; +import com.opensymphony.xwork2.util.ValueStack; +import com.opensymphony.xwork2.util.ValueStackFactory; +import com.opensymphony.xwork2.util.reflection.ReflectionContextState; +import org.apache.commons.lang3.BooleanUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.action.NoParameters; +import org.apache.struts2.dispatcher.HttpParameters; +import org.apache.struts2.dispatcher.Parameter; + +import java.util.Collection; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.regex.Pattern; + +import static org.apache.commons.lang3.StringUtils.normalizeSpace; + +/** + * This interceptor sets all parameters on the value stack. + */ +public class ParametersInterceptor extends MethodFilterInterceptor { + + private static final Logger LOG = LogManager.getLogger(ParametersInterceptor.class); + + protected static final int PARAM_NAME_MAX_LENGTH = 100; + + private static final Pattern DMI_IGNORED_PATTERN = Pattern.compile("^(action|method):.*", Pattern.CASE_INSENSITIVE); + + private int paramNameMaxLength = PARAM_NAME_MAX_LENGTH; + private boolean devMode = false; + private boolean dmiEnabled = false; + + protected boolean ordered = false; + + private ValueStackFactory valueStackFactory; + private ExcludedPatternsChecker excludedPatterns; + private AcceptedPatternsChecker acceptedPatterns; + private Set<Pattern> excludedValuePatterns = null; + private Set<Pattern> acceptedValuePatterns = null; + + @Inject + public void setValueStackFactory(ValueStackFactory valueStackFactory) { + this.valueStackFactory = valueStackFactory; + } + + @Inject(StrutsConstants.STRUTS_DEVMODE) + public void setDevMode(String mode) { + this.devMode = BooleanUtils.toBoolean(mode); + } + + @Inject + public void setExcludedPatterns(ExcludedPatternsChecker excludedPatterns) { + this.excludedPatterns = excludedPatterns; + } + + @Inject + public void setAcceptedPatterns(AcceptedPatternsChecker acceptedPatterns) { + this.acceptedPatterns = acceptedPatterns; + } + + @Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false) + protected void setDynamicMethodInvocation(String dmiEnabled) { + this.dmiEnabled = Boolean.parseBoolean(dmiEnabled); + } + + /** + * If the param name exceeds the configured maximum length it will not be + * accepted. + * + * @param paramNameMaxLength Maximum length of param names + */ + public void setParamNameMaxLength(int paramNameMaxLength) { + this.paramNameMaxLength = paramNameMaxLength; + } + + static private int countOGNLCharacters(String s) { + int count = 0; + for (int i = s.length() - 1; i >= 0; i--) { + char c = s.charAt(i); + if (c == '.' || c == '[') count++; + } + return count; + } + + /** + * Compares based on number of '.' and '[' characters (fewer is higher) + */ + static final Comparator<String> rbCollator = (s1, s2) -> { + int l1 = countOGNLCharacters(s1); + int l2 = countOGNLCharacters(s2); + return l1 < l2 ? -1 : (l2 < l1 ? 1 : s1.compareTo(s2)); + }; + + @Override + public String doIntercept(ActionInvocation invocation) throws Exception { + Object action = invocation.getAction(); + if (!(action instanceof NoParameters)) { + ActionContext ac = invocation.getInvocationContext(); + HttpParameters parameters = retrieveParameters(ac); + + if (LOG.isDebugEnabled()) { + LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters))); Review Comment: ## Logging should not be vulnerable to injection attacks <!--SONAR_ISSUE_KEY:AYzNw0sJG01541rIzgj_-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AYzNw0sJG01541rIzgj_&open=AYzNw0sJG01541rIzgj_&pullRequest=829">SonarCloud</a></p> [Show more details](https://github.com/apache/struts/security/code-scanning/394) ########## core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java: ########## @@ -0,0 +1,559 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor.parameter; + +import com.opensymphony.xwork2.ActionContext; +import com.opensymphony.xwork2.ActionInvocation; +import com.opensymphony.xwork2.TextProvider; +import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.interceptor.MethodFilterInterceptor; +import com.opensymphony.xwork2.interceptor.ValidationAware; +import com.opensymphony.xwork2.security.AcceptedPatternsChecker; +import com.opensymphony.xwork2.security.ExcludedPatternsChecker; +import com.opensymphony.xwork2.util.ClearableValueStack; +import com.opensymphony.xwork2.util.MemberAccessValueStack; +import com.opensymphony.xwork2.util.TextParseUtil; +import com.opensymphony.xwork2.util.ValueStack; +import com.opensymphony.xwork2.util.ValueStackFactory; +import com.opensymphony.xwork2.util.reflection.ReflectionContextState; +import org.apache.commons.lang3.BooleanUtils; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.StrutsConstants; +import org.apache.struts2.action.NoParameters; +import org.apache.struts2.dispatcher.HttpParameters; +import org.apache.struts2.dispatcher.Parameter; + +import java.util.Collection; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.regex.Pattern; + +import static org.apache.commons.lang3.StringUtils.normalizeSpace; + +/** + * This interceptor sets all parameters on the value stack. + */ +public class ParametersInterceptor extends MethodFilterInterceptor { + + private static final Logger LOG = LogManager.getLogger(ParametersInterceptor.class); + + protected static final int PARAM_NAME_MAX_LENGTH = 100; + + private static final Pattern DMI_IGNORED_PATTERN = Pattern.compile("^(action|method):.*", Pattern.CASE_INSENSITIVE); + + private int paramNameMaxLength = PARAM_NAME_MAX_LENGTH; + private boolean devMode = false; + private boolean dmiEnabled = false; + + protected boolean ordered = false; + + private ValueStackFactory valueStackFactory; + private ExcludedPatternsChecker excludedPatterns; + private AcceptedPatternsChecker acceptedPatterns; + private Set<Pattern> excludedValuePatterns = null; + private Set<Pattern> acceptedValuePatterns = null; + + @Inject + public void setValueStackFactory(ValueStackFactory valueStackFactory) { + this.valueStackFactory = valueStackFactory; + } + + @Inject(StrutsConstants.STRUTS_DEVMODE) + public void setDevMode(String mode) { + this.devMode = BooleanUtils.toBoolean(mode); + } + + @Inject + public void setExcludedPatterns(ExcludedPatternsChecker excludedPatterns) { + this.excludedPatterns = excludedPatterns; + } + + @Inject + public void setAcceptedPatterns(AcceptedPatternsChecker acceptedPatterns) { + this.acceptedPatterns = acceptedPatterns; + } + + @Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false) + protected void setDynamicMethodInvocation(String dmiEnabled) { + this.dmiEnabled = Boolean.parseBoolean(dmiEnabled); + } + + /** + * If the param name exceeds the configured maximum length it will not be + * accepted. + * + * @param paramNameMaxLength Maximum length of param names + */ + public void setParamNameMaxLength(int paramNameMaxLength) { + this.paramNameMaxLength = paramNameMaxLength; + } + + static private int countOGNLCharacters(String s) { + int count = 0; + for (int i = s.length() - 1; i >= 0; i--) { + char c = s.charAt(i); + if (c == '.' || c == '[') count++; + } + return count; + } + + /** + * Compares based on number of '.' and '[' characters (fewer is higher) + */ + static final Comparator<String> rbCollator = (s1, s2) -> { + int l1 = countOGNLCharacters(s1); + int l2 = countOGNLCharacters(s2); + return l1 < l2 ? -1 : (l2 < l1 ? 1 : s1.compareTo(s2)); + }; + + @Override + public String doIntercept(ActionInvocation invocation) throws Exception { + Object action = invocation.getAction(); + if (!(action instanceof NoParameters)) { + ActionContext ac = invocation.getInvocationContext(); + HttpParameters parameters = retrieveParameters(ac); + + if (LOG.isDebugEnabled()) { + LOG.debug("Setting params {}", normalizeSpace(getParameterLogMap(parameters))); + } + + if (parameters != null) { + Map<String, Object> contextMap = ac.getContextMap(); + try { + ReflectionContextState.setCreatingNullObjects(contextMap, true); + ReflectionContextState.setDenyMethodExecution(contextMap, true); + ReflectionContextState.setReportingConversionErrors(contextMap, true); + + ValueStack stack = ac.getValueStack(); + setParameters(action, stack, parameters); + } finally { + ReflectionContextState.setCreatingNullObjects(contextMap, false); + ReflectionContextState.setDenyMethodExecution(contextMap, false); + ReflectionContextState.setReportingConversionErrors(contextMap, false); + } + } + } + return invocation.invoke(); + } + + /** + * Gets the parameter map to apply from wherever appropriate + * + * @param ac The action context + * @return The parameter map to apply + */ + protected HttpParameters retrieveParameters(ActionContext ac) { + return ac.getParameters(); + } + + + /** + * Adds the parameters into context's ParameterMap + * + * @param ac The action context + * @param newParams The parameter map to apply + * <p> + * In this class this is a no-op, since the parameters were fetched from the same location. + * In subclasses both retrieveParameters() and addParametersToContext() should be overridden. + * </p> + */ + protected void addParametersToContext(ActionContext ac, Map<String, ?> newParams) { + } + + protected void setParameters(final Object action, ValueStack stack, HttpParameters parameters) { + HttpParameters params; + Map<String, Parameter> acceptableParameters; + if (ordered) { + params = HttpParameters.create().withComparator(getOrderedComparator()).withParent(parameters).build(); + acceptableParameters = new TreeMap<>(getOrderedComparator()); + } else { + params = HttpParameters.create().withParent(parameters).build(); + acceptableParameters = new TreeMap<>(); + } + + for (Map.Entry<String, Parameter> entry : params.entrySet()) { + String parameterName = entry.getKey(); + boolean isAcceptableParameter = isAcceptableParameter(parameterName, action); + isAcceptableParameter &= isAcceptableParameterValue(entry.getValue(), action); + + if (isAcceptableParameter) { + acceptableParameters.put(parameterName, entry.getValue()); + } + } + + ValueStack newStack = valueStackFactory.createValueStack(stack); + boolean clearableStack = newStack instanceof ClearableValueStack; + if (clearableStack) { + //if the stack's context can be cleared, do that to prevent OGNL + //from having access to objects in the stack, see XW-641 + ((ClearableValueStack) newStack).clearContextValues(); + Map<String, Object> context = newStack.getContext(); + ReflectionContextState.setCreatingNullObjects(context, true); + ReflectionContextState.setDenyMethodExecution(context, true); + ReflectionContextState.setReportingConversionErrors(context, true); + + //keep locale from original context + newStack.getActionContext().withLocale(stack.getActionContext().getLocale()).withValueStack(stack); + } + + boolean memberAccessStack = newStack instanceof MemberAccessValueStack; + if (memberAccessStack) { + //block or allow access to properties + //see WW-2761 for more details + MemberAccessValueStack accessValueStack = (MemberAccessValueStack) newStack; + accessValueStack.useAcceptProperties(acceptedPatterns.getAcceptedPatterns()); + accessValueStack.useExcludeProperties(excludedPatterns.getExcludedPatterns()); + } + + for (Map.Entry<String, Parameter> entry : acceptableParameters.entrySet()) { + String name = entry.getKey(); + Parameter value = entry.getValue(); + try { + newStack.setParameter(name, value.getObject()); + } catch (RuntimeException e) { + if (devMode) { + notifyDeveloperParameterException(action, name, e.getMessage()); + } + } + } + + if (clearableStack) { + stack.getActionContext().withConversionErrors(newStack.getActionContext().getConversionErrors()); + } + + addParametersToContext(ActionContext.getContext(), acceptableParameters); + } + + protected void notifyDeveloperParameterException(Object action, String property, String message) { + String developerNotification = "Unexpected Exception caught setting '" + property + "' on '" + action.getClass() + ": " + message; + if (action instanceof TextProvider) { + TextProvider tp = (TextProvider) action; + developerNotification = tp.getText("devmode.notification", + "Developer Notification:\n{0}", + new String[]{developerNotification} + ); + } + + LOG.error(developerNotification); + + if (action instanceof ValidationAware) { + // see https://issues.apache.org/jira/browse/WW-4066 + Collection<String> messages = ((ValidationAware) action).getActionMessages(); + messages.add(message); + ((ValidationAware) action).setActionMessages(messages); + } + } + + /** + * Checks if name of parameter can be accepted or thrown away + * + * @param name parameter name + * @param action current action + * @return true if parameter is accepted + */ + protected boolean isAcceptableParameter(String name, Object action) { + ParameterNameAware parameterNameAware = (action instanceof ParameterNameAware) ? (ParameterNameAware) action : null; + return acceptableName(name) && (parameterNameAware == null || parameterNameAware.acceptableParameterName(name)); + } + + /** + * Checks if parameter value can be accepted or thrown away + * + * @param param the parameter + * @param action current action + * @return true if parameter is accepted + */ + protected boolean isAcceptableParameterValue(Parameter param, Object action) { + ParameterValueAware parameterValueAware = (action instanceof ParameterValueAware) ? (ParameterValueAware) action : null; + boolean acceptableParamValue = (parameterValueAware == null || parameterValueAware.acceptableParameterValue(param.getValue())); + if (hasParamValuesToExclude() || hasParamValuesToAccept()) { + // Additional validations to process + acceptableParamValue &= acceptableValue(param.getName(), param.getValue()); + } + return acceptableParamValue; + } + + /** + * Gets an instance of the comparator to use for the ordered sorting. Override this + * method to customize the ordering of the parameters as they are set to the + * action. + * + * @return A comparator to sort the parameters + */ + protected Comparator<String> getOrderedComparator() { + return rbCollator; + } + + protected String getParameterLogMap(HttpParameters parameters) { + if (parameters == null) { + return "NONE"; + } + + StringBuilder logEntry = new StringBuilder(); + for (Map.Entry<String, Parameter> entry : parameters.entrySet()) { + logEntry.append(entry.getKey()); + logEntry.append(" => "); + logEntry.append(entry.getValue().getValue()); + logEntry.append(" "); + } + + return logEntry.toString(); + } + + /** + * Validates the name passed is: + * * Within the max length of a parameter name + * * Is not excluded + * * Is accepted + * + * @param name - Name to check + * @return true if accepted + */ + protected boolean acceptableName(String name) { + if (isIgnoredDMI(name)) { + LOG.trace("DMI is enabled, ignoring DMI method: {}", name); + return false; + } + boolean accepted = isWithinLengthLimit(name) && !isExcluded(name) && isAccepted(name); + if (devMode && accepted) { // notify only when in devMode + LOG.debug("Parameter [{}] was accepted and will be appended to action!", name); + } + return accepted; + } + + private boolean isIgnoredDMI(String name) { + if (dmiEnabled) { + return DMI_IGNORED_PATTERN.matcher(name).matches(); + } else { + return false; + } + } + + /** + * Validates: + * * Value is null/blank + * * Value is not excluded + * * Value is accepted + * + * @param name - Param name (for logging) + * @param value - value to check + * @return true if accepted + */ + protected boolean acceptableValue(String name, String value) { + boolean accepted = (value == null || value.isEmpty() || (!isParamValueExcluded(value) && isParamValueAccepted(value))); + if (!accepted) { + String message = "Value [{}] of parameter [{}] was not accepted and will be dropped!"; + if (devMode) { + LOG.warn(message, normalizeSpace(value), normalizeSpace(name)); + } else { + LOG.debug(message, normalizeSpace(value), normalizeSpace(name)); Review Comment: ## Logging should not be vulnerable to injection attacks <!--SONAR_ISSUE_KEY:AYzNw0sJG01541rIzgkA-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AYzNw0sJG01541rIzgkA&open=AYzNw0sJG01541rIzgkA&pullRequest=829">SonarCloud</a></p> [Show more details](https://github.com/apache/struts/security/code-scanning/395) Issue Time Tracking ------------------- Worklog Id: (was: 897753) Time Spent: 1h 20m (was: 1h 10m) > Implement annotation mechanism for injectable fields via parameters > ------------------------------------------------------------------- > > Key: WW-5352 > URL: https://issues.apache.org/jira/browse/WW-5352 > Project: Struts 2 > Issue Type: Improvement > Components: Core, Core Interceptors > Reporter: Kusal Kithul-Godage > Priority: Minor > Fix For: 6.4.0 > > Time Spent: 1h 20m > Remaining Estimate: 0h > > struts.parameters.requireAnnotations > > Require an explicit annotation '@StrutsParameter' on one of: > Getter/Setter/Field/ReturnType for injecting parameters. > > This mechanism is intended to be a more usable replacement for > 'ParameterNameAware' -- This message was sent by Atlassian Jira (v8.20.10#820010)