From a1c4cb60a2a848e2b715eb37fd20ea95c8b9a356 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 16 Jul 2025 18:34:35 +0200 Subject: [PATCH] Uses a dedicated RequestContext to avoid NPE --- .../multipart/AbstractMultiPartRequest.java | 30 ++++++- .../multipart/JakartaMultiPartRequest.java | 86 ++++++++++++++----- .../JakartaStreamMultiPartRequest.java | 19 +--- .../multipart/StrutsRequestContext.java | 61 +++++++++++++ 4 files changed, 154 insertions(+), 42 deletions(-) create mode 100644 core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsRequestContext.java diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java index 90ecbe816..038cef4bc 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java @@ -18,6 +18,8 @@ */ package org.apache.struts2.dispatcher.multipart; +import org.apache.commons.fileupload2.core.DiskFileItemFactory; +import org.apache.commons.fileupload2.core.RequestContext; import org.apache.struts2.inject.Inject; import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException; @@ -43,6 +45,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.apache.commons.lang3.StringUtils.normalizeSpace; + /** * Abstract class with some helper methods, it should be used * when starting development of another implementation of {@link MultiPartRequest} @@ -187,7 +191,21 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { * @param charset used charset from incoming request * @param saveDir a temporary folder to store uploaded files (not always needed) */ - protected abstract JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path saveDir); + protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path saveDir) { + DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder(); + + LOG.debug("Using file save directory: {}", saveDir); + builder.setPath(saveDir); + + LOG.debug("Sets buffer size: {}", bufferSize); + builder.setBufferSize(bufferSize); + + LOG.debug("Using charset: {}", charset); + builder.setCharset(charset); + + DiskFileItemFactory factory = builder.get(); + return new JakartaServletDiskFileUpload(factory); + } protected JakartaServletDiskFileUpload prepareServletFileUpload(Charset charset, Path saveDir) { JakartaServletDiskFileUpload servletFileUpload = createJakartaFileUpload(charset, saveDir); @@ -207,11 +225,15 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { return servletFileUpload; } + protected RequestContext createRequestContext(HttpServletRequest request) { + return new StrutsRequestContext(request); + } + protected boolean exceedsMaxStringLength(String fieldName, String fieldValue) { if (maxStringLength != null && fieldValue.length() > maxStringLength) { if (LOG.isDebugEnabled()) { LOG.debug("Form field: {} of size: {} bytes exceeds limit of: {}.", - sanitizeNewlines(fieldName), fieldValue.length(), maxStringLength); + normalizeSpace(fieldName), fieldValue.length(), maxStringLength); } LocalizedMessage localizedMessage = new LocalizedMessage(this.getClass(), STRUTS_MESSAGES_UPLOAD_ERROR_PARAMETER_TOO_LONG_KEY, null, @@ -234,7 +256,7 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { try { processUpload(request, saveDir); } catch (FileUploadException e) { - LOG.debug("Error parsing the multi-part request!", e); + LOG.warn("Error parsing the multi-part request!", e); Class exClass = FileUploadException.class; Object[] args = new Object[]{}; @@ -257,7 +279,7 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { errors.add(errorMessage); } } catch (IOException e) { - LOG.debug("Unable to parse request", e); + LOG.warn("Unable to parse request", e); LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(), new Object[]{}); if (!errors.contains(errorMessage)) { errors.add(errorMessage); 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 d2ef13b1d..dbd344b5d 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 @@ -20,12 +20,13 @@ package org.apache.struts2.dispatcher.multipart; import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.fileupload2.core.DiskFileItem; -import org.apache.commons.fileupload2.core.DiskFileItemFactory; +import org.apache.commons.fileupload2.core.RequestContext; import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import java.io.File; import java.io.IOException; import java.nio.charset.Charset; import java.nio.file.Path; @@ -41,6 +42,11 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { private static final Logger LOG = LogManager.getLogger(JakartaMultiPartRequest.class); + /** + * List to track all DiskFileItem instances for proper cleanup + */ + private final List diskFileItems = new ArrayList<>(); + @Override protected void processUpload(HttpServletRequest request, String saveDir) throws IOException { Charset charset = readCharsetEncoding(request); @@ -48,7 +54,12 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { JakartaServletDiskFileUpload servletFileUpload = prepareServletFileUpload(charset, Path.of(saveDir)); - for (DiskFileItem item : servletFileUpload.parseRequest(request)) { + RequestContext requestContext = createRequestContext(request); + + for (DiskFileItem item : servletFileUpload.parseRequest(requestContext)) { + // Track all DiskFileItem instances for cleanup + diskFileItems.add(item); + LOG.debug(() -> "Processing a form field: " + normalizeSpace(item.getFieldName())); if (item.isFormField()) { processNormalFormField(item, charset); @@ -59,23 +70,6 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } } - @Override - protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path saveDir) { - DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder(); - - LOG.debug("Using file save directory: {}", saveDir); - builder.setPath(saveDir); - - LOG.debug("Sets minimal buffer size to always write file to disk"); - builder.setBufferSize(1); - - LOG.debug("Using charset: {}", charset); - builder.setCharset(charset); - - DiskFileItemFactory factory = builder.get(); - return new JakartaServletDiskFileUpload(factory); - } - protected void processNormalFormField(DiskFileItem item, Charset charset) throws IOException { LOG.debug("Item: {} is a normal form field", normalizeSpace(item.getName())); @@ -114,7 +108,30 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } if (item.isInMemory()) { - LOG.warn(() -> "Storing uploaded files just in memory isn't supported currently, skipping file: %s!".formatted(normalizeSpace(item.getName()))); + LOG.debug("Creating temporary file representing in-memory uploaded item: {}", normalizeSpace(item.getFieldName())); + try { + File tempFile = File.createTempFile("struts_upload_", "_" + item.getName()); + tempFile.deleteOnExit(); // Ensure cleanup on JVM exit + + // Write the in-memory content to the temporary file + try (java.io.FileOutputStream fos = new java.io.FileOutputStream(tempFile)) { + fos.write(item.get()); + } + + UploadedFile uploadedFile = StrutsUploadedFile.Builder + .create(tempFile) + .withOriginalName(item.getName()) + .withContentType(item.getContentType()) + .withInputName(item.getFieldName()) + .build(); + values.add(uploadedFile); + + LOG.debug("Created temporary file for in-memory uploaded item: {} at {}", + normalizeSpace(item.getName()), tempFile.getAbsolutePath()); + } catch (IOException e) { + LOG.warn("Failed to create temporary file for in-memory uploaded item: {}", + normalizeSpace(item.getName()), e); + } } else { UploadedFile uploadedFile = StrutsUploadedFile.Builder .create(item.getPath().toFile()) @@ -128,4 +145,33 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { uploadedFiles.put(item.getFieldName(), values); } + /** + * Override cleanUp to ensure all DiskFileItem instances are properly cleaned up + */ + @Override + public void cleanUp() { + super.cleanUp(); + try { + LOG.debug("Clean up all DiskFileItem instances (both form fields and file uploads"); + for (DiskFileItem item : diskFileItems) { + try { + if (item.isInMemory()) { + LOG.debug("Cleaning up in-memory item: {}", normalizeSpace(item.getFieldName())); + } else { + LOG.debug("Cleaning up disk item: {} at {}", normalizeSpace(item.getFieldName()), item.getPath()); + if (item.getPath() != null && item.getPath().toFile().exists()) { + if (!item.getPath().toFile().delete()) { + LOG.warn("There was a problem attempting to delete temporary file: {}", item.getPath()); + } + } + } + } catch (Exception e) { + LOG.warn("Error cleaning up DiskFileItem: {}", normalizeSpace(item.getFieldName()), e); + } + } + } finally { + diskFileItems.clear(); + } + } + } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index 17b6f377b..769b03e42 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java @@ -19,7 +19,6 @@ package org.apache.struts2.dispatcher.multipart; import jakarta.servlet.http.HttpServletRequest; -import org.apache.commons.fileupload2.core.DiskFileItemFactory; import org.apache.commons.fileupload2.core.FileItemInput; import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException; import org.apache.commons.fileupload2.core.FileUploadSizeException; @@ -45,7 +44,7 @@ import java.util.UUID; import static org.apache.commons.lang3.StringUtils.normalizeSpace; /** - * Multi-part form data request adapter for Jakarta Commons FileUpload package that + * Multipart form data request adapter for Jakarta Commons FileUpload package that * leverages the streaming API rather than the traditional non-streaming API. *

* For more details see WW-3025 @@ -82,22 +81,6 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { }); } - protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path location) { - DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder(); - - LOG.debug("Using file save directory: {}", location); - builder.setPath(location); - - LOG.debug("Sets buffer size: {}", bufferSize); - builder.setBufferSize(bufferSize); - - LOG.debug("Using charset: {}", charset); - builder.setCharset(charset); - - DiskFileItemFactory factory = builder.get(); - return new JakartaServletDiskFileUpload(factory); - } - private String readStream(InputStream inputStream) throws IOException { ByteArrayOutputStream result = new ByteArrayOutputStream(); byte[] buffer = new byte[1024]; diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsRequestContext.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsRequestContext.java new file mode 100644 index 000000000..1437649cc --- /dev/null +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsRequestContext.java @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.dispatcher.multipart; + +import jakarta.servlet.http.HttpServletRequest; +import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletRequestContext; + +/** + * Provides a specialized request context for Struts applications, + * extending the Jakarta Servlet request context to add custom handling + * for multipart-related requests. + *

+ * This class overrides multipart detection logic to ensure that requests + * without a content type are not treated as multipart, improving robustness + * in file upload scenarios. + */ +public class StrutsRequestContext extends JakartaServletRequestContext { + + /** + * Constructs a context for this request. + * + * @param request The request to which this context applies. + */ + public StrutsRequestContext(HttpServletRequest request) { + super(request); + } + + /** + * Determines if the current request is multipart-related. + *

+ * This implementation first checks if the request's content type is set. + * If the content type is {@code null}, it returns {@code false} immediately. + * Otherwise, it delegates to the superclass implementation to perform + * further checks. + * + * @return {@code true} if the request is multipart-related; {@code false} otherwise. + */ + @Override + public boolean isMultipartRelated() { + if (this.getRequest().getContentType() == null) { + return false; + } + return super.isMultipartRelated(); + } +}