[WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package #861

lukaszlenart · 2024-01-24T08:10:38Z

Also refactors the Jakarta based parsers as they have a lot in common

core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java

core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java

-            LOG.debug("No file has been uploaded for the field: {}", sanitizeNewlines(item.getFieldName()));
-            return;
-        }
+        LOG.debug("Using charset: {} or default: {}", charset, defaultEncoding);


core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java

-                    // if the requestSizePermitted check failed based on the
-                    // complete content-size of the request.
-                    else {
+        LOG.debug("Using charset: {} or default: {}", charset, defaultEncoding);


lukaszlenart · 2024-01-27T11:01:35Z

/cc: @burtonrhodes

lukaszlenart · 2024-01-27T11:04:14Z

/cc: @deki

burtonrhodes · 2024-01-27T18:20:56Z

@lukaszlenart Looks good. The current version of this PR works well in my initial testing (dev) environment.

burtonrhodes · 2024-01-27T18:25:02Z

core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java

-                errorMessage = buildErrorMessage(e, new Object[]{
-                        ex.getContentType()
-                });
+        for (DiskFileItem item : servletFileUpload.parseRequest(request)) {


@lukaszlenart Quick thought on the maxFiles setting: I still think it makes more sense that maxFiles would apply to only file fields and not all form fields in the request. However it appears that this logic is actually baked into the JakartaServletFileUpload class itself when calling servletFileUpload.parseRequest(). Wonder why they coded it this way??

Anyway, if we ever wanted to change this, you could set the maxFiles to unlimited in the servletFileUpload object, and then perform the files count check in the if (item.isFormField()... logic below. Just a thought.

In thinking about it more, that might be a security issue, as the serletFileUpload object would upload everything first (something that the setting is trying to prevent), only to perform the check after the processing is done. However, the maxSize / maxFileSize might offset that security risk??

I assume so, this can be changed with a proper implementation of Streaming API which can then calculate the max size of uploaded files internally in Struts ... hm or maybe even right now, let me think this through 🤔

@burtonrhodes introduced a new constant maxSizeOfFiles which controls how many files you can upload based on their size - this works with JakartaStreamMultiPartRequest only.

Yeah this is by design (see this comment), none of the other options mitigate against this as the risk is processing a very large amount of request parts, not necessarily large ones. Though we can probably raise our default limit from 256 to 1000 (used in latest Tomcat).

kusalk · 2024-01-29T09:40:42Z

core/src/main/java/org/apache/struts2/action/UploadedFilesAware.java

@@ -27,14 +27,14 @@
 * The {@link org.apache.struts2.interceptor.ActionFileUploadInterceptor} will use the interface
 * to notify action about the multiple uploaded files.
 */
-public interface UploadedFilesAware {
+public interface UploadedFilesAware<T> {


Not sure I understand the benefit of this generic type given this interface is coupled with the ActionFileUploadInterceptor. In what situation would an action implement UploadedFilesAware<NotFile>?

This is a preparation to support InputStream and allow to stream files directly into actions, yet I need to thinks this through as this change became huge :\

Oh okay - yeah I just don't know if it makes sense for a marker interface to have a generic type - might just need 2 marker interfaces instead

kusalk · 2024-01-29T09:41:32Z

core/src/main/java/org/apache/struts2/StrutsConstants.java

@@ -142,18 +142,25 @@ public final class StrutsConstants {
    /** The maximum size of a multipart request (file upload) */
    public static final String STRUTS_MULTIPART_MAXSIZE = "struts.multipart.maxSize";

+    /** The maximum size of all uploaded files.
+       Used only with {@link org.apache.struts2.dispatcher.multipart.JakartaStreamMultiPartRequest} */
+    public static final String STRUTS_MULTIPART_MAXSIZE_OF_FILES = "struts.multipart.maxSizeOfFiles";


Still not very consistent - shouldn't it be MAX_SIZE_OF_FILES?

IDEA doesn't complain about MAXSIZE that's why I left it, but you are right :)

kusalk · 2024-01-29T09:49:53Z

core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java

-                errorMessage = buildErrorMessage(e, new Object[]{
-                        ex.getContentType()
-                });
+        for (DiskFileItem item : servletFileUpload.parseRequest(request)) {


Yeah this is by design (see this comment), none of the other options mitigate against this as the risk is processing a very large amount of request parts, not necessarily large ones. Though we can probably raise our default limit from 256 to 1000 (used in latest Tomcat).

kusalk · 2024-01-29T10:03:23Z

core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java

-                    LOG.warn("Failed to delete '{}' due to security exception above.", file.getName(), se);
-                }
-            }
+        Long currentFilesSize = actualSizeOfUploadedFiles();


This seems expensive to compute from scratch every time a file field is encountered, especially when the application might not have even configured a maxSizeOfFiles limit!

Tbh I'm not convinced this option adds much value in the first place - I think maxSize serves this need well enough. I'm trying to think in what situation would an application need to configure maxSizeOfFiles instead of or in addition to maxSize. They want to accepts lots of big form fields but want to limit the amount of data written to disk? I guess it's possible

This seems expensive to compute from scratch every time a file field is encountered, especially when the application might not have even configured a maxSizeOfFiles limit!

Fixed!

kusalk

Lot of refactoring here so tricky to follow all the changes, but looks good at a glance

kusalk · 2024-01-29T19:05:24Z

core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java

-                    LOG.warn("Failed to delete '{}' due to security exception above.", file.getName(), se);
-                }
-            }
+        Long currentFilesSize = maxSizeOfFiles != null ? actualSizeOfUploadedFiles() : null;


You can just inline this right?
if (maxSizeOfFiles != null && actualSizeOfUploadedFiles() + file.length() >= maxSizeOfFiles) {

Not really as it is used inside the if clause in exceedsMaxSizeOfFiles

Missed that, all good :)

burtonrhodes · 2024-01-29T21:07:25Z

Just FYI, I implemented this PR on a small subset of my server cluster and have been receiving the following ambiguous errors in some of the upload requests. I can't duplicate it in any testing environment, but curious if you might have a clue as to why Struts is creating an actionError with no value in the error, instead it prints "{0}". For this implementation, I am not using the stream parser

Error printed by the code below

Unidentified ActionError present for file upload: [Error uploading: {0}!, Error uploading: {0}!]

Code that quickly checks for any file upload errors

if (hasActionErrors()) {
    List<String> uploadErrors = (List<String>) getActionErrors();
    log.error("Unidentified ActionError present for file upload: " + uploadErrors);
}

A redacted version of the action

public class UploadLocalFileJsonAction extends ActionSupport implements Validateable, UploadedFilesAware<UploadedFile<File>> {

    private FileAttachmentUpload fileAttachmentUpload;

    @Override
    public void withUploadedFiles(List<UploadedFile<UploadedFile<File>>> uploadedFiles) {
        if (!uploadedFiles.isEmpty()) {
            fileAttachmentUpload = UploadedFileHelper.createFromUploadedFile(uploadedFiles.get(0));
        }
    }

    public void validate() {
        // First check to see if there are any errors from the file upload
        if (hasActionErrors()) {
            List<String> uploadErrors = (List<String>) getActionErrors();
            log.error("Unidentified ActionError present for file upload: " + uploadErrors);
        }
    }

    public String execute() throws Exception {
        // Omitted for brevity
    }
}

lukaszlenart · 2024-01-30T08:25:10Z

@kusalk sorry for that, I know it's a big change yet I wanted to fix all the issues I have discovered around Jakarta file upload at once. Also all the changes related to UploadedFile are mean to support other approaches as struts2-gea-plugin.

lukaszlenart · 2024-01-30T08:38:57Z

@burtonrhodes I tried to play with the Showcase app and right now there is a problem uploading large files - I get blank page, yet the app is using the new Action Upload Interceptor

sonarqubecloud · 2024-01-31T08:51:15Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
87.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

lukaszlenart · 2024-02-01T07:19:46Z

@burtonrhodes did you have a chance to re-test the latest changes? Everything works fine when testing with the Showcase app yet it uses the new action upload interceptor, but still the same MultiPartParser is used behind the scene. I even added a dedicated integration test to cover max files constraint.

burtonrhodes · 2024-02-01T07:56:41Z

@lukaszlenart yes, all looks good on my end.

lukaszlenart · 2024-02-01T08:23:24Z

LGTM 💯

criderp · 2024-05-21T15:11:33Z

core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java

+     */
+    public String[] getParameterValues(String name) {
+        return parameters.getOrDefault(name, Collections.emptyList())
+                .toArray(String[]::new);


This change broke query parameters in multipart requests. Their values are being lost. The old implementation returned null if the parameter didn't exist, but it now returns an empty array. MultiPartRequestWrapper::getParameterValues relies on the fact that it returns null, though, so it's not working correctly anymore.

This is the old implementation:

public String[] getParameterValues(String name) { List<String> v = params.get(name); if (v != null && !v.isEmpty()) { return v.toArray(new String[0]); } return null; }

And this is the MultiPartRequestWrapper method that doesn't work correctly after the change:

public String[] getParameterValues(String name) { return ((multi == null) || (multi.getParameterValues(name) == null)) ? super.getParameterValues(name) : multi.getParameterValues(name); }

@criderp please register this bug in JIRA, thanks!

WW-5388 Uses the latest JakartaEE FileUpload Servlet 6 package

8c161f4

Also refactors the Jakarta based parsers as they have a lot in common

sepe81 reviewed Jan 24, 2024

View reviewed changes

core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java Outdated Show resolved Hide resolved

lukaszlenart mentioned this pull request Jan 24, 2024

Struts 7.0.x #792

Merged

lukaszlenart added 3 commits January 24, 2024 21:20

WW-5388 Updates tests to match new logic

b68b6ab

WW-5388 Simplifies code

ac9b9ad

WW-5388 Avoids unconditional invocations of method in logs

c37a6ed

lukaszlenart force-pushed the feature/WW-5388-upload-servlet6 branch 4 times, most recently from ad2f820 to 1f9e0cd Compare January 27, 2024 08:15

github-advanced-security bot found potential problems Jan 27, 2024

View reviewed changes

lukaszlenart force-pushed the feature/WW-5388-upload-servlet6 branch 2 times, most recently from 4601311 to 0eead10 Compare January 27, 2024 09:26

WW-5388 Adds tests per each Jakarta parser

3294ed0

lukaszlenart force-pushed the feature/WW-5388-upload-servlet6 branch from 0eead10 to 3294ed0 Compare January 27, 2024 10:20

lukaszlenart marked this pull request as ready for review January 27, 2024 11:00

lukaszlenart requested review from sepe81, rgielen, aleksandr-m, yasserzamani, sdutry and kusalk and removed request for sepe81 January 27, 2024 11:00

burtonrhodes reviewed Jan 27, 2024

View reviewed changes

lukaszlenart added 2 commits January 28, 2024 07:36

WW-5388 Drops unused defaultLocale

d07792a

WW-5388 Uses temp directory to avoid problems on Jenkins

51bf8de

lukaszlenart force-pushed the feature/WW-5388-upload-servlet6 branch from 9c4c860 to 51bf8de Compare January 28, 2024 08:47

lukaszlenart force-pushed the feature/WW-5388-upload-servlet6 branch 6 times, most recently from 9f2e912 to 2155d17 Compare January 28, 2024 13:20

lukaszlenart requested review from burtonrhodes and sepe81 January 29, 2024 05:49

kusalk reviewed Jan 29, 2024

View reviewed changes

lukaszlenart force-pushed the feature/WW-5388-upload-servlet6 branch 3 times, most recently from 5ca2764 to 61ba63b Compare January 29, 2024 17:50

WW-5388 Introduces new constant to allow control max number of files

e2215c8

lukaszlenart force-pushed the feature/WW-5388-upload-servlet6 branch from 61ba63b to e2215c8 Compare January 29, 2024 18:09

kusalk approved these changes Jan 29, 2024

View reviewed changes

lukaszlenart force-pushed the feature/WW-5388-upload-servlet6 branch from 94cec3b to 18e8c94 Compare January 30, 2024 08:46

WW-5388 Extracts method to read charset

d024ccd

lukaszlenart force-pushed the feature/WW-5388-upload-servlet6 branch from 18e8c94 to d024ccd Compare January 30, 2024 08:53

WW-5388 Adds dedicated integration test to cover max size upload

e9e3c0a

lukaszlenart merged commit 84d350d into release/struts-7-0-x Feb 1, 2024
7 checks passed

lukaszlenart deleted the feature/WW-5388-upload-servlet6 branch February 1, 2024 08:23

lukaszlenart mentioned this pull request Feb 11, 2024

[WW-5388] Fixes a few issues in Servlet 6 file upload usage #873

Merged

criderp reviewed May 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package #861

[WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package #861

lukaszlenart commented Jan 24, 2024

lukaszlenart commented Jan 27, 2024

lukaszlenart commented Jan 27, 2024

burtonrhodes commented Jan 27, 2024

burtonrhodes Jan 27, 2024 •

edited

Loading

burtonrhodes Jan 27, 2024 •

edited

Loading

lukaszlenart Jan 28, 2024

lukaszlenart Jan 28, 2024

kusalk Jan 29, 2024

kusalk Jan 29, 2024

lukaszlenart Jan 29, 2024

kusalk Jan 29, 2024

lukaszlenart Jan 29, 2024

kusalk Jan 29, 2024

lukaszlenart Jan 29, 2024

kusalk Jan 29, 2024

kusalk Jan 29, 2024

kusalk Jan 29, 2024

lukaszlenart Jan 29, 2024

kusalk left a comment

kusalk Jan 29, 2024

lukaszlenart Jan 30, 2024

kusalk Jan 30, 2024

burtonrhodes commented Jan 29, 2024 •

edited

Loading

lukaszlenart commented Jan 30, 2024

lukaszlenart commented Jan 30, 2024

sonarqubecloud bot commented Jan 31, 2024

lukaszlenart commented Feb 1, 2024

burtonrhodes commented Feb 1, 2024

lukaszlenart commented Feb 1, 2024

criderp May 21, 2024

lukaszlenart May 21, 2024

[WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package #861

[WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package #861

Conversation

lukaszlenart commented Jan 24, 2024

lukaszlenart commented Jan 27, 2024

lukaszlenart commented Jan 27, 2024

burtonrhodes commented Jan 27, 2024

burtonrhodes Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

burtonrhodes Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kusalk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burtonrhodes commented Jan 29, 2024 • edited Loading

lukaszlenart commented Jan 30, 2024

lukaszlenart commented Jan 30, 2024

sonarqubecloud bot commented Jan 31, 2024

Quality Gate passed

lukaszlenart commented Feb 1, 2024

burtonrhodes commented Feb 1, 2024

lukaszlenart commented Feb 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burtonrhodes Jan 27, 2024 •

edited

Loading

burtonrhodes Jan 27, 2024 •

edited

Loading

burtonrhodes commented Jan 29, 2024 •

edited

Loading