Merge pull request #980 from apache/7.0.x/merge-master-2024-07-08

Merge master to 7.0.x, 2024-07-08

Author: kusalk

core/src/main/java/com/opensymphony/xwork2/config/providers/EnvsValueSubstitutor.java

@@ -46,7 +46,7 @@ public EnvsValueSubstitutor() {
     public String substitute(String value) {
         LOG.debug("Substituting value {} with proper System variable or environment variable", value);
 
-        String substituted = sysStrSubstitutor.replace(value);
-        return envStrSubstitutor.replace(substituted);
+        String substituted = envStrSubstitutor.replace(value);
+        return sysStrSubstitutor.replace(substituted);
     }
 }

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 <code>(hasActionErrors() || hasFieldErrors())</code>
      */
-    boolean hasErrors();
+    default boolean hasErrors() {
+        return hasActionErrors() || hasFieldErrors();
+    }
 
     /**
      * Check whether there are any field errors associated with this action.

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 ");

core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java

@@ -47,7 +47,6 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.regex.Pattern;
 
 import static com.opensymphony.xwork2.util.ConfigParseUtil.toClassesSet;
@@ -68,9 +67,6 @@ public class OgnlUtil {
 
     private static final Logger LOG = LogManager.getLogger(OgnlUtil.class);
 
-    // Flag used to reduce flooding logs with WARNs about using DevMode excluded packages
-    private final AtomicBoolean warnReported = new AtomicBoolean(false);
-
     private final OgnlCache<String, Object> expressionCache;
     private final OgnlCache<Class<?>, BeanInfo> beanInfoCache;
     private TypeConverter defaultConverter;
@@ -80,11 +76,6 @@ public class OgnlUtil {
     private boolean enableExpressionCache = true;
     private boolean enableEvalExpression;
 
-    private String devModeExcludedClasses = "";
-    private String devModeExcludedPackageNamePatterns = "";
-    private String devModeExcludedPackageNames = "";
-    private String devModeExcludedPackageExemptClasses = "";
-
     private Container container;
 
     /**
@@ -164,9 +155,12 @@ protected void setExcludedClasses(String commaDelimitedClasses) {
         // Must be set directly on SecurityMemberAccess
     }
 
-    @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false)
+    /**
+     * @deprecated since 6.5.0, no replacement.
+     */
+    @Deprecated
     protected void setDevModeExcludedClasses(String commaDelimitedClasses) {
-        this.devModeExcludedClasses = commaDelimitedClasses;
+        // Must be set directly on SecurityMemberAccess
     }
 
     /**
@@ -177,9 +171,12 @@ protected void setExcludedPackageNamePatterns(String commaDelimitedPackagePatter
         // Must be set directly on SecurityMemberAccess
     }
 
-    @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false)
+    /**
+     * @deprecated since 6.5.0, no replacement.
+     */
+    @Deprecated
     protected void setDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) {
-        this.devModeExcludedPackageNamePatterns = commaDelimitedPackagePatterns;
+        // Must be set directly on SecurityMemberAccess
     }
 
     /**
@@ -190,9 +187,12 @@ protected void setExcludedPackageNames(String commaDelimitedPackageNames) {
         // Must be set directly on SecurityMemberAccess
     }
 
-    @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false)
+    /**
+     * @deprecated since 6.5.0, no replacement.
+     */
+    @Deprecated
     protected void setDevModeExcludedPackageNames(String commaDelimitedPackageNames) {
-        this.devModeExcludedPackageNames = commaDelimitedPackageNames;
+        // Must be set directly on SecurityMemberAccess
     }
 
     /**
@@ -203,9 +203,12 @@ public void setExcludedPackageExemptClasses(String commaDelimitedClasses) {
         // Must be set directly on SecurityMemberAccess
     }
 
-    @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false)
+    /**
+     * @deprecated since 6.5.0, no replacement.
+     */
+    @Deprecated
     public void setDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) {
-        this.devModeExcludedPackageExemptClasses = commaDelimitedClasses;
+        // Must be set directly on SecurityMemberAccess
     }
 
     /**
@@ -856,6 +859,11 @@ protected Map<String, Object> createDefaultContext(Object root) {
         return createDefaultContext(root, null);
     }
 
+    /**
+     * Note that the allowlist capability is not enforced by the {@link OgnlContext} returned by this method. Currently,
+     * this context is only leveraged by some public methods on {@link OgnlUtil} which are called by
+     * {@link OgnlReflectionProvider}.
+     */
     protected Map<String, Object> createDefaultContext(Object root, ClassResolver resolver) {
         if (resolver == null) {
             resolver = container.getInstance(RootAccessor.class);
@@ -867,17 +875,6 @@ protected Map<String, Object> createDefaultContext(Object root, ClassResolver re
         SecurityMemberAccess memberAccess = container.getInstance(SecurityMemberAccess.class);
         memberAccess.useEnforceAllowlistEnabled(Boolean.FALSE.toString());
 
-        if (devMode) {
-            if (!warnReported.get()) {
-                warnReported.set(true);
-                LOG.warn("Working in devMode, using devMode excluded classes and packages!");
-            }
-            memberAccess.useExcludedClasses(devModeExcludedClasses);
-            memberAccess.useExcludedPackageNamePatterns(devModeExcludedPackageNamePatterns);
-            memberAccess.useExcludedPackageNames(devModeExcludedPackageNames);
-            memberAccess.useExcludedPackageExemptClasses(devModeExcludedPackageExemptClasses);
-        }
-
         return Ognl.createDefaultContext(root, memberAccess, resolver, defaultConverter);
     }
 

core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

@@ -51,6 +51,8 @@
 import static java.util.Collections.emptySet;
 import static java.util.Collections.singletonList;
 import static java.util.Collections.unmodifiableSet;
+import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES;
+import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES;
 
 /**
  * Allows access decisions to be made on the basis of whether a member is static or not.
@@ -77,16 +79,28 @@ public class SecurityMemberAccess implements MemberAccess {
 
     private final ProviderAllowlist providerAllowlist;
     private final ThreadAllowlist threadAllowlist;
+
     private boolean allowStaticFieldAccess = true;
+
     private Set<Pattern> excludeProperties = emptySet();
     private Set<Pattern> acceptProperties = emptySet();
+
     private Set<String> excludedClasses = unmodifiableSet(new HashSet<>(singletonList(Object.class.getName())));
     private Set<Pattern> excludedPackageNamePatterns = emptySet();
     private Set<String> excludedPackageNames = emptySet();
     private Set<String> excludedPackageExemptClasses = emptySet();
+
+    private volatile boolean isDevModeInit;
+    private boolean isDevMode;
+    private Set<String> devModeExcludedClasses = unmodifiableSet(new HashSet<>(singletonList(Object.class.getName())));
+    private Set<Pattern> devModeExcludedPackageNamePatterns = emptySet();
+    private Set<String> devModeExcludedPackageNames = emptySet();
+    private Set<String> devModeExcludedPackageExemptClasses = emptySet();
+
     private boolean enforceAllowlistEnabled = false;
     private Set<Class<?>> allowlistClasses = emptySet();
     private Set<String> allowlistPackageNames = emptySet();
+
     private boolean disallowProxyObjectAccess = false;
     private boolean disallowProxyMemberAccess = false;
     private boolean disallowDefaultPackageAccess = false;
@@ -209,25 +223,71 @@ public boolean isAccessible(Map context, Object target, Member member, String pr
      * @return {@code true} if member access is allowed
      */
     protected boolean checkAllowlist(Object target, Member member) {
-        Class<?> memberClass = member.getDeclaringClass();
         if (!enforceAllowlistEnabled) {
+            logAllowlistDisabled();
             return true;
         }
+
+        if (!disallowProxyObjectAccess && target != null && ProxyUtil.isProxy(target)) {
+            // If `disallowProxyObjectAccess` is not set, allow resolving Hibernate entities to their underlying
+            // classes/members. This allows the allowlist capability to continue working and offer some level of
+            // protection in applications where the developer has accepted the risk of allowing OGNL access to Hibernate
+            // entities. This is preferred to having to disable the allowlist capability entirely.
+            Object newTarget = ProxyUtil.getHibernateProxyTarget(target);
+            if (newTarget != target) {
+                logAllowlistHibernateEntity(target, newTarget);
+                target = newTarget;
+                member = ProxyUtil.resolveTargetMember(member, newTarget);
+            }
+        }
+
+        Class<?> memberClass = member.getDeclaringClass();
         if (!isClassAllowlisted(memberClass)) {
-            LOG.warn(format("Declaring class [{0}] of member type [{1}] is not allowlisted!", memberClass, member));
+            LOG.warn("Declaring class [{}] of member type [{}] is not allowlisted! Add to '{}' or '{}' configuration.",
+                    memberClass, member, STRUTS_ALLOWLIST_CLASSES, STRUTS_ALLOWLIST_PACKAGE_NAMES);
             return false;
         }
         if (target == null || target.getClass() == memberClass) {
             return true;
         }
         Class<?> targetClass = target.getClass();
         if (!isClassAllowlisted(targetClass)) {
-            LOG.warn(format("Target class [{0}] of target [{1}] is not allowlisted!", targetClass, target));
+            LOG.warn("Target class [{}] of target [{}] is not allowlisted! Add to '{}' or '{}' configuration.",
+                    targetClass, target, STRUTS_ALLOWLIST_CLASSES, STRUTS_ALLOWLIST_PACKAGE_NAMES);
             return false;
         }
         return true;
     }
 
+    private void logAllowlistDisabled() {
+        if (!isDevMode && !LOG.isDebugEnabled()) {
+            return;
+        }
+        String msg = "OGNL allowlist is disabled!" +
+                " We strongly recommend keeping it enabled to protect against critical vulnerabilities." +
+                " Set the configuration `{0}=true` to enable it.";
+        Object[] args = {StrutsConstants.STRUTS_ALLOWLIST_ENABLE};
+        if (isDevMode) {
+            LOG.warn(msg, args);
+        } else {
+            LOG.debug(msg, args);
+        }
+    }
+
+    private void logAllowlistHibernateEntity(Object original, Object resolved) {
+        if (!isDevMode && !LOG.isDebugEnabled()) {
+            return;
+        }
+        String msg = "Hibernate entity [{}] resolved to [{}] for purpose of OGNL allowlisting." +
+                " We don't recommend executing OGNL expressions against Hibernate entities, you may disallow this behaviour using the configuration `{}=true`.";
+        Object[] args = {original, resolved, StrutsConstants.STRUTS_DISALLOW_PROXY_OBJECT_ACCESS};
+        if (isDevMode) {
+            LOG.warn(msg, args);
+        } else {
+            LOG.debug(msg, args);
+        }
+    }
+
     protected boolean isClassAllowlisted(Class<?> clazz) {
         return allowlistClasses.contains(clazz)
                 || ALLOWLIST_REQUIRED_CLASSES.contains(clazz)
@@ -241,6 +301,7 @@ protected boolean isClassAllowlisted(Class<?> clazz) {
      * @return {@code true} if member access is allowed
      */
     protected boolean checkExclusionList(Object target, Member member) {
+        useDevModeConfiguration();
         Class<?> memberClass = member.getDeclaringClass();
         if (isClassExcluded(memberClass)) {
             LOG.warn("Declaring class of member type [{}] is excluded!", memberClass);
@@ -436,12 +497,12 @@ public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) {
         this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled);
     }
 
-    @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_CLASSES, required = false)
+    @Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false)
     public void useAllowlistClasses(String commaDelimitedClasses) {
         this.allowlistClasses = toClassObjectsSet(commaDelimitedClasses);
     }
 
-    @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false)
+    @Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false)
     public void useAllowlistPackageNames(String commaDelimitedPackageNames) {
         this.allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames);
     }
@@ -460,4 +521,41 @@ public void useDisallowProxyMemberAccess(String disallowProxyMemberAccess) {
     public void useDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) {
         this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess);
     }
+
+    @Inject(StrutsConstants.STRUTS_DEVMODE)
+    protected void useDevMode(String devMode) {
+        this.isDevMode = BooleanUtils.toBoolean(devMode);
+    }
+
+    @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false)
+    public void useDevModeExcludedClasses(String commaDelimitedClasses) {
+        this.devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses);
+    }
+
+    @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false)
+    public void useDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) {
+        this.devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns);
+    }
+
+    @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false)
+    public void useDevModeExcludedPackageNames(String commaDelimitedPackageNames) {
+        this.devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames);
+    }
+
+    @Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false)
+    public void useDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) {
+        this.devModeExcludedPackageExemptClasses = toClassesSet(commaDelimitedClasses);
+    }
+
+    private void useDevModeConfiguration() {
+        if (!isDevMode || isDevModeInit) {
+            return;
+        }
+        isDevModeInit = true;
+        LOG.warn("Working in devMode, using devMode excluded classes and packages!");
+        excludedClasses = devModeExcludedClasses;
+        excludedPackageNamePatterns = devModeExcludedPackageNamePatterns;
+        excludedPackageNames = devModeExcludedPackageNames;
+        excludedPackageExemptClasses = devModeExcludedPackageExemptClasses;
+    }
 }

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);
+        }
+    }
+
+}