diff --git a/core/src/main/java/com/opensymphony/xwork2/interceptor/ValidationAware.java b/core/src/main/java/com/opensymphony/xwork2/interceptor/ValidationAware.java index c19d099f90..485cb42fb6 100644 --- a/core/src/main/java/com/opensymphony/xwork2/interceptor/ValidationAware.java +++ b/core/src/main/java/com/opensymphony/xwork2/interceptor/ValidationAware.java @@ -25,8 +25,6 @@ /** * ValidationAware classes can accept Action (class level) or field level error messages. Action level messages are kept * in a Collection. Field level error messages are kept in a Map from String field name to a List of field error msgs. - * - * @author plightbo */ public interface ValidationAware { @@ -119,7 +117,9 @@ public interface ValidationAware { * * @return (hasActionErrors() || hasFieldErrors()) */ - boolean hasErrors(); + default boolean hasErrors() { + return hasActionErrors() || hasFieldErrors(); + } /** * Check whether there are any field errors associated with this action. diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/ErrorMessageBuilder.java b/core/src/main/java/com/opensymphony/xwork2/ognl/ErrorMessageBuilder.java index b904f4ba12..ad54b2015d 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/ErrorMessageBuilder.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/ErrorMessageBuilder.java @@ -33,7 +33,7 @@ private ErrorMessageBuilder() { } public ErrorMessageBuilder errorSettingExpressionWithValue(String expr, Object value) { - appenExpression(expr); + appendExpression(expr); if (value instanceof Object[]) { appendValueAsArray((Object[]) value, message); } else { @@ -42,7 +42,7 @@ public ErrorMessageBuilder errorSettingExpressionWithValue(String expr, Object v return this; } - private void appenExpression(String expr) { + private void appendExpression(String expr) { message.append("Error setting expression '"); message.append(expr); message.append("' with value "); diff --git a/core/src/main/java/com/opensymphony/xwork2/util/DebugUtils.java b/core/src/main/java/com/opensymphony/xwork2/util/DebugUtils.java new file mode 100644 index 0000000000..3fdf8b0a70 --- /dev/null +++ b/core/src/main/java/com/opensymphony/xwork2/util/DebugUtils.java @@ -0,0 +1,42 @@ +/* + * 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 com.opensymphony.xwork2.util; + +import com.opensymphony.xwork2.TextProvider; +import com.opensymphony.xwork2.interceptor.ValidationAware; +import org.apache.logging.log4j.Logger; + +/** + * @since 6.5.0 + */ +public final class DebugUtils { + + public static void notifyDeveloperOfError(Logger log, Object action, String message) { + if (action instanceof TextProvider) { + TextProvider tp = (TextProvider) action; + message = tp.getText("devmode.notification", "Developer Notification:\n{0}", new String[]{message}); + } + log.error(message); + if (action instanceof ValidationAware) { + ValidationAware validationAware = (ValidationAware) action; + validationAware.addActionError(message); + } + } + +} diff --git a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java index e9215e5339..239bc6d6ca 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java @@ -20,10 +20,8 @@ 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.DefaultAcceptedPatternsChecker; import com.opensymphony.xwork2.security.ExcludedPatternsChecker; @@ -56,7 +54,6 @@ import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.util.Arrays; -import java.util.Collection; import java.util.Comparator; import java.util.HashSet; import java.util.Map; @@ -67,6 +64,8 @@ import static com.opensymphony.xwork2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS; import static com.opensymphony.xwork2.security.DefaultAcceptedPatternsChecker.NESTING_CHARS_STR; +import static com.opensymphony.xwork2.util.DebugUtils.notifyDeveloperOfError; +import static java.lang.String.format; import static java.util.Collections.unmodifiableSet; import static java.util.stream.Collectors.joining; import static org.apache.commons.lang3.StringUtils.indexOfAny; @@ -317,19 +316,8 @@ protected void applyParametersOnStack(ValueStack stack, Map p } protected void notifyDeveloperParameterException(Object action, String property, String message) { - String logMsg = "Unexpected Exception caught setting '" + property + "' on '" + action.getClass() + ": " + message; - if (action instanceof TextProvider) { - TextProvider tp = (TextProvider) action; - logMsg = tp.getText("devmode.notification", "Developer Notification:\n{0}", new String[]{logMsg}); - } - LOG.error(logMsg); - - if (action instanceof ValidationAware) { - ValidationAware validationAware = (ValidationAware) action; - Collection messages = validationAware.getActionMessages(); - messages.add(message); - validationAware.setActionMessages(messages); - } + String logMsg = format("Unexpected Exception caught setting '%s' on '%s: %s", property, action.getClass(), message); + notifyDeveloperOfError(LOG, action, logMsg); } /** @@ -388,23 +376,37 @@ protected boolean hasValidAnnotatedMember(String rootProperty, Object action, lo return hasValidAnnotatedField(action, rootProperty, paramDepth); } - if (hasValidAnnotatedPropertyDescriptor(propDescOpt.get(), paramDepth)) { + if (hasValidAnnotatedPropertyDescriptor(action, propDescOpt.get(), paramDepth)) { return true; } return hasValidAnnotatedField(action, rootProperty, paramDepth); } + /** + * @deprecated since 6.5.0, use {@link #hasValidAnnotatedPropertyDescriptor(Object, PropertyDescriptor, long)} + * instead. + */ + @Deprecated protected boolean hasValidAnnotatedPropertyDescriptor(PropertyDescriptor propDesc, long paramDepth) { + return hasValidAnnotatedPropertyDescriptor(null, propDesc, paramDepth); + } + + protected boolean hasValidAnnotatedPropertyDescriptor(Object action, PropertyDescriptor propDesc, long paramDepth) { Method relevantMethod = paramDepth == 0 ? propDesc.getWriteMethod() : propDesc.getReadMethod(); if (relevantMethod == null) { return false; } if (getPermittedInjectionDepth(relevantMethod) < paramDepth) { - LOG.debug( - "Parameter injection for method [{}] on action [{}] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", + String logMessage = format( + "Parameter injection for method [%s] on action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", relevantMethod.getName(), relevantMethod.getDeclaringClass().getName()); + if (devMode) { + notifyDeveloperOfError(LOG, action, logMessage); + } else { + LOG.debug(logMessage); + } return false; } if (paramDepth >= 1) { @@ -455,10 +457,15 @@ protected boolean hasValidAnnotatedField(Object action, String fieldName, long p return false; } if (getPermittedInjectionDepth(field) < paramDepth) { - LOG.debug( - "Parameter injection for field [{}] on action [{}] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", + String logMessage = format( + "Parameter injection for field [%s] on action [%s] rejected. Ensure it is annotated with @StrutsParameter with an appropriate 'depth'.", fieldName, action.getClass().getName()); + if (devMode) { + notifyDeveloperOfError(LOG, action, logMessage); + } else { + LOG.debug(logMessage); + } return false; } if (paramDepth >= 1) { @@ -533,7 +540,7 @@ protected String getParameterLogMap(HttpParameters parameters) { return "NONE"; } return parameters.entrySet().stream() - .map(entry -> String.format("%s => %s ", entry.getKey(), entry.getValue().getValue())) + .map(entry -> format("%s => %s ", entry.getKey(), entry.getValue().getValue())) .collect(joining()); } diff --git a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java index a142014b3e..b15594f956 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java @@ -116,15 +116,17 @@ public void testInsecureParameters() throws Exception { pi.setParameters(action, vs, HttpParameters.create(params).build()); // then - assertEquals(3, action.getActionMessages().size()); + assertEquals(3, action.getActionErrors().size()); - String msg1 = action.getActionMessage(0); - String msg2 = action.getActionMessage(1); - String msg3 = action.getActionMessage(2); + List actionErrors = new ArrayList<>(action.getActionErrors()); - assertEquals("Error setting expression 'expression' with value '#f=#_memberAccess.getClass().getDeclaredField('allowStaticMethodAccess'),#f.setAccessible(true),#f.set(#_memberAccess,true),#req=@org.apache.struts2.ServletActionContext@getRequest(),#resp=@org.apache.struts2.ServletActionContext@getResponse().getWriter(),#resp.println(#req.getRealPath('/')),#resp.close()'", msg1); - assertEquals("Error setting expression 'name' with value '(#context[\"xwork.MethodAccessor.denyMethodExecution\"]= new java.lang.Boolean(false), #_memberAccess[\"allowStaticMethodAccess\"]= new java.lang.Boolean(true), @java.lang.Runtime@getRuntime().exec('mkdir /tmp/PWNAGE'))(meh)'", msg2); - assertEquals("Error setting expression 'top['name'](0)' with value 'true'", msg3); + String msg1 = actionErrors.get(0); + String msg2 = actionErrors.get(1); + String msg3 = actionErrors.get(2); + + assertEquals("Unexpected Exception caught setting 'expression' on 'class org.apache.struts2.interceptor.parameter.ValidateAction: Error setting expression 'expression' with value '#f=#_memberAccess.getClass().getDeclaredField('allowStaticMethodAccess'),#f.setAccessible(true),#f.set(#_memberAccess,true),#req=@org.apache.struts2.ServletActionContext@getRequest(),#resp=@org.apache.struts2.ServletActionContext@getResponse().getWriter(),#resp.println(#req.getRealPath('/')),#resp.close()'", msg1); + assertEquals("Unexpected Exception caught setting 'name' on 'class org.apache.struts2.interceptor.parameter.ValidateAction: Error setting expression 'name' with value '(#context[\"xwork.MethodAccessor.denyMethodExecution\"]= new java.lang.Boolean(false), #_memberAccess[\"allowStaticMethodAccess\"]= new java.lang.Boolean(true), @java.lang.Runtime@getRuntime().exec('mkdir /tmp/PWNAGE'))(meh)'", msg2); + assertEquals("Unexpected Exception caught setting 'top['name'](0)' on 'class org.apache.struts2.interceptor.parameter.ValidateAction: Error setting expression 'top['name'](0)' with value 'true'", msg3); assertNull(action.getName()); } @@ -201,15 +203,16 @@ protected boolean isExcluded(String paramName) { pi.setParameters(action, vs, HttpParameters.create(params).build()); // then - assertEquals(3, action.getActionMessages().size()); + assertEquals(3, action.getActionErrors().size()); - String msg1 = action.getActionMessage(0); - String msg2 = action.getActionMessage(1); - String msg3 = action.getActionMessage(2); + List actionErrors = new ArrayList<>(action.getActionErrors()); + String msg1 = actionErrors.get(0); + String msg2 = actionErrors.get(1); + String msg3 = actionErrors.get(2); - assertEquals("Error setting expression 'class.classLoader.defaultAssertionStatus' with value 'true'", msg1); - assertEquals("Error setting expression 'class.classLoader.jarPath' with value 'bad'", msg2); - assertEquals("Error setting expression 'model.class.classLoader.jarPath' with value 'very bad'", msg3); + assertEquals("Unexpected Exception caught setting 'class.classLoader.defaultAssertionStatus' on 'class org.apache.struts2.interceptor.parameter.ValidateAction: Error setting expression 'class.classLoader.defaultAssertionStatus' with value 'true'", msg1); + assertEquals("Unexpected Exception caught setting 'class.classLoader.jarPath' on 'class org.apache.struts2.interceptor.parameter.ValidateAction: Error setting expression 'class.classLoader.jarPath' with value 'bad'", msg2); + assertEquals("Unexpected Exception caught setting 'model.class.classLoader.jarPath' on 'class org.apache.struts2.interceptor.parameter.ValidateAction: Error setting expression 'model.class.classLoader.jarPath' with value 'very bad'", msg3); assertFalse(excluded.get(pollution1)); assertFalse(excluded.get(pollution2)); @@ -582,8 +585,8 @@ public void testNonexistentParametersGetLoggedInDevMode() throws Exception { container.inject(config.getInterceptors().get(0).getInterceptor()); ActionProxy proxy = actionProxyFactory.createActionProxy("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME, null, extraContext.getContextMap()); proxy.execute(); - final String actionMessage = "" + ((SimpleAction) proxy.getAction()).getActionMessages().toArray()[0]; - assertTrue(actionMessage.contains("Error setting expression 'not_a_property' with value 'There is no action property named like this'")); + final String actionError = "" + ((SimpleAction) proxy.getAction()).getActionErrors().toArray()[0]; + assertTrue(actionError.contains("Error setting expression 'not_a_property' with value 'There is no action property named like this'")); } public void testNonexistentParametersAreIgnoredInProductionMode() throws Exception { @@ -1014,59 +1017,65 @@ protected void setUp() throws Exception { class ValidateAction implements ValidationAware { private final List messages = new LinkedList<>(); + private final List errors = new LinkedList<>(); private String name; + @Override public void setActionErrors(Collection errorMessages) { } + @Override public Collection getActionErrors() { - return null; + return errors; } + @Override public void setActionMessages(Collection messages) { } + @Override public Collection getActionMessages() { return messages; } + @Override public void setFieldErrors(Map> errorMap) { } + @Override public Map> getFieldErrors() { return null; } + @Override public void addActionError(String anErrorMessage) { + errors.add(anErrorMessage); } + @Override public void addActionMessage(String aMessage) { messages.add(aMessage); } + @Override public void addFieldError(String fieldName, String errorMessage) { } + @Override public boolean hasActionErrors() { - return false; + return !errors.isEmpty(); } + @Override public boolean hasActionMessages() { return !messages.isEmpty(); } - public boolean hasErrors() { - return false; - } - + @Override public boolean hasFieldErrors() { return false; } - public String getActionMessage(int index) { - return messages.get(index); - } - public String getName() { return name; }