You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@struts.apache.org by "lukaszlenart (via GitHub)" <gi...@apache.org> on 2024/01/24 08:10:41 UTC
[PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
lukaszlenart opened a new pull request, #861:
URL: https://github.com/apache/struts/pull/861
Closes [WW-5388](https://issues.apache.org/jira/browse/WW-5388)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1915301295
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[86.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "criderp (via GitHub)" <gi...@apache.org>.
criderp commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1608506180
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java:
##########
@@ -171,4 +298,109 @@ protected String getCanonicalName(final String originalFileName) {
return fileName;
}
+ protected String sanitizeNewlines(String before) {
+ return before.replaceAll("\\R", "_");
+ }
+
+ /* (non-Javadoc)
+ * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getErrors()
+ */
+ public List<LocalizedMessage> getErrors() {
+ return errors;
+ }
+
+ /* (non-Javadoc)
+ * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileParameterNames()
+ */
+ public Enumeration<String> 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<T>[] 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<String> 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<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) {
+ return parameters.getOrDefault(name, Collections.emptyList())
+ .toArray(String[]::new);
Review Comment:
This change broke query parameters in multipart requests. Their values are being lost. The old implementation returned null if the parameter didn't exist, but it now returns an empty array. `MultiPartRequestWrapper::getParameterValues` relies on the fact that it returns null, though, so it's not working correctly anymore.
This is the old implementation:
```java
public String[] getParameterValues(String name) {
List<String> v = params.get(name);
if (v != null && !v.isEmpty()) {
return v.toArray(new String[0]);
}
return null;
}
```
And this is the `MultiPartRequestWrapper` method that doesn't work correctly after the change:
```java
public String[] getParameterValues(String name) {
return ((multi == null) || (multi.getParameterValues(name) == null)) ? super.getParameterValues(name) : multi.getParameterValues(name);
}
```
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913588018
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[85.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1469980120
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java:
##########
@@ -52,309 +51,159 @@
*
* @since 2.3.18
*/
-public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
+public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest<File> {
- static final Logger LOG = LogManager.getLogger(JakartaStreamMultiPartRequest.class);
+ private 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()
+ * Processes the upload.
+ *
+ * @param request the servlet request
+ * @param saveDir location of the save dir
*/
- 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());
- }
+ @Override
+ protected void processUpload(HttpServletRequest request, String saveDir) throws IOException {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
+
+ 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 -> {
+ 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, location);
}
- }
+ });
}
- /* (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;
- }
+ protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path location) {
+ DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder();
- List<String> types = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- types.add(fileInfo.getContentType());
- }
+ LOG.debug("Using file save directory: {}", location);
+ builder.setPath(location);
- return types.toArray(new String[0]);
- }
+ LOG.debug("Sets buffer size: {}", bufferSize);
+ builder.setBufferSize(bufferSize);
- /* (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;
- }
+ LOG.debug("Using charset: {}", charset);
+ builder.setCharset(charset);
- return infos.stream().map(fileInfo ->
- StrutsUploadedFile.Builder.create(fileInfo.getFile())
- .withContentType(fileInfo.contentType)
- .withOriginalName(fileInfo.originalName)
- .build()
- ).toArray(UploadedFile[]::new);
+ DiskFileItemFactory factory = builder.get();
+ return new JakartaServletDiskFileUpload(factory);
}
- /* (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()));
+ 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 names.toArray(new String[0]);
+ return result.toString(StandardCharsets.UTF_8);
}
- /* (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)
+ /**
+ * Processes the FileItem as a normal form field.
+ *
+ * @param fileItemInput a form field item input
*/
- public String[] getFilesystemName(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
+ protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IOException {
+ String fieldName = fileItemInput.getFieldName();
+ String fieldValue = readStream(fileItemInput.getInputStream());
- List<String> names = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- names.add(fileInfo.getFile().getName());
+ if (exceedsMaxStringLength(fieldName, fieldValue)) {
+ return;
}
- 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);
+ List<String> values;
+ if (parameters.containsKey(fieldName)) {
+ values = parameters.get(fieldName);
+ } else {
+ values = new ArrayList<>();
+ parameters.put(fieldName, values);
}
- return null;
+ values.add(fieldValue);
}
- /* (non-Javadoc)
- * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterNames()
+ /**
+ * @return actual size of already uploaded files
*/
- public Enumeration<String> getParameterNames() {
- return Collections.enumeration(parameters.keySet());
+ protected Long actualSizeOfUploadedFiles() {
+ return uploadedFiles.values().stream()
+ .map(files -> files.stream().map(UploadedFile::length).reduce(0L, Long::sum))
+ .reduce(0L, Long::sum);
}
- /* (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[]{});
+ private boolean exceedsMaxFiles(FileItemInput fileItemInput) {
+ if (maxFiles != null && maxFiles == uploadedFiles.size()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Cannot accept another file: {} as it will exceed max files: {}",
+ sanitizeNewlines(fileItemInput.getName()), maxFiles);
+ }
+ LocalizedMessage errorMessage = buildErrorMessage(new FileUploadFileCountLimitException(
+ String.format("File %s exceeds allowed maximum number of files %s",
+ fileItemInput.getName(), maxFiles),
+ maxFiles, uploadedFiles.size()),
+ new Object[]{maxFiles, uploadedFiles.size()});
if (!errors.contains(errorMessage)) {
errors.add(errorMessage);
}
+ return true;
}
+ return false;
}
- /**
- * Processes the upload.
- *
- * @param request the servlet request
- * @param saveDir location of the save dir
- */
- protected void processUpload(HttpServletRequest request, String saveDir) throws Exception {
-
- // Sanity check that the request is a multi-part/form-data request.
- if (JakartaServletFileUpload.isMultipartContent(request)) {
-
- // Sanity check on request size.
- boolean requestSizePermitted = isRequestSizePermitted(request);
-
- // 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);
- }
- FileItemInputIterator i = servletFileUpload.getItemIterator(request);
-
- // Iterate the file items
- while (i.hasNext()) {
- try {
- FileItemInput itemStream = i.next();
-
- // If the file item stream is a form field, delegate to the
- // field item stream handler
- if (itemStream.isFormField()) {
- processFileItemStreamAsFormField(itemStream);
- }
-
- // 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 {
-
- // prevent processing file field item if request size not allowed.
- if (!requestSizePermitted) {
- addFileSkippedError(itemStream.getName(), request);
- LOG.debug("Skipped stream '{}', request maximum size ({}) exceeded.", itemStream.getName(), maxSize);
- continue;
- }
-
- processFileItemStreamAsFileField(itemStream, saveDir);
- }
- } catch (IOException e) {
- LOG.warn("Error occurred during process upload", e);
- }
- }
+ private void exceedsMaxSizeOfFiles(FileItemInput fileItemInput, File file, Long currentFilesSize) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("File: {} of size: {} exceeds allowed max size: {}, actual size of already uploaded files: {}",
+ sanitizeNewlines(fileItemInput.getName()), file.length(), maxSizeOfFiles, currentFilesSize
+ );
}
- }
-
- /**
- * Defines whether the request allowed based on content length.
- *
- * @param request the servlet request
- * @return true if request size is permitted
- */
- protected boolean isRequestSizePermitted(HttpServletRequest request) {
- // if maxSize is specified as -1, there is no sanity check and it's
- // safe to return true for any request, delegating the failure
- // checks later in the upload process.
- if (maxSize == null || maxSize == -1 || request == null) {
- return true;
+ LocalizedMessage errorMessage = buildErrorMessage(new FileUploadSizeException(
+ String.format("Size %s of file %s exceeds allowed max size %s",
+ file.length(), fileItemInput.getName(), maxSizeOfFiles),
+ maxSizeOfFiles, currentFilesSize),
+ new Object[]{maxSizeOfFiles, currentFilesSize});
+ if (!errors.contains(errorMessage)) {
+ errors.add(errorMessage);
}
- return request.getContentLength() < maxSize;
- }
-
- /**
- * @param request the servlet request
- * @return the request content length.
- */
- protected long getRequestSize(HttpServletRequest request) {
- return request != null ? request.getContentLength() : 0;
- }
-
- /**
- * Add a file skipped message notification for action messages.
- *
- * @param fileName file name
- * @param request the servlet request
- */
- protected void addFileSkippedError(String fileName, HttpServletRequest request) {
- String exceptionMessage = "Skipped file " + fileName + "; request size limit exceeded.";
- long allowedMaxSize = maxSize != null ? maxSize : -1;
- FileUploadSizeException exception = new FileUploadSizeException(exceptionMessage, getRequestSize(request), allowedMaxSize);
- LocalizedMessage message = buildErrorMessage(exception, new Object[]{fileName, getRequestSize(request), allowedMaxSize});
- if (!errors.contains(message)) {
- errors.add(message);
+ if (!file.delete() && LOG.isWarnEnabled()) {
+ LOG.warn("Cannot delete file: {} which exceeds maximum size: {} of all files!",
+ sanitizeNewlines(fileItemInput.getName()), maxSizeOfFiles);
}
}
/**
- * Processes the FileItemStream as a Form Field.
+ * Processes the FileItem as a file field.
*
- * @param itemStream file item stream
+ * @param fileItemInput file item representing upload file
+ * @param location location
*/
- protected void processFileItemStreamAsFormField(FileItemInput itemStream) {
- String fieldName = itemStream.getFieldName();
- try {
- List<String> values;
-
- String fieldValue = itemStream.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 '{}'.", fieldName, e);
+ 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;
}
- }
- /**
- * Processes the FileItemStream as a file field.
- *
- * @param itemStream file item stream
- * @param location location
- */
- protected void processFileItemStreamAsFileField(FileItemInput itemStream, String location) {
- // Skip file uploads that don't have a file name - meaning that no file was selected.
- if (itemStream.getName() == null || itemStream.getName().trim().isEmpty()) {
- LOG.debug("No file has been uploaded for the field: {}", itemStream.getFieldName());
+ if (exceedsMaxFiles(fileItemInput)) {
return;
}
- File file = null;
- try {
- // Create the temporary upload file.
- file = createTemporaryFile(itemStream.getName(), location);
+ File file = createTemporaryFile(fileItemInput.getName(), location);
+ streamFileToDisk(fileItemInput, file);
- if (streamFileToDisk(itemStream, file)) {
- createFileInfoFromItemStream(itemStream, file);
- }
- } catch (IOException e) {
- if (file != null) {
- try {
- file.delete();
- } catch (SecurityException se) {
- LOG.warn("Failed to delete '{}' due to security exception above.", file.getName(), se);
- }
- }
+ Long currentFilesSize = actualSizeOfUploadedFiles();
Review Comment:
> This seems expensive to compute from scratch every time a file field is encountered, especially when the application might not have even configured a `maxSizeOfFiles` limit!
Fixed!
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart merged PR #861:
URL: https://github.com/apache/struts/pull/861
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1469989073
##########
core/src/main/java/org/apache/struts2/action/UploadedFilesAware.java:
##########
@@ -27,14 +27,14 @@
* The {@link org.apache.struts2.interceptor.ActionFileUploadInterceptor} will use the interface
* to notify action about the multiple uploaded files.
*/
-public interface UploadedFilesAware {
+public interface UploadedFilesAware<T> {
Review Comment:
Reverted
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "burtonrhodes (via GitHub)" <gi...@apache.org>.
burtonrhodes commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1468546626
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -21,375 +21,116 @@
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.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.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.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<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 {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
- // maps parameter name -> List of param values
- protected Map<String, List<String>> params = new HashMap<>();
+ JakartaServletDiskFileUpload servletFileUpload =
+ prepareServletFileUpload(Charset.forName(charset), Path.of(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 : servletFileUpload.parseRequest(request)) {
Review Comment:
@lukaszlenart Quick thought on the maxFiles setting: I still think it makes more sense that maxFiles would apply to only file fields and not all form fields in the request. However it appears that this logic is actually baked into the JakartaServletFileUpload class itself when calling `servletFileUpload.parseRequest()`. Wonder why they coded it this way??
Anyway, if we ever wanted to change this, you could set the maxFiles to unlimited in the servletFileUpload object, and then perform the files count check in the `if (it.isFormFIeld()...` logic below. Just a thought.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913093914
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[88.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1469408800
##########
core/src/main/java/org/apache/struts2/action/UploadedFilesAware.java:
##########
@@ -27,14 +27,14 @@
* The {@link org.apache.struts2.interceptor.ActionFileUploadInterceptor} will use the interface
* to notify action about the multiple uploaded files.
*/
-public interface UploadedFilesAware {
+public interface UploadedFilesAware<T> {
Review Comment:
This is a preparation to support `InputStream` and allow to stream files directly into actions, yet I need to thinks this through as this change became huge :\
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1920753839
LGTM 💯
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1912172055
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate failed**
Failed conditions
[76.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list) (required ≥ 80%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1908887616
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate failed**
Failed conditions
[64.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list) (required ≥ 80%)
[5.6% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list) (required ≤ 3%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1470711866
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java:
##########
@@ -52,309 +51,159 @@
*
* @since 2.3.18
*/
-public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
+public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest<File> {
- static final Logger LOG = LogManager.getLogger(JakartaStreamMultiPartRequest.class);
+ private 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()
+ * Processes the upload.
+ *
+ * @param request the servlet request
+ * @param saveDir location of the save dir
*/
- 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());
- }
+ @Override
+ protected void processUpload(HttpServletRequest request, String saveDir) throws IOException {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
+
+ 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 -> {
+ 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, location);
}
- }
+ });
}
- /* (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;
- }
+ protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path location) {
+ DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder();
- List<String> types = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- types.add(fileInfo.getContentType());
- }
+ LOG.debug("Using file save directory: {}", location);
+ builder.setPath(location);
- return types.toArray(new String[0]);
- }
+ LOG.debug("Sets buffer size: {}", bufferSize);
+ builder.setBufferSize(bufferSize);
- /* (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;
- }
+ LOG.debug("Using charset: {}", charset);
+ builder.setCharset(charset);
- return infos.stream().map(fileInfo ->
- StrutsUploadedFile.Builder.create(fileInfo.getFile())
- .withContentType(fileInfo.contentType)
- .withOriginalName(fileInfo.originalName)
- .build()
- ).toArray(UploadedFile[]::new);
+ DiskFileItemFactory factory = builder.get();
+ return new JakartaServletDiskFileUpload(factory);
}
- /* (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()));
+ 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 names.toArray(new String[0]);
+ return result.toString(StandardCharsets.UTF_8);
}
- /* (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)
+ /**
+ * Processes the FileItem as a normal form field.
+ *
+ * @param fileItemInput a form field item input
*/
- public String[] getFilesystemName(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
+ protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IOException {
+ String fieldName = fileItemInput.getFieldName();
+ String fieldValue = readStream(fileItemInput.getInputStream());
- List<String> names = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- names.add(fileInfo.getFile().getName());
+ if (exceedsMaxStringLength(fieldName, fieldValue)) {
+ return;
}
- 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);
+ List<String> values;
+ if (parameters.containsKey(fieldName)) {
+ values = parameters.get(fieldName);
+ } else {
+ values = new ArrayList<>();
+ parameters.put(fieldName, values);
}
- return null;
+ values.add(fieldValue);
}
- /* (non-Javadoc)
- * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterNames()
+ /**
+ * @return actual size of already uploaded files
*/
- public Enumeration<String> getParameterNames() {
- return Collections.enumeration(parameters.keySet());
+ protected Long actualSizeOfUploadedFiles() {
+ return uploadedFiles.values().stream()
+ .map(files -> files.stream().map(UploadedFile::length).reduce(0L, Long::sum))
+ .reduce(0L, Long::sum);
}
- /* (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[]{});
+ private boolean exceedsMaxFiles(FileItemInput fileItemInput) {
+ if (maxFiles != null && maxFiles == uploadedFiles.size()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Cannot accept another file: {} as it will exceed max files: {}",
+ sanitizeNewlines(fileItemInput.getName()), maxFiles);
+ }
+ LocalizedMessage errorMessage = buildErrorMessage(new FileUploadFileCountLimitException(
+ String.format("File %s exceeds allowed maximum number of files %s",
+ fileItemInput.getName(), maxFiles),
+ maxFiles, uploadedFiles.size()),
+ new Object[]{maxFiles, uploadedFiles.size()});
if (!errors.contains(errorMessage)) {
errors.add(errorMessage);
}
+ return true;
}
+ return false;
}
- /**
- * Processes the upload.
- *
- * @param request the servlet request
- * @param saveDir location of the save dir
- */
- protected void processUpload(HttpServletRequest request, String saveDir) throws Exception {
-
- // Sanity check that the request is a multi-part/form-data request.
- if (JakartaServletFileUpload.isMultipartContent(request)) {
-
- // Sanity check on request size.
- boolean requestSizePermitted = isRequestSizePermitted(request);
-
- // 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);
- }
- FileItemInputIterator i = servletFileUpload.getItemIterator(request);
-
- // Iterate the file items
- while (i.hasNext()) {
- try {
- FileItemInput itemStream = i.next();
-
- // If the file item stream is a form field, delegate to the
- // field item stream handler
- if (itemStream.isFormField()) {
- processFileItemStreamAsFormField(itemStream);
- }
-
- // 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 {
-
- // prevent processing file field item if request size not allowed.
- if (!requestSizePermitted) {
- addFileSkippedError(itemStream.getName(), request);
- LOG.debug("Skipped stream '{}', request maximum size ({}) exceeded.", itemStream.getName(), maxSize);
- continue;
- }
-
- processFileItemStreamAsFileField(itemStream, saveDir);
- }
- } catch (IOException e) {
- LOG.warn("Error occurred during process upload", e);
- }
- }
+ private void exceedsMaxSizeOfFiles(FileItemInput fileItemInput, File file, Long currentFilesSize) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("File: {} of size: {} exceeds allowed max size: {}, actual size of already uploaded files: {}",
+ sanitizeNewlines(fileItemInput.getName()), file.length(), maxSizeOfFiles, currentFilesSize
+ );
}
- }
-
- /**
- * Defines whether the request allowed based on content length.
- *
- * @param request the servlet request
- * @return true if request size is permitted
- */
- protected boolean isRequestSizePermitted(HttpServletRequest request) {
- // if maxSize is specified as -1, there is no sanity check and it's
- // safe to return true for any request, delegating the failure
- // checks later in the upload process.
- if (maxSize == null || maxSize == -1 || request == null) {
- return true;
+ LocalizedMessage errorMessage = buildErrorMessage(new FileUploadSizeException(
+ String.format("Size %s of file %s exceeds allowed max size %s",
+ file.length(), fileItemInput.getName(), maxSizeOfFiles),
+ maxSizeOfFiles, currentFilesSize),
+ new Object[]{maxSizeOfFiles, currentFilesSize});
+ if (!errors.contains(errorMessage)) {
+ errors.add(errorMessage);
}
- return request.getContentLength() < maxSize;
- }
-
- /**
- * @param request the servlet request
- * @return the request content length.
- */
- protected long getRequestSize(HttpServletRequest request) {
- return request != null ? request.getContentLength() : 0;
- }
-
- /**
- * Add a file skipped message notification for action messages.
- *
- * @param fileName file name
- * @param request the servlet request
- */
- protected void addFileSkippedError(String fileName, HttpServletRequest request) {
- String exceptionMessage = "Skipped file " + fileName + "; request size limit exceeded.";
- long allowedMaxSize = maxSize != null ? maxSize : -1;
- FileUploadSizeException exception = new FileUploadSizeException(exceptionMessage, getRequestSize(request), allowedMaxSize);
- LocalizedMessage message = buildErrorMessage(exception, new Object[]{fileName, getRequestSize(request), allowedMaxSize});
- if (!errors.contains(message)) {
- errors.add(message);
+ if (!file.delete() && LOG.isWarnEnabled()) {
+ LOG.warn("Cannot delete file: {} which exceeds maximum size: {} of all files!",
+ sanitizeNewlines(fileItemInput.getName()), maxSizeOfFiles);
}
}
/**
- * Processes the FileItemStream as a Form Field.
+ * Processes the FileItem as a file field.
*
- * @param itemStream file item stream
+ * @param fileItemInput file item representing upload file
+ * @param location location
*/
- protected void processFileItemStreamAsFormField(FileItemInput itemStream) {
- String fieldName = itemStream.getFieldName();
- try {
- List<String> values;
-
- String fieldValue = itemStream.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 '{}'.", fieldName, e);
+ 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;
}
- }
- /**
- * Processes the FileItemStream as a file field.
- *
- * @param itemStream file item stream
- * @param location location
- */
- protected void processFileItemStreamAsFileField(FileItemInput itemStream, String location) {
- // Skip file uploads that don't have a file name - meaning that no file was selected.
- if (itemStream.getName() == null || itemStream.getName().trim().isEmpty()) {
- LOG.debug("No file has been uploaded for the field: {}", itemStream.getFieldName());
+ if (exceedsMaxFiles(fileItemInput)) {
return;
}
- File file = null;
- try {
- // Create the temporary upload file.
- file = createTemporaryFile(itemStream.getName(), location);
+ File file = createTemporaryFile(fileItemInput.getName(), location);
+ streamFileToDisk(fileItemInput, file);
- if (streamFileToDisk(itemStream, file)) {
- createFileInfoFromItemStream(itemStream, file);
- }
- } catch (IOException e) {
- if (file != null) {
- try {
- file.delete();
- } catch (SecurityException se) {
- LOG.warn("Failed to delete '{}' due to security exception above.", file.getName(), se);
- }
- }
+ Long currentFilesSize = maxSizeOfFiles != null ? actualSizeOfUploadedFiles() : null;
Review Comment:
Not really as it is used inside the `if` clause in `exceedsMaxSizeOfFiles`
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "burtonrhodes (via GitHub)" <gi...@apache.org>.
burtonrhodes commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1920712327
@lukaszlenart yes, all looks good on my end.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "burtonrhodes (via GitHub)" <gi...@apache.org>.
burtonrhodes commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1468547208
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -21,375 +21,116 @@
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.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.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.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<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 {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
- // maps parameter name -> List of param values
- protected Map<String, List<String>> params = new HashMap<>();
+ JakartaServletDiskFileUpload servletFileUpload =
+ prepareServletFileUpload(Charset.forName(charset), Path.of(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 : servletFileUpload.parseRequest(request)) {
Review Comment:
In thinking about it more, that might be a security issue, as the serletFileUpload object would upload everything first (something that the setting is trying to prevent), only to perform the check after the processing is done. Ok, never mind. ;-)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913117492
/cc: @deki
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1909400822
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
No data about Coverage
[0.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1909601464
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate failed**
Failed conditions
[42.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list) (required ≥ 80%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sepe81 (via GitHub)" <gi...@apache.org>.
sepe81 commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1464504379
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java:
##########
@@ -171,4 +171,8 @@ protected String getCanonicalName(final String originalFileName) {
return fileName;
}
+ protected String sanitizeNewlines(String before) {
+ return before.replaceAll("[\n\r]", "_");
Review Comment:
```suggestion
return before.replaceAll("\R", "_");
```
maybe use this as more concise way to represent line breaks?
>Any Unicode linebreak sequence, is equivalent to \u000D\u000A|[\u000A\u000B\u000C\u000D\u0085\u2028\u2029]
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913074838
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate failed**
Failed conditions
[78.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list) (required ≥ 80%)
[B Security Rating on New Code](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) (required ≥ A)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913516396
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[87.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913520121
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[88.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1916335927
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[2 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[85.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1911767440
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate failed**
Failed conditions
[73.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list) (required ≥ 80%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1465493536
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java:
##########
@@ -171,4 +171,8 @@ protected String getCanonicalName(final String originalFileName) {
return fileName;
}
+ protected String sanitizeNewlines(String before) {
+ return before.replaceAll("[\n\r]", "_");
Review Comment:
done
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1911723344
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate failed**
Failed conditions
[65.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list) (required ≥ 80%)
[E Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) (required ≥ A)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "burtonrhodes (via GitHub)" <gi...@apache.org>.
burtonrhodes commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1468546626
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -21,375 +21,116 @@
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.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.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.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<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 {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
- // maps parameter name -> List of param values
- protected Map<String, List<String>> params = new HashMap<>();
+ JakartaServletDiskFileUpload servletFileUpload =
+ prepareServletFileUpload(Charset.forName(charset), Path.of(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 : servletFileUpload.parseRequest(request)) {
Review Comment:
@lukaszlenart Quick thought on the maxFiles setting: I still think it makes more sense that maxFiles would apply to only file fields and not all form fields in the request. However it appears that this logic is actually baked into the JakartaServletFileUpload class itself when calling `servletFileUpload.parseRequest()`. Wonder why they coded it this way??
Anyway, if we ever wanted to change this, you could set the maxFiles to unlimited in the servletFileUpload object, and then perform the files count check in the `if (item.isFormField()...` logic below. Just a thought.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1468840919
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -21,375 +21,116 @@
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.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.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.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<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 {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
- // maps parameter name -> List of param values
- protected Map<String, List<String>> params = new HashMap<>();
+ JakartaServletDiskFileUpload servletFileUpload =
+ prepareServletFileUpload(Charset.forName(charset), Path.of(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 : servletFileUpload.parseRequest(request)) {
Review Comment:
@burtonrhodes introduced a new constant `maxSizeOfFiles` which controls how many files you can upload based on their size - this works with `JakartaStreamMultiPartRequest` only.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913596224
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[85.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1916351667
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[2 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[87.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1608537187
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java:
##########
@@ -171,4 +298,109 @@ protected String getCanonicalName(final String originalFileName) {
return fileName;
}
+ protected String sanitizeNewlines(String before) {
+ return before.replaceAll("\\R", "_");
+ }
+
+ /* (non-Javadoc)
+ * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getErrors()
+ */
+ public List<LocalizedMessage> getErrors() {
+ return errors;
+ }
+
+ /* (non-Javadoc)
+ * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getFileParameterNames()
+ */
+ public Enumeration<String> 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<T>[] 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<String> 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<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) {
+ return parameters.getOrDefault(name, Collections.emptyList())
+ .toArray(String[]::new);
Review Comment:
@criderp please register this bug in [JIRA](https://issues.apache.org/jira/browse/WW), thanks!
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913108322
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[87.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "burtonrhodes (via GitHub)" <gi...@apache.org>.
burtonrhodes commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1915569409
Just FYI, I implemented this PR on a small subset of my server cluster and have been receiving the following ambiguous errors in some of the upload requests. I can't duplicate it in any testing environment, but curious if you might have a clue as to why Struts is creating an actionError with no value for the file. For this implementation, I am **_not_** using the stream parser
_Error printed by the code below_
```
Unidentified ActionError present for file upload: [Error uploading: {0}!, Error uploading: {0}!]
```
_Code that quickly checks for any file upload errors_
```
if (hasActionErrors()) {
List<String> uploadErrors = (List<String>) getActionErrors();
log.error("Unidentified ActionError present for file upload: " + uploadErrors);
}
```
_A redacted version of the action_
```
public class UploadLocalFileJsonAction extends ActionSupport implements Validateable, UploadedFilesAware<UploadedFile<File>> {
private FileAttachmentUpload fileAttachmentUpload;
@Override
public void withUploadedFiles(List<UploadedFile<UploadedFile<File>>> uploadedFiles) {
if (!uploadedFiles.isEmpty()) {
fileAttachmentUpload = UploadedFileHelper.createFromUploadedFile(uploadedFiles.get(0));
}
}
public void validate() {
// First check to see if there are any errors from the file upload
if (hasActionErrors()) {
List<String> uploadErrors = (List<String>) getActionErrors();
log.error("Unidentified ActionError present for file upload: " + uploadErrors);
}
}
public String execute() throws Exception {
// Omitted for brevity
}
}
```
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1916363079
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[87.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1470071366
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java:
##########
@@ -52,309 +51,159 @@
*
* @since 2.3.18
*/
-public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
+public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest<File> {
- static final Logger LOG = LogManager.getLogger(JakartaStreamMultiPartRequest.class);
+ private 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()
+ * Processes the upload.
+ *
+ * @param request the servlet request
+ * @param saveDir location of the save dir
*/
- 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());
- }
+ @Override
+ protected void processUpload(HttpServletRequest request, String saveDir) throws IOException {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
+
+ 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 -> {
+ 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, location);
}
- }
+ });
}
- /* (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;
- }
+ protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path location) {
+ DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder();
- List<String> types = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- types.add(fileInfo.getContentType());
- }
+ LOG.debug("Using file save directory: {}", location);
+ builder.setPath(location);
- return types.toArray(new String[0]);
- }
+ LOG.debug("Sets buffer size: {}", bufferSize);
+ builder.setBufferSize(bufferSize);
- /* (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;
- }
+ LOG.debug("Using charset: {}", charset);
+ builder.setCharset(charset);
- return infos.stream().map(fileInfo ->
- StrutsUploadedFile.Builder.create(fileInfo.getFile())
- .withContentType(fileInfo.contentType)
- .withOriginalName(fileInfo.originalName)
- .build()
- ).toArray(UploadedFile[]::new);
+ DiskFileItemFactory factory = builder.get();
+ return new JakartaServletDiskFileUpload(factory);
}
- /* (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()));
+ 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 names.toArray(new String[0]);
+ return result.toString(StandardCharsets.UTF_8);
}
- /* (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)
+ /**
+ * Processes the FileItem as a normal form field.
+ *
+ * @param fileItemInput a form field item input
*/
- public String[] getFilesystemName(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
+ protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IOException {
+ String fieldName = fileItemInput.getFieldName();
+ String fieldValue = readStream(fileItemInput.getInputStream());
- List<String> names = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- names.add(fileInfo.getFile().getName());
+ if (exceedsMaxStringLength(fieldName, fieldValue)) {
+ return;
}
- 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);
+ List<String> values;
+ if (parameters.containsKey(fieldName)) {
+ values = parameters.get(fieldName);
+ } else {
+ values = new ArrayList<>();
+ parameters.put(fieldName, values);
}
- return null;
+ values.add(fieldValue);
}
- /* (non-Javadoc)
- * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterNames()
+ /**
+ * @return actual size of already uploaded files
*/
- public Enumeration<String> getParameterNames() {
- return Collections.enumeration(parameters.keySet());
+ protected Long actualSizeOfUploadedFiles() {
+ return uploadedFiles.values().stream()
+ .map(files -> files.stream().map(UploadedFile::length).reduce(0L, Long::sum))
+ .reduce(0L, Long::sum);
}
- /* (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[]{});
+ private boolean exceedsMaxFiles(FileItemInput fileItemInput) {
+ if (maxFiles != null && maxFiles == uploadedFiles.size()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Cannot accept another file: {} as it will exceed max files: {}",
+ sanitizeNewlines(fileItemInput.getName()), maxFiles);
+ }
+ LocalizedMessage errorMessage = buildErrorMessage(new FileUploadFileCountLimitException(
+ String.format("File %s exceeds allowed maximum number of files %s",
+ fileItemInput.getName(), maxFiles),
+ maxFiles, uploadedFiles.size()),
+ new Object[]{maxFiles, uploadedFiles.size()});
if (!errors.contains(errorMessage)) {
errors.add(errorMessage);
}
+ return true;
}
+ return false;
}
- /**
- * Processes the upload.
- *
- * @param request the servlet request
- * @param saveDir location of the save dir
- */
- protected void processUpload(HttpServletRequest request, String saveDir) throws Exception {
-
- // Sanity check that the request is a multi-part/form-data request.
- if (JakartaServletFileUpload.isMultipartContent(request)) {
-
- // Sanity check on request size.
- boolean requestSizePermitted = isRequestSizePermitted(request);
-
- // 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);
- }
- FileItemInputIterator i = servletFileUpload.getItemIterator(request);
-
- // Iterate the file items
- while (i.hasNext()) {
- try {
- FileItemInput itemStream = i.next();
-
- // If the file item stream is a form field, delegate to the
- // field item stream handler
- if (itemStream.isFormField()) {
- processFileItemStreamAsFormField(itemStream);
- }
-
- // 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 {
-
- // prevent processing file field item if request size not allowed.
- if (!requestSizePermitted) {
- addFileSkippedError(itemStream.getName(), request);
- LOG.debug("Skipped stream '{}', request maximum size ({}) exceeded.", itemStream.getName(), maxSize);
- continue;
- }
-
- processFileItemStreamAsFileField(itemStream, saveDir);
- }
- } catch (IOException e) {
- LOG.warn("Error occurred during process upload", e);
- }
- }
+ private void exceedsMaxSizeOfFiles(FileItemInput fileItemInput, File file, Long currentFilesSize) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("File: {} of size: {} exceeds allowed max size: {}, actual size of already uploaded files: {}",
+ sanitizeNewlines(fileItemInput.getName()), file.length(), maxSizeOfFiles, currentFilesSize
+ );
}
- }
-
- /**
- * Defines whether the request allowed based on content length.
- *
- * @param request the servlet request
- * @return true if request size is permitted
- */
- protected boolean isRequestSizePermitted(HttpServletRequest request) {
- // if maxSize is specified as -1, there is no sanity check and it's
- // safe to return true for any request, delegating the failure
- // checks later in the upload process.
- if (maxSize == null || maxSize == -1 || request == null) {
- return true;
+ LocalizedMessage errorMessage = buildErrorMessage(new FileUploadSizeException(
+ String.format("Size %s of file %s exceeds allowed max size %s",
+ file.length(), fileItemInput.getName(), maxSizeOfFiles),
+ maxSizeOfFiles, currentFilesSize),
+ new Object[]{maxSizeOfFiles, currentFilesSize});
+ if (!errors.contains(errorMessage)) {
+ errors.add(errorMessage);
}
- return request.getContentLength() < maxSize;
- }
-
- /**
- * @param request the servlet request
- * @return the request content length.
- */
- protected long getRequestSize(HttpServletRequest request) {
- return request != null ? request.getContentLength() : 0;
- }
-
- /**
- * Add a file skipped message notification for action messages.
- *
- * @param fileName file name
- * @param request the servlet request
- */
- protected void addFileSkippedError(String fileName, HttpServletRequest request) {
- String exceptionMessage = "Skipped file " + fileName + "; request size limit exceeded.";
- long allowedMaxSize = maxSize != null ? maxSize : -1;
- FileUploadSizeException exception = new FileUploadSizeException(exceptionMessage, getRequestSize(request), allowedMaxSize);
- LocalizedMessage message = buildErrorMessage(exception, new Object[]{fileName, getRequestSize(request), allowedMaxSize});
- if (!errors.contains(message)) {
- errors.add(message);
+ if (!file.delete() && LOG.isWarnEnabled()) {
+ LOG.warn("Cannot delete file: {} which exceeds maximum size: {} of all files!",
+ sanitizeNewlines(fileItemInput.getName()), maxSizeOfFiles);
}
}
/**
- * Processes the FileItemStream as a Form Field.
+ * Processes the FileItem as a file field.
*
- * @param itemStream file item stream
+ * @param fileItemInput file item representing upload file
+ * @param location location
*/
- protected void processFileItemStreamAsFormField(FileItemInput itemStream) {
- String fieldName = itemStream.getFieldName();
- try {
- List<String> values;
-
- String fieldValue = itemStream.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 '{}'.", fieldName, e);
+ 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;
}
- }
- /**
- * Processes the FileItemStream as a file field.
- *
- * @param itemStream file item stream
- * @param location location
- */
- protected void processFileItemStreamAsFileField(FileItemInput itemStream, String location) {
- // Skip file uploads that don't have a file name - meaning that no file was selected.
- if (itemStream.getName() == null || itemStream.getName().trim().isEmpty()) {
- LOG.debug("No file has been uploaded for the field: {}", itemStream.getFieldName());
+ if (exceedsMaxFiles(fileItemInput)) {
return;
}
- File file = null;
- try {
- // Create the temporary upload file.
- file = createTemporaryFile(itemStream.getName(), location);
+ File file = createTemporaryFile(fileItemInput.getName(), location);
+ streamFileToDisk(fileItemInput, file);
- if (streamFileToDisk(itemStream, file)) {
- createFileInfoFromItemStream(itemStream, file);
- }
- } catch (IOException e) {
- if (file != null) {
- try {
- file.delete();
- } catch (SecurityException se) {
- LOG.warn("Failed to delete '{}' due to security exception above.", file.getName(), se);
- }
- }
+ Long currentFilesSize = maxSizeOfFiles != null ? actualSizeOfUploadedFiles() : null;
Review Comment:
You can just inline this right?
`if (maxSizeOfFiles != null && actualSizeOfUploadedFiles() + file.length() >= maxSizeOfFiles) {`
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913092004
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[83.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913524320
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[89.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "burtonrhodes (via GitHub)" <gi...@apache.org>.
burtonrhodes commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1468547208
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -21,375 +21,116 @@
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.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.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.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<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 {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
- // maps parameter name -> List of param values
- protected Map<String, List<String>> params = new HashMap<>();
+ JakartaServletDiskFileUpload servletFileUpload =
+ prepareServletFileUpload(Charset.forName(charset), Path.of(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 : servletFileUpload.parseRequest(request)) {
Review Comment:
In thinking about it more, that might be a security issue, as the serletFileUpload object would upload everything first (something that the setting is trying to prevent), only to perform the check after the processing is done.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913475935
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
Kudos, no new issues were introduced!
[0 New issues](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[87.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1469409762
##########
core/src/main/java/org/apache/struts2/StrutsConstants.java:
##########
@@ -142,18 +142,25 @@ public final class StrutsConstants {
/** The maximum size of a multipart request (file upload) */
public static final String STRUTS_MULTIPART_MAXSIZE = "struts.multipart.maxSize";
+ /** The maximum size of all uploaded files.
+ Used only with {@link org.apache.struts2.dispatcher.multipart.JakartaStreamMultiPartRequest} */
+ public static final String STRUTS_MULTIPART_MAXSIZE_OF_FILES = "struts.multipart.maxSizeOfFiles";
Review Comment:
IDEA doesn't complain about `MAXSIZE` that's why I left it, but you are right :)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "burtonrhodes (via GitHub)" <gi...@apache.org>.
burtonrhodes commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1468546626
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -21,375 +21,116 @@
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.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.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.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<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 {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
- // maps parameter name -> List of param values
- protected Map<String, List<String>> params = new HashMap<>();
+ JakartaServletDiskFileUpload servletFileUpload =
+ prepareServletFileUpload(Charset.forName(charset), Path.of(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 : servletFileUpload.parseRequest(request)) {
Review Comment:
@lukaszlenart Quick thought on the maxFiles setting: I still think it makes more sense that maxFiles would apply to only file fields and not all form fields in the request. However it appears that this logic is actually baked into the JakartaServletFileUpload class itself when calling `servletFileUpload.parseRequest()`. Wonder why they coded it this way??
Anyway, if we ever wanted to change this, you could set the maxFiles to unlimited in the servletFileUpload object, and then perform the files count check in the `if (item.isFormFIeld()...` logic below. Just a thought.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1468797523
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -21,375 +21,116 @@
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.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.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.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<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 {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
- // maps parameter name -> List of param values
- protected Map<String, List<String>> params = new HashMap<>();
+ JakartaServletDiskFileUpload servletFileUpload =
+ prepareServletFileUpload(Charset.forName(charset), Path.of(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 : servletFileUpload.parseRequest(request)) {
Review Comment:
I assume so, this can be changed with a proper implementation of Streaming API which can then calculate the max size of uploaded files internally in Struts ... hm or maybe even right now, let me think this through 🤔
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913590738
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[85.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1469425591
##########
core/src/main/java/org/apache/struts2/action/UploadedFilesAware.java:
##########
@@ -27,14 +27,14 @@
* The {@link org.apache.struts2.interceptor.ActionFileUploadInterceptor} will use the interface
* to notify action about the multiple uploaded files.
*/
-public interface UploadedFilesAware {
+public interface UploadedFilesAware<T> {
Review Comment:
Oh okay - yeah I just don't know if it makes sense for a marker interface to have a generic type - might just need 2 marker interfaces instead
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1915256501
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[85.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1916305656
@kusalk sorry for that, I know it's a big change yet I wanted to fix all the issues I have discovered around Jakarta file upload at once. Also all the changes related to `UploadedFile` are mean to support other approaches as [struts2-gea-plugin](https://github.com/struts-community-plugins/struts2-gea-plugin).
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1911738634
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate failed**
Failed conditions
[65.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list) (required ≥ 80%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1908862515
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate failed**
Failed conditions
[64.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list) (required ≥ 80%)
[5.4% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list) (required ≤ 3%)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "burtonrhodes (via GitHub)" <gi...@apache.org>.
burtonrhodes commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1468547208
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -21,375 +21,116 @@
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.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.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.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<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 {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
- // maps parameter name -> List of param values
- protected Map<String, List<String>> params = new HashMap<>();
+ JakartaServletDiskFileUpload servletFileUpload =
+ prepareServletFileUpload(Charset.forName(charset), Path.of(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 : servletFileUpload.parseRequest(request)) {
Review Comment:
In thinking about it more, that might be a security issue, as the serletFileUpload object would upload everything first (something that the setting is trying to prevent), only to perform the check after the processing is done. However, the maxSize / maxFileSize might offset that security risk??
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913116602
/cc: @burtonrhodes
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913584861
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[86.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1915266113
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[85.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1915271260
## [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[1 New issue](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=861&resolved=false&inNewCodePeriod=true)
[86.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=861&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=861)
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #861:
URL: https://github.com/apache/struts/pull/861#discussion_r1469317669
##########
core/src/main/java/org/apache/struts2/action/UploadedFilesAware.java:
##########
@@ -27,14 +27,14 @@
* The {@link org.apache.struts2.interceptor.ActionFileUploadInterceptor} will use the interface
* to notify action about the multiple uploaded files.
*/
-public interface UploadedFilesAware {
+public interface UploadedFilesAware<T> {
Review Comment:
Not sure I understand the benefit of this generic type given this interface is coupled with the `ActionFileUploadInterceptor`. In what situation would an action implement `UploadedFilesAware<NotFile>`?
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java:
##########
@@ -52,309 +51,159 @@
*
* @since 2.3.18
*/
-public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
+public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest<File> {
- static final Logger LOG = LogManager.getLogger(JakartaStreamMultiPartRequest.class);
+ private 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()
+ * Processes the upload.
+ *
+ * @param request the servlet request
+ * @param saveDir location of the save dir
*/
- 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());
- }
+ @Override
+ protected void processUpload(HttpServletRequest request, String saveDir) throws IOException {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
+
+ 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 -> {
+ 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, location);
}
- }
+ });
}
- /* (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;
- }
+ protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path location) {
+ DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder();
- List<String> types = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- types.add(fileInfo.getContentType());
- }
+ LOG.debug("Using file save directory: {}", location);
+ builder.setPath(location);
- return types.toArray(new String[0]);
- }
+ LOG.debug("Sets buffer size: {}", bufferSize);
+ builder.setBufferSize(bufferSize);
- /* (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;
- }
+ LOG.debug("Using charset: {}", charset);
+ builder.setCharset(charset);
- return infos.stream().map(fileInfo ->
- StrutsUploadedFile.Builder.create(fileInfo.getFile())
- .withContentType(fileInfo.contentType)
- .withOriginalName(fileInfo.originalName)
- .build()
- ).toArray(UploadedFile[]::new);
+ DiskFileItemFactory factory = builder.get();
+ return new JakartaServletDiskFileUpload(factory);
}
- /* (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()));
+ 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 names.toArray(new String[0]);
+ return result.toString(StandardCharsets.UTF_8);
}
- /* (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)
+ /**
+ * Processes the FileItem as a normal form field.
+ *
+ * @param fileItemInput a form field item input
*/
- public String[] getFilesystemName(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
+ protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IOException {
+ String fieldName = fileItemInput.getFieldName();
+ String fieldValue = readStream(fileItemInput.getInputStream());
- List<String> names = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- names.add(fileInfo.getFile().getName());
+ if (exceedsMaxStringLength(fieldName, fieldValue)) {
+ return;
}
- 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);
+ List<String> values;
+ if (parameters.containsKey(fieldName)) {
+ values = parameters.get(fieldName);
+ } else {
+ values = new ArrayList<>();
+ parameters.put(fieldName, values);
}
- return null;
+ values.add(fieldValue);
}
- /* (non-Javadoc)
- * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterNames()
+ /**
+ * @return actual size of already uploaded files
*/
- public Enumeration<String> getParameterNames() {
- return Collections.enumeration(parameters.keySet());
+ protected Long actualSizeOfUploadedFiles() {
+ return uploadedFiles.values().stream()
+ .map(files -> files.stream().map(UploadedFile::length).reduce(0L, Long::sum))
+ .reduce(0L, Long::sum);
}
- /* (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[]{});
+ private boolean exceedsMaxFiles(FileItemInput fileItemInput) {
+ if (maxFiles != null && maxFiles == uploadedFiles.size()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Cannot accept another file: {} as it will exceed max files: {}",
+ sanitizeNewlines(fileItemInput.getName()), maxFiles);
+ }
+ LocalizedMessage errorMessage = buildErrorMessage(new FileUploadFileCountLimitException(
+ String.format("File %s exceeds allowed maximum number of files %s",
+ fileItemInput.getName(), maxFiles),
+ maxFiles, uploadedFiles.size()),
+ new Object[]{maxFiles, uploadedFiles.size()});
if (!errors.contains(errorMessage)) {
errors.add(errorMessage);
}
+ return true;
}
+ return false;
}
- /**
- * Processes the upload.
- *
- * @param request the servlet request
- * @param saveDir location of the save dir
- */
- protected void processUpload(HttpServletRequest request, String saveDir) throws Exception {
-
- // Sanity check that the request is a multi-part/form-data request.
- if (JakartaServletFileUpload.isMultipartContent(request)) {
-
- // Sanity check on request size.
- boolean requestSizePermitted = isRequestSizePermitted(request);
-
- // 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);
- }
- FileItemInputIterator i = servletFileUpload.getItemIterator(request);
-
- // Iterate the file items
- while (i.hasNext()) {
- try {
- FileItemInput itemStream = i.next();
-
- // If the file item stream is a form field, delegate to the
- // field item stream handler
- if (itemStream.isFormField()) {
- processFileItemStreamAsFormField(itemStream);
- }
-
- // 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 {
-
- // prevent processing file field item if request size not allowed.
- if (!requestSizePermitted) {
- addFileSkippedError(itemStream.getName(), request);
- LOG.debug("Skipped stream '{}', request maximum size ({}) exceeded.", itemStream.getName(), maxSize);
- continue;
- }
-
- processFileItemStreamAsFileField(itemStream, saveDir);
- }
- } catch (IOException e) {
- LOG.warn("Error occurred during process upload", e);
- }
- }
+ private void exceedsMaxSizeOfFiles(FileItemInput fileItemInput, File file, Long currentFilesSize) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("File: {} of size: {} exceeds allowed max size: {}, actual size of already uploaded files: {}",
+ sanitizeNewlines(fileItemInput.getName()), file.length(), maxSizeOfFiles, currentFilesSize
+ );
}
- }
-
- /**
- * Defines whether the request allowed based on content length.
- *
- * @param request the servlet request
- * @return true if request size is permitted
- */
- protected boolean isRequestSizePermitted(HttpServletRequest request) {
- // if maxSize is specified as -1, there is no sanity check and it's
- // safe to return true for any request, delegating the failure
- // checks later in the upload process.
- if (maxSize == null || maxSize == -1 || request == null) {
- return true;
+ LocalizedMessage errorMessage = buildErrorMessage(new FileUploadSizeException(
+ String.format("Size %s of file %s exceeds allowed max size %s",
+ file.length(), fileItemInput.getName(), maxSizeOfFiles),
+ maxSizeOfFiles, currentFilesSize),
+ new Object[]{maxSizeOfFiles, currentFilesSize});
+ if (!errors.contains(errorMessage)) {
+ errors.add(errorMessage);
}
- return request.getContentLength() < maxSize;
- }
-
- /**
- * @param request the servlet request
- * @return the request content length.
- */
- protected long getRequestSize(HttpServletRequest request) {
- return request != null ? request.getContentLength() : 0;
- }
-
- /**
- * Add a file skipped message notification for action messages.
- *
- * @param fileName file name
- * @param request the servlet request
- */
- protected void addFileSkippedError(String fileName, HttpServletRequest request) {
- String exceptionMessage = "Skipped file " + fileName + "; request size limit exceeded.";
- long allowedMaxSize = maxSize != null ? maxSize : -1;
- FileUploadSizeException exception = new FileUploadSizeException(exceptionMessage, getRequestSize(request), allowedMaxSize);
- LocalizedMessage message = buildErrorMessage(exception, new Object[]{fileName, getRequestSize(request), allowedMaxSize});
- if (!errors.contains(message)) {
- errors.add(message);
+ if (!file.delete() && LOG.isWarnEnabled()) {
+ LOG.warn("Cannot delete file: {} which exceeds maximum size: {} of all files!",
+ sanitizeNewlines(fileItemInput.getName()), maxSizeOfFiles);
}
}
/**
- * Processes the FileItemStream as a Form Field.
+ * Processes the FileItem as a file field.
*
- * @param itemStream file item stream
+ * @param fileItemInput file item representing upload file
+ * @param location location
*/
- protected void processFileItemStreamAsFormField(FileItemInput itemStream) {
- String fieldName = itemStream.getFieldName();
- try {
- List<String> values;
-
- String fieldValue = itemStream.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 '{}'.", fieldName, e);
+ 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;
}
- }
- /**
- * Processes the FileItemStream as a file field.
- *
- * @param itemStream file item stream
- * @param location location
- */
- protected void processFileItemStreamAsFileField(FileItemInput itemStream, String location) {
- // Skip file uploads that don't have a file name - meaning that no file was selected.
- if (itemStream.getName() == null || itemStream.getName().trim().isEmpty()) {
- LOG.debug("No file has been uploaded for the field: {}", itemStream.getFieldName());
+ if (exceedsMaxFiles(fileItemInput)) {
return;
}
- File file = null;
- try {
- // Create the temporary upload file.
- file = createTemporaryFile(itemStream.getName(), location);
+ File file = createTemporaryFile(fileItemInput.getName(), location);
+ streamFileToDisk(fileItemInput, file);
- if (streamFileToDisk(itemStream, file)) {
- createFileInfoFromItemStream(itemStream, file);
- }
- } catch (IOException e) {
- if (file != null) {
- try {
- file.delete();
- } catch (SecurityException se) {
- LOG.warn("Failed to delete '{}' due to security exception above.", file.getName(), se);
- }
- }
+ Long currentFilesSize = actualSizeOfUploadedFiles();
Review Comment:
This seems expensive to compute from scratch every time a file field is encountered, especially when the application might not have even configured a `maxSizeOfFiles` limit!
##########
core/src/main/java/org/apache/struts2/StrutsConstants.java:
##########
@@ -142,18 +142,25 @@ public final class StrutsConstants {
/** The maximum size of a multipart request (file upload) */
public static final String STRUTS_MULTIPART_MAXSIZE = "struts.multipart.maxSize";
+ /** The maximum size of all uploaded files.
+ Used only with {@link org.apache.struts2.dispatcher.multipart.JakartaStreamMultiPartRequest} */
+ public static final String STRUTS_MULTIPART_MAXSIZE_OF_FILES = "struts.multipart.maxSizeOfFiles";
Review Comment:
Still not very consistent - shouldn't it be `MAX_SIZE_OF_FILES`?
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -21,375 +21,116 @@
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.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.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.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<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 {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
- // maps parameter name -> List of param values
- protected Map<String, List<String>> params = new HashMap<>();
+ JakartaServletDiskFileUpload servletFileUpload =
+ prepareServletFileUpload(Charset.forName(charset), Path.of(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 : servletFileUpload.parseRequest(request)) {
Review Comment:
Yeah this is by design (see [this comment](https://github.com/apache/commons-fileupload/pull/203#issuecomment-1438293083)), none of the other options mitigate against this as the risk is processing a very large amount of request parts, not necessarily large ones. Though we can probably raise our default limit from 256 to 1000 (used in latest Tomcat).
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java:
##########
@@ -52,309 +51,159 @@
*
* @since 2.3.18
*/
-public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
+public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest<File> {
- static final Logger LOG = LogManager.getLogger(JakartaStreamMultiPartRequest.class);
+ private 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()
+ * Processes the upload.
+ *
+ * @param request the servlet request
+ * @param saveDir location of the save dir
*/
- 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());
- }
+ @Override
+ protected void processUpload(HttpServletRequest request, String saveDir) throws IOException {
+ String charset = StringUtils.isBlank(request.getCharacterEncoding())
+ ? defaultEncoding
+ : request.getCharacterEncoding();
+
+ 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 -> {
+ 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, location);
}
- }
+ });
}
- /* (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;
- }
+ protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset, Path location) {
+ DiskFileItemFactory.Builder builder = DiskFileItemFactory.builder();
- List<String> types = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- types.add(fileInfo.getContentType());
- }
+ LOG.debug("Using file save directory: {}", location);
+ builder.setPath(location);
- return types.toArray(new String[0]);
- }
+ LOG.debug("Sets buffer size: {}", bufferSize);
+ builder.setBufferSize(bufferSize);
- /* (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;
- }
+ LOG.debug("Using charset: {}", charset);
+ builder.setCharset(charset);
- return infos.stream().map(fileInfo ->
- StrutsUploadedFile.Builder.create(fileInfo.getFile())
- .withContentType(fileInfo.contentType)
- .withOriginalName(fileInfo.originalName)
- .build()
- ).toArray(UploadedFile[]::new);
+ DiskFileItemFactory factory = builder.get();
+ return new JakartaServletDiskFileUpload(factory);
}
- /* (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()));
+ 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 names.toArray(new String[0]);
+ return result.toString(StandardCharsets.UTF_8);
}
- /* (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)
+ /**
+ * Processes the FileItem as a normal form field.
+ *
+ * @param fileItemInput a form field item input
*/
- public String[] getFilesystemName(String fieldName) {
- List<FileInfo> infos = fileInfos.get(fieldName);
- if (infos == null) {
- return null;
- }
+ protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IOException {
+ String fieldName = fileItemInput.getFieldName();
+ String fieldValue = readStream(fileItemInput.getInputStream());
- List<String> names = new ArrayList<>(infos.size());
- for (FileInfo fileInfo : infos) {
- names.add(fileInfo.getFile().getName());
+ if (exceedsMaxStringLength(fieldName, fieldValue)) {
+ return;
}
- 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);
+ List<String> values;
+ if (parameters.containsKey(fieldName)) {
+ values = parameters.get(fieldName);
+ } else {
+ values = new ArrayList<>();
+ parameters.put(fieldName, values);
}
- return null;
+ values.add(fieldValue);
}
- /* (non-Javadoc)
- * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getParameterNames()
+ /**
+ * @return actual size of already uploaded files
*/
- public Enumeration<String> getParameterNames() {
- return Collections.enumeration(parameters.keySet());
+ protected Long actualSizeOfUploadedFiles() {
+ return uploadedFiles.values().stream()
+ .map(files -> files.stream().map(UploadedFile::length).reduce(0L, Long::sum))
+ .reduce(0L, Long::sum);
}
- /* (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[]{});
+ private boolean exceedsMaxFiles(FileItemInput fileItemInput) {
+ if (maxFiles != null && maxFiles == uploadedFiles.size()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Cannot accept another file: {} as it will exceed max files: {}",
+ sanitizeNewlines(fileItemInput.getName()), maxFiles);
+ }
+ LocalizedMessage errorMessage = buildErrorMessage(new FileUploadFileCountLimitException(
+ String.format("File %s exceeds allowed maximum number of files %s",
+ fileItemInput.getName(), maxFiles),
+ maxFiles, uploadedFiles.size()),
+ new Object[]{maxFiles, uploadedFiles.size()});
if (!errors.contains(errorMessage)) {
errors.add(errorMessage);
}
+ return true;
}
+ return false;
}
- /**
- * Processes the upload.
- *
- * @param request the servlet request
- * @param saveDir location of the save dir
- */
- protected void processUpload(HttpServletRequest request, String saveDir) throws Exception {
-
- // Sanity check that the request is a multi-part/form-data request.
- if (JakartaServletFileUpload.isMultipartContent(request)) {
-
- // Sanity check on request size.
- boolean requestSizePermitted = isRequestSizePermitted(request);
-
- // 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);
- }
- FileItemInputIterator i = servletFileUpload.getItemIterator(request);
-
- // Iterate the file items
- while (i.hasNext()) {
- try {
- FileItemInput itemStream = i.next();
-
- // If the file item stream is a form field, delegate to the
- // field item stream handler
- if (itemStream.isFormField()) {
- processFileItemStreamAsFormField(itemStream);
- }
-
- // 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 {
-
- // prevent processing file field item if request size not allowed.
- if (!requestSizePermitted) {
- addFileSkippedError(itemStream.getName(), request);
- LOG.debug("Skipped stream '{}', request maximum size ({}) exceeded.", itemStream.getName(), maxSize);
- continue;
- }
-
- processFileItemStreamAsFileField(itemStream, saveDir);
- }
- } catch (IOException e) {
- LOG.warn("Error occurred during process upload", e);
- }
- }
+ private void exceedsMaxSizeOfFiles(FileItemInput fileItemInput, File file, Long currentFilesSize) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("File: {} of size: {} exceeds allowed max size: {}, actual size of already uploaded files: {}",
+ sanitizeNewlines(fileItemInput.getName()), file.length(), maxSizeOfFiles, currentFilesSize
+ );
}
- }
-
- /**
- * Defines whether the request allowed based on content length.
- *
- * @param request the servlet request
- * @return true if request size is permitted
- */
- protected boolean isRequestSizePermitted(HttpServletRequest request) {
- // if maxSize is specified as -1, there is no sanity check and it's
- // safe to return true for any request, delegating the failure
- // checks later in the upload process.
- if (maxSize == null || maxSize == -1 || request == null) {
- return true;
+ LocalizedMessage errorMessage = buildErrorMessage(new FileUploadSizeException(
+ String.format("Size %s of file %s exceeds allowed max size %s",
+ file.length(), fileItemInput.getName(), maxSizeOfFiles),
+ maxSizeOfFiles, currentFilesSize),
+ new Object[]{maxSizeOfFiles, currentFilesSize});
+ if (!errors.contains(errorMessage)) {
+ errors.add(errorMessage);
}
- return request.getContentLength() < maxSize;
- }
-
- /**
- * @param request the servlet request
- * @return the request content length.
- */
- protected long getRequestSize(HttpServletRequest request) {
- return request != null ? request.getContentLength() : 0;
- }
-
- /**
- * Add a file skipped message notification for action messages.
- *
- * @param fileName file name
- * @param request the servlet request
- */
- protected void addFileSkippedError(String fileName, HttpServletRequest request) {
- String exceptionMessage = "Skipped file " + fileName + "; request size limit exceeded.";
- long allowedMaxSize = maxSize != null ? maxSize : -1;
- FileUploadSizeException exception = new FileUploadSizeException(exceptionMessage, getRequestSize(request), allowedMaxSize);
- LocalizedMessage message = buildErrorMessage(exception, new Object[]{fileName, getRequestSize(request), allowedMaxSize});
- if (!errors.contains(message)) {
- errors.add(message);
+ if (!file.delete() && LOG.isWarnEnabled()) {
+ LOG.warn("Cannot delete file: {} which exceeds maximum size: {} of all files!",
+ sanitizeNewlines(fileItemInput.getName()), maxSizeOfFiles);
}
}
/**
- * Processes the FileItemStream as a Form Field.
+ * Processes the FileItem as a file field.
*
- * @param itemStream file item stream
+ * @param fileItemInput file item representing upload file
+ * @param location location
*/
- protected void processFileItemStreamAsFormField(FileItemInput itemStream) {
- String fieldName = itemStream.getFieldName();
- try {
- List<String> values;
-
- String fieldValue = itemStream.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 '{}'.", fieldName, e);
+ 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;
}
- }
- /**
- * Processes the FileItemStream as a file field.
- *
- * @param itemStream file item stream
- * @param location location
- */
- protected void processFileItemStreamAsFileField(FileItemInput itemStream, String location) {
- // Skip file uploads that don't have a file name - meaning that no file was selected.
- if (itemStream.getName() == null || itemStream.getName().trim().isEmpty()) {
- LOG.debug("No file has been uploaded for the field: {}", itemStream.getFieldName());
+ if (exceedsMaxFiles(fileItemInput)) {
return;
}
- File file = null;
- try {
- // Create the temporary upload file.
- file = createTemporaryFile(itemStream.getName(), location);
+ File file = createTemporaryFile(fileItemInput.getName(), location);
+ streamFileToDisk(fileItemInput, file);
- if (streamFileToDisk(itemStream, file)) {
- createFileInfoFromItemStream(itemStream, file);
- }
- } catch (IOException e) {
- if (file != null) {
- try {
- file.delete();
- } catch (SecurityException se) {
- LOG.warn("Failed to delete '{}' due to security exception above.", file.getName(), se);
- }
- }
+ Long currentFilesSize = actualSizeOfUploadedFiles();
Review Comment:
Tbh I'm not convinced this option adds much value in the first place - I think `maxSize` serves this need well enough. I'm trying to think in what situation would an application need to configure `maxSizeOfFiles` instead of or in addition to `maxSize`. They want to accepts lots of big form fields but want to limit the amount of data written to disk? I guess it's possible
--
This 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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "burtonrhodes (via GitHub)" <gi...@apache.org>.
burtonrhodes commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1913283055
@lukaszlenart Looks good. The current version of this PR works well in my initial testing (dev) environment.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1916328285
@burtonrhodes I tried to play with the Showcase app and right now there is a problem uploading large files - I get blank page, yet the app is using the new Action Upload Interceptor
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [WW-5388] Uses the latest JakartaEE FileUpload Servlet 6 package [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on PR #861:
URL: https://github.com/apache/struts/pull/861#issuecomment-1920656665
@burtonrhodes did you have a chance to re-test the latest changes? Everything works fine when testing with the Showcase app yet it uses the new action upload interceptor, but still the same MultiPartParser is used behind the scene. I even added a dedicated integration test to cover max files constraint.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org