Merge pull request #760 from apache/WW-5340-subclassable

WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing
apache · Oct 6, 2023 · fc03a2b · fc03a2b
2 parents 46c29ae + f4029f8
commit fc03a2b
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 28 deletions.
diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
@@ -100,7 +100,7 @@ public void parse(HttpServletRequest request, String saveDir) throws IOException
     protected void processUpload(HttpServletRequest request, String saveDir) throws FileUploadException, UnsupportedEncodingException {
         if (ServletFileUpload.isMultipartContent(request)) {
             for (FileItem item : parseRequest(request, saveDir)) {
-                LOG.debug("Found file item: [{}]", item.getFieldName());
+                LOG.debug("Found file item: [{}]", sanitizeNewlines(item.getFieldName()));
                 if (item.isFormField()) {
                     processNormalFormField(item, request.getCharacterEncoding());
                 } else {
@@ -115,7 +115,7 @@ protected void processFileField(FileItem item) {
 
         // Skip file uploads that don't have a file name - meaning that no file was selected.
         if (item.getName() == null || item.getName().trim().isEmpty()) {
-            LOG.debug("No file has been uploaded for the field: {}", item.getFieldName());
+            LOG.debug("No file has been uploaded for the field: {}", sanitizeNewlines(item.getFieldName()));
             return;
         }
 
@@ -142,26 +142,22 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu
             }
 
             long size = item.getSize();
-            if (size == 0) {
-                values.add(StringUtils.EMPTY);
-            } else if (size > maxStringLength) {
+            if (size > maxStringLength) {
+                LOG.debug("Form field {} of size {} bytes exceeds limit of {}.", sanitizeNewlines(item.getFieldName()), size, maxStringLength);
                 String errorKey = "struts.messages.upload.error.parameter.too.long";
                 LocalizedMessage localizedMessage = new LocalizedMessage(this.getClass(), errorKey, null,
-                    new Object[]{item.getFieldName(), maxStringLength, size});
-
+                        new Object[]{item.getFieldName(), maxStringLength, size});
                 if (!errors.contains(localizedMessage)) {
                     errors.add(localizedMessage);
                 }
                 return;
-
-            } else if (charset != null) {
-                values.add(item.getString(charset));
+            }
+            if (size == 0) {
+                values.add(StringUtils.EMPTY);
+            } else if (charset == null) {
+                values.add(item.getString()); // WW-633
             } else {
-                // note: see https://issues.apache.org/jira/browse/WW-633
-                // basically, in some cases the charset may be null, so
-                // we're just going to try to "other" method (no idea if this
-                // will work)
-                values.add(item.getString());
+                values.add(item.getString(charset));
             }
             params.put(item.getFieldName(), values);
         } finally {
@@ -366,4 +362,7 @@ public void cleanUp() {
         }
     }
 
+    private String sanitizeNewlines(String before) {
+        return before.replaceAll("[\n\r]", "_");
+    }
 }
diff --git a/core/src/main/java/org/apache/struts2/ognl/StrutsOgnlGuard.java b/core/src/main/java/org/apache/struts2/ognl/StrutsOgnlGuard.java
@@ -71,28 +71,38 @@ public boolean isRawExpressionBlocked(String expr) {
 
     @Override
     public boolean isParsedTreeBlocked(Object tree) {
-        return containsExcludedNodeType(tree);
+        if (!(tree instanceof Node) || skipTreeCheck((Node) tree)) {
+            return false;
+        }
+        return recurseNodes((Node) tree);
     }
 
-    protected boolean containsExcludedNodeType(Object tree) {
-        if (!(tree instanceof Node) || excludedNodeTypes.isEmpty()) {
-            return false;
+    protected boolean skipTreeCheck(Node tree) {
+        return excludedNodeTypes.isEmpty();
+    }
+
+    protected boolean recurseNodes(Node node) {
+        if (checkNode(node)) {
+            return true;
+        }
+        for (int i = 0; i < node.jjtGetNumChildren(); i++) {
+            if (recurseNodes(node.jjtGetChild(i))) {
+                return true;
+            }
         }
-        return recurseExcludedNodeType((Node) tree);
+        return false;
+    }
+
+    protected boolean checkNode(Node node) {
+        return containsExcludedNodeType(node);
     }
 
-    protected boolean recurseExcludedNodeType(Node node) {
+    protected boolean containsExcludedNodeType(Node node) {
         String nodeClassName = node.getClass().getName();
         if (excludedNodeTypes.contains(nodeClassName)) {
             LOG.warn("Expression contains blocked node type [{}]", nodeClassName);
             return true;
-        } else {
-            for (int i = 0; i < node.jjtGetNumChildren(); i++) {
-                if (recurseExcludedNodeType(node.jjtGetChild(i))) {
-                    return true;
-                }
-            }
-            return false;
         }
+        return false;
     }
 }