From 20eafb632721627b4bef2463e80b7dad42fd4dd6 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 6 Oct 2023 04:16:57 +1100 Subject: [PATCH 1/3] WW-5340 Mild refactor StrutsOgnlGuard for easier subclassing --- .../apache/struts2/ognl/StrutsOgnlGuard.java | 38 ++++++++++++------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/ognl/StrutsOgnlGuard.java b/core/src/main/java/org/apache/struts2/ognl/StrutsOgnlGuard.java index 262aec362..0cb4d1d93 100644 --- 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 class StrutsOgnlGuard implements OgnlGuard { @Override public boolean isParsedTreeBlocked(Object tree) { - return containsExcludedNodeType(tree); - } - - protected boolean containsExcludedNodeType(Object tree) { - if (!(tree instanceof Node) || excludedNodeTypes.isEmpty()) { + if (!(tree instanceof Node) || skipTreeCheck((Node) tree)) { return false; } - return recurseExcludedNodeType((Node) tree); + return recurseNodes((Node) tree); } - protected boolean recurseExcludedNodeType(Node node) { + 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 false; + } + + protected boolean checkNode(Node node) { + return containsExcludedNodeType(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; } } From 276ede4c88f67b5e1d1182928301ceff12e6fcba Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 6 Oct 2023 04:17:31 +1100 Subject: [PATCH 2/3] WW-5340 Add debug logging for rejected form fields --- .../multipart/JakartaMultiPartRequest.java | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 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 index de5a3e968..8e4b60b5f 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java @@ -142,26 +142,22 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } 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 {}.", 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 { From f4029f8fd039f4069dec67235e9fdbc4b1075d50 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 6 Oct 2023 15:47:47 +1100 Subject: [PATCH 3/3] WW-5340 Sanitize field names before logging --- .../dispatcher/multipart/JakartaMultiPartRequest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 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 index 8e4b60b5f..20b948fd3 100644 --- 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 class JakartaMultiPartRequest extends AbstractMultiPartRequest { 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 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { // 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; } @@ -143,7 +143,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { long size = item.getSize(); if (size > maxStringLength) { - LOG.debug("Form field {} of size {} bytes exceeds limit of {}.", item.getFieldName(), 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}); @@ -362,4 +362,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } } + private String sanitizeNewlines(String before) { + return before.replaceAll("[\n\r]", "_"); + } }