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