[
https://issues.apache.org/jira/browse/WW-5388?focusedWorklogId=902080&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-902080
]
ASF GitHub Bot logged work on WW-5388:
--------------------------------------
Author: ASF GitHub Bot
Created on: 27/Jan/24 08:22
Start Date: 27/Jan/24 08:22
Worklog Time Spent: 10m
Work Description: github-advanced-security[bot] commented on code in PR
#861:
URL: https://github.com/apache/struts/pull/861#discussion_r1468415429
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java:
##########
@@ -52,324 +48,119 @@
*
* @since 2.3.18
*/
-public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
+public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest<File> {
- static final Logger LOG =
LogManager.getLogger(JakartaStreamMultiPartRequest.class);
-
- /**
- * Map between file fields and file data.
- */
- protected Map<String, List<FileInfo>> fileInfos = new HashMap<>();
-
- /**
- * Map between non-file fields and values.
- */
- protected Map<String, List<String>> 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<FileInfo> fileInfoList : fileInfos.values()) {
- for (FileInfo fileInfo : fileInfoList) {
- File file = fileInfo.getFile();
- LOG.debug("Deleting file '{}'.", file.getName());
- if (!file.delete()) {
- LOG.warn("There was a problem attempting to delete file
'{}'.", file.getName());
- }
- }
- }
- }
-
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getContentType(java.lang.String)
- */
- public String[] getContentType(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
-
- List<String> types = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- types.add(fileInfo.getContentType());
- }
-
- return types.toArray(new String[0]);
- }
-
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFile(java.lang.String)
- */
- public UploadedFile[] getFile(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
-
- return infos.stream().map(fileInfo ->
- StrutsUploadedFile.Builder.create(fileInfo.getFile())
- .withContentType(fileInfo.contentType)
- .withOriginalName(fileInfo.originalName)
- .build()
- ).toArray(UploadedFile[]::new);
- }
-
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileNames(java.lang.String)
- */
- public String[] getFileNames(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
-
- List<String> names = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- names.add(getCanonicalName(fileInfo.getOriginalName()));
- }
-
- return names.toArray(new String[0]);
- }
-
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileParameterNames()
- */
- public Enumeration<String> getFileParameterNames() {
- return Collections.enumeration(fileInfos.keySet());
- }
-
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFilesystemName(java.lang.String)
- */
- public String[] getFilesystemName(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
-
- List<String> names = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- names.add(fileInfo.getFile().getName());
- }
-
- return names.toArray(new String[0]);
- }
-
- /* (non-Javadoc)
- * @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameter(java.lang.String)
- */
- public String getParameter(String name) {
- List<String> 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<String> 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<String> 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);
- }
- }
- }
+ private static final Logger LOG =
LogManager.getLogger(JakartaStreamMultiPartRequest.class);
/**
* Processes the upload.
*
* @param request the servlet request
* @param saveDir location of the save dir
*/
- protected void processUpload(HttpServletRequest request, String saveDir)
throws Exception {
-
+ @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)) {
+ if (!JakartaServletFileUpload.isMultipartContent(request)) {
+ LOG.debug("Http request isn't: {}, stop processing",
AbstractFileUpload.MULTIPART_FORM_DATA);
+ return;
+ }
- // Sanity check on request size.
- boolean requestSizePermitted = isRequestSizePermitted(request);
+ JakartaServletFileUpload<DiskFileItem, DiskFileItemFactory>
servletFileUpload =
+ prepareServletFileUpload(request.getCharacterEncoding(),
saveDir);
- // Interface with Commons FileUpload API
- // Using the Streaming API
- JakartaServletFileUpload<DiskFileItem, DiskFileItemFactory>
servletFileUpload = new JakartaServletFileUpload<>();
- if (maxSize != null) {
- servletFileUpload.setSizeMax(maxSize);
- }
- if (maxFiles != null) {
- servletFileUpload.setFileCountMax(maxFiles);
- }
- if (maxFileSize != null) {
- servletFileUpload.setFileSizeMax(maxFileSize);
+ LOG.debug("Using Jakarta Stream API to process request");
+ servletFileUpload.getItemIterator(request).forEachRemaining(item -> {
+ if (item.isFormField()) {
+ LOG.debug(() -> "Processing a form field: " +
sanitizeNewlines(item.getFieldName()));
+ processFileItemAsFormField(item);
+ } else {
+ LOG.debug(() -> "Processing a file: " +
sanitizeNewlines(item.getFieldName()));
+ processFileItemAsFileField(item, saveDir);
}
- FileItemInputIterator i =
servletFileUpload.getItemIterator(request);
+ });
+ }
- // Iterate the file items
- while (i.hasNext()) {
- try {
- FileItemInput itemStream = i.next();
+ protected JakartaServletDiskFileUpload createJakartaFileUpload(String
charset, String saveDir) {
+ DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder();
+ if (saveDir != null) {
+ LOG.debug("Using file save directory: {}", saveDir);
+ builder.setPath(saveDir);
+ }
- // If the file item stream is a form field, delegate to the
- // field item stream handler
- if (itemStream.isFormField()) {
- processFileItemStreamAsFormField(itemStream);
- }
+ LOG.debug("Sets buffer size: {}", bufferSize);
+ builder.setBufferSize(bufferSize);
- // Delegate the file item stream for a file field to the
- // file item stream handler, but delegation is skipped
- // if the requestSizePermitted check failed based on the
- // complete content-size of the request.
- else {
+ LOG.debug("Using charset: {} or default: {}", charset,
defaultEncoding);
Review Comment:
## Logging should not be vulnerable to injection attacks
<!--SONAR_ISSUE_KEY:AY1KAquc8H-R3IcxNTE7-->Change this code to not log
user-controlled data. <p>See more on <a
href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AY1KAquc8H-R3IcxNTE7&open=AY1KAquc8H-R3IcxNTE7&pullRequest=861";>SonarCloud</a></p>
[Show more
details](https://github.com/apache/struts/security/code-scanning/425)
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -19,377 +19,121 @@
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.FileItem;
-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.JakartaServletFileUpload;
+import
org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
+import
org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletFileUpload;
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.io.UncheckedIOException;
import java.nio.charset.Charset;
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<File> {
- static final Logger LOG =
LogManager.getLogger(JakartaMultiPartRequest.class);
+ private static final Logger LOG =
LogManager.getLogger(JakartaMultiPartRequest.class);
- // maps parameter name -> List of FileItem objects
- protected Map<String, List<FileItem>> files = new HashMap<>();
+ @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;
+ }
- // maps parameter name -> List of param values
- protected Map<String, List<String>> params = new HashMap<>();
+ JakartaServletFileUpload<DiskFileItem, DiskFileItemFactory> upload =
+ prepareServletFileUpload(request.getCharacterEncoding(),
saveDir);
- /**
- * 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) {
- FileUploadByteCountLimitException ex =
(FileUploadByteCountLimitException) e;
- errorMessage = buildErrorMessage(e, new Object[]{
- ex.getFieldName(), ex.getFileName(),
ex.getPermitted(), ex.getActualSize()
- });
- } else if (e instanceof FileUploadFileCountLimitException) {
- FileUploadFileCountLimitException ex =
(FileUploadFileCountLimitException) e;
- errorMessage = buildErrorMessage(e, new Object[]{
- ex.getPermitted(), ex.getActualSize()
- });
- } else if (e instanceof FileUploadSizeException) {
- FileUploadSizeException ex = (FileUploadSizeException) e;
- errorMessage = buildErrorMessage(e, new Object[]{
- ex.getPermitted(), ex.getActualSize()
- });
- } else if (e instanceof FileUploadContentTypeException) {
- FileUploadContentTypeException ex =
(FileUploadContentTypeException) e;
- errorMessage = buildErrorMessage(e, new Object[]{
- ex.getContentType()
- });
+ for (DiskFileItem item : upload.parseRequest(request)) {
+ LOG.debug(() -> "Processing a form field: " +
sanitizeNewlines(item.getFieldName()));
+ if (item.isFormField()) {
+ processNormalFormField(item, request.getCharacterEncoding());
} 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);
+ LOG.debug(() -> "Processing a file: " +
sanitizeNewlines(item.getFieldName()));
+ processFileField(item);
}
}
}
- protected void processUpload(HttpServletRequest request, String saveDir)
throws IOException {
-
- if (JakartaServletFileUpload.isMultipartContent(request)) {
- for (FileItem item : parseRequest(request, saveDir)) {
- LOG.debug("Found file item: [{}]",
sanitizeNewlines(item.getFieldName()));
- if (item.isFormField()) {
- processNormalFormField(item,
request.getCharacterEncoding());
- } else {
- processFileField(item);
- }
- }
+ protected JakartaServletDiskFileUpload createJakartaFileUpload(String
charset, String saveDir) {
+ DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder();
+ if (saveDir != null) {
+ LOG.debug("Using file save directory: {}", saveDir);
+ builder.setPath(saveDir);
}
- }
- protected void processFileField(FileItem item) {
- LOG.debug("Item is a file upload");
+ LOG.debug("Sets minimal buffer size to always write file to disk");
+ builder.setBufferSize(1);
- // Skip file uploads that don't have a file name - meaning that no
file was selected.
- if (item.getName() == null || item.getName().trim().isEmpty()) {
- LOG.debug("No file has been uploaded for the field: {}",
sanitizeNewlines(item.getFieldName()));
- return;
- }
+ LOG.debug("Using charset: {} or default: {}", charset,
defaultEncoding);
Review Comment:
## Logging should not be vulnerable to injection attacks
<!--SONAR_ISSUE_KEY:AY1KAqt48H-R3IcxNTE6-->Change this code to not log
user-controlled data. <p>See more on <a
href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AY1KAqt48H-R3IcxNTE6&open=AY1KAqt48H-R3IcxNTE6&pullRequest=861";>SonarCloud</a></p>
[Show more
details](https://github.com/apache/struts/security/code-scanning/424)
Issue Time Tracking
-------------------
Worklog Id: (was: 902080)
Time Spent: 2h 10m (was: 2h)
> Upgrade Commons Fileupload to FileUpload Jakarta Servlet 6
> ----------------------------------------------------------
>
> Key: WW-5388
> URL: https://issues.apache.org/jira/browse/WW-5388
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.0.0
>
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> There is a new version of JakartaEE FileUpload
> {code:xml}
> <dependency>
> <groupId>org.apache.commons</groupId>
> <artifactId>commons-fileupload2-jakarta-servlet6</artifactId>
> <version>2.0.0-M2</version>
> </dependency>
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
