Merge pull request #1010 from apache/7.0.x/WW-5454-log-security-warnings

WW-5454 Log warnings on startup for disabled critical security features
apache · Aug 6, 2024 · 3267a1a · 3267a1a
2 parents 817b0cd + f41fe80
commit 3267a1a
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 5 deletions.
diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java
@@ -46,6 +46,7 @@
 import static com.opensymphony.xwork2.util.ConfigParseUtil.toNewPackageNamesSet;
 import static com.opensymphony.xwork2.util.ConfigParseUtil.toNewPatternsSet;
 import static com.opensymphony.xwork2.util.ConfigParseUtil.toPackageNamesSet;
+import static com.opensymphony.xwork2.util.DebugUtils.logWarningForFirstOccurrence;
 import static java.text.MessageFormat.format;
 import static java.util.Collections.emptySet;
 import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES;
@@ -87,7 +88,6 @@ public class SecurityMemberAccess implements MemberAccess {
     private Set<String> excludedPackageNames = emptySet();
     private Set<String> excludedPackageExemptClasses = emptySet();
 
-    private static volatile boolean isDevModeLogged = false;
     private volatile boolean isDevModeInit;
     private boolean isDevMode;
     private Set<String> devModeExcludedClasses = Set.of(Object.class.getName());
@@ -459,6 +459,13 @@ public void useExcludedPackageExemptClasses(String commaDelimitedClasses) {
     @Inject(value = StrutsConstants.STRUTS_ALLOWLIST_ENABLE, required = false)
     public void useEnforceAllowlistEnabled(String enforceAllowlistEnabled) {
         this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled);
+        if (!this.enforceAllowlistEnabled) {
+            String msg = "OGNL allowlist is disabled!" +
+                    " We strongly recommend keeping it enabled to protect against critical vulnerabilities." +
+                    " Set the configuration `{}=true` to enable it." +
+                    " Please refer to the Struts 7.0 migration guide and security documentation for further information.";
+            logWarningForFirstOccurrence("allowlist", LOG, msg, StrutsConstants.STRUTS_ALLOWLIST_ENABLE);
+        }
     }
 
     @Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false)
@@ -515,11 +522,9 @@ private void useDevModeConfiguration() {
         if (!isDevMode || isDevModeInit) {
             return;
         }
+        logWarningForFirstOccurrence("devMode", LOG,
+                "DevMode enabled, using DevMode excluded classes and packages for OGNL security enforcement!");
         isDevModeInit = true;
-        if (!isDevModeLogged) {
-            LOG.warn("Working in devMode, using devMode excluded classes and packages!");
-            isDevModeLogged = true;
-        }
         excludedClasses = devModeExcludedClasses;
         excludedPackageNamePatterns = devModeExcludedPackageNamePatterns;
         excludedPackageNames = devModeExcludedPackageNames;

diff --git a/core/src/main/java/com/opensymphony/xwork2/util/DebugUtils.java b/core/src/main/java/com/opensymphony/xwork2/util/DebugUtils.java
@@ -22,11 +22,16 @@
 import com.opensymphony.xwork2.interceptor.ValidationAware;
 import org.apache.logging.log4j.Logger;
 
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
 /**
  * @since 6.5.0
  */
 public final class DebugUtils {
 
+    private static final Set<String> IS_LOGGED = ConcurrentHashMap.newKeySet();
+
     public static void notifyDeveloperOfError(Logger log, Object action, String message) {
         if (action instanceof TextProvider tp) {
             message = tp.getText("devmode.notification", "Developer Notification:\n{0}", new String[]{message});
@@ -37,4 +42,12 @@ public static void notifyDeveloperOfError(Logger log, Object action, String mess
         }
     }
 
+    /**
+     * @since 7.0
+     */
+    public static void logWarningForFirstOccurrence(String key, Logger log, String msg, Object... args) {
+        if (IS_LOGGED.add(key)) {
+            log.warn(msg, args);
+        }
+    }
 }
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
@@ -64,6 +64,7 @@
 
 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.logWarningForFirstOccurrence;
 import static com.opensymphony.xwork2.util.DebugUtils.notifyDeveloperOfError;
 import static java.lang.String.format;
 import static java.util.Collections.unmodifiableSet;
@@ -115,6 +116,13 @@ public void setDevMode(String mode) {
     @Inject(value = StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS, required = false)
     public void setRequireAnnotations(String requireAnnotations) {
         this.requireAnnotations = BooleanUtils.toBoolean(requireAnnotations);
+        if (!this.requireAnnotations) {
+            String msg = "@StrutsParameter annotation requirement is disabled!" +
+                    " We strongly recommend keeping it enabled to protect against critical vulnerabilities." +
+                    " Set the configuration `{}=true` to enable it." +
+                    " Please refer to the Struts 7.0 migration guide and security documentation for further information.";
+            logWarningForFirstOccurrence("strutsParameter", LOG, msg, StrutsConstants.STRUTS_PARAMETERS_REQUIRE_ANNOTATIONS);
+        }
     }
 
     /**