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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
