[jira] [Work logged] (WW-5528) Multipart uploads with invalid characters in file or field name are silently dropped

[ASF GitHub Bot (Jira)]

     [ 
https://issues.apache.org/jira/browse/WW-5528?focusedWorklogId=955793&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-955793
 ]

ASF GitHub Bot logged work on WW-5528:
--------------------------------------

                Author: ASF GitHub Bot
            Created on: 06/Feb/25 08:05
            Start Date: 06/Feb/25 08:05
    Worklog Time Spent: 10m 
      Work Description: github-advanced-security[bot] commented on code in PR 
#1215:
URL: https://github.com/apache/struts/pull/1215#discussion_r1944270920


##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java:
##########
@@ -194,4 +191,32 @@
         return patternsChecker.isExcluded(fileName).isExcluded();
     }
 
+    protected boolean isInvalidInput(String fieldName, String fileName) {
+        // Skip file uploads that don't have a file name - meaning that no 
file was selected.
+        if (fileName == null || fileName.trim().isEmpty()) {
+            LOG.debug(() -> "No file has been uploaded for the field: " + 
normalizeSpace(fieldName));
+            return true;
+        }
+
+        if (isExcluded(fileName)) {
+            String normalizedFileName = normalizeSpace(fileName);
+            LOG.debug("File name [{}] is not accepted", normalizedFileName);

Review Comment:
   ## Logging should not be vulnerable to injection attacks
   
   <!--SONAR_ISSUE_KEY:AZTaSXRkbZEgA2nfhi-F-->Change this code to not log 
user-controlled data. <p>See more on <a 
href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AZTaSXRkbZEgA2nfhi-F&open=AZTaSXRkbZEgA2nfhi-F&pullRequest=1215";>SonarQube
 Cloud</a></p>
   
   [Show more 
details](https://github.com/apache/struts/security/code-scanning/972)



##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -154,10 +142,10 @@
 
     protected void processNormalFormField(FileItem item, String charset) 
throws UnsupportedEncodingException {
         try {
-            LOG.debug("Item is a normal form field");
+            String fieldName = item.getFieldName();
+            LOG.debug("Item: {} is a normal form field", fieldName);

Review Comment:
   ## Logging should not be vulnerable to injection attacks
   
   <!--SONAR_ISSUE_KEY:AZTaSXPsbZEgA2nfhi9s-->Change this code to not log 
user-controlled data. <p>See more on <a 
href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AZTaSXPsbZEgA2nfhi9s&open=AZTaSXPsbZEgA2nfhi9s&pullRequest=1215";>SonarQube
 Cloud</a></p>
   
   [Show more 
details](https://github.com/apache/struts/security/code-scanning/968)



##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java:
##########
@@ -194,4 +191,32 @@
         return patternsChecker.isExcluded(fileName).isExcluded();
     }
 
+    protected boolean isInvalidInput(String fieldName, String fileName) {
+        // Skip file uploads that don't have a file name - meaning that no 
file was selected.
+        if (fileName == null || fileName.trim().isEmpty()) {
+            LOG.debug(() -> "No file has been uploaded for the field: " + 
normalizeSpace(fieldName));
+            return true;
+        }
+
+        if (isExcluded(fileName)) {
+            String normalizedFileName = normalizeSpace(fileName);
+            LOG.debug("File name [{}] is not accepted", normalizedFileName);
+            errors.add(new LocalizedMessage(getClass(), 
STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_NAME, null,
+                    new String[]{normalizedFileName}));
+            return true;
+        }
+
+        return isInvalidInput(fieldName);
+    }
+
+    protected boolean isInvalidInput(String fieldName) {
+        if (isExcluded(fieldName)) {
+            String normalizedFieldName = normalizeSpace(fieldName);
+            LOG.debug("Form field [{}}] is rejected!", normalizedFieldName);

Review Comment:
   ## Logging should not be vulnerable to injection attacks
   
   <!--SONAR_ISSUE_KEY:AZTaSXRkbZEgA2nfhi-E-->Change this code to not log 
user-controlled data. <p>See more on <a 
href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AZTaSXRkbZEgA2nfhi-E&open=AZTaSXRkbZEgA2nfhi-E&pullRequest=1215";>SonarQube
 Cloud</a></p>
   
   [Show more 
details](https://github.com/apache/struts/security/code-scanning/971)





Issue Time Tracking
-------------------

    Worklog Id:     (was: 955793)
    Time Spent: 1h  (was: 50m)

> Multipart uploads with invalid characters in file or field name are silently 
> dropped
> ------------------------------------------------------------------------------------
>
>                 Key: WW-5528
>                 URL: https://issues.apache.org/jira/browse/WW-5528
>             Project: Struts 2
>          Issue Type: Bug
>          Components: Core
>    Affects Versions: 6.7.2, 7.0.2
>            Reporter: Kusal Kithul-Godage
>            Priority: Major
>          Time Spent: 1h
>  Remaining Estimate: 0h
>
> They should instead be reported as errors via MultiPartRequest#getErrors



--
This message was sent by Atlassian Jira
(v8.20.10#820010)