From 3294ed08d18febf88e1b3f7dddf7012b1ec95abc Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 26 Jan 2024 10:12:06 +0100 Subject: [PATCH] WW-5388 Adds tests per each Jakarta parser --- core/pom.xml | 4 +- .../multipart/AbstractMultiPartRequest.java | 224 +++++++++- .../multipart/JakartaMultiPartRequest.java | 354 +++------------ .../JakartaStreamMultiPartRequest.java | 294 ++++--------- .../multipart/StrutsUploadedFile.java | 11 +- .../dispatcher/multipart/UploadedFile.java | 4 +- .../AbstractMultiPartRequestTest.java | 408 ++++++++++++++++++ .../JakartaMultiPartRequestTest.java | 56 +++ .../JakartaStreamMultiPartRequestTest.java | 70 ++- .../ActionFileUploadInterceptorTest.java | 2 +- 10 files changed, 863 insertions(+), 564 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java create mode 100644 core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java diff --git a/core/pom.xml b/core/pom.xml index 9b9b94dff..a52453c08 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -19,7 +19,8 @@ * under the License. */ --> - + 4.0.0 org.apache.struts @@ -217,7 +218,6 @@ org.apache.commons commons-fileupload2-jakarta-servlet6 - 2.0.0-M2 commons-io 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 84bfc8056..9fcb3160e 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 @@ -21,20 +21,33 @@ package org.apache.struts2.dispatcher.multipart; import com.opensymphony.xwork2.LocaleProviderFactory; import com.opensymphony.xwork2.inject.Inject; import jakarta.servlet.http.HttpServletRequest; +import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException; +import org.apache.commons.fileupload2.core.FileUploadContentTypeException; +import org.apache.commons.fileupload2.core.FileUploadException; +import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException; +import org.apache.commons.fileupload2.core.FileUploadSizeException; +import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; import org.apache.struts2.dispatcher.LocalizedMessage; +import java.io.IOException; +import java.nio.charset.Charset; +import java.nio.file.Path; import java.util.ArrayList; +import java.util.Collections; +import java.util.Enumeration; +import java.util.HashMap; import java.util.List; import java.util.Locale; +import java.util.Map; /** * Abstract class with some helper methods, it should be used * when starting development of another implementation of {@link MultiPartRequest} */ -public abstract class AbstractMultiPartRequest implements MultiPartRequest { +public abstract class AbstractMultiPartRequest implements MultiPartRequest { protected static final String STRUTS_MESSAGES_UPLOAD_ERROR_PARAMETER_TOO_LONG_KEY = "struts.messages.upload.error.parameter.too.long"; @@ -82,6 +95,16 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { */ protected Locale defaultLocale = Locale.ENGLISH; + /** + * Map between file fields and file data. + */ + protected Map>> uploadedFiles = new HashMap<>(); + + /** + * Map between non-file fields and values. + */ + protected Map> parameters = new HashMap<>(); + /** * @param bufferSize Sets the buffer size to be used. */ @@ -133,6 +156,102 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { } } + /** + * Process the request extract file upload data + * + * @param request current {@link HttpServletRequest} + * @param saveDir a temporary directory to store files + */ + protected abstract void processUpload(HttpServletRequest request, String saveDir) throws IOException; + + /** + * Creates an instance of {@link JakartaServletDiskFileUpload} used by the parser to extract uploaded files + * + * @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 prepareServletFileUpload(Charset charset, Path saveDir) { + JakartaServletDiskFileUpload servletFileUpload = createJakartaFileUpload(charset, saveDir); + + if (maxSize != null) { + LOG.debug("Applies max size: {} to file upload request", maxSize); + servletFileUpload.setSizeMax(maxSize); + } + if (maxFiles != null) { + LOG.debug("Applies max files number: {} to file upload request", maxFiles); + servletFileUpload.setFileCountMax(maxFiles); + } + if (maxFileSize != null) { + LOG.debug("Applies max size of single file: {} to file upload request", maxFileSize); + servletFileUpload.setFileSizeMax(maxFileSize); + } + return servletFileUpload; + } + + 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); + } + LocalizedMessage localizedMessage = new LocalizedMessage(this.getClass(), + STRUTS_MESSAGES_UPLOAD_ERROR_PARAMETER_TOO_LONG_KEY, null, + new Object[]{fieldName, maxStringLength, fieldValue.length()}); + if (!errors.contains(localizedMessage)) { + errors.add(localizedMessage); + } + return true; + } + return false; + } + + /** + * Processes the upload. + * + * @param request the servlet request + * @param saveDir location of the save dir + */ + public void parse(HttpServletRequest request, String saveDir) throws IOException { + try { + setLocale(request); + processUpload(request, saveDir); + } catch (FileUploadException e) { + LOG.debug("Request exceeded size limit!", e); + LocalizedMessage errorMessage; + if (e instanceof FileUploadByteCountLimitException ex) { + errorMessage = buildErrorMessage(e, new Object[]{ + ex.getFieldName(), ex.getFileName(), ex.getPermitted(), ex.getActualSize() + }); + } else if (e instanceof FileUploadFileCountLimitException ex) { + errorMessage = buildErrorMessage(e, new Object[]{ + ex.getPermitted(), ex.getActualSize() + }); + } else if (e instanceof FileUploadSizeException ex) { + errorMessage = buildErrorMessage(e, new Object[]{ + ex.getPermitted(), ex.getActualSize() + }); + } else if (e instanceof FileUploadContentTypeException ex) { + errorMessage = buildErrorMessage(e, new Object[]{ + ex.getContentType() + }); + } else { + errorMessage = buildErrorMessage(e, new Object[]{}); + } + + if (!errors.contains(errorMessage)) { + errors.add(errorMessage); + } + } catch (Exception e) { + LOG.debug("Unable to parse request", e); + LocalizedMessage errorMessage = buildErrorMessage(e, new Object[]{}); + if (!errors.contains(errorMessage)) { + errors.add(errorMessage); + } + } + } + /** * Build error message. * @@ -147,13 +266,6 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { return new LocalizedMessage(this.getClass(), errorKey, e.getMessage(), args); } - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getErrors() - */ - public List getErrors() { - return errors; - } - /** * @param originalFileName file name * @return the canonical name based on the supplied filename @@ -175,4 +287,100 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { return before.replaceAll("\\R", "_"); } + /* (non-Javadoc) + * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getErrors() + */ + public List getErrors() { + return errors; + } + + /* (non-Javadoc) + * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileParameterNames() + */ + public Enumeration getFileParameterNames() { + return Collections.enumeration(uploadedFiles.keySet()); + } + + /* (non-Javadoc) + * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getContentType(java.lang.String) + */ + public String[] getContentType(String fieldName) { + return uploadedFiles.getOrDefault(fieldName, Collections.emptyList()).stream() + .map(UploadedFile::getContentType) + .toArray(String[]::new); + } + + /* (non-Javadoc) + * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFile(java.lang.String) + */ + @SuppressWarnings("unchecked") + public UploadedFile[] getFile(String fieldName) { + return uploadedFiles.getOrDefault(fieldName, Collections.emptyList()) + .toArray(UploadedFile[]::new); + } + + /* (non-Javadoc) + * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileNames(java.lang.String) + */ + public String[] getFileNames(String fieldName) { + return uploadedFiles.getOrDefault(fieldName, Collections.emptyList()).stream() + .map(file -> getCanonicalName(file.getName())) + .toArray(String[]::new); + } + + /* (non-Javadoc) + * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFilesystemName(java.lang.String) + */ + public String[] getFilesystemName(String fieldName) { + return uploadedFiles.getOrDefault(fieldName, Collections.emptyList()).stream() + .map(UploadedFile::getAbsolutePath) + .toArray(String[]::new); + } + + /* (non-Javadoc) + * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameter(java.lang.String) + */ + public String getParameter(String name) { + List paramValue = parameters.getOrDefault(name, Collections.emptyList()); + if (!paramValue.isEmpty()) { + return paramValue.get(0); + } + + return null; + } + + /* (non-Javadoc) + * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterNames() + */ + public Enumeration getParameterNames() { + return Collections.enumeration(parameters.keySet()); + } + + /* (non-Javadoc) + * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterValues(java.lang.String) + */ + public String[] getParameterValues(String name) { + return parameters.getOrDefault(name, Collections.emptyList()) + .toArray(String[]::new); + } + + /* (non-Javadoc) + * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#cleanUp() + */ + public void cleanUp() { + LOG.debug("Performing File Upload temporary storage cleanup."); + for (List> uploadedFileList : uploadedFiles.values()) { + for (UploadedFile uploadedFile : uploadedFileList) { + if (uploadedFile.isFile()) { + LOG.debug("Deleting file: {}", uploadedFile.getName()); + if (!uploadedFile.delete()) { + LOG.warn("There was a problem attempting to delete file: {}", uploadedFile.getName()); + } + } else { + LOG.debug("File: {} already deleted", uploadedFile.getName()); + } + } + } + } + } 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 95c25977f..59a9e7f9e 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 @@ -19,102 +19,40 @@ package org.apache.struts2.dispatcher.multipart; import jakarta.servlet.http.HttpServletRequest; -import org.apache.commons.fileupload2.core.AbstractFileUpload; import org.apache.commons.fileupload2.core.DiskFileItem; import org.apache.commons.fileupload2.core.DiskFileItemFactory; -import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException; -import org.apache.commons.fileupload2.core.FileUploadContentTypeException; -import org.apache.commons.fileupload2.core.FileUploadException; -import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException; -import org.apache.commons.fileupload2.core.FileUploadSizeException; -import org.apache.commons.fileupload2.core.RequestContext; -import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletFileUpload; +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 org.apache.struts2.dispatcher.LocalizedMessage; +import java.io.File; import java.io.IOException; -import java.io.InputStream; import java.nio.charset.Charset; +import java.nio.file.Path; import java.util.ArrayList; -import java.util.Collections; -import java.util.Enumeration; -import java.util.HashMap; import java.util.List; -import java.util.Map; -import java.util.Set; /** - * Multipart form data request adapter for Jakarta Commons Fileupload package. + * Multipart form data request adapter for Jakarta Commons FileUpload package. */ -public class JakartaMultiPartRequest extends AbstractMultiPartRequest { +public class JakartaMultiPartRequest extends AbstractMultiPartRequest { private static final Logger LOG = LogManager.getLogger(JakartaMultiPartRequest.class); - protected Map> uploadedFiles = new HashMap<>(); - - /** - * Keeps info about normal form fields - */ - protected Map> params = new HashMap<>(); - - /** - * Creates a new request wrapper to handle multipart data using methods adapted from Jason Pell's - * multipart classes (see class description). - * - * @param saveDir the directory to save off the file - * @param request the request containing the multipart - * @throws java.io.IOException is thrown if encoding fails. - */ - public void parse(HttpServletRequest request, String saveDir) throws IOException { - try { - setLocale(request); - processUpload(request, saveDir); - } catch (FileUploadException e) { - LOG.debug("Request exceeded size limit!", e); - LocalizedMessage errorMessage; - if (e instanceof FileUploadByteCountLimitException ex) { - errorMessage = buildErrorMessage(e, new Object[]{ - ex.getFieldName(), ex.getFileName(), ex.getPermitted(), ex.getActualSize() - }); - } else if (e instanceof FileUploadFileCountLimitException ex) { - errorMessage = buildErrorMessage(e, new Object[]{ - ex.getPermitted(), ex.getActualSize() - }); - } else if (e instanceof FileUploadSizeException ex) { - errorMessage = buildErrorMessage(e, new Object[]{ - ex.getPermitted(), ex.getActualSize() - }); - } else if (e instanceof FileUploadContentTypeException ex) { - errorMessage = buildErrorMessage(e, new Object[]{ - ex.getContentType() - }); - } else { - errorMessage = buildErrorMessage(e, new Object[]{}); - } - - if (!errors.contains(errorMessage)) { - errors.add(errorMessage); - } - } catch (Exception e) { - LOG.debug("Unable to parse request", e); - LocalizedMessage errorMessage = buildErrorMessage(e, new Object[]{}); - if (!errors.contains(errorMessage)) { - errors.add(errorMessage); - } - } - } - + @Override protected void processUpload(HttpServletRequest request, String saveDir) throws IOException { - if (!JakartaServletFileUpload.isMultipartContent(request)) { - LOG.debug("Http request isn't: {}, stop processing", AbstractFileUpload.MULTIPART_FORM_DATA); - return; - } - for (DiskFileItem item : parseRequest(request, saveDir)) { + String charset = StringUtils.isBlank(request.getCharacterEncoding()) + ? defaultEncoding + : request.getCharacterEncoding(); + + JakartaServletDiskFileUpload servletFileUpload = + prepareServletFileUpload(Charset.forName(charset), Path.of(saveDir)); + + for (DiskFileItem item : servletFileUpload.parseRequest(request)) { LOG.debug(() -> "Processing a form field: " + sanitizeNewlines(item.getFieldName())); if (item.isFormField()) { - processNormalFormField(item, request.getCharacterEncoding()); + processNormalFormField(item, charset); } else { LOG.debug(() -> "Processing a file: " + sanitizeNewlines(item.getFieldName())); processFileField(item); @@ -122,6 +60,51 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } } + 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, String charset) throws IOException { + try { + LOG.debug("Item: {} is a normal form field", item.getName()); + Charset encoding = StringUtils.isBlank(charset) ? Charset.forName(defaultEncoding) : Charset.forName(charset); + + List values; + String fieldName = item.getFieldName(); + if (parameters.get(fieldName) != null) { + values = parameters.get(fieldName); + } else { + values = new ArrayList<>(); + } + + String fieldValue = item.getString(encoding); + if (exceedsMaxStringLength(fieldName, fieldValue)) { + return; + } + if (item.getSize() == 0) { + values.add(StringUtils.EMPTY); + } else { + values.add(fieldValue); + } + parameters.put(fieldName, values); + } finally { + item.delete(); + } + } + protected void processFileField(DiskFileItem 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()) { @@ -129,220 +112,25 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { return; } - List values; + List> values; if (uploadedFiles.get(item.getFieldName()) != null) { values = uploadedFiles.get(item.getFieldName()); } else { values = new ArrayList<>(); } - UploadedFile uploadedFile = StrutsUploadedFile.Builder - .create(item.getPath().toFile()) - .withOriginalName(item.getName()) - .withContentType(item.getContentType()) - .build(); - values.add(uploadedFile); + if (item.isInMemory()) { + LOG.warn("Storing uploaded files just in memory isn't supported currently, skipping file: {}!", item.getName()); + } else { + UploadedFile uploadedFile = StrutsUploadedFile.Builder + .create(item.getPath().toFile()) + .withOriginalName(item.getName()) + .withContentType(item.getContentType()) + .build(); + values.add(uploadedFile); + } uploadedFiles.put(item.getFieldName(), values); } - protected void processNormalFormField(DiskFileItem item, String charset) throws IOException { - try { - LOG.debug("Item is a normal form field"); - Charset encoding = Charset.forName(charset); - - List values; - if (params.get(item.getFieldName()) != null) { - values = params.get(item.getFieldName()); - } else { - values = new ArrayList<>(); - } - - long size = item.getSize(); - if (size > maxStringLength) { - if (LOG.isDebugEnabled()) { - LOG.debug("Form field: {} of size: {} bytes exceeds limit of: {}.", sanitizeNewlines(item.getFieldName()), size, maxStringLength); - } - LocalizedMessage localizedMessage = new LocalizedMessage(this.getClass(), - STRUTS_MESSAGES_UPLOAD_ERROR_PARAMETER_TOO_LONG_KEY, null, - new Object[]{item.getFieldName(), maxStringLength, size}); - if (!errors.contains(localizedMessage)) { - errors.add(localizedMessage); - } - return; - } - if (size == 0) { - values.add(StringUtils.EMPTY); - } else { - values.add(item.getString(encoding)); - } - params.put(item.getFieldName(), values); - } finally { - item.delete(); - } - } - - protected List parseRequest(HttpServletRequest servletRequest, String saveDir) throws FileUploadException { - DiskFileItemFactory fileItemFactory = createDiskFileItemFactory(saveDir); - JakartaServletFileUpload upload = createServletFileUpload(fileItemFactory); - return upload.parseRequest(createRequestContext(servletRequest)); - } - - protected JakartaServletFileUpload createServletFileUpload(DiskFileItemFactory fileItemFactory) { - JakartaServletFileUpload servletFileUpload = new JakartaServletFileUpload<>(fileItemFactory); - if (maxSize != null) { - servletFileUpload.setSizeMax(maxSize); - } - if (maxFiles != null) { - servletFileUpload.setFileCountMax(maxFiles); - } - if (maxFileSize != null) { - servletFileUpload.setFileSizeMax(maxFileSize); - } - return servletFileUpload; - } - - protected DiskFileItemFactory createDiskFileItemFactory(String saveDir) { - DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder(); - if (saveDir != null) { - LOG.debug("Using file save directory: {}", saveDir); - builder.setPath(saveDir); - } - // sets minimal buffer size to always write file to disk - builder.setBufferSize(1); - return builder.get(); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileParameterNames() - */ - public Enumeration getFileParameterNames() { - return Collections.enumeration(uploadedFiles.keySet()); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getContentType(java.lang.String) - */ - public String[] getContentType(String fieldName) { - List uploadedFilesList = uploadedFiles.get(fieldName); - - if (uploadedFilesList == null) { - return null; - } - return uploadedFilesList.stream().map(UploadedFile::getContentType).toArray(String[]::new); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFile(java.lang.String) - */ - public UploadedFile[] getFile(String fieldName) { - List uploadedFileList = uploadedFiles.get(fieldName); - - if (uploadedFileList == null) { - return null; - } - return uploadedFileList.toArray(UploadedFile[]::new); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileNames(java.lang.String) - */ - public String[] getFileNames(String fieldName) { - List uploadedFileList = uploadedFiles.get(fieldName); - - if (uploadedFileList == null) { - return null; - } - return uploadedFileList.stream() - .map(file -> getCanonicalName(file.getName())) - .toArray(String[]::new); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFilesystemName(java.lang.String) - */ - public String[] getFilesystemName(String fieldName) { - List uploadedFileList = uploadedFiles.get(fieldName); - - if (uploadedFileList == null) { - return null; - } - return uploadedFileList.stream().map(UploadedFile::getAbsolutePath).toArray(String[]::new); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameter(java.lang.String) - */ - public String getParameter(String name) { - List paramValue = params.get(name); - if (paramValue != null && !paramValue.isEmpty()) { - return paramValue.get(0); - } - - return null; - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterNames() - */ - public Enumeration getParameterNames() { - return Collections.enumeration(params.keySet()); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterValues(java.lang.String) - */ - public String[] getParameterValues(String name) { - List v = params.get(name); - if (v != null && !v.isEmpty()) { - return v.toArray(new String[0]); - } - - return null; - } - - /** - * Creates a RequestContext needed by Jakarta Commons Upload. - * - * @param req the request. - * @return a new request context. - */ - protected RequestContext createRequestContext(final HttpServletRequest req) { - return new RequestContext() { - public String getCharacterEncoding() { - return req.getCharacterEncoding(); - } - - public String getContentType() { - return req.getContentType(); - } - - public long getContentLength() { - return req.getContentLength(); - } - - public InputStream getInputStream() throws IOException { - InputStream in = req.getInputStream(); - if (in == null) { - throw new IOException("Missing content in the request"); - } - return req.getInputStream(); - } - }; - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#cleanUp() - */ - public void cleanUp() { - Set names = uploadedFiles.keySet(); - names.forEach(name -> { - List uploadedFileList = uploadedFiles.get(name); - uploadedFileList.forEach(file -> { - LOG.debug("Removing file: {}", file.getName()); - file.delete(); - }); - }); - } - } 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 c220e34f7..3514652e4 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,28 +19,25 @@ package org.apache.struts2.dispatcher.multipart; import jakarta.servlet.http.HttpServletRequest; -import org.apache.commons.fileupload2.core.AbstractFileUpload; -import org.apache.commons.fileupload2.core.DiskFileItem; import org.apache.commons.fileupload2.core.DiskFileItemFactory; import org.apache.commons.fileupload2.core.FileItemInput; -import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletFileUpload; +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 org.apache.logging.log4j.message.ParameterizedMessage; -import org.apache.struts2.dispatcher.LocalizedMessage; import java.io.BufferedOutputStream; +import java.io.ByteArrayOutputStream; import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; +import java.nio.file.Path; import java.util.ArrayList; -import java.util.Collections; -import java.util.Enumeration; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.UUID; /** @@ -51,157 +48,25 @@ import java.util.UUID; * * @since 2.3.18 */ -public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { +public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { private static final Logger LOG = LogManager.getLogger(JakartaStreamMultiPartRequest.class); - /** - * Map between file fields and file data. - */ - protected Map> uploadedFiles = new HashMap<>(); - - /** - * Map between non-file fields and values. - */ - protected Map> parameters = new HashMap<>(); - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#cleanUp() - */ - public void cleanUp() { - LOG.debug("Performing File Upload temporary storage cleanup."); - for (List uploadedFileList : uploadedFiles.values()) { - for (UploadedFile uploadedFile : uploadedFileList) { - LOG.debug("Deleting file '{}'.", uploadedFile.getName()); - if (!uploadedFile.delete()) { - LOG.warn("There was a problem attempting to delete file '{}'.", uploadedFile.getName()); - } - } - } - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getContentType(java.lang.String) - */ - public String[] getContentType(String fieldName) { - List uploadedFileList = uploadedFiles.get(fieldName); - if (uploadedFileList == null) { - return null; - } - return uploadedFileList.stream().map(UploadedFile::getContentType).toArray(String[]::new); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFile(java.lang.String) - */ - public UploadedFile[] getFile(String fieldName) { - List uploadedFileList = uploadedFiles.get(fieldName); - if (uploadedFileList == null) { - return null; - } - return uploadedFileList.toArray(UploadedFile[]::new); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileNames(java.lang.String) - */ - public String[] getFileNames(String fieldName) { - List uploadedFileList = uploadedFiles.get(fieldName); - if (uploadedFileList == null) { - return null; - } - return uploadedFileList.stream().map(UploadedFile::getOriginalName).toArray(String[]::new); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileParameterNames() - */ - public Enumeration getFileParameterNames() { - return Collections.enumeration(uploadedFiles.keySet()); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFilesystemName(java.lang.String) - */ - public String[] getFilesystemName(String fieldName) { - List uploadedFileList = uploadedFiles.get(fieldName); - if (uploadedFileList == null) { - return null; - } - return uploadedFileList.stream().map(UploadedFile::getAbsolutePath).toArray(String[]::new); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameter(java.lang.String) - */ - public String getParameter(String name) { - List values = parameters.get(name); - if (values != null && !values.isEmpty()) { - return values.get(0); - } - return null; - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterNames() - */ - public Enumeration getParameterNames() { - return Collections.enumeration(parameters.keySet()); - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterValues(java.lang.String) - */ - public String[] getParameterValues(String name) { - List values = parameters.get(name); - if (values != null && !values.isEmpty()) { - return values.toArray(new String[0]); - } - return null; - } - - /* (non-Javadoc) - * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#parse(jakarta.servlet.http.HttpServletRequest, java.lang.String) - */ - public void parse(HttpServletRequest request, String saveDir) throws IOException { - try { - setLocale(request); - processUpload(request, saveDir); - } catch (Exception e) { - LOG.debug("Error occurred during parsing of multi part request", e); - LocalizedMessage errorMessage = buildErrorMessage(e, new Object[]{}); - if (!errors.contains(errorMessage)) { - errors.add(errorMessage); - } - } - } - /** * Processes the upload. * * @param request the servlet request * @param saveDir location of the save dir */ + @Override protected void processUpload(HttpServletRequest request, String saveDir) throws IOException { - // Sanity check that the request is a multi-part/form-data request. - if (!JakartaServletFileUpload.isMultipartContent(request)) { - LOG.debug("Http request isn't: {}, stop processing", AbstractFileUpload.MULTIPART_FORM_DATA); - return; - } + String charset = StringUtils.isBlank(request.getCharacterEncoding()) + ? defaultEncoding + : request.getCharacterEncoding(); - JakartaServletFileUpload servletFileUpload = new JakartaServletFileUpload<>(); - if (maxSize != null) { - LOG.debug("Applies max size: {} to file upload request", maxSize); - servletFileUpload.setSizeMax(maxSize); - } - if (maxFiles != null) { - LOG.debug("Applies max files number: {} to file upload request", maxFiles); - servletFileUpload.setFileCountMax(maxFiles); - } - if (maxFileSize != null) { - LOG.debug("Applies max size of single file: {} to file upload request", maxFileSize); - servletFileUpload.setFileSizeMax(maxFileSize); - } + Path location = Path.of(saveDir); + JakartaServletDiskFileUpload servletFileUpload = + prepareServletFileUpload(Charset.forName(charset), location); LOG.debug("Using Jakarta Stream API to process request"); servletFileUpload.getItemIterator(request).forEachRemaining(item -> { @@ -210,32 +75,57 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { processFileItemAsFormField(item); } else { LOG.debug(() -> "Processing a file: " + sanitizeNewlines(item.getFieldName())); - processFileItemAsFileField(item, saveDir); + processFileItemAsFileField(item, location); } }); } + 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]; + for (int length; (length = inputStream.read(buffer)) != -1; ) { + result.write(buffer, 0, length); + } + return result.toString(StandardCharsets.UTF_8); + } + /** * Processes the FileItem as a normal form field. * * @param fileItemInput a form field item input */ - protected void processFileItemAsFormField(FileItemInput fileItemInput) { + protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IOException { String fieldName = fileItemInput.getFieldName(); - try { - List values; + String fieldValue = readStream(fileItemInput.getInputStream()); - String fieldValue = fileItemInput.getInputStream().toString(); - if (!parameters.containsKey(fieldName)) { - values = new ArrayList<>(); - parameters.put(fieldName, values); - } else { - values = parameters.get(fieldName); - } - values.add(fieldValue); - } catch (IOException e) { - LOG.warn(() -> "Failed to handle form field: " + sanitizeNewlines(fieldName), e); + if (exceedsMaxStringLength(fieldName, fieldValue)) { + return; } + + List values; + if (parameters.containsKey(fieldName)) { + values = parameters.get(fieldName); + } else { + values = new ArrayList<>(); + parameters.put(fieldName, values); + } + values.add(fieldValue); } /** @@ -244,32 +134,16 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { * @param fileItemInput file item representing upload file * @param location location */ - protected void processFileItemAsFileField(FileItemInput fileItemInput, String location) { + protected void processFileItemAsFileField(FileItemInput fileItemInput, Path location) throws IOException { // Skip file uploads that don't have a file name - meaning that no file was selected. if (fileItemInput.getName() == null || fileItemInput.getName().trim().isEmpty()) { LOG.debug(() -> "No file has been uploaded for the field: " + sanitizeNewlines(fileItemInput.getFieldName())); return; } - File file = null; - try { - // Create the temporary upload file. - file = createTemporaryFile(fileItemInput.getName(), location); - - if (streamFileToDisk(fileItemInput, file)) { - createUploadedFile(fileItemInput, file); - } - } catch (IOException e) { - if (file != null) { - try { - if (!file.delete()) { - LOG.warn("Could not delete the file: {}", file.getAbsoluteFile()); - } - } catch (SecurityException se) { - LOG.warn("Failed to delete '{}' due to security exception above.", file.getName(), se); - } - } - } + File file = createTemporaryFile(fileItemInput.getName(), location); + streamFileToDisk(fileItemInput, file); + createUploadedFile(fileItemInput, file); } /** @@ -278,27 +152,11 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { * @param fileName file name * @param location location * @return a temporary file based on the given filename and location - * @throws IOException in case of IO errors */ - protected File createTemporaryFile(String fileName, String location) throws IOException { - String name = fileName - .substring(fileName.lastIndexOf('/') + 1) - .substring(fileName.lastIndexOf('\\') + 1); - - String prefix = name; - String suffix = ""; - - if (name.contains(".")) { - prefix = name.substring(0, name.lastIndexOf('.')); - suffix = name.substring(name.lastIndexOf('.')); - } - - if (prefix.length() < 3) { - prefix = UUID.randomUUID().toString(); - } - - File file = File.createTempFile(prefix + "_", suffix, new File(location)); - LOG.debug("Creating temporary file '{}' (originally '{}').", file.getName(), fileName); + protected File createTemporaryFile(String fileName, Path location) { + String uid = UUID.randomUUID().toString().replace("-", "_"); + File file = location.resolve("upload_" + uid + ".tmp").toFile(); + LOG.debug("Creating temporary file: {} (originally: {})", file.getName(), fileName); return file; } @@ -307,23 +165,16 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { * * @param fileItemInput file item input * @param file the file - * @return true if stream was successfully - * @throws IOException in case of IO errors */ - protected boolean streamFileToDisk(FileItemInput fileItemInput, File file) throws IOException { - try (InputStream input = fileItemInput.getInputStream(); - OutputStream output = new BufferedOutputStream(Files.newOutputStream(file.toPath()), bufferSize)) { + protected void streamFileToDisk(FileItemInput fileItemInput, File file) throws IOException { + InputStream input = fileItemInput.getInputStream(); + try (OutputStream output = new BufferedOutputStream(Files.newOutputStream(file.toPath()), bufferSize)) { byte[] buffer = new byte[bufferSize]; - LOG.debug("Streaming file using buffer size: {}", bufferSize); + LOG.debug("Streaming file: {} using buffer size: {}", fileItemInput.getName(), bufferSize); for (int length; ((length = input.read(buffer)) > 0); ) { output.write(buffer, 0, length); } - } catch (IOException e) { - LOG.error(new ParameterizedMessage("Cannot write input file: {} into file stream: {}", - fileItemInput.getName(), file.getAbsolutePath()), e); - return false; } - return true; } /** @@ -333,22 +184,21 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { * @param file the file */ protected void createUploadedFile(FileItemInput fileItemInput, File file) { - // gather attributes from file upload stream. String fileName = fileItemInput.getName(); String fieldName = fileItemInput.getFieldName(); - // create internal structure - UploadedFile uploadedFile = StrutsUploadedFile.Builder + + UploadedFile uploadedFile = StrutsUploadedFile.Builder .create(file) .withOriginalName(fileName) .withContentType(fileItemInput.getContentType()) .build(); - // append or create new entry. - if (!uploadedFiles.containsKey(fieldName)) { - List infos = new ArrayList<>(); + + if (uploadedFiles.containsKey(fieldName)) { + uploadedFiles.get(fieldName).add(uploadedFile); + } else { + List> infos = new ArrayList<>(); infos.add(uploadedFile); uploadedFiles.put(fieldName, infos); - } else { - uploadedFiles.get(fieldName).add(uploadedFile); } } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java index 5976f578f..2359b222c 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/StrutsUploadedFile.java @@ -20,7 +20,7 @@ package org.apache.struts2.dispatcher.multipart; import java.io.File; -public class StrutsUploadedFile implements UploadedFile { +public class StrutsUploadedFile implements UploadedFile { private final File file; private final String contentType; @@ -28,6 +28,7 @@ public class StrutsUploadedFile implements UploadedFile { /** * Use builder instead of constructor + * * @param file an uploaded file * @deprecated since Struts 6.4.0 */ @@ -87,9 +88,9 @@ public class StrutsUploadedFile implements UploadedFile { @Override public String toString() { return "StrutsUploadedFile{" + - "contentType='" + contentType + '\'' + - ", originalName='" + originalName + '\'' + - '}'; + "contentType='" + contentType + '\'' + + ", originalName='" + originalName + '\'' + + '}'; } public static class Builder { @@ -115,7 +116,7 @@ public class StrutsUploadedFile implements UploadedFile { return this; } - public UploadedFile build() { + public UploadedFile build() { return new StrutsUploadedFile(this.file, this.contentType, this.originalName); } } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java index ada27ff6c..7b70c1785 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/UploadedFile.java @@ -23,7 +23,7 @@ import java.io.Serializable; /** * Virtual representation of a uploaded file used by {@link MultiPartRequest} */ -public interface UploadedFile extends Serializable { +public interface UploadedFile extends Serializable { Long length(); @@ -37,7 +37,7 @@ public interface UploadedFile extends Serializable { String getAbsolutePath(); - Object getContent(); + T getContent(); String getContentType(); diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java new file mode 100644 index 000000000..f96383a7a --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java @@ -0,0 +1,408 @@ +/* + * 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 org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; +import org.apache.struts2.dispatcher.LocalizedMessage; +import org.assertj.core.api.InstanceOfAssertFactories; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.springframework.mock.web.MockHttpServletRequest; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; + +import static org.assertj.core.api.Assertions.assertThat; + +abstract class AbstractMultiPartRequestTest { + + protected MockHttpServletRequest mockRequest; + + protected final String boundary = "_boundary_"; + protected final String endline = "\r\n"; + + protected AbstractMultiPartRequest multiPart; + protected Path tempDir; + + abstract protected AbstractMultiPartRequest createMultipartRequest(); + + @Before + public void before() { + mockRequest = new MockHttpServletRequest(); + mockRequest.setCharacterEncoding(StandardCharsets.UTF_8.name()); + mockRequest.setMethod("post"); + mockRequest.setContentType("multipart/form-data; boundary=" + boundary); + + multiPart = createMultipartRequest(); + tempDir = Paths.get("target", "multi-part-test"); + } + + @After + public void after() { + multiPart.cleanUp(); + } + + @Test + public void uploadedFilesToDisk() throws IOException { + // given + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + // when + multiPart.setBufferSize("1"); // always write files into disk + multiPart.parse(mockRequest, tempDir.toString()); + + // then + assertThat(multiPart.getErrors()) + .isEmpty(); + + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable() + .asList() + .containsOnly("file1", "file2"); + assertThat(multiPart.getFile("file1")).allSatisfy(file -> { + assertThat(file.isFile()) + .isTrue(); + assertThat(file.getOriginalName()) + .isEqualTo("test1.csv"); + assertThat(file.getContentType()) + .isEqualTo("text/csv"); + assertThat(file.getContent()).asInstanceOf(InstanceOfAssertFactories.FILE) + .exists() + .content() + .isEqualTo("1,2,3,4"); + }); + assertThat(multiPart.getFile("file2")).allSatisfy(file -> { + assertThat(file.isFile()) + .isTrue(); + assertThat(file.getOriginalName()) + .isEqualTo("test2.csv"); + assertThat(file.getContentType()) + .isEqualTo("text/csv"); + assertThat(file.getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .exists() + .content() + .isEqualTo("5,6,7,8"); + ; + }); + } + + @Test + public void uploadedFilesWithLargeBuffer() throws IOException { + // given + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + // when + multiPart.setBufferSize("8192"); // streams files into disk using larger buffer + multiPart.parse(mockRequest, tempDir.toString()); + + // then + assertThat(multiPart.getErrors()) + .isEmpty(); + + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable() + .asList() + .containsOnly("file1", "file2"); + assertThat(multiPart.getFile("file1")).allSatisfy(file -> { + assertThat(file.isFile()) + .isTrue(); + assertThat(file.getOriginalName()) + .isEqualTo("test1.csv"); + assertThat(file.getContentType()) + .isEqualTo("text/csv"); + assertThat(file.getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .exists() + .content() + .isEqualTo("1,2,3,4"); + }); + assertThat(multiPart.getFile("file2")).allSatisfy(file -> { + assertThat(file.isFile()) + .isTrue(); + assertThat(file.getOriginalName()) + .isEqualTo("test2.csv"); + assertThat(file.getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .exists() + .content() + .isEqualTo("5,6,7,8"); + }); + } + + @Test + public void cleanUp() throws IOException { + // given + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + // when + multiPart.parse(mockRequest, tempDir.toString()); + + // then + assertThat(multiPart.getErrors()) + .isEmpty(); + + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable() + .asList() + .containsOnly("file1", "file2"); + assertThat(multiPart.getFile("file1")).allSatisfy(file -> { + assertThat(file.isFile()) + .isTrue(); + assertThat(file.getOriginalName()) + .isEqualTo("test1.csv"); + assertThat(file.getContentType()) + .isEqualTo("text/csv"); + assertThat(file.getContent()).asInstanceOf(InstanceOfAssertFactories.FILE) + .exists() + .content() + .isEqualTo("1,2,3,4"); + }); + assertThat(multiPart.getFile("file2")).allSatisfy(file -> { + assertThat(file.isFile()) + .isTrue(); + assertThat(file.getOriginalName()) + .isEqualTo("test2.csv"); + assertThat(file.getContentType()) + .isEqualTo("text/csv"); + assertThat(file.getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .exists() + .content() + .isEqualTo("5,6,7,8"); + ; + }); + + // when + multiPart.cleanUp(); + + // then + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable() + .asList() + .containsOnly("file1", "file2"); + assertThat(multiPart.getFile("file1")).allSatisfy(file -> { + assertThat(file.isFile()) + .isFalse(); + assertThat(file.getOriginalName()) + .isEqualTo("test1.csv"); + assertThat(file.getContentType()) + .isEqualTo("text/csv"); + assertThat(file.getContent()).asInstanceOf(InstanceOfAssertFactories.FILE) + .doesNotExist(); + }); + assertThat(multiPart.getFile("file2")).allSatisfy(file -> { + assertThat(file.isFile()) + .isFalse(); + assertThat(file.getOriginalName()) + .isEqualTo("test2.csv"); + assertThat(file.getContentType()) + .isEqualTo("text/csv"); + assertThat(file.getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .doesNotExist(); + }); + } + + @Test + public void nonMultiPartUpload() throws IOException { + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + endline + "--" + boundary + "--"; + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + // given + mockRequest.setContentType(""); + + // when + multiPart.parse(mockRequest, tempDir.toString()); + + // then + assertThat(multiPart.getErrors()) + .map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadContentTypeException"); + + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable() + .asList() + .isEmpty(); + } + + @Test + public void maxSize() throws IOException { + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + multiPart.setMaxSize("1"); + multiPart.parse(mockRequest, tempDir.toString()); + + Arrays.stream(multiPart.getFile("file1")).findFirst().map(UploadedFile::length); + + assertThat(multiPart.getErrors()) + .map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadSizeException"); + } + + @Test + public void maxFilesSize() throws IOException { + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + multiPart.setMaxFileSize("1"); + multiPart.parse(mockRequest, tempDir.toString()); + + assertThat(multiPart.getErrors()) + .map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadByteCountLimitException"); + } + + @Test + public void maxStringLength() throws IOException { + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + formField("longText", "very long text") + + formField("shortText", "short text") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + multiPart.setMaxStringLength("10"); + multiPart.parse(mockRequest, tempDir.toString()); + + assertThat(multiPart.getErrors()) + .map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.parameter.too.long"); + } + + @Test + public void mismatchCharset() throws IOException { + // give + String content = formFile("file1", "test1.csv", "Ł,Ś,Ż,Ó") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + // when + mockRequest.setCharacterEncoding(null); + multiPart.setDefaultEncoding(StandardCharsets.ISO_8859_1.name()); + multiPart.parse(mockRequest, tempDir.toString()); + + // then + assertThat(multiPart.getErrors()) + .isEmpty(); + + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable() + .asList() + .containsOnly("file1"); + assertThat(multiPart.getFile("file1")).allSatisfy(file -> { + assertThat(file.isFile()) + .isTrue(); + assertThat(file.getOriginalName()) + .isEqualTo("test1.csv"); + assertThat(file.getContentType()) + .isEqualTo("text/csv"); + assertThat(file.getContent()) + .asInstanceOf(InstanceOfAssertFactories.FILE) + .exists() + .content() + .isEqualTo("Ł,Ś,Ż,Ó"); + }); + } + + @Test + public void normalFields() throws IOException { + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + formField("longText", "very long text") + + formField("shortText", "short text") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + multiPart.parse(mockRequest, tempDir.toString()); + + assertThat(multiPart.getErrors()) + .isEmpty(); + + assertThat(multiPart.getParameterNames().asIterator()).toIterable() + .hasSize(2) + .contains("longText", "shortText"); + assertThat(multiPart.getParameterValues("longText")) + .contains("very long text"); + assertThat(multiPart.getParameterValues("shortText")) + .contains("short text"); + assertThat(multiPart.getParameter("longText")) + .isEqualTo("very long text"); + assertThat(multiPart.getParameter("shortText")) + .isEqualTo("short text"); + } + + protected String formFile(String fieldName, String filename, String content) { + return endline + + "--" + boundary + endline + + "Content-Disposition: form-data; name=\"" + fieldName + "\"; filename=\"" + filename + "\"" + + endline + + "Content-Type: text/csv" + + endline + + endline + + content; + } + + protected String formField(String fieldName, String content) { + return endline + + "--" + boundary + endline + + "Content-Disposition: form-data; name=\"" + fieldName + "\"" + + endline + + endline + + content; + } +} diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java new file mode 100644 index 000000000..056f68cf3 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java @@ -0,0 +1,56 @@ +/* + * 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 org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; +import org.apache.struts2.dispatcher.LocalizedMessage; +import org.junit.Test; + +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; + +import static org.assertj.core.api.Assertions.assertThat; + +public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest { + + @Override + protected AbstractMultiPartRequest createMultipartRequest() { + return new JakartaMultiPartRequest(); + } + + @Test + public void maxFiles() throws IOException { + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.US_ASCII)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + multiPart.setMaxFiles("1"); + multiPart.parse(mockRequest, tempDir.toString()); + + assertThat(multiPart.errors) + .map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadFileCountLimitException"); + } + +} \ No newline at end of file diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java index 89a01b8a5..111466fb6 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java @@ -18,54 +18,42 @@ */ package org.apache.struts2.dispatcher.multipart; -import java.io.ByteArrayInputStream; +import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; +import org.apache.struts2.dispatcher.LocalizedMessage; +import org.junit.Test; + +import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; -import java.nio.file.Path; -import java.nio.file.Paths; -import org.apache.struts2.dispatcher.LocalizedMessage; -import org.junit.Assert; -import org.junit.Before; -import org.junit.Test; -import org.mockito.Mockito; -import org.springframework.mock.web.DelegatingServletInputStream; +import static org.assertj.core.api.Assertions.assertThat; -import jakarta.servlet.http.HttpServletRequest; +public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestTest { -public class JakartaStreamMultiPartRequestTest { - - private JakartaStreamMultiPartRequest multiPart; - private Path tempDir; - - @Before - public void initialize() { - multiPart = new JakartaStreamMultiPartRequest(); - tempDir = Paths.get("target", "multi-part-test"); + @Override + protected AbstractMultiPartRequest createMultipartRequest() { + return new JakartaStreamMultiPartRequest(); } - /** - * Number of bytes in files greater than 2GB overflow the {@code int} primative. - * The {@link HttpServletRequest#getContentLength()} returns {@literal -1} - * when the header is not present or the size is greater than {@link Integer#MAX_VALUE}. - */ @Test - public void unknownContentLength() throws IOException { - HttpServletRequest request = Mockito.mock(HttpServletRequest.class); - Mockito.when(request.getContentType()).thenReturn("multipart/form-data; charset=utf-8; boundary=__X_BOUNDARY__"); - Mockito.when(request.getMethod()).thenReturn("POST"); - Mockito.when(request.getContentLength()).thenReturn(-1); - String entity = "\r\n--__X_BOUNDARY__\r\n" + - "Content-Disposition: form-data; name=\"upload\"; filename=\"test.csv\"\r\n" + - "Content-Type: text/csv\r\n\r\n1,2\r\n\r\n" + - "--__X_BOUNDARY__\r\n" + - "Content-Disposition: form-data; name=\"upload2\"; filename=\"test2.csv\"\r\n" + - "Content-Type: text/csv\r\n\r\n3,4\r\n\r\n" + - "--__X_BOUNDARY__--\r\n"; - Mockito.when(request.getInputStream()).thenReturn(new DelegatingServletInputStream(new ByteArrayInputStream(entity.getBytes(StandardCharsets.UTF_8)))); - multiPart.setMaxSize("4"); - multiPart.parse(request, tempDir.toString()); - LocalizedMessage next = multiPart.getErrors().iterator().next(); - Assert.assertEquals(next.getTextKey(), "struts.messages.upload.error.FileUploadSizeException"); + public void maxFilesNotSupportedInJakartaStreamMultiPartRequest() throws IOException { + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formFile("file2", "test2.csv", "5,6,7,8") + + endline + "--" + boundary + "--"; + + mockRequest.setContent(content.getBytes(StandardCharsets.US_ASCII)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + multiPart.setMaxFiles("1"); + multiPart.parse(mockRequest, tempDir.toString()); + + assertThat(multiPart.errors) + .map(LocalizedMessage::getTextKey) + .isEmpty(); + assertThat(multiPart.getFileParameterNames().asIterator()).toIterable() + .hasSize(2) + .contains("file1", "file2"); } + } diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index fced80c65..fe9468b37 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -333,7 +333,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { String content = encodeTextFile("test.html", "text/plain", plainContent) + encodeTextFile("test1.html", "text/html", htmlContent) + encodeTextFile("test2.html", "text/html", htmlContent) + - endline + "--" + boundary + "--";; + endline + "--" + boundary + "--"; req.setContent(content.getBytes()); assertTrue(JakartaServletDiskFileUpload.isMultipartContent(req));