[GitHub] [struts] lukaszlenart commented on a change in pull request #469: WW-5115 Reduce logging for DMI excluded parameters.

[GitBox <gi...@apache.org>]

lukaszlenart commented on a change in pull request #469:
URL: https://github.com/apache/struts/pull/469#discussion_r563171653



##########
File path: core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
##########
@@ -324,12 +324,12 @@ protected boolean isAccepted(String paramName) {
     protected boolean isExcluded(String paramName) {
         ExcludedPatternsChecker.IsExcluded result = excludedPatterns.isExcluded(paramName);
         if (result.isExcluded()) {
-            if (devMode) { // warn only when in devMode
+            if (devMode && result.isLog()) { // warn only when in devMode and required

Review comment:
       What's the reason of ignoring some patterns and some not in logging?

##########
File path: core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java
##########
@@ -84,6 +85,14 @@ protected void setDynamicMethodInvocation(String dmiValue) {
         if (!BooleanUtils.toBoolean(dmiValue)) {
             LOG.debug("DMI is disabled, adding DMI related excluded patterns");
             setAdditionalExcludePatterns("^(action|method):.*");
+        } else {
+            LOG.debug("DMI is enabled, adding DMI related ignored patterns");
+            ignoredPatterns = new HashSet<>();
+            try {
+                ignoredPatterns.add(Pattern.compile("^(action|method):.*", Pattern.CASE_INSENSITIVE));
+            } finally {
+            	ignoredPatterns = Collections.unmodifiableSet(ignoredPatterns);
+            }

Review comment:
       Basically right now it doesn't matter if DMI is enabled or not, `action|method` will be excluded. Wouldn't be better to just extend the base RegEx? Or setup this pattern in constructor?
   
   ```java
   public DefaultExcludedPatternsChecker() {
       setExcludedPatterns(EXCLUDED_PATTERNS);
       setAdditionalExcludePatterns("^(action|method):.*");
   }
   ```
   
   So then additional `ignoredPatterns` field is not needed.
   

##########
File path: core/src/main/java/com/opensymphony/xwork2/security/ExcludedPatternsChecker.java
##########
@@ -66,18 +66,24 @@
 
         private final boolean excluded;
         private final String excludedPattern;
+        private final boolean log;

Review comment:
       I'm not a fun of such flags as they are quite misleading and control something out of scope of this class.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org