[
https://issues.apache.org/jira/browse/WW-5352?focusedWorklogId=897750&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-897750
]
ASF GitHub Bot logged work on WW-5352:
--------------------------------------
Author: ASF GitHub Bot
Created on: 03/Jan/24 03:56
Start Date: 03/Jan/24 03:56
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_r1440047404
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -0,0 +1,557 @@
+/*
+ * 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;
+
+/**
+ * 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 {}", getParameterLogMap(parameters));
Review Comment:
## Logging should not be vulnerable to injection attacks
<!--SONAR_ISSUE_KEY:AYzNdMMm8ZfvrnGxTt_h-->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_h&open=AYzNdMMm8ZfvrnGxTt_h&pullRequest=829";>SonarCloud</a></p>
[Show more
details](https://github.com/apache/struts/security/code-scanning/391)
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -0,0 +1,557 @@
+/*
+ * 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;
+
+/**
+ * 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 {}", 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, value, name);
Review Comment:
## Logging should not be vulnerable to injection attacks
<!--SONAR_ISSUE_KEY:AYzNdMMm8ZfvrnGxTt_g-->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_g&open=AYzNdMMm8ZfvrnGxTt_g&pullRequest=829";>SonarCloud</a></p>
[Show more
details](https://github.com/apache/struts/security/code-scanning/390)
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java:
##########
@@ -0,0 +1,557 @@
+/*
+ * 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;
+
+/**
+ * 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 {}", 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, value, name);
+ } else {
+ LOG.debug(message, value, 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/392)
Issue Time Tracking
-------------------
Worklog Id: (was: 897750)
Time Spent: 50m (was: 40m)
> 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: 50m
> 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)
