[
https://issues.apache.org/jira/browse/WW-5474?focusedWorklogId=1031697&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1031697
]
ASF GitHub Bot logged work on WW-5474:
--------------------------------------
Author: ASF GitHub Bot
Created on: 22/Jul/26 14:40
Start Date: 22/Jul/26 14:40
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1806:
URL: https://github.com/apache/struts/pull/1806#discussion_r3631172184
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java:
##########
@@ -115,18 +115,29 @@ protected void processUpload(HttpServletRequest request,
String saveDir) throws
RequestContext requestContext = createRequestContext(request);
- for (DiskFileItem item :
servletFileUpload.parseRequest(requestContext)) {
- // Track all DiskFileItem instances for cleanup - this is critical
for security
- // as it ensures temporary files are properly cleaned up even if
processing fails
- diskFileItems.add(item);
-
+ int fileCount = 0;
+ int parameterCount = 0;
+ // parseRequest() fully materializes every part (spilling large ones
to disk) before we
+ // iterate, so register them all for cleanup up front - otherwise a
fail-closed breach
+ // mid-loop would leak the temp files of every part after the
breaching one.
+ List<DiskFileItem> items =
servletFileUpload.parseRequest(requestContext);
+ diskFileItems.addAll(items);
+ for (DiskFileItem item : items) {
LOG.debug(() -> "Processing a form field: " +
normalizeSpace(item.getFieldName()));
if (item.isFormField()) {
// Process regular form fields (text inputs, checkboxes, etc.)
+ if (item.getFieldName() != null) {
+ enforceMaxParameterCount(parameterCount,
item.getFieldName());
+ parameterCount++;
+ }
processNormalFormField(item, charset);
} else {
- // Process file upload fields
+ // Process file upload fields (only count parts that carry an
actual file)
LOG.debug(() -> "Processing a file: " +
normalizeSpace(item.getFieldName()));
+ if (item.getName() != null &&
!item.getName().trim().isEmpty()) {
+ enforceMaxFiles(fileCount, item.getName());
+ fileCount++;
+ }
Review Comment:
`fileCount` is incremented (and may throw
`FileUploadFileCountLimitException`) before validating `item.getFieldName()`.
`processFileField` skips items with a null field name, and the stream parser
also skips before enforcing the file limit, so counting here can cause the two
parsers to diverge on malformed parts (filename present but fieldName null).
Consider only enforcing/counting when the part would actually be accepted
(i.e., fieldName non-null).
##########
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java:
##########
@@ -226,9 +239,10 @@ protected JakartaServletDiskFileUpload
prepareServletFileUpload(Charset charset,
LOG.debug("Applies max size: {} to file upload request", maxSize);
servletFileUpload.setMaxSize(maxSize);
}
- if (maxFiles != null) {
- LOG.debug("Applies max files number: {} to file upload request",
maxFiles);
- servletFileUpload.setMaxFileCount(maxFiles);
+ if (maxFiles != null && maxFiles >= 0 && maxParameterCount != null &&
maxParameterCount >= 0) {
+ long maxParts = maxFiles + maxParameterCount;
+ LOG.debug("Applies total parts backstop: {} to file upload
request", maxParts);
+ servletFileUpload.setMaxFileCount(maxParts);
}
Review Comment:
`maxParts` is computed via `maxFiles + maxParameterCount` without overflow
handling. Extremely large configured values can overflow to a negative number,
which would unintentionally disable the commons-fileupload2 backstop (and
potentially change semantics if the library treats negative as “unlimited”).
Use `Math.addExact` (or an explicit overflow check) and clamp to a safe maximum.
Issue Time Tracking
-------------------
Worklog Id: (was: 1031697)
Time Spent: 40m (was: 0.5h)
> struts.multipart.maxFiles does not work as described/expected
> -------------------------------------------------------------
>
> Key: WW-5474
> URL: https://issues.apache.org/jira/browse/WW-5474
> Project: Struts 2
> Issue Type: Bug
> Affects Versions: 6.3.0
> Reporter: nikos dimitrakas
> Priority: Major
> Fix For: 7.3.0
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> According to the documentation the property struts.multipart.maxFiles (which
> defaults to 256) is supposed to set a limit to how many files a multi-part
> form submission may include. But in reality, the specified value sets a limit
> to how many parameter values the form may submit, including strings,
> integers, booleans (from all input fields, checkboxes, hiddens, etc).
> The problem is in the method JakartaMultiPartRequest.parsePrequest (part of
> struts) when in the last line it calls upload.parseRequest which is in
> FileUploadBase (part of commons-fileupload). That methods tries to find the
> FileItems from the provided RequestContext, but considers every parameter
> value to be a FileItem and then throws a new FileCountLimitExceededException
> when the number of parameter values (of any type) reaches the fileCountMax.
> I am not sure if the problem is in struts or in fileupload. Maybe the
> provided RequestContext should somehow only include the file parameters so
> that only they get counted. Or perhaps the documentation should be changed to
> clarify that struts.multipart.maxFiles specifies the maximum number of
> parameter values a multipart form may include.
> I also found the issue
> [FILEUPLOAD-351|https://issues.apache.org/jira/browse/FILEUPLOAD-351] that
> seems to point out the confusing behaviour. But until the issue has been
> resolved in fileupload, struts behaves contrary to its own documentation at
> [https://struts.apache.org/core-developers/file-upload]
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
